C#: Simplify the ConstantCondition query.

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

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

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

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

csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql

@@ -41,7 +41,9 @@ module ConstCondInput implements ConstCond::InputSig<ControlFlow::BasicBlock> {
 module ConstCondImpl = ConstCond::Make<Location, Cfg, ConstCondInput>;
 
 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,72 +110,38 @@ 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. */
 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

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

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

csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.cs → 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
     }