Merge pull request #20558 from aschackmull/csharp/guards3

C#: Instantiate shared Guards and shared ControlFlowReachability and replace nullness

Author: aschackmull

cpp/ql/test/library-tests/controlflow/guards/GuardsControl.expected

@@ -115,7 +115,6 @@
 | test.c:127:9:127:9 | 1 | not 0 | test.c:131:10:132:16 | { ... } |
 | test.c:131:7:131:7 | b | not 0 | test.c:131:10:132:16 | { ... } |
 | test.c:131:7:131:7 | b | true | test.c:131:10:132:16 | { ... } |
-| test.c:137:7:137:7 | 0 | 0 | test.c:142:3:136:10 | return ... |
 | test.c:137:7:137:7 | 0 | false | test.c:142:3:136:10 | return ... |
 | test.c:145:16:145:16 | x | 0 | test.c:146:11:147:9 | { ... } |
 | test.c:146:7:146:8 | ! ... | true | test.c:146:11:147:9 | { ... } |

csharp/ql/lib/change-notes/2025-10-03-nullness.md

@@ -0,0 +1,4 @@
+---
+category: majorAnalysis
+---
+* The representation of the C# control-flow graph has been significantly changed. This has minor effects on a wide range of queries including both minor improvements and minor regressions, for example, improved precision has been observed for `cs/inefficient-containskey` and `cs/stringbuilder-creation-in-loop`. Two queries stand out as being significantly affected with great improvements: `cs/dereferenced-value-may-be-null` has been completely rewritten which removes a very significant number of false positives. Furthermore, `cs/constant-condition` has been updated to report many new results - these new results are primarily expected to be true positives, but a few new false positives are expected as well. As part of these changes, `cs/dereferenced-value-may-be-null` has been changed from a `path-problem` query to a `problem` query, so paths are no longer reported for this query.

csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowElement.qll

@@ -40,6 +40,12 @@ class ControlFlowElement extends ExprOrStmtParent, @control_flow_element {
    */
   Nodes::ElementNode getAControlFlowNode() { result.getAstNode() = this }
 
+  /** Gets the control flow node for this element. */
+  ControlFlow::Node getControlFlowNode() { result.getAstNode() = this }
+
+  /** Gets the basic block in which this element occurs. */
+  BasicBlock getBasicBlock() { result = this.getAControlFlowNode().getBasicBlock() }
+
   /**
    * Gets a first control flow node executed within this element.
    */

csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowGraph.qll

@@ -251,6 +251,9 @@ module ControlFlow {
       }
     }
 
+    /** A control flow node indicating normal termination of a callable. */
+    class NormalExitNode extends AnnotatedExitNode instanceof Impl::NormalExitNode { }
+
     /** A node for a callable exit point. */
     class ExitNode extends Node instanceof Impl::ExitNode {
       /** Gets the callable that this exit applies to. */
@@ -292,13 +295,7 @@ module ControlFlow {
 
     class Split = Splitting::Split;
 
-    class FinallySplit = Splitting::FinallySplitting::FinallySplit;
-
     class ExceptionHandlerSplit = Splitting::ExceptionHandlerSplitting::ExceptionHandlerSplit;
-
-    class BooleanSplit = Splitting::BooleanSplitting::BooleanSplit;
-
-    class LoopSplit = Splitting::LoopSplitting::LoopSplit;
   }
 
   class BasicBlock = BBs::BasicBlock;

csharp/ql/lib/semmle/code/csharp/controlflow/ControlFlowReachability.qll

@@ -0,0 +1,57 @@
+/**
+ * Provides an implementation of local (intraprocedural) control flow reachability.
+ */
+
+import csharp
+private import codeql.controlflow.ControlFlow
+private import semmle.code.csharp.controlflow.BasicBlocks
+private import semmle.code.csharp.controlflow.Guards as Guards
+private import semmle.code.csharp.ExprOrStmtParent
+
+private module ControlFlowInput implements
+  InputSig<Location, ControlFlow::Node, ControlFlow::BasicBlock>
+{
+  private import csharp as CS
+
+  AstNode getEnclosingAstNode(ControlFlow::Node node) {
+    node.getAstNode() = result
+    or
+    not exists(node.getAstNode()) and result = node.getEnclosingCallable()
+  }
+
+  class AstNode = ExprOrStmtParent;
+
+  AstNode getParent(AstNode node) { result = node.getParent() }
+
+  class FinallyBlock extends AstNode {
+    FinallyBlock() { any(TryStmt try).getFinally() = this }
+  }
+
+  class Expr = CS::Expr;
+
+  class SourceVariable = Ssa::SourceVariable;
+
+  class SsaDefinition = Ssa::Definition;
+
+  class SsaWriteDefinition extends SsaDefinition instanceof Ssa::ExplicitDefinition {
+    Expr getDefinition() { result = super.getADefinition().getSource() }
+  }
+
+  class SsaPhiNode = Ssa::PhiNode;
+
+  class SsaUncertainDefinition = Ssa::UncertainDefinition;
+
+  class GuardValue = Guards::GuardValue;
+
+  predicate ssaControlsBranchEdge(SsaDefinition def, BasicBlock bb1, BasicBlock bb2, GuardValue v) {
+    Guards::Guards::ssaControlsBranchEdge(def, bb1, bb2, v)
+  }
+
+  predicate ssaControls(SsaDefinition def, BasicBlock bb, GuardValue v) {
+    Guards::Guards::ssaControls(def, bb, v)
+  }
+
+  import Guards::Guards::InternalUtil
+}
+
+module ControlFlowReachability = Make<Location, Cfg, ControlFlowInput>;

csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll

@@ -13,6 +13,284 @@ private import semmle.code.csharp.frameworks.System
 private import semmle.code.csharp.frameworks.system.Linq
 private import semmle.code.csharp.frameworks.system.Collections
 private import semmle.code.csharp.frameworks.system.collections.Generic
+private import codeql.controlflow.Guards as SharedGuards
+
+private module GuardsInput implements
+  SharedGuards::InputSig<Location, ControlFlow::Node, ControlFlow::BasicBlock>
+{
+  private import csharp as CS
+
+  class NormalExitNode = ControlFlow::Nodes::NormalExitNode;
+
+  class AstNode = ControlFlowElement;
+
+  class Expr = CS::Expr;
+
+  private newtype TConstantValue =
+    TStringValue(string value) { any(StringLiteral s).getValue() = value }
+
+  class ConstantValue extends TConstantValue {
+    string toString() { this = TStringValue(result) }
+  }
+
+  abstract class ConstantExpr extends Expr {
+    predicate isNull() { none() }
+
+    boolean asBooleanValue() { none() }
+
+    int asIntegerValue() { none() }
+
+    ConstantValue asConstantValue() { none() }
+  }
+
+  private class NullConstant extends ConstantExpr {
+    NullConstant() { nullValueImplied(this) }
+
+    override predicate isNull() { any() }
+  }
+
+  private class BooleanConstant extends ConstantExpr instanceof BoolLiteral {
+    override boolean asBooleanValue() { result = super.getBoolValue() }
+  }
+
+  private predicate intConst(Expr e, int i) {
+    e.getValue().toInt() = i and
+    (
+      e.getType() instanceof Enum
+      or
+      e.getType() instanceof IntegralType
+    )
+  }
+
+  private class IntegerConstant extends ConstantExpr {
+    IntegerConstant() { intConst(this, _) }
+
+    override int asIntegerValue() { intConst(this, result) }
+  }
+
+  private class EnumConst extends ConstantExpr {
+    EnumConst() { this.getType() instanceof Enum and this.hasValue() }
+
+    override int asIntegerValue() { result = this.getValue().toInt() }
+  }
+
+  private class StringConstant extends ConstantExpr instanceof StringLiteral {
+    override ConstantValue asConstantValue() { result = TStringValue(this.getValue()) }
+  }
+
+  class NonNullExpr extends Expr {
+    NonNullExpr() { nonNullValueImplied(this) }
+  }
+
+  class Case extends AstNode instanceof CS::Case {
+    Expr getSwitchExpr() { super.getExpr() = result }
+
+    predicate isDefaultCase() { this instanceof DefaultCase }
+
+    ConstantExpr asConstantCase() { super.getPattern() = result }
+
+    private predicate hasEdge(BasicBlock bb1, BasicBlock bb2, MatchingCompletion c) {
+      exists(PatternExpr pe |
+        super.getPattern() = pe and
+        c.isValidFor(pe) and
+        bb1.getLastNode() = pe.getAControlFlowNode() and
+        bb1.getASuccessor(c.getAMatchingSuccessorType()) = bb2
+      )
+    }
+
+    predicate matchEdge(BasicBlock bb1, BasicBlock bb2) {
+      exists(MatchingCompletion c | this.hasEdge(bb1, bb2, c) and c.isMatch())
+    }
+
+    predicate nonMatchEdge(BasicBlock bb1, BasicBlock bb2) {
+      exists(MatchingCompletion c | this.hasEdge(bb1, bb2, c) and c.isNonMatch())
+    }
+  }
+
+  abstract private class BinExpr extends BinaryOperation { }
+
+  class AndExpr extends BinExpr {
+    AndExpr() {
+      this instanceof LogicalAndExpr or
+      this instanceof BitwiseAndExpr
+    }
+  }
+
+  class OrExpr extends BinExpr {
+    OrExpr() {
+      this instanceof LogicalOrExpr or
+      this instanceof BitwiseOrExpr
+    }
+  }
+
+  class NotExpr = LogicalNotExpr;
+
+  class IdExpr extends Expr {
+    IdExpr() { this instanceof AssignExpr or this instanceof CastExpr }
+
+    Expr getEqualChildExpr() {
+      result = this.(AssignExpr).getRValue()
+      or
+      result = this.(CastExpr).getExpr()
+    }
+  }
+
+  predicate equalityTest(Expr eqtest, Expr left, Expr right, boolean polarity) {
+    exists(ComparisonTest ct |
+      ct.getExpr() = eqtest and
+      ct.getFirstArgument() = left and
+      ct.getSecondArgument() = right
+    |
+      ct.getComparisonKind().isEquality() and polarity = true
+      or
+      ct.getComparisonKind().isInequality() and polarity = false
+    )
+    or
+    exists(IsExpr ie, PatternExpr pat |
+      ie = eqtest and
+      ie.getExpr() = left and
+      ie.getPattern() = pat
+    |
+      right = pat.(ConstantPatternExpr) and
+      polarity = true
+      or
+      right = pat.(NotPatternExpr).getPattern().(ConstantPatternExpr) and
+      polarity = false
+    )
+  }
+
+  class ConditionalExpr = CS::ConditionalExpr;
+
+  class Parameter = CS::Parameter;
+
+  private int parameterPosition() { result in [-1, any(Parameter p).getPosition()] }
+
+  class ParameterPosition extends int {
+    ParameterPosition() { this = parameterPosition() }
+  }
+
+  class ArgumentPosition extends int {
+    ArgumentPosition() { this = parameterPosition() }
+  }
+
+  pragma[inline]
+  predicate parameterMatch(ParameterPosition ppos, ArgumentPosition apos) { ppos = apos }
+
+  final private class FinalCallable = Callable;
+
+  class NonOverridableMethod extends FinalCallable {
+    NonOverridableMethod() { not this.(Overridable).isOverridableOrImplementable() }
+
+    Parameter getParameter(ParameterPosition ppos) {
+      super.getParameter(ppos) = result and
+      not result.isParams()
+    }
+
+    Expr getAReturnExpr() { this.canReturn(result) }
+  }
+
+  class NonOverridableMethodCall extends Expr instanceof Call {
+    NonOverridableMethod getMethod() { super.getTarget().getUnboundDeclaration() = result }
+
+    Expr getArgument(ArgumentPosition apos) {
+      result = super.getArgumentForParameter(any(Parameter p | p.getPosition() = apos))
+    }
+  }
+}
+
+private module GuardsImpl = SharedGuards::Make<Location, Cfg, GuardsInput>;
+
+class GuardValue = GuardsImpl::GuardValue;
+
+private module LogicInput implements GuardsImpl::LogicInputSig {
+  class SsaDefinition extends Ssa::Definition {
+    Expr getARead() { super.getARead() = result }
+  }
+
+  class SsaWriteDefinition extends SsaDefinition instanceof Ssa::ExplicitDefinition {
+    Expr getDefinition() { result = super.getADefinition().getSource() }
+  }
+
+  class SsaPhiNode extends SsaDefinition instanceof Ssa::PhiNode {
+    predicate hasInputFromBlock(SsaDefinition inp, BasicBlock bb) {
+      super.hasInputFromBlock(inp, bb)
+    }
+  }
+
+  predicate parameterDefinition(Parameter p, SsaDefinition def) {
+    def.(Ssa::ImplicitParameterDefinition).getParameter() = p
+  }
+
+  predicate additionalNullCheck(GuardsImpl::PreGuard guard, GuardValue val, Expr e, boolean isNull) {
+    // Comparison with a non-`null` value, for example `x?.Length > 0`
+    exists(ComparisonTest ct, ComparisonKind ck, Expr arg | ct.getExpr() = guard |
+      e instanceof DereferenceableExpr and
+      ct.getAnArgument() = e and
+      ct.getAnArgument() = arg and
+      arg = any(NullValue nv | nv.isNonNull()).getAnExpr() and
+      ck = ct.getComparisonKind() and
+      e != arg and
+      isNull = false and
+      not ck.isEquality() and
+      not ck.isInequality() and
+      val.asBooleanValue() = true
+    )
+    or
+    // Call to `string.IsNullOrEmpty()` or `string.IsNullOrWhiteSpace()`
+    exists(MethodCall mc, string name | guard = mc |
+      mc.getTarget() = any(SystemStringClass c).getAMethod(name) and
+      name.regexpMatch("IsNullOr(Empty|WhiteSpace)") and
+      mc.getArgument(0) = e and
+      val.asBooleanValue() = false and
+      isNull = false
+    )
+    or
+    guard =
+      any(PatternMatch pm |
+        e instanceof DereferenceableExpr and
+        e = pm.getExpr() and
+        (
+          val.asBooleanValue().booleanNot() = patternMatchesNull(pm.getPattern()) and
+          isNull = false
+          or
+          exists(TypePatternExpr tpe |
+            // E.g. `x is string` where `x` has type `string`
+            typePattern(guard, tpe, tpe.getCheckedType()) and
+            val.asBooleanValue() = false and
+            isNull = true
+          )
+        )
+      )
+    or
+    e.(DereferenceableExpr).hasNullableType() and
+    guard =
+      any(PropertyAccess pa |
+        pa.getQualifier() = e and
+        pa.getTarget().hasName("HasValue") and
+        val.asBooleanValue().booleanNot() = isNull
+      )
+  }
+
+  predicate additionalImpliesStep(
+    GuardsImpl::PreGuard g1, GuardValue v1, GuardsImpl::PreGuard g2, GuardValue v2
+  ) {
+    g1 instanceof DereferenceableExpr and
+    g1 = getNullEquivParent(g2) and
+    v1.isNullness(_) and
+    v2 = v1
+    or
+    g1 instanceof DereferenceableExpr and
+    g2 = getANullImplyingChild(g1) and
+    v1.isNonNullValue() and
+    v2 = v1
+    or
+    g2 = g1.(NullCoalescingExpr).getAnOperand() and
+    v1.isNullValue() and
+    v2 = v1
+  }
+}
+
+module Guards = GuardsImpl::Logic<LogicInput>;
 
 /** An expression whose value may control the execution of another element. */
 class Guard extends Expr {