unified: Ensure 'static' modifier is preserved for accessors

Modifiers like 'static' did not get preserved when extracting accessors, making most of the tests fail for static getters/setters. All those tests now pass.

Author: asgerf

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

@@ -18,9 +18,9 @@ struct SwiftContext {
     /// 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
-    /// `patternBinding` of a `variableDecl`, or the binding modifier on each
-    /// accessor of a property.
+    /// outer rule — e.g. the `let`/`var` binding modifier, attributes and
+    /// modifiers (`static`, `public`, …) on each `patternBinding` of a
+    /// `variableDecl`, or on each accessor of a property.
     outer_modifiers: Vec<yeast::OutputId>,
     /// True when the current child of a flattening outer rule is not
     /// the first one — its inner rule should emit a
@@ -325,6 +325,8 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
         // block.
         rule!(
             (variableDecl
+                attributes: _* @attrs
+                modifiers: _* @mods
                 bindingSpecifier: @@spec
                 bindings: (patternBinding
                     pattern: (identifierPattern identifier: @@name)
@@ -333,6 +335,8 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
             =>
             (accessor_declaration
                 modifier: (modifier #{spec})
+                modifier: {attrs}
+                modifier: {mods}
                 name_node: (identifier #{name})
                 return_type: {ty}
                 accessor_kind: {ctx.literal_at_start_of("accessor_kind", "get", body)}
@@ -355,6 +359,8 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
         // test). Must precede the plain `variableDecl` rules.
         rule!(
             (variableDecl
+                attributes: _* @attrs
+                modifiers: _* @mods
                 bindingSpecifier: @@spec
                 bindings: (patternBinding
                     pattern: (identifierPattern identifier: @@name)
@@ -363,7 +369,12 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
                     accessorBlock: (accessorBlock accessors: (accessorDecl)+ @@accessors)))
             =>
             member* {
-                ctx.outer_modifiers = vec![tree!((modifier #{spec}))];
+                // The binding (`let`/`var`) leads, then attributes then modifiers,
+                // matching the plain `variableDecl` rule.
+                ctx.outer_modifiers = std::iter::once(tree!((modifier #{spec})))
+                    .chain(attrs)
+                    .chain(mods)
+                    .collect();
                 ctx.property_name_node = Some(tree!((identifier #{name})));
                 ctx.property_type = Some(ty);
                 let mut result = Vec::new();
@@ -390,12 +401,12 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
             }
         ),
         // Each `accessorDecl` becomes an `accessor_declaration`, reading the
-        // property name/type and the binding/chained modifiers from `ctx`. The
+        // property name/type and the modifiers from `ctx`. The
         // accessor kind comes straight from the specifier keyword
         // (`get`/`set`/`willSet`/`didSet`). The body is optional: a body-bearing
         // accessor (a computed getter/setter, or a `willSet`/`didSet` observer)
-        // carries the binding modifier and a translated body, whereas a bodyless
-        // one (a protocol requirement) carries neither. The property context is
+        // has a translated body, whereas a bodyless one (a protocol requirement)
+        // has none. 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.
         //
@@ -430,11 +441,7 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
                     }
                     (None, None) => None,
                 };
-                let binding = if body.is_some() {
-                    ctx.outer_modifiers.clone()
-                } else {
-                    Vec::new()
-                };
+                let modifiers = ctx.outer_modifiers.clone();
                 let chained = chained_modifier(&mut ctx);
                 let name = ctx
                     .property_name_node
@@ -452,7 +459,7 @@ fn translation_rules() -> Vec<Rule<SwiftContext>> {
                     None => None,
                 };
                 tree!((accessor_declaration
-                    modifier: {binding}
+                    modifier: {modifiers}
                     modifier: {chained}
                     name_node: {name}
                     return_type: {return_ty}

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

@@ -111,15 +111,19 @@ top_level source="⟨body⟩"
           name_node: identifier "P" source="P"
           member:
             accessor_declaration source="⟨accessor_kind⟩"
+              modifier: modifier "var" source="var" (external)
               name_node: identifier "foo" source="foo" (external)
               accessor_kind: accessor_kind "get" source="get"
               return_type: identifier "Int" source="Int" (external)
             accessor_declaration source="⟨accessor_kind⟩"
+              modifier: modifier "var" source="var" (external)
               name_node: identifier "bar" source="bar" (external)
               accessor_kind: accessor_kind "get" source="get"
               return_type: identifier "String" source="String" (external)
             accessor_declaration source="⟨accessor_kind⟩⟨parameter⟩"
-              modifier: modifier "chained_declaration" source=""
+              modifier:
+                modifier "var" source="var" (external)
+                modifier "chained_declaration" source=""
               name_node: identifier "bar" source="bar" (external)
               accessor_kind: accessor_kind "set" source="set"
               parameter:
@@ -128,6 +132,7 @@ top_level source="⟨body⟩"
                   type: identifier "String" source="String" (external)
                   pattern: identifier "newValue" source="set"
             accessor_declaration source="⟨accessor_kind⟩"
+              modifier: modifier "var" source="var" (external)
               name_node: identifier "count" source="count" (external)
               accessor_kind: accessor_kind "get" source="get"
               return_type: identifier "Int" source="Int" (external)

unified/ql/test/library-tests/dataflow/accessors_static.swift

@@ -7,7 +7,7 @@ func t1() {
     class C {
         static var x: String { source("t1.1") }
     }
-    sink(C.x)  // $ MISSING: hasValueFlow=t1.1
+    sink(C.x)  // $ hasValueFlow=t1.1
 }
 
 func t2() {
@@ -18,7 +18,7 @@ func t2() {
             get { source("t2.1") }
         }
     }
-    sink(C.x)  // $ MISSING: hasValueFlow=t2.1
+    sink(C.x)  // $ hasValueFlow=t2.1
 }
 
 func t3() {
@@ -28,14 +28,14 @@ func t3() {
             set {}
         }
     }
-    sink(C.x)  // $ MISSING: hasValueFlow=t3.1
+    sink(C.x)  // $ hasValueFlow=t3.1
 }
 
 func t4() {
     class C {
         static var x: String {
             get {}
-            set { sink(newValue) }  // $ MISSING: hasValueFlow=t4.1
+            set { sink(newValue) }  // $ hasValueFlow=t4.1
         }
     }
     C.x = source("t4.1")
@@ -45,7 +45,7 @@ func t5() {
     class C {
         static var x: String {
             get { source("t5.1") }
-            set { sink(newValue) }  // $ MISSING: hasValueFlow=t5.1
+            set { sink(newValue) }  // $ hasValueFlow=t5.1
         }
     }
     C.x = C.x
@@ -56,11 +56,11 @@ func t6() {
         static var x: String { source("t6.1") }
 
         func method() {
-            sink(Self.x)  // $ MISSING: hasValueFlow=t6.1
+            sink(Self.x)  // $ hasValueFlow=t6.1
         }
 
         static func staticMethod() {
-            sink(Self.x)  // $ MISSING: hasValueFlow=t6.1
+            sink(Self.x)  // $ hasValueFlow=t6.1
             sink(x)  // $ hasValueFlow=t6.1
         }
     }

unified/ql/test/library-tests/dataflow/test.expected

@@ -11,6 +11,18 @@ edges
 | accessors_local.swift:39:9:39:11 | newValue | accessors_local.swift:39:20:39:27 | newValue | provenance |  |
 | accessors_local.swift:41:5:41:5 | x | accessors_local.swift:39:9:39:11 | newValue | provenance |  |
 | accessors_local.swift:41:9:41:9 | x | accessors_local.swift:41:5:41:5 | x | provenance |  |
+| accessors_static.swift:8:32:8:45 | source(...) | accessors_static.swift:10:10:10:12 | ... .x | provenance |  |
+| accessors_static.swift:18:19:18:32 | source(...) | accessors_static.swift:21:10:21:12 | ... .x | provenance |  |
+| accessors_static.swift:27:19:27:32 | source(...) | accessors_static.swift:31:10:31:12 | ... .x | provenance |  |
+| accessors_static.swift:38:13:38:15 | newValue | accessors_static.swift:38:24:38:31 | newValue | provenance |  |
+| accessors_static.swift:41:5:41:7 | ... .x | accessors_static.swift:38:13:38:15 | newValue | provenance |  |
+| accessors_static.swift:41:11:41:24 | source(...) | accessors_static.swift:41:5:41:7 | ... .x | provenance |  |
+| accessors_static.swift:47:19:47:32 | source(...) | accessors_static.swift:51:11:51:13 | ... .x | provenance |  |
+| accessors_static.swift:48:13:48:15 | newValue | accessors_static.swift:48:24:48:31 | newValue | provenance |  |
+| accessors_static.swift:51:5:51:7 | ... .x | accessors_static.swift:48:13:48:15 | newValue | provenance |  |
+| accessors_static.swift:51:11:51:13 | ... .x | accessors_static.swift:51:5:51:7 | ... .x | provenance |  |
+| accessors_static.swift:56:32:56:45 | source(...) | accessors_static.swift:59:18:59:23 | ... .x | provenance |  |
+| accessors_static.swift:56:32:56:45 | source(...) | accessors_static.swift:63:18:63:23 | ... .x | provenance |  |
 | accessors_static.swift:56:32:56:45 | source(...) | accessors_static.swift:64:18:64:18 | x | provenance |  |
 | calls.swift:7:19:7:19 | x | calls.swift:8:14:8:14 | x | provenance |  |
 | calls.swift:10:12:10:25 | source(...) | calls.swift:7:19:7:19 | x | provenance |  |
@@ -488,7 +500,24 @@ nodes
 | accessors_local.swift:39:20:39:27 | newValue | semmle.label | newValue |
 | accessors_local.swift:41:5:41:5 | x | semmle.label | x |
 | accessors_local.swift:41:9:41:9 | x | semmle.label | x |
+| accessors_static.swift:8:32:8:45 | source(...) | semmle.label | source(...) |
+| accessors_static.swift:10:10:10:12 | ... .x | semmle.label | ... .x |
+| accessors_static.swift:18:19:18:32 | source(...) | semmle.label | source(...) |
+| accessors_static.swift:21:10:21:12 | ... .x | semmle.label | ... .x |
+| accessors_static.swift:27:19:27:32 | source(...) | semmle.label | source(...) |
+| accessors_static.swift:31:10:31:12 | ... .x | semmle.label | ... .x |
+| accessors_static.swift:38:13:38:15 | newValue | semmle.label | newValue |
+| accessors_static.swift:38:24:38:31 | newValue | semmle.label | newValue |
+| accessors_static.swift:41:5:41:7 | ... .x | semmle.label | ... .x |
+| accessors_static.swift:41:11:41:24 | source(...) | semmle.label | source(...) |
+| accessors_static.swift:47:19:47:32 | source(...) | semmle.label | source(...) |
+| accessors_static.swift:48:13:48:15 | newValue | semmle.label | newValue |
+| accessors_static.swift:48:24:48:31 | newValue | semmle.label | newValue |
+| accessors_static.swift:51:5:51:7 | ... .x | semmle.label | ... .x |
+| accessors_static.swift:51:11:51:13 | ... .x | semmle.label | ... .x |
 | accessors_static.swift:56:32:56:45 | source(...) | semmle.label | source(...) |
+| accessors_static.swift:59:18:59:23 | ... .x | semmle.label | ... .x |
+| accessors_static.swift:63:18:63:23 | ... .x | semmle.label | ... .x |
 | accessors_static.swift:64:18:64:18 | x | semmle.label | x |
 | calls.swift:7:19:7:19 | x | semmle.label | x |
 | calls.swift:8:14:8:14 | x | semmle.label | x |
@@ -1073,6 +1102,13 @@ testFailures
 | accessors_local.swift:25:10:25:10 | x | accessors_local.swift:22:15:22:28 | source(...) | accessors_local.swift:25:10:25:10 | x | $@ | accessors_local.swift:22:15:22:28 | source(...) | source(...) |
 | accessors_local.swift:31:20:31:27 | newValue | accessors_local.swift:33:9:33:22 | source(...) | accessors_local.swift:31:20:31:27 | newValue | $@ | accessors_local.swift:33:9:33:22 | source(...) | source(...) |
 | accessors_local.swift:39:20:39:27 | newValue | accessors_local.swift:38:15:38:28 | source(...) | accessors_local.swift:39:20:39:27 | newValue | $@ | accessors_local.swift:38:15:38:28 | source(...) | source(...) |
+| accessors_static.swift:10:10:10:12 | ... .x | accessors_static.swift:8:32:8:45 | source(...) | accessors_static.swift:10:10:10:12 | ... .x | $@ | accessors_static.swift:8:32:8:45 | source(...) | source(...) |
+| accessors_static.swift:21:10:21:12 | ... .x | accessors_static.swift:18:19:18:32 | source(...) | accessors_static.swift:21:10:21:12 | ... .x | $@ | accessors_static.swift:18:19:18:32 | source(...) | source(...) |
+| accessors_static.swift:31:10:31:12 | ... .x | accessors_static.swift:27:19:27:32 | source(...) | accessors_static.swift:31:10:31:12 | ... .x | $@ | accessors_static.swift:27:19:27:32 | source(...) | source(...) |
+| accessors_static.swift:38:24:38:31 | newValue | accessors_static.swift:41:11:41:24 | source(...) | accessors_static.swift:38:24:38:31 | newValue | $@ | accessors_static.swift:41:11:41:24 | source(...) | source(...) |
+| accessors_static.swift:48:24:48:31 | newValue | accessors_static.swift:47:19:47:32 | source(...) | accessors_static.swift:48:24:48:31 | newValue | $@ | accessors_static.swift:47:19:47:32 | source(...) | source(...) |
+| accessors_static.swift:59:18:59:23 | ... .x | accessors_static.swift:56:32:56:45 | source(...) | accessors_static.swift:59:18:59:23 | ... .x | $@ | accessors_static.swift:56:32:56:45 | source(...) | source(...) |
+| accessors_static.swift:63:18:63:23 | ... .x | accessors_static.swift:56:32:56:45 | source(...) | accessors_static.swift:63:18:63:23 | ... .x | $@ | accessors_static.swift:56:32:56:45 | source(...) | source(...) |
 | accessors_static.swift:64:18:64:18 | x | accessors_static.swift:56:32:56:45 | source(...) | accessors_static.swift:64:18:64:18 | x | $@ | accessors_static.swift:56:32:56:45 | source(...) | source(...) |
 | calls.swift:8:14:8:14 | x | calls.swift:10:12:10:25 | source(...) | calls.swift:8:14:8:14 | x | $@ | calls.swift:10:12:10:25 | source(...) | source(...) |
 | calls.swift:17:10:17:17 | target(...) | calls.swift:15:16:15:29 | source(...) | calls.swift:17:10:17:17 | target(...) | $@ | calls.swift:15:16:15:29 | source(...) | source(...) |