Fix incorrect reference removal in AssignOnlyPropertyReferenceEliminator when properties have identical names at different scopes. Closes #394 (#433)

ileitch · web-flow · commit 6ef55c90076c · 2021-11-28T14:07:04.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -15,6 +15,8 @@
 - Unused function parameters on unimplemented protocol function members are now retained, as the function may still be referenced from an existential type.
 - Fix incorrect redundant public accessibility on a public superclass when a subclass is used in another module.
 - Result Builder static methods are now retained.
+- Assign-only properties that are assigned multiple times in the same method are now correctly identified as assign-only.
+- Fix issue where properties with identical names at different scopes could cause inconsistent results from assign-only property analysis.
 
 ## 2.8.2 (2021-11-06)
 
diff --git a/Sources/PeripheryKit/Analyzer/Visitors/AssignOnlyPropertyReferenceEliminator.swift b/Sources/PeripheryKit/Analyzer/Visitors/AssignOnlyPropertyReferenceEliminator.swift
@@ -18,35 +18,40 @@ final class AssignOnlyPropertyReferenceEliminator: SourceGraphVisitor {
         guard !configuration.retainAssignOnlyProperties else { return }
 
         let setters = graph.declarations(ofKind: .functionAccessorSetter)
-
         var assignOnlyProperties: [Declaration: [Reference]] = [:]
 
         for setter in setters {
             let references = graph.references(to: setter)
 
             for reference in references {
-                guard let caller = reference.parent,
-                      let propertyName = setter.name?.replacingOccurrences(of: "setter:", with: ""),
-                      let propertyReference = caller.references.first(where: { $0.kind.isVariableKind && $0.name == propertyName })
-                else { continue }
-
-                guard let property = graph.explicitDeclaration(withUsr: propertyReference.usr),
-                      !property.isComplexProperty
+                // Ensure the property being assigned is simple.
+                guard
+                    let caller = reference.parent,
+                    let property = setter.parent,
+                    !property.isComplexProperty
                 else { continue }
 
                 // A protocol property can technically be assigned and never used when the protocol is used as an existential
                 // type, however communicating that succinctly would be very tricky, and most likely just lead to confusion.
                 // Here we filter out protocol properties and thus restrict this analysis only to concrete properties.
-                if property.parent?.kind == .protocol {
-                    continue
+                guard property.parent?.kind != .protocol else { continue }
+
+                // Find all references to the property at the same call site as the setter reference.
+                let propertyReferences = caller.references.filter {
+                    property.usrs.contains($0.usr) && $0.location == reference.location
                 }
 
-                let propertyGetterUSRs = property.declarations.first { $0.kind == .functionAccessorGetter }?.usrs ?? []
+                // Determine if the containing method contains references to the property's getter accessor.
+                let propertyGetterUSRs = property.declarations
+                    .filter { $0.kind == .functionAccessorGetter }
+                    .flatMap { $0.usrs }
                 let hasGetterReference = caller.references
                     .contains { $0.kind == .functionAccessorGetter && propertyGetterUSRs.contains($0.usr) }
 
                 if !hasGetterReference {
-                    assignOnlyProperties[property, default: []].append(propertyReference)
+                    propertyReferences.forEach {
+                        assignOnlyProperties[property, default: []].append($0)
+                    }
                 }
             }
         }
diff --git a/Sources/PeripheryKit/Indexer/Reference.swift b/Sources/PeripheryKit/Indexer/Reference.swift
@@ -86,10 +86,6 @@ public final class Reference {
             rawValue.hasPrefix("function")
         }
 
-        var isVariableKind: Bool {
-            rawValue.hasPrefix("var")
-        }
-
         var declarationEquivalent: Declaration.Kind? {
             Declaration.Kind(rawValue: rawValue)
         }
diff --git a/Tests/Fixtures/RetentionFixtures/testSimpleAssignOnlyPropertyNameConflict.swift b/Tests/Fixtures/RetentionFixtures/testSimpleAssignOnlyPropertyNameConflict.swift
@@ -0,0 +1,15 @@
+import Foundation
+
+public class FixtureClass131 {
+    static var someProperty: String?
+    var someProperty: String
+
+    public init() {
+        someProperty = ""
+        Self.someProperty = ""
+    }
+
+    public func someMethod() {
+        _ = Self.someProperty
+    }
+}
diff --git a/Tests/Fixtures/RetentionFixtures/testSimplePropertyAssignedButNeverRead.swift b/Tests/Fixtures/RetentionFixtures/testSimplePropertyAssignedButNeverRead.swift
@@ -4,6 +4,7 @@ public class FixtureClass70 {
     static var simpleStaticUnreadVar: String!
     var simpleUnreadVar: String
     var simpleUnreadShadowedVar: String
+    var simpleUnreadVarAssignedMultiple: String
     var complexUnreadVar1: String {
         willSet {
             print("complex")
@@ -25,6 +26,8 @@ public class FixtureClass70 {
     init() {
         simpleUnreadVar = "Hello"
         simpleUnreadShadowedVar = "Hello"
+        simpleUnreadVarAssignedMultiple = "1"
+        simpleUnreadVarAssignedMultiple = "2"
         complexUnreadVar1 = "Hello"
         readVar = "Hello"
         FixtureClass70.simpleStaticUnreadVar = "Hello"
diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift
@@ -745,27 +745,6 @@ final class RetentionTest: SourceGraphTestCase {
         }
     }
 
-    func testRetainsAssignOnlyPropertyTypes() {
-        configuration.retainAssignOnlyProperties = false
-        configuration.retainAssignOnlyPropertyTypes = ["CustomType", "(CustomType, String)", "Swift.Double"]
-
-        analyze(retainPublic: true) {
-            assertReferenced(.class("FixtureClass123")) {
-                self.assertReferenced(.varInstance("retainedSimpleProperty"))
-                self.assertReferenced(.varInstance("retainedModulePrefixedProperty"))
-                self.assertReferenced(.varInstance("retainedTupleProperty"))
-                self.assertReferenced(.varInstance("retainedDestructuredPropertyA"))
-                self.assertReferenced(.varInstance("retainedMultipleBindingPropertyA"))
-
-                self.assertNotReferenced(.varInstance("notRetainedSimpleProperty"))
-                self.assertNotReferenced(.varInstance("notRetainedModulePrefixedProperty"))
-                self.assertNotReferenced(.varInstance("notRetainedTupleProperty"))
-                self.assertNotReferenced(.varInstance("notRetainedDestructuredPropertyB"))
-                self.assertNotReferenced(.varInstance("notRetainedMultipleBindingPropertyB"))
-            }
-        }
-    }
-
     func testNestedDeclarations() {
         analyze(retainPublic: true) {
             assertReferenced(.class("FixtureClass102")) {
@@ -903,32 +882,6 @@ final class RetentionTest: SourceGraphTestCase {
         }
     }
 
-    func testSimplePropertyAssignedButNeverRead() {
-        analyze(retainPublic: true) {
-            assertReferenced(.class("FixtureClass70")) {
-                self.assertNotReferenced(.varInstance("simpleUnreadVar"))
-                self.assertNotReferenced(.varInstance("simpleUnreadShadowedVar"))
-                self.assertNotReferenced(.varStatic("simpleStaticUnreadVar"))
-                self.assertReferenced(.varInstance("complexUnreadVar1"))
-                self.assertReferenced(.varInstance("complexUnreadVar2"))
-                self.assertReferenced(.varInstance("readVar"))
-            }
-        }
-
-        configuration.retainAssignOnlyProperties = true
-
-        analyze(retainPublic: true) {
-            assertReferenced(.class("FixtureClass70")) {
-                self.assertReferenced(.varInstance("simpleUnreadVar"))
-                self.assertReferenced(.varInstance("simpleUnreadShadowedVar"))
-                self.assertReferenced(.varStatic("simpleStaticUnreadVar"))
-                self.assertReferenced(.varInstance("complexUnreadVar1"))
-                self.assertReferenced(.varInstance("complexUnreadVar2"))
-                self.assertReferenced(.varInstance("readVar"))
-            }
-        }
-    }
-
     func testRetainsProtocolsViaCompositeTypealias() {
         analyze(retainPublic: true) {
             assertReferenced(.protocol("Fixture200"))
@@ -1013,6 +966,66 @@ final class RetentionTest: SourceGraphTestCase {
     }
     #endif
 
+    // MARK: - Assign-only properties
+
+    func testSimplePropertyAssignedButNeverRead() {
+        analyze(retainPublic: true) {
+            assertReferenced(.class("FixtureClass70")) {
+                self.assertNotReferenced(.varInstance("simpleUnreadVar"))
+                self.assertNotReferenced(.varInstance("simpleUnreadShadowedVar"))
+                self.assertNotReferenced(.varInstance("simpleUnreadVarAssignedMultiple"))
+                self.assertNotReferenced(.varStatic("simpleStaticUnreadVar"))
+                self.assertReferenced(.varInstance("complexUnreadVar1"))
+                self.assertReferenced(.varInstance("complexUnreadVar2"))
+                self.assertReferenced(.varInstance("readVar"))
+            }
+        }
+
+        configuration.retainAssignOnlyProperties = true
+
+        analyze(retainPublic: true) {
+            assertReferenced(.class("FixtureClass70")) {
+                self.assertReferenced(.varInstance("simpleUnreadVar"))
+                self.assertReferenced(.varInstance("simpleUnreadShadowedVar"))
+                self.assertReferenced(.varInstance("simpleUnreadVarAssignedMultiple"))
+                self.assertReferenced(.varStatic("simpleStaticUnreadVar"))
+                self.assertReferenced(.varInstance("complexUnreadVar1"))
+                self.assertReferenced(.varInstance("complexUnreadVar2"))
+                self.assertReferenced(.varInstance("readVar"))
+            }
+        }
+    }
+
+    func testSimpleAssignOnlyPropertyNameConflict() {
+        analyze(retainPublic: true) {
+            assertReferenced(.class("FixtureClass131")) {
+                self.assertNotReferenced(.varInstance("someProperty"))
+                self.assertReferenced(.varStatic("someProperty"))
+            }
+        }
+    }
+
+    func testRetainsAssignOnlyPropertyTypes() {
+        configuration.retainAssignOnlyProperties = false
+        configuration.retainAssignOnlyPropertyTypes = ["CustomType", "(CustomType, String)", "Swift.Double"]
+
+        analyze(retainPublic: true) {
+            assertReferenced(.class("FixtureClass123")) {
+                self.assertReferenced(.varInstance("retainedSimpleProperty"))
+                self.assertReferenced(.varInstance("retainedModulePrefixedProperty"))
+                self.assertReferenced(.varInstance("retainedTupleProperty"))
+                self.assertReferenced(.varInstance("retainedDestructuredPropertyA"))
+                self.assertReferenced(.varInstance("retainedMultipleBindingPropertyA"))
+
+                self.assertNotReferenced(.varInstance("notRetainedSimpleProperty"))
+                self.assertNotReferenced(.varInstance("notRetainedModulePrefixedProperty"))
+                self.assertNotReferenced(.varInstance("notRetainedTupleProperty"))
+                self.assertNotReferenced(.varInstance("notRetainedDestructuredPropertyB"))
+                self.assertNotReferenced(.varInstance("notRetainedMultipleBindingPropertyB"))
+            }
+        }
+    }
+
     // MARK: - Unused Parameters
 
     func testRetainsParamUsedInOverriddenMethod() {

Original file line number	Diff line number	Diff line change
`@@ -86,10 +86,6 @@ public final class Reference {`
`86`	`86`	`rawValue.hasPrefix("function")`
`87`	`87`	`}`
`88`	`88`
`89`		`- var isVariableKind: Bool {`
`90`		`- rawValue.hasPrefix("var")`
`91`		`- }`
`92`		`-`
`93`	`89`	`var declarationEquivalent: Declaration.Kind? {`
`94`	`90`	`Declaration.Kind(rawValue: rawValue)`
`95`	`91`	`}`