Merge pull request github#13284 from github/redsun82/swift-remove-property-wrapper-inconsistencies

Swift: remove some AST and CFG inconsistencies

Author: redsun82

swift/ql/.generated.list

@@ -0,0 +1,5 @@
+---
+category: fix
+---
+
+* Fixed a number of inconsistencies in the abstract syntax tree (AST) and in the control-flow graph (CFG). This may lead to more results in queries that use these libraries, or libraries that depend on them (such as dataflow).

swift/ql/.gitattributes

@@ -111,31 +111,21 @@ module Stmts {
 
     override predicate propagatesAbnormal(ControlFlowElement node) { none() }
 
-    private predicate isBodyOfTapExpr() { any(TapExpr tap).getBody() = ast }
-
-    // Note: If the brace statement is the body of a `TapExpr`, the first element is the variable
-    // declaration (see https://github.com/apple/swift/blob/main/include/swift/AST/Expr.h#L848)
-    // that's initialized by the `TapExpr`. In `TapExprTree` we've already visited this declaration,
-    // along with its initializer. So we skip the first element here.
-    private AstNode getFirstElement() {
-      if this.isBodyOfTapExpr() then result = ast.getElement(1) else result = ast.getFirstElement()
-    }
-
     override predicate first(ControlFlowElement first) {
       this.firstInner(first)
       or
-      not exists(this.getFirstElement()) and first.asAstNode() = ast
+      not exists(ast.getFirstElement()) and first.asAstNode() = ast
     }
 
     override predicate last(ControlFlowElement last, Completion c) {
       this.lastInner(last, c)
       or
-      not exists(this.getFirstElement()) and
+      not exists(ast.getFirstElement()) and
       last.asAstNode() = ast and
       c instanceof SimpleCompletion
     }
 
-    predicate firstInner(ControlFlowElement first) { astFirst(this.getFirstElement(), first) }
+    predicate firstInner(ControlFlowElement first) { astFirst(ast.getFirstElement(), first) }
 
     /** Gets the body of the i'th `defer` statement. */
     private BraceStmt getDeferStmtBody(int i) {
@@ -969,6 +959,15 @@ module Decls {
         result.asAstNode() = ast.getPattern(j).getFullyUnresolved()
       )
       or
+      // synthesized pattern bindings for property wrappers may be sharing the init with the backed
+      // variable declaration, so we need to skip those
+      not exists(VarDecl decl |
+        ast =
+          [
+            decl.getPropertyWrapperBackingVarBinding(),
+            decl.getPropertyWrapperProjectionVarBinding()
+          ]
+      ) and
       exists(int j |
         i = 2 * j + 1 and
         result.asAstNode() = ast.getInit(j).getFullyConverted()
@@ -1397,20 +1396,12 @@ module Exprs {
     override TapExpr ast;
 
     final override ControlFlowElement getChildElement(int i) {
-      // We first visit the local variable declaration.
+      // We first visit the expression that gives the local variable its initial value.
       i = 0 and
-      result.asAstNode() = ast.getVar()
-      or
-      // Then we visit the expression that gives the local variable its initial value.
-      i = 1 and
       result.asAstNode() = ast.getSubExpr().getFullyConverted()
       or
-      // And finally, we visit the body that potentially mutates the local variable.
-      // Note that the CFG for the body will skip the first element in the
-      // body because it's guaranteed to be the variable declaration
-      // that we've already visited at i = 0. See the explanation
-      // in `BraceStmtTree` for why this is necessary.
-      i = 2 and
+      // And then we visit the body that potentially mutates the local variable.
+      i = 1 and
       result.asAstNode() = ast.getBody()
     }
   }

swift/ql/lib/change-notes/2023-05-25-fix-ast-and-cfg-inconsistencies.md

@@ -28,7 +28,7 @@ private predicate shouldPrint(Locatable e) { any(PrintAstConfiguration config).s
 /**
  * An AST node that should be printed.
  */
-private newtype TPrintAstNode = TLocatable(Locatable ast)
+private newtype TPrintAstNode = TPrintLocatable(Locatable ast)
 
 /**
  * A node in the output tree.
@@ -60,6 +60,11 @@ class PrintAstNode extends TPrintAstNode {
    * the property is `key`.
    */
   string getProperty(string key) { none() }
+
+  /**
+   * Gets the underlying AST node, if any.
+   */
+  abstract Locatable getAstNode();
 }
 
 private string prettyPrint(Locatable e) {
@@ -73,10 +78,10 @@ private class Unresolved extends Locatable {
 /**
  * A graph node representing a real Locatable node.
  */
-class PrintLocatable extends PrintAstNode, TLocatable {
+class PrintLocatable extends PrintAstNode, TPrintLocatable {
   Locatable ast;
 
-  PrintLocatable() { this = TLocatable(ast) }
+  PrintLocatable() { this = TPrintLocatable(ast) }
 
   override string toString() { result = prettyPrint(ast) }
 
@@ -87,9 +92,9 @@ class PrintLocatable extends PrintAstNode, TLocatable {
       c = getChildAndAccessor(ast, i, accessor) and
       (
         // use even indexes for normal children, leaving odd slots for conversions if any
-        child = TLocatable(c) and index = 2 * i and label = accessor
+        child = TPrintLocatable(c) and index = 2 * i and label = accessor
         or
-        child = TLocatable(c.getFullyUnresolved().(Unresolved)) and
+        child = TPrintLocatable(c.getFullyUnresolved().(Unresolved)) and
         index = 2 * i + 1 and
         (
           if c instanceof Expr
@@ -100,6 +105,8 @@ class PrintLocatable extends PrintAstNode, TLocatable {
     )
   }
 
+  final override Locatable getAstNode() { result = ast }
+
   final override Location getLocation() { result = ast.getLocation() }
 }
 
@@ -112,17 +119,38 @@ class PrintUnresolved extends PrintLocatable {
 
   override predicate hasChild(PrintAstNode child, int index, string label) {
     // only print immediate unresolved children from the "parallel" AST
-    child = TLocatable(getImmediateChildAndAccessor(ast, index, label).(Unresolved))
+    child = TPrintLocatable(getImmediateChildAndAccessor(ast, index, label).(Unresolved))
   }
 }
 
+private predicate hasPropertyWrapperElement(VarDecl d, Locatable a) {
+  a = [d.getPropertyWrapperBackingVar(), d.getPropertyWrapperProjectionVar()] or
+  a = [d.getPropertyWrapperBackingVarBinding(), d.getPropertyWrapperProjectionVarBinding()]
+}
+
 /**
- * A specialization of graph node for `VarDecl`, to add typing information.
+ * A specialization of graph node for `VarDecl`, to add typing information and deal with ambiguity
+ * over property wrapper children.
  */
 class PrintVarDecl extends PrintLocatable {
   override VarDecl ast;
 
   override string getProperty(string key) { key = "Type" and result = ast.getType().toString() }
+
+  override predicate hasChild(PrintAstNode child, int index, string label) {
+    PrintLocatable.super.hasChild(child, index, label) and
+    // exclude property wrapper related children when they are already listed in the enclosing
+    // nominal type declaration or for a wrapped parameter for which this is a virtual local variable copy
+    not exists(Locatable childAst |
+      childAst = child.getAstNode() and
+      hasPropertyWrapperElement(ast, childAst) and
+      (
+        childAst = ast.getDeclaringDecl().getAMember()
+        or
+        ast instanceof ConcreteVarDecl and hasPropertyWrapperElement(any(ParamDecl p), childAst)
+      )
+    )
+  }
 }
 
 /**
@@ -135,3 +163,23 @@ class PrintFunction extends PrintLocatable {
     key = "InterfaceType" and result = ast.getInterfaceType().toString()
   }
 }
+
+/**
+ * A specialization of graph node for `PatternBindingDecl`, to solve ambiguity on `getInit`.
+ * When a property wrapper is involved, `getInit` may become shared between the explicit binding and
+ * the implicit compiler synthesized one.
+ */
+class PrintPatternBindingDecl extends PrintLocatable {
+  override PatternBindingDecl ast;
+
+  override predicate hasChild(PrintAstNode child, int index, string label) {
+    PrintLocatable.super.hasChild(child, index, label) and
+    // exclude `getInit` that are already the initializer of a variable that has this as a property wrapper backer
+    not exists(Expr init, VarDecl var |
+      init = child.getAstNode() and
+      init = ast.getAnInit() and
+      var.getPropertyWrapperBackingVarBinding() = ast and
+      var.getParentInitializer() = init
+    )
+  }
+}