Fixed false positive for params of foreign protocol in subclass (#268)

* Fixed false positive for public protocols with extensions * Renamed fixtures and updated changelog * Fixed false positive for params of foreign protocol in subclass * Removed unused params * Renamed fixtures and updated changelog * Updated changelog Co-authored-by: Ian Leitch <[email protected]>
peripheryapp · Feb 11, 2021 · 1f19db3 · 1f19db3
1 parent ae369fc
commit 1f19db3
Show file tree

Hide file tree

Showing 4 changed files with 81 additions and 28 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,7 @@
 - In Swift 5.3 and lower, all optional protocol members are now retained in order to workaround a Swift bug. This bug is resolved in Swift 5.4.
 - Cases of public enums are now also retained when using `--retain-public`.
 - Empty public protocols that have an implementation in extensions are no longer identified as redundant.
+- Foreign protocol method parameters are no longer identified as unused.
 
 ## 2.4.1 (2020-12-20)
 

diff --git a/Sources/PeripheryKit/Analyzer/Visitors/UnusedParameterRetainer.swift b/Sources/PeripheryKit/Analyzer/Visitors/UnusedParameterRetainer.swift
@@ -19,9 +19,7 @@ final class UnusedParameterRetainer: SourceGraphVisitor {
         let functionDecls: Set<Declaration> = Set(paramDecls.compactMap { $0.parent as? Declaration })
 
         for functionDecl in functionDecls {
-            let paramDecls = functionDecl.unusedParameters
-            retain(paramDecls, inForeignProtocolFunction: functionDecl)
-            retain(paramDecls, inOverriddenFunction: functionDecl)
+            retainIfNeeded(params: functionDecl.unusedParameters, inMethod: functionDecl)
         }
 
         let protocolDecls = graph.declarations(ofKind: .protocol)
@@ -58,44 +56,73 @@ final class UnusedParameterRetainer: SourceGraphVisitor {
 
     // MARK: - Private
 
-    private func retain(_ params: Set<Declaration>, inForeignProtocolFunction decl: Declaration) {
-        guard let refKind = decl.kind.referenceEquivalent,
-              let related = decl.related.first(where: { $0.kind == refKind && $0.name == decl.name }) else { return }
+    private func retainIfNeeded(params: Set<Declaration>, inMethod methodDeclaration: Declaration) {
+        let allMethodDeclarations: [Declaration]
 
-        if graph.explicitDeclaration(withUsr: related.usr) == nil {
-            params.forEach { graph.markRetained($0) }
+        if let classDeclaration = methodDeclaration.parent as? Declaration, classDeclaration.kind == .class {
+            let allClassDeclarations = [classDeclaration]
+                + graph.superclasses(of: classDeclaration)
+                + graph.subclasses(of: classDeclaration)
+
+            allMethodDeclarations = allClassDeclarations.compactMap { declaration in
+                declaration
+                    .declarations
+                    .lazy
+                    .filter { $0.kind == methodDeclaration.kind }
+                    .first { $0.name == methodDeclaration.name }
+            }
+
+            retainIfNeeded(
+                params: params,
+                inOverridenMethods: allMethodDeclarations
+            )
+        } else {
+            allMethodDeclarations = [methodDeclaration]
         }
+
+        retainParamsIfNeeded(inForeignProtocolMethods: allMethodDeclarations)
     }
 
-    private func retain(_ params: Set<Declaration>, inOverriddenFunction decl: Declaration) {
-        guard let classDecl = decl.parent as? Declaration,
-              classDecl.kind == .class else { return }
+    private func retainParamsIfNeeded(inForeignProtocolMethods methodDeclarations: [Declaration]) {
+        guard let methodDeclaration = methodDeclarations.first else {
+            return
+        }
 
-        let superclasses = graph.superclasses(of: classDecl)
-        let subclasses = graph.subclasses(of: classDecl)
-        let allClasses = superclasses + [classDecl] + subclasses
-        var functionDecls: [Declaration] = []
+        guard let referenceKind = methodDeclaration.kind.referenceEquivalent else {
+            return
+        }
 
-        allClasses.forEach {
-            let functionDecl = $0.declarations.first {
-                $0.kind == decl.kind && $0.name == decl.name
-            }
+        let foreignReferences = methodDeclaration
+            .related
+            .lazy
+            .filter { $0.kind == referenceKind }
+            .filter { $0.name == methodDeclaration.name }
+            .filter { self.graph.explicitDeclaration(withUsr: $0.usr) == nil }
 
-            if let functionDecl = functionDecl {
-                functionDecls.append(functionDecl)
-            }
+        guard !foreignReferences.isEmpty else {
+            return
         }
 
-        guard let firstFunctionDecl = functionDecls.first else { return }
+        methodDeclarations
+            .lazy
+            .flatMap { $0.unusedParameters }
+            .forEach { graph.markRetained($0) }
+    }
+
+    private func retainIfNeeded(params: Set<Declaration>, inOverridenMethods methodDeclarations: [Declaration]) {
+        let isSuperclassForeign = methodDeclarations.allSatisfy { declaration in
+            declaration.modifiers.contains("override")
+        }
 
-        if firstFunctionDecl.modifiers.contains("override") {
+        if isSuperclassForeign {
             // Must be overriding a declaration in a foreign class.
-            functionDecls.forEach {
-                $0.unusedParameters.forEach { graph.markRetained($0) }
-            }
+            methodDeclarations
+                .lazy
+                .flatMap { $0.unusedParameters }
+                .forEach { graph.markRetained($0) }
         } else {
             // Retain all params that are used in any of the functions.
-            retain(params, usedIn: functionDecls)
+            retain(params, usedIn: methodDeclarations)
         }
     }
 

diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift
@@ -852,6 +852,18 @@ class RetentionTest: SourceGraphTestCase {
         }
     }
 
+    func testRetainsForeignProtocolParametersInSubclass() {
+        analyze(retainPublic: true) {
+            XCTAssertReferenced((.varParameter, "zone"),
+                                descendentOf: (.functionMethodInstance, "copy(with:)"),
+                                (.class, "FixtureClass109"))
+
+            XCTAssertReferenced((.varParameter, "zone"),
+                                descendentOf: (.functionMethodInstance, "copy(with:)"),
+                                (.class, "FixtureClass109Subclass"))
+        }
+    }
+
     func testRetainsForeignProtocolParameters() {
         analyze(retainPublic: true) {
             XCTAssertReferenced((.varParameter, "decoder"),

diff --git a/Tests/RetentionFixtures/testRetainsForeignProtocolParametersInSubclass.swift b/Tests/RetentionFixtures/testRetainsForeignProtocolParametersInSubclass.swift
@@ -0,0 +1,13 @@
+import Foundation
+
+public class FixtureClass109: NSCopying {
+    public func copy(with zone: NSZone?) -> Any {
+        FixtureClass109()
+    }
+}
+
+public class FixtureClass109Subclass: FixtureClass109 {
+    public override func copy(with zone: NSZone?) -> Any {
+        FixtureClass109Subclass()
+    }
+}