From 1f19db3f0b3df0b00dcf9a94a299557439ff1a56 Mon Sep 17 00:00:00 2001 From: Almaz Ibragimov Date: Thu, 11 Feb 2021 14:07:26 +0300 Subject: [PATCH] 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 --- CHANGELOG.md | 1 + .../Visitors/UnusedParameterRetainer.swift | 83 ++++++++++++------- Tests/PeripheryTests/RetentionTest.swift | 12 +++ ...sForeignProtocolParametersInSubclass.swift | 13 +++ 4 files changed, 81 insertions(+), 28 deletions(-) create mode 100644 Tests/RetentionFixtures/testRetainsForeignProtocolParametersInSubclass.swift diff --git a/CHANGELOG.md b/CHANGELOG.md index 9631453cbd..498dd6ed9f 100644 --- 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 index 9e923b45fc..2be76c2767 100644 --- 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 = 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, 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, 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, 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, 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 index da87f6ac49..0343d458e0 100644 --- 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 new file mode 100644 index 0000000000..b9dad09a84 --- /dev/null +++ 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() + } +}