From 056be6d5048b275f680b89bbaa85cace8b5f6072 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Fri, 27 Mar 2026 15:08:21 +0100 Subject: [PATCH 1/3] C#: Simplify the ConstantCondition query. --- .../Control-Flow/ConstantCondition.ql | 113 ++++-------------- .../ConstantCondition/ConstantCondition.cs | 10 +- .../ConstantCondition.expected | 16 +-- .../ConstantConditionalExpressionCondition.cs | 2 +- .../ConstantCondition/ConstantDoCondition.cs | 4 +- .../ConstantCondition/ConstantForCondition.cs | 2 +- .../ConstantCondition/ConstantIfCondition.cs | 7 +- .../ConstantIsNullOrEmpty.cs | 11 +- .../ConstantWhileCondition.cs | 2 +- .../ConstantCondition/ConstantCondition.cs | 2 +- .../ConstantCondition.expected | 2 - 11 files changed, 51 insertions(+), 120 deletions(-) diff --git a/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql b/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql index a58e19049ff9..c1eb996863c5 100644 --- a/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql +++ b/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql @@ -41,7 +41,9 @@ module ConstCondInput implements ConstCond::InputSig { module ConstCondImpl = ConstCond::Make; predicate nullCheck(Expr e, boolean direct) { - exists(QualifiableExpr qe | qe.isConditional() and qe.getQualifier() = e and direct = true) + exists(QualifiableExpr qe | qe.isConditional() and direct = true | + qe.getQualifier() = e or qe.(ExtensionMethodCall).getArgument(0) = e + ) or exists(NullCoalescingOperation nce | nce.getLeftOperand() = e and direct = true) or @@ -108,57 +110,14 @@ class ConstantGuard extends ConstantCondition { class ConstantBooleanCondition extends ConstantCondition { boolean b; - ConstantBooleanCondition() { isConstantCondition(this, b) } + ConstantBooleanCondition() { isConstantComparison(this, b) } override string getMessage() { result = "Condition always evaluates to '" + b + "'." } - - override predicate isWhiteListed() { - // E.g. `x ?? false` - this.(BoolLiteral) = any(NullCoalescingOperation nce).getRightOperand() or - // No need to flag logical operations when the operands are constant - isConstantCondition(this.(LogicalNotExpr).getOperand(), _) or - this = - any(LogicalAndExpr lae | - isConstantCondition(lae.getAnOperand(), false) - or - isConstantCondition(lae.getLeftOperand(), true) and - isConstantCondition(lae.getRightOperand(), true) - ) or - this = - any(LogicalOrExpr loe | - isConstantCondition(loe.getAnOperand(), true) - or - isConstantCondition(loe.getLeftOperand(), false) and - isConstantCondition(loe.getRightOperand(), false) - ) - } } -/** A constant condition in an `if` statement or a conditional expression. */ -class ConstantIfCondition extends ConstantBooleanCondition { - ConstantIfCondition() { - this = any(IfStmt is).getCondition().getAChildExpr*() or - this = any(ConditionalExpr ce).getCondition().getAChildExpr*() - } - - override predicate isWhiteListed() { - ConstantBooleanCondition.super.isWhiteListed() - or - // It is a common pattern to use a local constant/constant field to control - // whether code parts must be executed or not - this instanceof AssignableRead and - not this instanceof ParameterRead - } -} - -/** A constant loop condition. */ -class ConstantLoopCondition extends ConstantBooleanCondition { - ConstantLoopCondition() { this = any(LoopStmt ls).getCondition() } - - override predicate isWhiteListed() { - // Clearly intentional infinite loops are allowed - this.(BoolLiteral).getBoolValue() = true - } +private Expr getQualifier(QualifiableExpr e) { + // `e.getQualifier()` does not work for calls to extension methods + result = e.getChildExpr(-1) } /** A constant nullness condition. */ @@ -166,14 +125,23 @@ class ConstantNullnessCondition extends ConstantCondition { boolean b; ConstantNullnessCondition() { - forex(ControlFlow::Node cfn | cfn = this.getAControlFlowNode() | - exists(ControlFlow::NullnessSuccessor t, ControlFlow::Node s | - s = cfn.getASuccessorByType(t) - | - b = t.getValue() and - not s.isJoin() - ) and - strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1 + nullCheck(this, true) and + exists(Expr stripped | stripped = this.(Expr).stripCasts() | + stripped.getType() = + any(ValueType t | + not t instanceof NullableType and + // Extractor bug: the type of `x?.Length` is reported as `int`, but it should + // be `int?` + not getQualifier*(stripped).(QualifiableExpr).isConditional() + ) and + b = false + or + stripped instanceof NullLiteral and + b = true + or + stripped.hasValue() and + not stripped instanceof NullLiteral and + b = false ) } @@ -184,39 +152,6 @@ class ConstantNullnessCondition extends ConstantCondition { } } -/** A constant matching condition. */ -class ConstantMatchingCondition extends ConstantCondition { - boolean b; - - ConstantMatchingCondition() { - this instanceof Expr and - forex(ControlFlow::Node cfn | cfn = this.getAControlFlowNode() | - exists(ControlFlow::MatchingSuccessor t | exists(cfn.getASuccessorByType(t)) | - b = t.getValue() - ) and - strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1 - ) - } - - override predicate isWhiteListed() { - exists(Switch se, Case c, int i | - c = se.getCase(i) and - c.getPattern() = this.(DiscardExpr) - | - i > 0 - or - i = 0 and - exists(Expr cond | c.getCondition() = cond and not isConstantCondition(cond, true)) - ) - or - this = any(PositionalPatternExpr ppe).getPattern(_) - } - - override string getMessage() { - if b = true then result = "Pattern always matches." else result = "Pattern never matches." - } -} - from ConstantCondition c, string msg, Guards::Guards::Guard reason, string reasonMsg where msg = c.getMessage() and diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs index 0445e152ec72..95e8cd38c117 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs @@ -59,9 +59,9 @@ void M1() { switch (1 + 2) { - case 2: // $ Alert + case 2: // Intentionally missing Alert break; - case 3: // $ Alert + case 3: // Intentionally missing Alert break; case int _: // GOOD break; @@ -72,7 +72,7 @@ void M2(string s) { switch ((object)s) { - case int _: // $ Alert + case int _: // Intentionally missing Alert break; case "": // GOOD break; @@ -92,7 +92,7 @@ string M4(object o) { return o switch { - _ => o.ToString() // $ Alert + _ => o.ToString() // GOOD, catch-all pattern is fine }; } @@ -138,7 +138,7 @@ string M9(int i) { switch (i) { - case var _: // $ Alert + case var _: // GOOD, catch-all pattern is fine return "even"; } } diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected index fc310e53fded..7d1d716386c9 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected @@ -4,28 +4,18 @@ | ConstantCondition.cs:47:17:47:18 | "" | Expression is never 'null'. | ConstantCondition.cs:47:17:47:18 | "" | dummy | | ConstantCondition.cs:48:13:48:19 | (...) ... | Expression is never 'null'. | ConstantCondition.cs:48:13:48:19 | (...) ... | dummy | | ConstantCondition.cs:49:13:49:14 | "" | Expression is never 'null'. | ConstantCondition.cs:49:13:49:14 | "" | dummy | -| ConstantCondition.cs:62:18:62:18 | 2 | Pattern never matches. | ConstantCondition.cs:62:18:62:18 | 2 | dummy | -| ConstantCondition.cs:64:18:64:18 | 3 | Pattern always matches. | ConstantCondition.cs:64:18:64:18 | 3 | dummy | -| ConstantCondition.cs:75:18:75:20 | access to type Int32 | Pattern never matches. | ConstantCondition.cs:75:18:75:20 | access to type Int32 | dummy | -| ConstantCondition.cs:95:13:95:13 | _ | Pattern always matches. | ConstantCondition.cs:95:13:95:13 | _ | dummy | | ConstantCondition.cs:114:13:114:14 | access to parameter b1 | Condition is always true because of $@. | ConstantCondition.cs:110:14:110:15 | access to parameter b1 | access to parameter b1 | | ConstantCondition.cs:114:19:114:20 | access to parameter b2 | Condition is always true because of $@. | ConstantCondition.cs:112:14:112:15 | access to parameter b2 | access to parameter b2 | -| ConstantCondition.cs:141:22:141:22 | _ | Pattern always matches. | ConstantCondition.cs:141:22:141:22 | _ | dummy | | ConstantConditionBad.cs:5:16:5:20 | ... > ... | Condition always evaluates to 'false'. | ConstantConditionBad.cs:5:16:5:20 | ... > ... | dummy | | ConstantConditionalExpressionCondition.cs:11:22:11:34 | ... == ... | Condition always evaluates to 'true'. | ConstantConditionalExpressionCondition.cs:11:22:11:34 | ... == ... | dummy | -| ConstantConditionalExpressionCondition.cs:12:21:12:25 | false | Condition always evaluates to 'false'. | ConstantConditionalExpressionCondition.cs:12:21:12:25 | false | dummy | | ConstantConditionalExpressionCondition.cs:13:21:13:30 | ... == ... | Condition always evaluates to 'true'. | ConstantConditionalExpressionCondition.cs:13:21:13:30 | ... == ... | dummy | -| ConstantForCondition.cs:9:29:9:33 | false | Condition always evaluates to 'false'. | ConstantForCondition.cs:9:29:9:33 | false | dummy | +| ConstantDoCondition.cs:15:22:15:34 | ... == ... | Condition always evaluates to 'true'. | ConstantDoCondition.cs:15:22:15:34 | ... == ... | dummy | +| ConstantDoCondition.cs:32:22:32:31 | ... == ... | Condition always evaluates to 'true'. | ConstantDoCondition.cs:32:22:32:31 | ... == ... | dummy | | ConstantForCondition.cs:11:29:11:34 | ... == ... | Condition always evaluates to 'false'. | ConstantForCondition.cs:11:29:11:34 | ... == ... | dummy | | ConstantIfCondition.cs:11:17:11:29 | ... == ... | Condition always evaluates to 'true'. | ConstantIfCondition.cs:11:17:11:29 | ... == ... | dummy | -| ConstantIfCondition.cs:14:17:14:21 | false | Condition always evaluates to 'false'. | ConstantIfCondition.cs:14:17:14:21 | false | dummy | | ConstantIfCondition.cs:17:17:17:26 | ... == ... | Condition always evaluates to 'true'. | ConstantIfCondition.cs:17:17:17:26 | ... == ... | dummy | -| ConstantIsNullOrEmpty.cs:10:21:10:54 | call to method IsNullOrEmpty | Condition always evaluates to 'false'. | ConstantIsNullOrEmpty.cs:10:21:10:54 | call to method IsNullOrEmpty | dummy | -| ConstantIsNullOrEmpty.cs:46:21:46:46 | call to method IsNullOrEmpty | Condition always evaluates to 'true'. | ConstantIsNullOrEmpty.cs:46:21:46:46 | call to method IsNullOrEmpty | dummy | -| ConstantIsNullOrEmpty.cs:50:21:50:44 | call to method IsNullOrEmpty | Condition always evaluates to 'true'. | ConstantIsNullOrEmpty.cs:50:21:50:44 | call to method IsNullOrEmpty | dummy | -| ConstantIsNullOrEmpty.cs:54:21:54:45 | call to method IsNullOrEmpty | Condition always evaluates to 'false'. | ConstantIsNullOrEmpty.cs:54:21:54:45 | call to method IsNullOrEmpty | dummy | +| ConstantIfCondition.cs:35:20:35:25 | ... >= ... | Condition always evaluates to 'true'. | ConstantIfCondition.cs:35:20:35:25 | ... >= ... | dummy | | ConstantNullCoalescingLeftHandOperand.cs:11:24:11:34 | access to constant NULL_OBJECT | Expression is never 'null'. | ConstantNullCoalescingLeftHandOperand.cs:11:24:11:34 | access to constant NULL_OBJECT | dummy | | ConstantNullCoalescingLeftHandOperand.cs:12:24:12:27 | null | Expression is always 'null'. | ConstantNullCoalescingLeftHandOperand.cs:12:24:12:27 | null | dummy | | ConstantWhileCondition.cs:12:20:12:32 | ... == ... | Condition always evaluates to 'true'. | ConstantWhileCondition.cs:12:20:12:32 | ... == ... | dummy | -| ConstantWhileCondition.cs:16:20:16:24 | false | Condition always evaluates to 'false'. | ConstantWhileCondition.cs:16:20:16:24 | false | dummy | | ConstantWhileCondition.cs:24:20:24:29 | ... == ... | Condition always evaluates to 'true'. | ConstantWhileCondition.cs:24:20:24:29 | ... == ... | dummy | diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionalExpressionCondition.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionalExpressionCondition.cs index 4cd56232627d..b84e746ae667 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionalExpressionCondition.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionalExpressionCondition.cs @@ -9,7 +9,7 @@ class Main public void Foo() { int i = (ZERO == 1 - 1) ? 0 : 1; // $ Alert - int j = false ? 0 : 1; // $ Alert + int j = false ? 0 : 1; // GOOD, literal false is likely intentional int k = " " == " " ? 0 : 1; // $ Alert int l = (" "[0] == ' ') ? 0 : 1; // Missing Alert int m = Bar() == 0 ? 0 : 1; // GOOD diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantDoCondition.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantDoCondition.cs index 07db7f0c0eeb..fadd44fefee3 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantDoCondition.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantDoCondition.cs @@ -12,7 +12,7 @@ public void Foo() do { break; - } while (ZERO == 1 - 1); // BAD + } while (ZERO == 1 - 1); // $ Alert do { break; @@ -29,7 +29,7 @@ public void Foo() do { break; - } while (" " == " "); // BAD + } while (" " == " "); // $ Alert do { break; diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantForCondition.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantForCondition.cs index 2da0589d1827..74bc709348a1 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantForCondition.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantForCondition.cs @@ -6,7 +6,7 @@ class Main { public void M() { - for (int i = 0; false; i++) // $ Alert + for (int i = 0; false; i++) // GOOD, literal false is likely intentional ; for (int i = 0; 0 == 1; i++) // $ Alert ; diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs index 04c91cc222da..8da2623e1f44 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs @@ -11,7 +11,7 @@ public void Foo() if (ZERO == 1 - 1) // $ Alert { } - if (false) // $ Alert + if (false) // GOOD { } if (" " == " ") // $ Alert @@ -30,6 +30,11 @@ public int Bar() return ZERO; } + public void UnsignedCheck(byte n) + { + while (n >= 0) { n--; } // $ Alert + } + } } diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs index 01e8353a20f4..97857777574a 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs @@ -7,7 +7,10 @@ internal class Program static void Main(string[] args) { { - if (string.IsNullOrEmpty(nameof(args))) // $ Alert + // All of the IsNullOrEmpty constant checks have been descoped + // from the query as it didn't seem worth the effort to keep them. + + if (string.IsNullOrEmpty(nameof(args))) // Missing Alert (always false) { } @@ -43,15 +46,15 @@ static void Main(string[] args) { } - if (string.IsNullOrEmpty(null)) // $ Alert + if (string.IsNullOrEmpty(null)) // Missing Alert { } - if (string.IsNullOrEmpty("")) // $ Alert + if (string.IsNullOrEmpty("")) // Missing Alert { } - if (string.IsNullOrEmpty(" ")) // $ Alert + if (string.IsNullOrEmpty(" ")) // Missing Alert { } } diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantWhileCondition.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantWhileCondition.cs index 59575e0de45e..f69cf1657328 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantWhileCondition.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantWhileCondition.cs @@ -13,7 +13,7 @@ public void Foo() { break; } - while (false) // $ Alert + while (false) // Silly, but likely intentional { break; } diff --git a/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs b/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs index 6f40759b3e67..caf9d85b6532 100644 --- a/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs +++ b/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs @@ -12,7 +12,7 @@ class ConstantMatching void M1() { var c1 = new C1(); - if (c1.Prop is int) // $ Alert + if (c1.Prop is int) // Descoped, no longer reported by the query. { } diff --git a/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected b/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected index b00cfb3115ef..e69de29bb2d1 100644 --- a/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected +++ b/csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected @@ -1,2 +0,0 @@ -| ConstantCondition.cs:15:13:15:26 | ... is ... | Condition always evaluates to 'false'. | ConstantCondition.cs:15:13:15:26 | ... is ... | dummy | -| ConstantCondition.cs:15:24:15:26 | access to type Int32 | Pattern never matches. | ConstantCondition.cs:15:24:15:26 | access to type Int32 | dummy | From 2a54dce5cbb1877b30c5763b4f9c9d65cef018ed Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Mon, 30 Mar 2026 11:44:02 +0200 Subject: [PATCH 2/3] C#: Remove redundant ConstantComparison.ql query. --- .../csharp-code-quality-extended.qls.expected | 1 - .../csharp-code-quality.qls.expected | 1 - .../csharp-security-and-quality.qls.expected | 1 - .../semmle/code/csharp/commons/Constants.qll | 13 ----- csharp/ql/src/CSI/CompareIdenticalValues.ql | 3 +- .../ql/src/Likely Bugs/ConstantComparison.cs | 2 - .../src/Likely Bugs/ConstantComparison.qhelp | 46 ---------------- .../ql/src/Likely Bugs/ConstantComparison.ql | 22 -------- .../csharp-security-and-quality.qls | 1 - .../ConstantCondition}/ConstantComparison.cs | 54 +++++++++---------- .../ConstantCondition.expected | 26 +++++++++ .../ConstantComparison.expected | 26 --------- .../ConstantComparison.qlref | 1 - .../Likely Bugs/ConstantComparison/options | 2 - 14 files changed, 54 insertions(+), 145 deletions(-) delete mode 100644 csharp/ql/src/Likely Bugs/ConstantComparison.cs delete mode 100644 csharp/ql/src/Likely Bugs/ConstantComparison.qhelp delete mode 100644 csharp/ql/src/Likely Bugs/ConstantComparison.ql rename csharp/ql/test/query-tests/{Likely Bugs/ConstantComparison => Bad Practices/Control-Flow/ConstantCondition}/ConstantComparison.cs (52%) delete mode 100644 csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.expected delete mode 100644 csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.qlref delete mode 100644 csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/options diff --git a/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality-extended.qls.expected b/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality-extended.qls.expected index fdc5e6eae9d1..c6361fe69c52 100644 --- a/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality-extended.qls.expected +++ b/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality-extended.qls.expected @@ -65,7 +65,6 @@ ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql ql/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql ql/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql -ql/csharp/ql/src/Likely Bugs/ConstantComparison.ql ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql ql/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql ql/csharp/ql/src/Likely Bugs/EqualityCheckOnFloats.ql diff --git a/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected b/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected index 6694cc8461b8..893eaeb75607 100644 --- a/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected +++ b/csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected @@ -38,7 +38,6 @@ ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql ql/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql ql/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql -ql/csharp/ql/src/Likely Bugs/ConstantComparison.ql ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql ql/csharp/ql/src/Likely Bugs/EqualityCheckOnFloats.ql ql/csharp/ql/src/Likely Bugs/EqualsArray.ql diff --git a/csharp/ql/integration-tests/posix/query-suite/csharp-security-and-quality.qls.expected b/csharp/ql/integration-tests/posix/query-suite/csharp-security-and-quality.qls.expected index b520a571fc8c..43c14b4281ca 100644 --- a/csharp/ql/integration-tests/posix/query-suite/csharp-security-and-quality.qls.expected +++ b/csharp/ql/integration-tests/posix/query-suite/csharp-security-and-quality.qls.expected @@ -69,7 +69,6 @@ ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql ql/csharp/ql/src/Likely Bugs/Collections/ContainerSizeCmpZero.ql ql/csharp/ql/src/Likely Bugs/Collections/ReadOnlyContainer.ql ql/csharp/ql/src/Likely Bugs/Collections/WriteOnlyContainer.ql -ql/csharp/ql/src/Likely Bugs/ConstantComparison.ql ql/csharp/ql/src/Likely Bugs/DangerousNonShortCircuitLogic.ql ql/csharp/ql/src/Likely Bugs/Dynamic/BadDynamicCall.ql ql/csharp/ql/src/Likely Bugs/EqualityCheckOnFloats.ql diff --git a/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll b/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll index ab2d9e0eef74..5025202eb217 100644 --- a/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll +++ b/csharp/ql/lib/semmle/code/csharp/commons/Constants.qll @@ -4,19 +4,6 @@ import csharp private import semmle.code.csharp.commons.ComparisonTest private import semmle.code.csharp.commons.StructuralComparison as StructuralComparison -pragma[noinline] -private predicate isConstantCondition0(ControlFlow::Node cfn, boolean b) { - exists(cfn.getASuccessorByType(any(ControlFlow::BooleanSuccessor t | t.getValue() = b))) and - strictcount(ControlFlow::SuccessorType t | exists(cfn.getASuccessorByType(t))) = 1 -} - -/** - * Holds if `e` is a condition that always evaluates to Boolean value `b`. - */ -predicate isConstantCondition(Expr e, boolean b) { - forex(ControlFlow::Node cfn | cfn = e.getAControlFlowNode() | isConstantCondition0(cfn, b)) -} - /** * Holds if comparison operation `co` is constant with the Boolean value `b`. * For example, the comparison `x > x` is constantly `false` in diff --git a/csharp/ql/src/CSI/CompareIdenticalValues.ql b/csharp/ql/src/CSI/CompareIdenticalValues.ql index 503067a8a3eb..fe79db082065 100644 --- a/csharp/ql/src/CSI/CompareIdenticalValues.ql +++ b/csharp/ql/src/CSI/CompareIdenticalValues.ql @@ -47,7 +47,6 @@ where not comparesIdenticalValuesNan(ct, _) and msg = "Comparison of identical values." ) and not isMutatingOperation(ct.getAnArgument().getAChild*()) and - not isConstantCondition(e, _) and // Avoid overlap with cs/constant-condition - not isConstantComparison(e, _) and // Avoid overlap with cs/constant-comparison + not isConstantComparison(e, _) and // Avoid overlap with cs/constant-condition not isExprInAssertion(e) select ct, msg diff --git a/csharp/ql/src/Likely Bugs/ConstantComparison.cs b/csharp/ql/src/Likely Bugs/ConstantComparison.cs deleted file mode 100644 index 5b0304b28189..000000000000 --- a/csharp/ql/src/Likely Bugs/ConstantComparison.cs +++ /dev/null @@ -1,2 +0,0 @@ - for (uint order = numberOfOrders; order >= 0; order--) - ProcessOrder(order); diff --git a/csharp/ql/src/Likely Bugs/ConstantComparison.qhelp b/csharp/ql/src/Likely Bugs/ConstantComparison.qhelp deleted file mode 100644 index 5e52142c84e5..000000000000 --- a/csharp/ql/src/Likely Bugs/ConstantComparison.qhelp +++ /dev/null @@ -1,46 +0,0 @@ - - - -

- Comparisons which always yield the same result are unnecessary and may indicate a bug in the - logic. This can happen when the data type of one of the operands has a limited range of values. - For example unsigned integers are always greater than or equal to zero, and byte - values are always less than 256. -

- -

The following expressions always have the same result:

-
    -
  • Unsigned < 0 is always false,
    • -
  • 0 > Unsigned is always false,
    • -
  • 0 ≤ Unsigned is always true,
    • -
  • Unsigned ≥ 0 is always true,
    • -
  • Unsigned == -1 is always false,
    • -
  • Byte < 512 is always true.
    • -
- - - -

- Examine the logic of the program to determine whether the comparison is necessary. - Either change the data types, or remove the unnecessary code. -

- - - -

The following example attempts to count down from numberOfOrders to 0, - however the loop never terminates because order is an unsigned integer and so the - condition order >= 0 is always true.

- - - -

The solution is to change the type of the variable order.

- - - -
  • - MSDN Library: C# Operators. -
    • -     -     \ No newline at end of file diff --git a/csharp/ql/src/Likely Bugs/ConstantComparison.ql b/csharp/ql/src/Likely Bugs/ConstantComparison.ql deleted file mode 100644 index 983523482142..000000000000 --- a/csharp/ql/src/Likely Bugs/ConstantComparison.ql +++ /dev/null @@ -1,22 +0,0 @@ -/** - * @name Comparison is constant - * @description The result of the comparison is always the same. - * @kind problem - * @problem.severity warning - * @precision high - * @id cs/constant-comparison - * @tags quality - * reliability - * correctness - */ - -import csharp -import semmle.code.csharp.commons.Assertions -import semmle.code.csharp.commons.Constants - -from ComparisonOperation cmp, boolean value -where - isConstantComparison(cmp, value) and - not isConstantCondition(cmp, _) and // Avoid overlap with cs/constant-condition - not isExprInAssertion(cmp) -select cmp, "This comparison is always " + value + "." diff --git a/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls b/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls index 21d39db383d3..9700c8b03410 100644 --- a/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls +++ b/csharp/ql/src/codeql-suites/csharp-security-and-quality.qls @@ -22,7 +22,6 @@ - cs/comparison-of-identical-expressions - cs/complex-block - cs/complex-condition - - cs/constant-comparison - cs/constant-condition - cs/coupled-types - cs/dereferenced-value-is-always-null diff --git a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.cs b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantComparison.cs similarity index 52% rename from csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.cs rename to csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantComparison.cs index c31d940e7f26..26f2c4573258 100644 --- a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.cs +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantComparison.cs @@ -16,16 +16,16 @@ void f() { bool good, bad; - bad = uintValue < 0; - bad = 0 > uintValue; - bad = 0 <= uintValue; - bad = uintValue >= 0; + bad = uintValue < 0; // $ Alert + bad = 0 > uintValue; // $ Alert + bad = 0 <= uintValue; // $ Alert + bad = uintValue >= 0; // $ Alert - bad = uintValue == -1; - bad = uintValue != -1; - bad = 256 == byteValue; - bad = 256 != byteValue; - bad = 1 != 0; + bad = uintValue == -1; // $ Alert + bad = uintValue != -1; // $ Alert + bad = 256 == byteValue; // $ Alert + bad = 256 != byteValue; // $ Alert + bad = 1 != 0; // $ Alert good = byteValue == 50; good = 50 != byteValue; @@ -35,61 +35,61 @@ void f() good = intValue <= 1u; good = 1u >= intValue; - good = charValue >= '0'; // Regression + good = charValue >= '0'; good = charValue < '0'; // Test ranges - bad = charValue <= 65535; - bad = charValue >= 0; + bad = charValue <= 65535; // $ Alert + bad = charValue >= 0; // $ Alert good = charValue < 255; good = charValue > 0; - bad = byteValue >= byte.MinValue; - bad = byteValue <= byte.MaxValue; + bad = byteValue >= byte.MinValue; // $ Alert + bad = byteValue <= byte.MaxValue; // $ Alert good = byteValue > byte.MinValue; good = byteValue < byte.MaxValue; - bad = sbyteValue >= sbyte.MinValue; - bad = sbyteValue <= sbyte.MaxValue; + bad = sbyteValue >= sbyte.MinValue; // $ Alert + bad = sbyteValue <= sbyte.MaxValue; // $ Alert good = sbyteValue < sbyte.MaxValue; good = sbyteValue > sbyte.MinValue; - bad = shortValue >= short.MinValue; - bad = shortValue <= short.MaxValue; + bad = shortValue >= short.MinValue; // $ Alert + bad = shortValue <= short.MaxValue; // $ Alert good = shortValue > short.MinValue; good = shortValue < short.MaxValue; - bad = ushortValue >= ushort.MinValue; - bad = ushortValue <= ushort.MaxValue; + bad = ushortValue >= ushort.MinValue; // $ Alert + bad = ushortValue <= ushort.MaxValue; // $ Alert good = ushortValue > ushort.MinValue; good = ushortValue < ushort.MaxValue; - bad = intValue >= int.MinValue; - bad = intValue <= int.MaxValue; + bad = intValue >= int.MinValue; // $ Alert + bad = intValue <= int.MaxValue; // $ Alert good = intValue > int.MinValue; good = intValue < int.MaxValue; - bad = uintValue >= uint.MinValue; + bad = uintValue >= uint.MinValue; // $ Alert good = uintValue > uint.MinValue; - bad = ulongValue >= ulong.MinValue; + bad = ulongValue >= ulong.MinValue; // $ Alert good = ulongValue > ulong.MinValue; // Explicit casts can cause large values to be truncated or // to wrap into negative values. good = (sbyte)byteValue >= 0; good = (sbyte)byteValue == -1; - bad = (sbyte)byteValue > 127; - bad = (sbyte)byteValue > (sbyte)127; + bad = (sbyte)byteValue > 127; // $ Alert + bad = (sbyte)byteValue > (sbyte)127; // $ Alert good = (int)uintValue == -1; good = (sbyte)uintValue == -1; - bad = (sbyte)uintValue == 256; + bad = (sbyte)uintValue == 256; // $ Alert System.Diagnostics.Debug.Assert(ulongValue >= ulong.MinValue); // GOOD } diff --git a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected index 7d1d716386c9..edf1f87232e8 100644 --- a/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected +++ b/csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected @@ -1,3 +1,29 @@ +| ConstantComparison.cs:19:15:19:27 | ... < ... | Condition always evaluates to 'false'. | ConstantComparison.cs:19:15:19:27 | ... < ... | dummy | +| ConstantComparison.cs:20:15:20:27 | ... > ... | Condition always evaluates to 'false'. | ConstantComparison.cs:20:15:20:27 | ... > ... | dummy | +| ConstantComparison.cs:21:15:21:28 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:21:15:21:28 | ... <= ... | dummy | +| ConstantComparison.cs:22:15:22:28 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:22:15:22:28 | ... >= ... | dummy | +| ConstantComparison.cs:24:15:24:29 | ... == ... | Condition always evaluates to 'false'. | ConstantComparison.cs:24:15:24:29 | ... == ... | dummy | +| ConstantComparison.cs:25:15:25:29 | ... != ... | Condition always evaluates to 'true'. | ConstantComparison.cs:25:15:25:29 | ... != ... | dummy | +| ConstantComparison.cs:26:15:26:30 | ... == ... | Condition always evaluates to 'false'. | ConstantComparison.cs:26:15:26:30 | ... == ... | dummy | +| ConstantComparison.cs:27:15:27:30 | ... != ... | Condition always evaluates to 'true'. | ConstantComparison.cs:27:15:27:30 | ... != ... | dummy | +| ConstantComparison.cs:28:15:28:20 | ... != ... | Condition always evaluates to 'true'. | ConstantComparison.cs:28:15:28:20 | ... != ... | dummy | +| ConstantComparison.cs:42:15:42:32 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:42:15:42:32 | ... <= ... | dummy | +| ConstantComparison.cs:43:15:43:28 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:43:15:43:28 | ... >= ... | dummy | +| ConstantComparison.cs:48:15:48:40 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:48:15:48:40 | ... >= ... | dummy | +| ConstantComparison.cs:49:15:49:40 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:49:15:49:40 | ... <= ... | dummy | +| ConstantComparison.cs:54:15:54:42 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:54:15:54:42 | ... >= ... | dummy | +| ConstantComparison.cs:55:15:55:42 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:55:15:55:42 | ... <= ... | dummy | +| ConstantComparison.cs:60:15:60:42 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:60:15:60:42 | ... >= ... | dummy | +| ConstantComparison.cs:61:15:61:42 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:61:15:61:42 | ... <= ... | dummy | +| ConstantComparison.cs:66:15:66:44 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:66:15:66:44 | ... >= ... | dummy | +| ConstantComparison.cs:67:15:67:44 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:67:15:67:44 | ... <= ... | dummy | +| ConstantComparison.cs:72:15:72:38 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:72:15:72:38 | ... >= ... | dummy | +| ConstantComparison.cs:73:15:73:38 | ... <= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:73:15:73:38 | ... <= ... | dummy | +| ConstantComparison.cs:78:15:78:40 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:78:15:78:40 | ... >= ... | dummy | +| ConstantComparison.cs:81:15:81:42 | ... >= ... | Condition always evaluates to 'true'. | ConstantComparison.cs:81:15:81:42 | ... >= ... | dummy | +| ConstantComparison.cs:88:15:88:36 | ... > ... | Condition always evaluates to 'false'. | ConstantComparison.cs:88:15:88:36 | ... > ... | dummy | +| ConstantComparison.cs:89:15:89:43 | ... > ... | Condition always evaluates to 'false'. | ConstantComparison.cs:89:15:89:43 | ... > ... | dummy | +| ConstantComparison.cs:92:15:92:37 | ... == ... | Condition always evaluates to 'false'. | ConstantComparison.cs:92:15:92:37 | ... == ... | dummy | | ConstantCondition.cs:38:18:38:29 | (...) ... | Expression is always 'null'. | ConstantCondition.cs:38:18:38:29 | (...) ... | dummy | | ConstantCondition.cs:39:18:39:24 | (...) ... | Expression is never 'null'. | ConstantCondition.cs:39:18:39:24 | (...) ... | dummy | | ConstantCondition.cs:46:17:46:26 | (...) ... | Expression is always 'null'. | ConstantCondition.cs:46:17:46:26 | (...) ... | dummy | diff --git a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.expected b/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.expected deleted file mode 100644 index 53f55501895a..000000000000 --- a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.expected +++ /dev/null @@ -1,26 +0,0 @@ -| ConstantComparison.cs:19:15:19:27 | ... < ... | This comparison is always false. | -| ConstantComparison.cs:20:15:20:27 | ... > ... | This comparison is always false. | -| ConstantComparison.cs:21:15:21:28 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:22:15:22:28 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:24:15:24:29 | ... == ... | This comparison is always false. | -| ConstantComparison.cs:25:15:25:29 | ... != ... | This comparison is always true. | -| ConstantComparison.cs:26:15:26:30 | ... == ... | This comparison is always false. | -| ConstantComparison.cs:27:15:27:30 | ... != ... | This comparison is always true. | -| ConstantComparison.cs:28:15:28:20 | ... != ... | This comparison is always true. | -| ConstantComparison.cs:42:15:42:32 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:43:15:43:28 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:48:15:48:40 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:49:15:49:40 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:54:15:54:42 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:55:15:55:42 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:60:15:60:42 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:61:15:61:42 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:66:15:66:44 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:67:15:67:44 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:72:15:72:38 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:73:15:73:38 | ... <= ... | This comparison is always true. | -| ConstantComparison.cs:78:15:78:40 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:81:15:81:42 | ... >= ... | This comparison is always true. | -| ConstantComparison.cs:88:15:88:36 | ... > ... | This comparison is always false. | -| ConstantComparison.cs:89:15:89:43 | ... > ... | This comparison is always false. | -| ConstantComparison.cs:92:15:92:37 | ... == ... | This comparison is always false. | diff --git a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.qlref b/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.qlref deleted file mode 100644 index 8566e49f6cc0..000000000000 --- a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.qlref +++ /dev/null @@ -1 +0,0 @@ -Likely Bugs/ConstantComparison.ql \ No newline at end of file diff --git a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/options b/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/options deleted file mode 100644 index 75c39b4541ba..000000000000 --- a/csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/options +++ /dev/null @@ -1,2 +0,0 @@ -semmle-extractor-options: /nostdlib /noconfig -semmle-extractor-options: --load-sources-from-project:${testdir}/../../../resources/stubs/_frameworks/Microsoft.NETCore.App/Microsoft.NETCore.App.csproj From 29500c7eb7a9b0616fec66841ba2570e11209d48 Mon Sep 17 00:00:00 2001 From: Anders Schack-Mulligen Date: Tue, 31 Mar 2026 09:29:28 +0200 Subject: [PATCH 3/3] C#: Add change note. --- .../src/change-notes/2026-03-31-constantcondition-simplify.md | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 csharp/ql/src/change-notes/2026-03-31-constantcondition-simplify.md diff --git a/csharp/ql/src/change-notes/2026-03-31-constantcondition-simplify.md b/csharp/ql/src/change-notes/2026-03-31-constantcondition-simplify.md new file mode 100644 index 000000000000..a1051d4c00f4 --- /dev/null +++ b/csharp/ql/src/change-notes/2026-03-31-constantcondition-simplify.md @@ -0,0 +1,4 @@ +--- +category: majorAnalysis +--- +* The `cs/constant-condition` query has been simplified. The query no longer reports trivially constant conditions as they were found to generally be intentional. As a result, it should now produce fewer false positives. Additionally, the simplification means that it now reports all the results that `cs/constant-comparison` used to report, and as consequence, that query has been deleted.