unified: Synthesize setter parameters and return type in accessors

Accessors previously had a 'type' field. Now, it has a return_type instead, which is only set for getters. For set, didSet, and willSet, we insert a synthetic newValue/oldValue parameter, which also carries the type previously stored in accessor_declaration.type.

Author: asgerf

unified/extractor/ast_types.yml

@@ -534,7 +534,7 @@ named:
     name_node: identifier
     accessor_kind: accessor_kind
     parameter*: parameter
-    type?: expr
+    return_type?: expr
     body?: block
 
   # "get", "set", or a language-specific kind like "didSet"

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

@@ -12,10 +12,10 @@ struct SwiftContext {
     /// `variableDecl` rule before translating the accessor block; read by the
     /// inner `accessorDecl` rules to name each `accessor_declaration`.
     property_name_node: Option<yeast::OutputId>,
-    /// Translated type node for the property type. Set (for computed
-    /// properties) by the accessor-bearing `variableDecl` rule; read by the
-    /// inner `accessorDecl` rules. Left `None` for stored properties with
-    /// observers, so their `willSet`/`didSet` accessors carry no type.
+    /// Translated type node for the property type. Set by the
+    /// accessor-bearing `variableDecl` rule; read by the inner `accessorDecl`
+    /// rules for both the getter return type and its `newValue`/`oldValue`
+    /// parameter type, when available.
     property_type: Option<yeast::OutputId>,
     /// Translated outer modifiers to attach to each child of a flattening
     /// outer rule — e.g. the `let`/`var` binding modifier on each
@@ -334,7 +334,7 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
             (accessor_declaration
                 modifier: (modifier #{spec})
                 name_node: (identifier #{name})
-                type: {ty}
+                return_type: {ty}
                 accessor_kind: {ctx.literal_at_start_of("accessor_kind", "get", body)}
                 body: {body})
         ),
@@ -345,12 +345,12 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
         //  * With an initializer (`var x: T = e { willSet {…} didSet {…} }`) it is
         //    a *stored* property with observers: emit the backing
         //    `variable_declaration` first, then one `accessor_declaration` per
-        //    observer (observers carry no type).
+        //    observer.
         //  * Without an initializer (`var v: T { get set }`, incl. protocol
-        //    requirements) it is a *computed* property: no backing variable; the
-        //    type is published so the get/set accessors carry it.
+        //    requirements) it is a *computed* property: no backing variable.
         //
-        // In both cases the first emitted declaration is unchained and every
+        // In both cases the type is published so every accessor carries it, and
+        // the first emitted declaration is unchained and every
         // subsequent one is tagged `chained_declaration` (the `!result.is_empty()`
         // test). Must precede the plain `variableDecl` rules.
         rule!(
@@ -365,6 +365,7 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
             member* {
                 ctx.outer_modifiers = vec![tree!((modifier #{spec}))];
                 ctx.property_name_node = Some(tree!((identifier #{name})));
+                ctx.property_type = Some(ty);
                 let mut result = Vec::new();
                 if let Some(val) = val {
                     // Stored property with observers: the initializer is not part
@@ -380,9 +381,6 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
                             type: {ty}
                             value: {val})
                     ));
-                } else {
-                    // Computed property: the accessors carry the type.
-                    ctx.property_type = Some(ty);
                 }
                 for acc in accessors.into_iter() {
                     ctx.is_chained = !result.is_empty();
@@ -400,27 +398,70 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
         // one (a protocol requirement) carries neither. The property context is
         // read out *before* translating the body, which resets `ctx` so the
         // accessor's context does not leak into the body subtree.
-        rule!(
-            (accessorDecl accessorSpecifier: @@spec body: _? @@body)
-            =>
-            (accessor_declaration
-                modifier: {
-                    if body.is_some() {
-                        ctx.outer_modifiers.clone()
-                    } else {
-                        Vec::new()
+        //
+        // `set` and `willSet` accessors get a parameter holding the incoming
+        // value, and `didSet` gets one holding the previous value. It is named
+        // explicitly (`set(v)`) or else implicitly `newValue`/`oldValue`.
+        rule!(
+            (accessorDecl
+                accessorSpecifier: @@spec
+                parameters: (accessorParameters name: @@param_name)?
+                body: _? @@body)
+            =>
+            accessor_declaration {
+                let spec_text = ctx.source_text(spec);
+                let implicit_param_name = match spec_text.as_str() {
+                    "set" | "willSet" => Some("newValue"),
+                    "didSet" => Some("oldValue"),
+                    _ => None,
+                };
+                let param_ty = ctx.property_type;
+                let param = match (param_name, implicit_param_name) {
+                    (Some(n), _) => {
+                        Some(tree!((parameter pattern: (identifier #{n}) type: {param_ty})))
                     }
-                }
-                modifier: {chained_modifier(&mut ctx)}
-                name_node: {
-                    ctx.property_name_node
-                        .ok_or("accessor outside property context")?
-                }
-                type: {ctx.property_type}
-                accessor_kind: (accessor_kind #{spec})
-                body: {ctx.reset(); body}
-                location: {spec}
-                location: {body})
+                    (None, Some(n)) => {
+                        let n = n.to_string();
+                        Some(tree!((parameter
+                            modifier: (modifier "generated")
+                            pattern: (identifier #{n} location: {spec})
+                            type: {param_ty}
+                            location: {spec})))
+                    }
+                    (None, None) => None,
+                };
+                let binding = if body.is_some() {
+                    ctx.outer_modifiers.clone()
+                } else {
+                    Vec::new()
+                };
+                let chained = chained_modifier(&mut ctx);
+                let name = ctx
+                    .property_name_node
+                    .ok_or("accessor outside property context")?;
+                let return_ty = if spec_text == "get" {
+                    ctx.property_type
+                } else {
+                    None
+                };
+                let body = match body {
+                    Some(block) => {
+                        ctx.reset();
+                        ctx.translate(block)?.into_iter().next()
+                    }
+                    None => None,
+                };
+                tree!((accessor_declaration
+                    modifier: {binding}
+                    modifier: {chained}
+                    name_node: {name}
+                    return_type: {return_ty}
+                    accessor_kind: (accessor_kind #{spec})
+                    parameter: {param}
+                    body: {body}
+                    location: {spec}
+                    location: {body}))
+            }
         ),
         // ---- Variables ----
         // The individual bindings of a `variableDecl`. The binding modifier and

unified/extractor/tests/corpus/swift/types/binding-modifier-does-not-leak-into-accessor-body.output

@@ -100,7 +100,7 @@ top_level source="var p: Int {\n  ⟨body⟩\n}"
           modifier: modifier "var" source="var" (external)
           name_node: identifier "p" source="p" (external)
           accessor_kind: accessor_kind "get" source="get"
-          type: identifier "Int" source="Int" (external)
+          return_type: identifier "Int" source="Int" (external)
           body:
             block source="{\n    ⟨stmt⟩\n  }"
               stmt:

unified/extractor/tests/corpus/swift/types/computed-property.output

@@ -113,11 +113,11 @@ top_level source="⟨body⟩"
               modifier: modifier "var" source="var"
               pattern: identifier "h" source="h"
               type: identifier "Double" source="Double"
-            accessor_declaration source="⟨modifier⟩ ⟨name_node⟩: ⟨type⟩ ⟨body⟩"
+            accessor_declaration source="⟨modifier⟩ ⟨name_node⟩: ⟨return_type⟩ ⟨body⟩"
               modifier: modifier "var" source="var"
               name_node: identifier "area" source="area"
               accessor_kind: accessor_kind "get" source=""
-              type: identifier "Double" source="Double"
+              return_type: identifier "Double" source="Double"
               body:
                 block source="{\n    ⟨stmt⟩\n  }"
                   stmt:

unified/extractor/tests/corpus/swift/types/property-with-getter-and-setter.output

@@ -120,19 +120,23 @@ top_level source="⟨body⟩"
               modifier: modifier "var" source="var" (external)
               name_node: identifier "v" source="v" (external)
               accessor_kind: accessor_kind "get" source="get"
-              type: identifier "Int" source="Int" (external)
+              return_type: identifier "Int" source="Int" (external)
               body:
                 block source="{ ⟨stmt⟩ }"
                   stmt:
                     return_expr source="return ⟨value⟩"
                       value: identifier "_v" source="_v"
-            accessor_declaration source="⟨accessor_kind⟩ ⟨body⟩"
+            accessor_declaration source="⟨accessor_kind⟩⟨parameter⟩ ⟨body⟩"
               modifier:
                 modifier "var" source="var" (external)
                 modifier "chained_declaration" source=""
               name_node: identifier "v" source="v" (external)
               accessor_kind: accessor_kind "set" source="set"
-              type: identifier "Int" source="Int" (external)
+              parameter:
+                parameter source="⟨pattern⟩"
+                  modifier: modifier "generated" source=""
+                  type: identifier "Int" source="Int" (external)
+                  pattern: identifier "newValue" source="set"
               body:
                 block source="{ ⟨stmt⟩ }"
                   stmt:

unified/extractor/tests/corpus/swift/types/protocol-with-read-only-and-read-write-property-requirements.output

@@ -113,17 +113,21 @@ top_level source="⟨body⟩"
             accessor_declaration source="⟨accessor_kind⟩"
               name_node: identifier "foo" source="foo" (external)
               accessor_kind: accessor_kind "get" source="get"
-              type: identifier "Int" source="Int" (external)
+              return_type: identifier "Int" source="Int" (external)
             accessor_declaration source="⟨accessor_kind⟩"
               name_node: identifier "bar" source="bar" (external)
               accessor_kind: accessor_kind "get" source="get"
-              type: identifier "String" source="String" (external)
-            accessor_declaration source="⟨accessor_kind⟩"
+              return_type: identifier "String" source="String" (external)
+            accessor_declaration source="⟨accessor_kind⟩⟨parameter⟩"
               modifier: modifier "chained_declaration" source=""
               name_node: identifier "bar" source="bar" (external)
               accessor_kind: accessor_kind "set" source="set"
-              type: identifier "String" source="String" (external)
+              parameter:
+                parameter source="⟨pattern⟩"
+                  modifier: modifier "generated" source=""
+                  type: identifier "String" source="String" (external)
+                  pattern: identifier "newValue" source="set"
             accessor_declaration source="⟨accessor_kind⟩"
               name_node: identifier "count" source="count" (external)
               accessor_kind: accessor_kind "get" source="get"
-              type: identifier "Int" source="Int" (external)
+              return_type: identifier "Int" source="Int" (external)

unified/extractor/tests/corpus/swift/variables/property-with-willset-and-didset-observers.output

@@ -110,12 +110,17 @@ top_level source="⟨body⟩"
               pattern: identifier "x" source="x"
               type: identifier "Int" source="Int"
               value: int_literal "0" source="0"
-            accessor_declaration source="⟨accessor_kind⟩ ⟨body⟩"
+            accessor_declaration source="⟨accessor_kind⟩⟨parameter⟩ ⟨body⟩"
               modifier:
                 modifier "var" source="var" (external)
                 modifier "chained_declaration" source=""
               name_node: identifier "x" source="x" (external)
               accessor_kind: accessor_kind "willSet" source="willSet"
+              parameter:
+                parameter source="⟨pattern⟩"
+                  modifier: modifier "generated" source=""
+                  type: identifier "Int" source="Int" (external)
+                  pattern: identifier "newValue" source="willSet"
               body:
                 block source="{ ⟨stmt⟩ }"
                   stmt:
@@ -124,12 +129,17 @@ top_level source="⟨body⟩"
                       argument:
                         argument source="⟨value⟩"
                           value: identifier "newValue" source="newValue"
-            accessor_declaration source="⟨accessor_kind⟩ ⟨body⟩"
+            accessor_declaration source="⟨accessor_kind⟩⟨parameter⟩ ⟨body⟩"
               modifier:
                 modifier "var" source="var" (external)
                 modifier "chained_declaration" source=""
               name_node: identifier "x" source="x" (external)
               accessor_kind: accessor_kind "didSet" source="didSet"
+              parameter:
+                parameter source="⟨pattern⟩"
+                  modifier: modifier "generated" source=""
+                  type: identifier "Int" source="Int" (external)
+                  pattern: identifier "oldValue" source="didSet"
               body:
                 block source="{ ⟨stmt⟩ }"
                   stmt:

unified/ql/lib/codeql/unified/internal/Ast.qll

@@ -123,8 +123,10 @@ module Unified {
     /** Gets the node corresponding to the field `parameter`. */
     final override F::Parameter getAParameter() { result = this.getParameter(_) }
 
-    /** Gets the node corresponding to the field `type`. */
-    final F::Expr getType() { unified_accessor_declaration_type(this, result) }
+    /** Gets the node corresponding to the field `return_type`. */
+    final override F::Expr getReturnType() {
+      unified_accessor_declaration_return_type(this, result)
+    }
 
     /** Gets a field or child node of this node. */
     final override F::AstNode getAFieldOrChild() {
@@ -133,7 +135,7 @@ module Unified {
       unified_accessor_declaration_modifier(this, _, result) or
       unified_accessor_declaration_def(this, _, result) or
       unified_accessor_declaration_parameter(this, _, result) or
-      unified_accessor_declaration_type(this, result)
+      unified_accessor_declaration_return_type(this, result)
     }
   }
 
@@ -1624,7 +1626,7 @@ module Unified {
       or
       result = node.(AccessorDeclaration).getParameter(i) and name = "getParameter"
       or
-      result = node.(AccessorDeclaration).getType() and i = -1 and name = "getType"
+      result = node.(AccessorDeclaration).getReturnType() and i = -1 and name = "getReturnType"
       or
       result = node.(Argument).getModifier(i) and name = "getModifier"
       or

unified/ql/lib/codeql/unified/internal/ExprPositions.qll

@@ -16,7 +16,7 @@ predicate isInTypeContext(Expr expr) {
   or
   expr = any(FunctionExpr n).getReturnType()
   or
-  expr = any(AccessorDeclaration n).getType()
+  expr = any(AccessorDeclaration n).getReturnType()
   or
   expr = any(Parameter n).getType()
   or

unified/ql/lib/unified.dbscheme

@@ -155,9 +155,9 @@ unified_accessor_declaration_parameter(
   unique int parameter: @unified_parameter ref
 );
 
-unified_accessor_declaration_type(
+unified_accessor_declaration_return_type(
   unique int unified_accessor_declaration: @unified_accessor_declaration ref,
-  unique int type__: @unified_expr ref
+  unique int return_type: @unified_expr ref
 );
 
 unified_accessor_declaration_def(