Merge pull request #14224 from rdmarsh2/rdmarsh2/swift/nil-coalescing-cfg

Swift: CFG and data flow for nil coalescing operator
2026-04-24 16:25:15 +02:00 · 2023-10-04 09:43:31 -04:00
parent 3703c5626f 06da5fd05c
commit f7ca8e5b39
12 changed files with 246 additions and 8 deletions
--- a/swift/ql/lib/change-notes/2023-09-15-nil-coalescing.md
+++ b/swift/ql/lib/change-notes/2023-09-15-nil-coalescing.md
@@ -0,0 +1,5 @@
+---
+category: minorAnalysis
+---
+
+* The nil-coalescing operator `??` is now supported by the CFG construction and dataflow libraries.
--- a/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowElements.qll
+++ b/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowElements.qll
@@ -12,7 +12,8 @@ newtype TControlFlowElement =
  TPropertyObserverElement(Accessor observer, AssignExpr assign) {
    isPropertyObserverElement(observer, assign)
  } or
-  TKeyPathElement(KeyPathExpr expr)
+  TKeyPathElement(KeyPathExpr expr) or
+  TNilCoalescingTestElement(NilCoalescingExpr expr)

 predicate isLValue(Expr e) { any(AssignExpr assign).getDest() = e }

@@ -204,3 +205,15 @@ class ClosureElement extends ControlFlowElement, TClosureElement {

  override string toString() { result = expr.toString() }
 }
+
+class NilCoalescingElement extends ControlFlowElement, TNilCoalescingTestElement {
+  NilCoalescingExpr expr;
+
+  NilCoalescingElement() { this = TNilCoalescingTestElement(expr) }
+
+  override Location getLocation() { result = expr.getLocation() }
+
+  NilCoalescingExpr getAst() { result = expr }
+
+  override string toString() { result = "emptiness test for " + expr.toString() }
+}
--- a/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll
+++ b/swift/ql/lib/codeql/swift/controlflow/internal/ControlFlowGraphImpl.qll
@@ -1181,7 +1181,7 @@ module Exprs {
    }

    /**
-     * An autoclosure expression that is generated as part of a logical operation.
+     * An autoclosure expression that is generated as part of a logical operation or nil coalescing expression.
     *
     * This is needed because the Swift AST for `b1 && b2` is really syntactic sugar a function call:
     * ```swift
@@ -1190,10 +1190,13 @@ module Exprs {
     * So the `true` edge from `b1` cannot just go to `b2` since this is an implicit autoclosure.
     * To handle this dig into the autoclosure when it's an operand of a logical operator.
     */
-    private class LogicalAutoClosureTree extends AstPreOrderTree {
+    private class ShortCircuitingAutoClosureTree extends AstPreOrderTree {
      override AutoClosureExpr ast;

-      LogicalAutoClosureTree() { ast = any(LogicalOperation op).getAnOperand() }
+      ShortCircuitingAutoClosureTree() {
+        ast = any(LogicalOperation op).getAnOperand() or
+        ast = any(NilCoalescingExpr expr).getAnOperand()
+      }

      override predicate last(ControlFlowElement last, Completion c) {
        exists(Completion completion | astLast(ast.getReturn(), last, completion) |
@@ -1219,7 +1222,7 @@ module Exprs {
    private class AutoClosureTree extends AstLeafTree {
      override AutoClosureExpr ast;

-      AutoClosureTree() { not this instanceof LogicalAutoClosureTree }
+      AutoClosureTree() { not this instanceof ShortCircuitingAutoClosureTree }
    }
  }

@@ -1559,7 +1562,9 @@ module Exprs {
      // This one is handled in `LogicalNotTree`.
      not ast instanceof UnaryLogicalOperation and
      // These are handled in `LogicalOrTree` and `LogicalAndTree`.
-      not ast instanceof BinaryLogicalOperation
+      not ast instanceof BinaryLogicalOperation and
+      // This one is handled in `NilCoalescingTree`
+      not ast instanceof NilCoalescingExpr
    }

    final override ControlFlowElement getChildElement(int i) {
@@ -1583,6 +1588,38 @@ module Exprs {
    }
  }

+  private class NilCoalescingTestTree extends LeafTree, NilCoalescingElement { }
+
+  private class NilCoalescingTree extends AstPostOrderTree {
+    override NilCoalescingExpr ast;
+
+    final override predicate propagatesAbnormal(ControlFlowElement child) {
+      child.asAstNode() = ast.getAnOperand().getFullyConverted()
+    }
+
+    final override predicate first(ControlFlowElement first) {
+      astFirst(ast.getLeftOperand().getFullyConverted(), first)
+    }
+
+    final override predicate succ(ControlFlowElement pred, ControlFlowElement succ, Completion c) {
+      astLast(ast.getLeftOperand().getFullyConverted(), pred, c) and
+      c instanceof NormalCompletion and
+      succ.(NilCoalescingTestTree).getAst() = ast
+      or
+      pred.(NilCoalescingTestTree).getAst() = ast and
+      exists(EmptinessCompletion ec | c = ec | ec.isEmpty()) and
+      astFirst(ast.getRightOperand().getFullyConverted(), succ)
+      or
+      pred.(NilCoalescingTestTree).getAst() = ast and
+      exists(EmptinessCompletion ec | c = ec | not ec.isEmpty()) and
+      succ.asAstNode() = ast
+      or
+      astLast(ast.getRightOperand().getFullyConverted(), pred, c) and
+      c instanceof NormalCompletion and
+      succ.asAstNode() = ast
+    }
+  }
+
  private class LogicalAndTree extends AstPostOrderTree {
    override LogicalAndExpr ast;

--- a/swift/ql/lib/codeql/swift/elements/expr/NilCoalescingExpr.qll
+++ b/swift/ql/lib/codeql/swift/elements/expr/NilCoalescingExpr.qll
@@ -0,0 +1,13 @@
+/**
+ * Provides a class for the Swift nil-coalesing expr (`??`)
+ */
+
+private import codeql.swift.elements.expr.Expr
+private import codeql.swift.elements.expr.BinaryExpr
+
+/**
+ * A Swift nil-coalesing expr (`??`).
+ */
+class NilCoalescingExpr extends BinaryExpr {
+  NilCoalescingExpr() { this.getStaticTarget().getName() = "??(_:_:)" }
+}
--- a/swift/ql/lib/swift.qll
+++ b/swift/ql/lib/swift.qll
@@ -4,6 +4,7 @@ import codeql.swift.elements
 import codeql.swift.elements.expr.ArithmeticOperation
 import codeql.swift.elements.expr.BitwiseOperation
 import codeql.swift.elements.expr.LogicalOperation
+import codeql.swift.elements.expr.NilCoalescingExpr
 import codeql.swift.elements.expr.InitializerLookupExpr
 import codeql.swift.elements.expr.MethodCallExpr
 import codeql.swift.elements.expr.InitializerCallExpr
--- a/swift/ql/test/library-tests/ast/PrintAst.expected
+++ b/swift/ql/test/library-tests/ast/PrintAst.expected
@@ -3300,6 +3300,44 @@ cfg.swift:
 #  526|     Type = Int
 #  533| [ConcreteVarDecl] i
 #  533|     Type = Int
+#  538| [NamedFunction] testNilCoalescing(x:)
+#  538|     InterfaceType = (Int?) -> Int
+#  538|   getParam(0): [ParamDecl] x
+#  538|       Type = Int?
+#  538|   getBody(): [BraceStmt] { ... }
+#  539|     getElement(0): [ReturnStmt] return ...
+#  539|       getResult(): [BinaryExpr] ... ??(_:_:) ...
+#  539|         getFunction(): [DeclRefExpr] ??(_:_:)
+#  539|         getArgument(0): [Argument] : x
+#  539|           getExpr(): [DeclRefExpr] x
+#  539|         getArgument(1): [Argument] : { ... }
+#  539|           getExpr(): [AutoClosureExpr] { ... }
+#  539|             getBody(): [BraceStmt] { ... }
+#  539|               getElement(0): [ReturnStmt] return ...
+#  539|                 getResult(): [IntegerLiteralExpr] 0
+#  542| [NamedFunction] testNilCoalescing2(x:)
+#  542|     InterfaceType = (Bool?) -> Int
+#  542|   getParam(0): [ParamDecl] x
+#  542|       Type = Bool?
+#  542|   getBody(): [BraceStmt] { ... }
+#  543|     getElement(0): [IfStmt] if ... then { ... } else { ... }
+#  543|       getCondition(): [StmtCondition] StmtCondition
+#  543|         getElement(0): [ConditionElement] ... ??(_:_:) ...
+#  543|           getBoolean(): [BinaryExpr] ... ??(_:_:) ...
+#  543|             getFunction(): [DeclRefExpr] ??(_:_:)
+#  543|             getArgument(0): [Argument] : x
+#  543|               getExpr(): [DeclRefExpr] x
+#  543|             getArgument(1): [Argument] : { ... }
+#  543|               getExpr(): [AutoClosureExpr] { ... }
+#  543|                 getBody(): [BraceStmt] { ... }
+#  543|                   getElement(0): [ReturnStmt] return ...
+#  543|                     getResult(): [BooleanLiteralExpr] false
+#  543|       getThen(): [BraceStmt] { ... }
+#  544|         getElement(0): [ReturnStmt] return ...
+#  544|           getResult(): [IntegerLiteralExpr] 1
+#  545|       getElse(): [BraceStmt] { ... }
+#  546|         getElement(0): [ReturnStmt] return ...
+#  546|           getResult(): [IntegerLiteralExpr] 0
 declarations.swift:
 #    1| [StructDecl] Foo
 #    2|   getMember(0): [PatternBindingDecl] var ... = ...
--- a/swift/ql/test/library-tests/ast/cfg.swift
+++ b/swift/ql/test/library-tests/ast/cfg.swift
@@ -534,3 +534,15 @@ func testAsyncFor () async {
        print(i)
    }
 }
+
+func testNilCoalescing(x: Int?) -> Int {
+  return x ?? 0
+}
+
+func testNilCoalescing2(x: Bool?) -> Int {
+  if x ?? false {
+    return 1
+  } else {
+    return 0
+  }
+}
--- a/swift/ql/test/library-tests/controlflow/graph/Cfg.expected
+++ b/swift/ql/test/library-tests/controlflow/graph/Cfg.expected
@@ -6180,3 +6180,99 @@ cfg.swift:

 #  534| i
 #-----|  -> (Any) ...
+
+#  538| enter testNilCoalescing(x:)
+#-----|  -> testNilCoalescing(x:)
+
+#  538| exit testNilCoalescing(x:)
+
+#  538| exit testNilCoalescing(x:) (abnormal)
+#-----|  -> exit testNilCoalescing(x:)
+
+#  538| exit testNilCoalescing(x:) (normal)
+#-----|  -> exit testNilCoalescing(x:)
+
+#  538| testNilCoalescing(x:)
+#-----|  -> x
+
+#  538| x
+#-----|  -> x
+
+#  539| return ...
+#-----| return -> exit testNilCoalescing(x:) (normal)
+
+#  539| x
+#-----|  -> emptiness test for ... ??(_:_:) ...
+
+#  539| ... ??(_:_:) ...
+#-----| exception -> exit testNilCoalescing(x:) (abnormal)
+#-----|  -> return ...
+
+#  539| emptiness test for ... ??(_:_:) ...
+#-----| non-empty -> ... ??(_:_:) ...
+#-----| empty -> { ... }
+
+#  539| 0
+#-----|  -> return ...
+
+#  539| return ...
+#-----|  -> ... ??(_:_:) ...
+
+#  539| { ... }
+#-----|  -> 0
+
+#  542| enter testNilCoalescing2(x:)
+#-----|  -> testNilCoalescing2(x:)
+
+#  542| exit testNilCoalescing2(x:)
+
+#  542| exit testNilCoalescing2(x:) (abnormal)
+#-----|  -> exit testNilCoalescing2(x:)
+
+#  542| exit testNilCoalescing2(x:) (normal)
+#-----|  -> exit testNilCoalescing2(x:)
+
+#  542| testNilCoalescing2(x:)
+#-----|  -> x
+
+#  542| x
+#-----|  -> if ... then { ... } else { ... }
+
+#  543| if ... then { ... } else { ... }
+#-----|  -> StmtCondition
+
+#  543| x
+#-----|  -> emptiness test for ... ??(_:_:) ...
+
+#  543| ... ??(_:_:) ...
+#-----| exception -> exit testNilCoalescing2(x:) (abnormal)
+#-----| true -> 1
+#-----| false -> 0
+
+#  543| StmtCondition
+#-----|  -> x
+
+#  543| emptiness test for ... ??(_:_:) ...
+#-----| non-empty -> ... ??(_:_:) ...
+#-----| empty -> { ... }
+
+#  543| false
+#-----|  -> return ...
+
+#  543| return ...
+#-----|  -> ... ??(_:_:) ...
+
+#  543| { ... }
+#-----|  -> false
+
+#  544| return ...
+#-----| return -> exit testNilCoalescing2(x:) (normal)
+
+#  544| 1
+#-----|  -> return ...
+
+#  546| return ...
+#-----| return -> exit testNilCoalescing2(x:) (normal)
+
+#  546| 0
+#-----|  -> return ...
--- a/swift/ql/test/library-tests/controlflow/graph/cfg.swift
+++ b/swift/ql/test/library-tests/controlflow/graph/cfg.swift
@@ -534,3 +534,15 @@ func testAsyncFor () async {
        print(i)
    }
 }
+
+func testNilCoalescing(x: Int?) -> Int {
+  return x ?? 0
+}
+
+func testNilCoalescing2(x: Bool?) -> Int {
+  if x ?? false {
+    return 1
+  } else {
+    return 0
+  }
+}
--- a/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected
+++ b/swift/ql/test/library-tests/dataflow/dataflow/DataFlow.expected
@@ -145,6 +145,8 @@ edges
 | test.swift:270:15:270:22 | call to source() | test.swift:270:15:270:31 | call to signum() |
 | test.swift:271:15:271:16 | ...? | test.swift:271:15:271:25 | call to signum() |
 | test.swift:271:15:271:25 | call to signum() | test.swift:271:15:271:25 | OptionalEvaluationExpr |
+| test.swift:275:20:275:27 | call to source() | test.swift:275:15:275:27 | ... ??(_:_:) ... |
+| test.swift:277:20:277:27 | call to source() | test.swift:277:15:277:27 | ... ??(_:_:) ... |
 | test.swift:279:26:279:26 | x [some:0] | test.swift:279:26:279:27 | ...! |
 | test.swift:279:26:279:27 | ...! | test.swift:279:15:279:31 | ... ? ... : ... |
 | test.swift:280:26:280:26 | x [some:0] | test.swift:280:26:280:27 | ...! |
@@ -713,6 +715,9 @@ nodes
 | test.swift:271:15:271:25 | call to signum() | semmle.label | call to signum() |
 | test.swift:274:15:274:20 | ... ??(_:_:) ... | semmle.label | ... ??(_:_:) ... |
 | test.swift:275:15:275:27 | ... ??(_:_:) ... | semmle.label | ... ??(_:_:) ... |
+| test.swift:275:20:275:27 | call to source() | semmle.label | call to source() |
+| test.swift:277:15:277:27 | ... ??(_:_:) ... | semmle.label | ... ??(_:_:) ... |
+| test.swift:277:20:277:27 | call to source() | semmle.label | call to source() |
 | test.swift:279:15:279:31 | ... ? ... : ... | semmle.label | ... ? ... : ... |
 | test.swift:279:26:279:26 | x [some:0] | semmle.label | x [some:0] |
 | test.swift:279:26:279:27 | ...! | semmle.label | ...! |
@@ -1262,6 +1267,8 @@ subpaths
 | test.swift:271:15:271:25 | OptionalEvaluationExpr | test.swift:259:12:259:19 | call to source() | test.swift:271:15:271:25 | OptionalEvaluationExpr | result |
 | test.swift:274:15:274:20 | ... ??(_:_:) ... | test.swift:259:12:259:19 | call to source() | test.swift:274:15:274:20 | ... ??(_:_:) ... | result |
 | test.swift:275:15:275:27 | ... ??(_:_:) ... | test.swift:259:12:259:19 | call to source() | test.swift:275:15:275:27 | ... ??(_:_:) ... | result |
+| test.swift:275:15:275:27 | ... ??(_:_:) ... | test.swift:275:20:275:27 | call to source() | test.swift:275:15:275:27 | ... ??(_:_:) ... | result |
+| test.swift:277:15:277:27 | ... ??(_:_:) ... | test.swift:277:20:277:27 | call to source() | test.swift:277:15:277:27 | ... ??(_:_:) ... | result |
 | test.swift:279:15:279:31 | ... ? ... : ... | test.swift:259:12:259:19 | call to source() | test.swift:279:15:279:31 | ... ? ... : ... | result |
 | test.swift:280:15:280:38 | ... ? ... : ... | test.swift:259:12:259:19 | call to source() | test.swift:280:15:280:38 | ... ? ... : ... | result |
 | test.swift:280:15:280:38 | ... ? ... : ... | test.swift:280:31:280:38 | call to source() | test.swift:280:15:280:38 | ... ? ... : ... | result |
--- a/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected
+++ b/swift/ql/test/library-tests/dataflow/dataflow/LocalFlow.expected
@@ -247,15 +247,19 @@
 | test.swift:272:15:272:25 | call to signum() | test.swift:272:15:272:25 | OptionalEvaluationExpr |
 | test.swift:274:15:274:15 | x | test.swift:274:15:274:20 | ... ??(_:_:) ... |
 | test.swift:274:15:274:15 | x | test.swift:275:15:275:15 | x |
+| test.swift:274:20:274:20 | 0 | test.swift:274:15:274:20 | ... ??(_:_:) ... |
 | test.swift:274:20:274:20 | { ... } | test.swift:274:15:274:20 | ... ??(_:_:) ... |
 | test.swift:275:15:275:15 | x | test.swift:275:15:275:27 | ... ??(_:_:) ... |
 | test.swift:275:15:275:15 | x | test.swift:279:15:279:15 | x |
+| test.swift:275:20:275:27 | call to source() | test.swift:275:15:275:27 | ... ??(_:_:) ... |
 | test.swift:275:20:275:27 | { ... } | test.swift:275:15:275:27 | ... ??(_:_:) ... |
 | test.swift:276:15:276:15 | y | test.swift:276:15:276:20 | ... ??(_:_:) ... |
 | test.swift:276:15:276:15 | y | test.swift:277:15:277:15 | y |
+| test.swift:276:20:276:20 | 0 | test.swift:276:15:276:20 | ... ??(_:_:) ... |
 | test.swift:276:20:276:20 | { ... } | test.swift:276:15:276:20 | ... ??(_:_:) ... |
 | test.swift:277:15:277:15 | y | test.swift:277:15:277:27 | ... ??(_:_:) ... |
 | test.swift:277:15:277:15 | y | test.swift:281:15:281:15 | y |
+| test.swift:277:20:277:27 | call to source() | test.swift:277:15:277:27 | ... ??(_:_:) ... |
 | test.swift:277:20:277:27 | { ... } | test.swift:277:15:277:27 | ... ??(_:_:) ... |
 | test.swift:279:15:279:15 | x | test.swift:279:26:279:26 | x |
 | test.swift:279:15:279:15 | x | test.swift:280:15:280:15 | x |
--- a/swift/ql/test/library-tests/dataflow/dataflow/test.swift
+++ b/swift/ql/test/library-tests/dataflow/dataflow/test.swift
@@ -272,9 +272,9 @@ func test_optionals(y: Int?) {
    sink(opt: y?.signum())

    sink(arg: x ?? 0) // $ flow=259
-    sink(arg: x ?? source()) // $ flow=259 MISSING: flow=276
+    sink(arg: x ?? source()) // $ flow=259 flow=275
    sink(arg: y ?? 0)
-    sink(arg: y ?? source()) // $ MISSING: flow=278
+    sink(arg: y ?? source()) // $ flow=277

    sink(arg: x != nil ? x! : 0) // $ flow=259
    sink(arg: x != nil ? x! : source()) // $ flow=259 flow=280