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

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.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 {