Merge pull request #22595 from asgerf/unified/binary-expr-assignment

Unified: Fix AST, CFG, and data flow rules for assignments

Author: asgerf

shared/ssa/codeql/ssa/Ssa.qll

@@ -1419,6 +1419,22 @@ module Make<
      */
     default predicate allowFlowIntoUncertainDef(UncertainWriteDefinition def) { none() }
 
+    /**
+     * Holds if the post-update node corresponding to the given `read` occurs at `bb,i`, meaning
+     * it will propagate to the next use (strictly) after that point in the CFG.
+     *
+     * The default is to use the CFG node associated with the read itself, meaning the post-update
+     * always flows to the next use. The default can however lead to spurious flow in cases like:
+     * ```
+     * x.f = foo(x)
+     * ```
+     * where the post-update node for `x` on the left-hand side flows into the `x` on the right-hand side.
+     *
+     * NOTE: When implementing this predicate, you must ensure that `variableRead` is defined to contain
+     * a synthetic read of this variable at `bb,i`.
+     */
+    default predicate postUpdateCfgNode(Expr read, BasicBlock bb, int i) { read.hasCfgNode(bb, i) }
+
     /** An abstract value that a `Guard` may evaluate to. */
     class GuardValue {
       /** Gets a textual representation of this value. */
@@ -1892,6 +1908,13 @@ module Make<
       override string toString() { result = "[input] " + def_.toString() }
     }
 
+    private predicate postUpdateNodeAt(
+      ExprPostUpdateNode node, BasicBlock bb, int i, SourceVariable v
+    ) {
+      node.getPreUpdateNode().(ReadNode).readsAt(_, _, v) and
+      DfInput::postUpdateCfgNode(node.getExpr(), bb, i)
+    }
+
     /**
      * Holds if `nodeFrom` corresponds to the reference to `v` at index `i` in
      * `bb`. The boolean `isUseStep` indicates whether `nodeFrom` is an actual
@@ -1907,7 +1930,10 @@ module Make<
         isUseStep = false
       )
       or
-      [nodeFrom, nodeFrom.(ExprPostUpdateNode).getPreUpdateNode()].(ReadNode).readsAt(bb, i, v) and
+      nodeFrom.(ReadNode).readsAt(bb, i, v) and
+      isUseStep = true
+      or
+      postUpdateNodeAt(nodeFrom, bb, i, v) and
       isUseStep = true
     }
 

unified/extractor/ast_types.yml

@@ -23,8 +23,6 @@ supertypes:
     - type_cast_expr
     - type_test_expr
     - if_expr
-    - assign_expr
-    - compound_assign_expr
     - pattern_guard_expr
     - empty_expr
     - block
@@ -159,17 +157,6 @@ named:
   unresolved_operator_sequence:
     element*: expr_or_operator
 
-  # Plain assignment
-  assign_expr:
-    target: expr
-    value: expr
-
-  # Compound assignment
-  compound_assign_expr:
-    target: expr
-    operator: infix_operator
-    value: expr
-
   # A function or method call, such as `f(x)` or `obj.m(x)`.
   #
   # Method calls are represented as a call whose `function` is a `member_access_expr`.

unified/extractor/src/languages/swift/swift.rs

@@ -129,13 +129,6 @@ fn member_chain(
     result
 }
 
-/// Compound-assignment operator spellings (`+=`, `<<=`, ...). Used to tell a
-/// compound assignment from an ordinary binary application, both of which
-/// arrive as a `binaryOperator`-based `infixOperatorExpr`.
-const COMPOUND_ASSIGN_OPS: &[&str] = &[
-    "+=", "-=", "*=", "/=", "%=", "<<=", ">>=", "&=", "|=", "^=", "&+=", "&-=", "&*=",
-];
-
 fn translation_rules() -> Vec<Rule<SwiftContext>> {
     vec![
         // ---- Top-level ----
@@ -243,29 +236,21 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
         // operator leaf. Used by `infixOperatorExpr` (folded) and `sequenceExpr`
         // (unresolved).
         rule!((binaryOperatorExpr operator: @op) => (infix_operator #{op})),
-        // Compound assignment (`x += y`) vs. an ordinary binary application
-        // (`a + b`): both are `binaryOperator`-based `infixOperatorExpr`s,
-        // distinguishable only by the operator's spelling. The query engine
-        // can't match on token text, so a small Rust block reads the spelling
-        // and routes to `compound_assign_expr` or `binary_expr`. The operator
-        // is captured raw (`@@op`) to read its spelling.
+        // A `binaryOperator`-based `infixOperatorExpr` represents both ordinary
+        // binary applications (`a + b`) and compound assignments (`x += y`).
+        // Both have the same target AST shape; the QL library distinguishes
+        // assignments by the operator spelling.
         rule!(
             (infixOperatorExpr leftOperand: @l operator: (binaryOperatorExpr) @@op rightOperand: @r)
             =>
-            expr {
-                if COMPOUND_ASSIGN_OPS.contains(&ctx.source_text(op).as_str()) {
-                    tree!((compound_assign_expr target: {l} operator: (infix_operator #{op}) value: {r}))
-                } else {
-                    tree!((binary_expr left: {l} operator: (infix_operator #{op}) right: {r}))
-                }
-            }
+            (binary_expr left: {l} operator: (infix_operator #{op}) right: {r})
         ),
-        // Plain assignment (`x = y`). In a folded chain the `=` is an
-        // `assignmentExpr` node (distinct from other operators), matched by kind.
+        // Plain assignment (`x = y`). In a folded chain the `=` is represented
+        // by an `assignmentExpr` node rather than a `binaryOperatorExpr`.
         rule!(
-            (infixOperatorExpr leftOperand: @l operator: (assignmentExpr) rightOperand: @r)
+            (infixOperatorExpr leftOperand: @l operator: (assignmentExpr) @op rightOperand: @r)
             =>
-            (assign_expr target: {l} value: {r})
+            (binary_expr left: {l} operator: (infix_operator #{op}) right: {r})
         ),
         // In an unresolved `sequenceExpr` (below) the operator positions are not
         // only `binaryOperatorExpr`s: a plain assignment (`=`), an `as`/`is` cast

unified/extractor/tests/corpus/swift/control-flow/if-case-let-with-shadowing-in-condition-value.output

@@ -151,7 +151,7 @@ top_level
           then:
             block
               stmt:
-                compound_assign_expr
-                  target: identifier "y"
+                binary_expr
+                  left: identifier "y"
                   operator: infix_operator "+="
-                  value: int_literal "1"
+                  right: int_literal "1"

unified/extractor/tests/corpus/swift/expressions/array-type-constructor.output

@@ -167,6 +167,7 @@ top_level
                       body:
                         block
                           stmt:
-                            assign_expr
-                              target: identifier "count"
-                              value: int_literal "0"
+                            binary_expr
+                              left: identifier "count"
+                              operator: infix_operator "="
+                              right: int_literal "0"

unified/extractor/tests/corpus/swift/functions/function-with-inout-parameter.output

@@ -71,7 +71,7 @@ top_level
           body:
             block
               stmt:
-                compound_assign_expr
-                  target: identifier "x"
+                binary_expr
+                  left: identifier "x"
                   operator: infix_operator "+="
-                  value: int_literal "1"
+                  right: int_literal "1"

unified/extractor/tests/corpus/swift/loops/repeat-while-loop.output

@@ -51,10 +51,10 @@ top_level
           body:
             block
               stmt:
-                compound_assign_expr
-                  target: identifier "x"
+                binary_expr
+                  left: identifier "x"
                   operator: infix_operator "-="
-                  value: int_literal "1"
+                  right: int_literal "1"
           condition:
             binary_expr
               left: identifier "x"

unified/extractor/tests/corpus/swift/loops/while-loop.output

@@ -57,7 +57,7 @@ top_level
           body:
             block
               stmt:
-                compound_assign_expr
-                  target: identifier "x"
+                binary_expr
+                  left: identifier "x"
                   operator: infix_operator "-="
-                  value: int_literal "1"
+                  right: int_literal "1"

unified/extractor/tests/corpus/swift/types/class-with-initializer.output

@@ -106,9 +106,10 @@ top_level
               body:
                 block
                   stmt:
-                    assign_expr
-                      target:
+                    binary_expr
+                      left:
                         member_access_expr
                           base: identifier "self"
                           member_name_node: identifier "x"
-                      value: identifier "x"
+                      operator: infix_operator "="
+                      right: identifier "x"

unified/extractor/tests/corpus/swift/types/class-with-method.output

@@ -90,7 +90,7 @@ top_level
               body:
                 block
                   stmt:
-                    compound_assign_expr
-                      target: identifier "n"
+                    binary_expr
+                      left: identifier "n"
                       operator: infix_operator "+="
-                      value: int_literal "1"
+                      right: int_literal "1"