Open class method params (#271)

* 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 * Fixed false positive for open methods parameters * Updated changelog Co-authored-by: Ian Leitch <[email protected]>
peripheryapp · Feb 11, 2021 · f4df299 · f4df299
1 parent 1f19db3
commit f4df299
Show file tree

Hide file tree

Showing 4 changed files with 42 additions and 15 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,6 +12,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`.
+- Open method parameters 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.
 

diff --git a/Sources/PeripheryKit/Analyzer/Visitors/UnusedParameterRetainer.swift b/Sources/PeripheryKit/Analyzer/Visitors/UnusedParameterRetainer.swift
@@ -47,7 +47,7 @@ final class UnusedParameterRetainer: SourceGraphVisitor {
                     if configuration.retainUnusedProtocolFuncParams {
                         functionDecl.unusedParameters.forEach { graph.markRetained($0) }
                     } else {
-                        retain(functionDecl.unusedParameters, usedIn: allFunctionDecls)
+                        retain(params: functionDecl.unusedParameters, usedIn: allFunctionDecls)
                     }
                 }
             }
@@ -64,12 +64,12 @@ final class UnusedParameterRetainer: SourceGraphVisitor {
                 + graph.superclasses(of: classDeclaration)
                 + graph.subclasses(of: classDeclaration)
 
-            allMethodDeclarations = allClassDeclarations.compactMap { declaration in
+            allMethodDeclarations = allClassDeclarations.flatMap { declaration in
                 declaration
                     .declarations
                     .lazy
                     .filter { $0.kind == methodDeclaration.kind }
-                    .first { $0.name == methodDeclaration.name }
+                    .filter { $0.name == methodDeclaration.name }
             }
 
             retainIfNeeded(
@@ -110,23 +110,28 @@ final class UnusedParameterRetainer: SourceGraphVisitor {
     }
 
     private func retainIfNeeded(params: Set<Declaration>, inOverridenMethods methodDeclarations: [Declaration]) {
-        let isSuperclassForeign = methodDeclarations.allSatisfy { declaration in
-            declaration.modifiers.contains("override")
+        guard let baseDeclaration = methodDeclarations.first(where: { !$0.modifiers.contains("override") }) else {
+            // Must be overriding a declaration in a foreign class.
+            return retainAllUnusedParams(inMethods: methodDeclarations)
         }
 
-        if isSuperclassForeign {
-            // Must be overriding a declaration in a foreign class.
-            methodDeclarations
-                .lazy
-                .flatMap { $0.unusedParameters }
-                .forEach { graph.markRetained($0) }
-        } else {
-            // Retain all params that are used in any of the functions.
-            retain(params, usedIn: methodDeclarations)
+        guard baseDeclaration.accessibility.value != .open || !configuration.retainPublic else {
+            // Parameters can be used in methods that are overridden from the outside
+            return retainAllUnusedParams(inMethods: methodDeclarations)
         }
+
+        // Retain all params that are used in any of the functions.
+        return retain(params: params, usedIn: methodDeclarations)
+    }
+
+    private func retainAllUnusedParams(inMethods methodDeclarations: [Declaration]) {
+        methodDeclarations
+            .lazy
+            .flatMap { $0.unusedParameters }
+            .forEach { graph.markRetained($0) }
     }
 
-    private func retain(_ params: Set<Declaration>, usedIn functionDecls: [Declaration]) {
+    private func retain(params: Set<Declaration>, usedIn functionDecls: [Declaration]) {
         for param in params {
             if isParam(param, usedInAnyOf: functionDecls) {
                 graph.markRetained(param)

diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift
@@ -1012,6 +1012,14 @@ class RetentionTest: SourceGraphTestCase {
         }
     }
 
+    func testRetainsOpenClassParameters() {
+        analyze(retainPublic: true) {
+            XCTAssertReferenced((.varParameter, "value"),
+                                descendentOf: (.functionMethodInstance, "doSomething(with:)"),
+                                (.class, "FixtureClass112"))
+        }
+    }
+
     func testIgnoreUnusedParamInUnusedFunction() {
         analyze() {
             XCTAssertNotReferenced((.class, "FixtureClass105"))

diff --git a/Tests/RetentionFixtures/testRetainsOpenClassParameters.swift b/Tests/RetentionFixtures/testRetainsOpenClassParameters.swift
@@ -0,0 +1,13 @@
+import Foundation
+
+open class FixtureClass112 {
+    open func doSomething(with value: Int) { }
+}
+
+public class FixtureClass112Retainer {
+    var instance: FixtureClass112?
+
+    public func doSomething() {
+        instance?.doSomething(with: .random(in: 0...10))
+    }
+}