Name the inherited protocols that should replace a redundant protocol…

… conformance
peripheryapp · Jan 2, 2024 · c2d32dc · c2d32dc
1 parent 8f5d47e
commit c2d32dc
Show file tree

Hide file tree

Showing 10 changed files with 73 additions and 21 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,7 @@
 - The option `--external-encodable-protocols` is deprecated, use `--external-codable-protocols` instead.
 - Assign-only properties on structs with synthesized initializers are now detected.
 - Added the `--retain-codable-properties` option to retain all properties on Codable types.
+- Results for redundant protocol conformances will now list the inherited protocols that should replace the redundant conformance.
 
 ##### Bug Fixes
 

diff --git a/Sources/PeripheryKit/Formatters/CsvFormatter.swift b/Sources/PeripheryKit/Formatters/CsvFormatter.swift
@@ -27,7 +27,7 @@ final class CsvFormatter: OutputFormatter {
             lines.append(line)
 
             switch result.annotation {
-            case let .redundantProtocol(references: references):
+            case let .redundantProtocol(references, inherited):
                 for ref in references {
                     let line = format(
                         kind: ref.kind.rawValue,
@@ -37,7 +37,7 @@ final class CsvFormatter: OutputFormatter {
                         accessibility: nil,
                         usrs: [ref.usr],
                         location: ref.location,
-                        hint: redundantConformanceHint)
+                        hint: redundantConformanceHint(with: inherited))
                     lines.append(line)
                 }
             default:

diff --git a/Sources/PeripheryKit/Formatters/JsonFormatter.swift b/Sources/PeripheryKit/Formatters/JsonFormatter.swift
@@ -28,7 +28,7 @@ final class JsonFormatter: OutputFormatter {
             jsonObject.append(object)
 
             switch result.annotation {
-            case let .redundantProtocol(references: references):
+            case let .redundantProtocol(references, inherited):
                 for ref in references {
                     let object: [AnyHashable: Any] = [
                         "kind": ref.kind.rawValue,
@@ -37,7 +37,7 @@ final class JsonFormatter: OutputFormatter {
                         "attributes": Array<String>(),
                         "accessibility": "",
                         "ids": [ref.usr],
-                        "hints": [redundantConformanceHint],
+                        "hints": [redundantConformanceHint(with: inherited)],
                         "location": locationDescription(ref.location)
                     ]
                     jsonObject.append(object)

diff --git a/Sources/PeripheryKit/Formatters/OutputFormatter.swift b/Sources/PeripheryKit/Formatters/OutputFormatter.swift
@@ -10,15 +10,22 @@ public protocol OutputFormatter: AnyObject {
 }
 
 extension OutputFormatter {
-    var redundantConformanceHint: String { "redundantConformance" }
+    func redundantConformanceHint(with inherited: Set<String>) -> String {
+        var msg = "redundantConformance"
+        if !inherited.isEmpty {
+            msg += "(replace with: '\(inherited.sorted().joined(separator: ", "))')"
+        }
+
+        return msg
+    }
 
     func describe(_ annotation: ScanResult.Annotation) -> String {
         switch annotation {
         case .unused:
             return "unused"
         case .assignOnlyProperty:
             return "assignOnlyProperty"
-        case .redundantProtocol(_):
+        case .redundantProtocol(_, _):
             return "redundantProtocol"
         case .redundantPublicAccessibility:
             return "redundantPublicAccessibility"
@@ -43,10 +50,16 @@ extension OutputFormatter {
                 description += " is unused"
             case .assignOnlyProperty:
                 description += " is assigned, but never used"
-            case let .redundantProtocol(references):
+            case let .redundantProtocol(references, inherited):
                 description += " is redundant as it's never used as an existential type"
                 secondaryResults = references.map {
-                    ($0.location, "Protocol '\(name)' conformance is redundant")
+                    var msg = "Protocol '\(name)' conformance is redundant"
+
+                    if !inherited.isEmpty {
+                        msg += ", replace with '\(inherited.sorted().joined(separator: ", "))'"
+                    }
+
+                    return ($0.location, msg)
                 }
             case let .redundantPublicAccessibility(modules):
                 let modulesJoined = modules.sorted().joined(separator: ", ")

diff --git a/Sources/PeripheryKit/ScanResult.swift b/Sources/PeripheryKit/ScanResult.swift
@@ -4,7 +4,7 @@ public struct ScanResult {
     enum Annotation {
         case unused
         case assignOnlyProperty
-        case redundantProtocol(references: Set<Reference>)
+        case redundantProtocol(references: Set<Reference>, inherited: Set<String>)
         case redundantPublicAccessibility(modules: Set<String>)
     }
 

diff --git a/Sources/PeripheryKit/ScanResultBuilder.swift b/Sources/PeripheryKit/ScanResultBuilder.swift
@@ -16,7 +16,8 @@ public struct ScanResultBuilder {
             .init(declaration: $0, annotation: .assignOnlyProperty)
         }
         let annotatedRedundantProtocols: [ScanResult] = redundantProtocols.map {
-            .init(declaration: $0.0, annotation: .redundantProtocol(references: $0.1))
+            let inherited = graph.inheritedTypeReferences(of: $0.0).compactMapSet { $0.name }
+            return .init(declaration: $0.0, annotation: .redundantProtocol(references: $0.1, inherited: inherited))
         }
         let annotatedRedundantPublicAccessibility: [ScanResult] = redundantPublicAccessibility.map {
             .init(declaration: $0.0, annotation: .redundantPublicAccessibility(modules: $0.1))

diff --git a/Sources/PeripheryKit/SourceGraph/SourceGraph.swift b/Sources/PeripheryKit/SourceGraph/SourceGraph.swift
@@ -255,19 +255,19 @@ public final class SourceGraph {
         }
     }
 
-    func inheritedTypeReferences(of decl: Declaration, seenDeclarations: Set<Declaration> = []) -> [Reference] {
-        var references: [Reference] = []
+    func inheritedTypeReferences(of decl: Declaration, seenDeclarations: Set<Declaration> = []) -> Set<Reference> {
+        var references = Set<Reference>()
 
         for reference in decl.immediateInheritedTypeReferences {
-            references.append(reference)
+            references.insert(reference)
 
             if let inheritedDecl = explicitDeclaration(withUsr: reference.usr) {
                 // Detect circular references. The following is valid Swift.
                 // class SomeClass {}
                 // extension SomeClass: SomeProtocol {}
                 // protocol SomeProtocol: SomeClass {}
                 guard !seenDeclarations.contains(inheritedDecl) else { continue }
-                references = inheritedTypeReferences(of: inheritedDecl, seenDeclarations: seenDeclarations.union([decl])) + references
+                references = inheritedTypeReferences(of: inheritedDecl, seenDeclarations: seenDeclarations.union([decl])).union(references)
             }
         }
 

diff --git a/Tests/Fixtures/RetentionFixtures/testRedundantProtocolThatInheritsOtherProtocols.swift b/Tests/Fixtures/RetentionFixtures/testRedundantProtocolThatInheritsOtherProtocols.swift
@@ -0,0 +1,14 @@
+import Foundation
+
+protocol FixtureProtocol128_Inherited {
+    func funcA()
+}
+
+protocol FixtureProtocol128: FixtureProtocol128_Inherited {
+    func funcB()
+}
+
+public class FixtureClass134: FixtureProtocol128 {
+    func funcA() {}
+    func funcB() {}
+}
diff --git a/Tests/PeripheryTests/RetentionTest.swift b/Tests/PeripheryTests/RetentionTest.swift
@@ -173,6 +173,19 @@ final class RetentionTest: FixtureSourceGraphTestCase {
         }
     }
 
+    func testRedundantProtocolThatInheritsOtherProtocols() {
+        analyze(retainPublic: true) {
+            assertReferenced(.class("FixtureClass134"))
+
+            assertReferenced(.protocol("FixtureProtocol128"))
+            assertRedundantProtocol(
+                "FixtureProtocol128",
+                implementedBy: .class("FixtureClass134"),
+                inherits: .protocol("FixtureProtocol128_Inherited")
+            )
+        }
+    }
+
     func testProtocolUsedAsExistentialType() {
         analyze(retainPublic: true) {
             assertReferenced(.class("FixtureClass119"))

diff --git a/Tests/Shared/SourceGraphTestCase.swift b/Tests/Shared/SourceGraphTestCase.swift
@@ -71,17 +71,27 @@ open class SourceGraphTestCase: XCTestCase {
         }
     }
 
-    func assertRedundantProtocol(_ name: String, implementedBy conformances: DeclarationDescription..., file: StaticString = #file, line: UInt = #line) {
+    func assertRedundantProtocol(
+        _ name: String,
+        implementedBy conformances: DeclarationDescription...,
+        inherits inheritedProtocols: DeclarationDescription...,
+        file: StaticString = #file, line: UInt = #line) {
         guard let declaration = materialize(.protocol(name), file: file, line: line) else { return }
 
-        if let references = Self.results.redundantProtocolDeclarations[declaration] {
-            let decls = references.compactMap { $0.parent }
+        if let tuple = Self.results.redundantProtocolDeclarations[declaration] {
+            let decls = tuple.references.compactMap { $0.parent }
 
             for conformance in conformances {
                 if !decls.contains(where: { $0.kind == conformance.kind && $0.name == conformance.name }) {
                     XCTFail("Expected \(conformance) to implement protocol '\(name)'.", file: file, line: line)
                 }
             }
+
+            for inherited in inheritedProtocols {
+                if !tuple.inherited.contains(inherited.name) {
+                    XCTFail("Expected \(name) to inherit protocol '\(inherited.name)'.", file: file, line: line)
+                }
+            }
         } else {
             XCTFail("Expected '\(name)' to be redundant.", file: file, line: line)
         }
@@ -231,10 +241,10 @@ private extension Array where Element == ScanResult {
         }
     }
 
-    var redundantProtocolDeclarations: [Declaration: [Reference]] {
-        reduce(into: [Declaration: [Reference]]()) { result, scanResult in
-            if case let .redundantProtocol(references) = scanResult.annotation {
-                result[scanResult.declaration, default: []].append(contentsOf: references)
+    var redundantProtocolDeclarations: [Declaration: (references: Set<Reference>, inherited: Set<String>)] {
+        reduce(into: .init()) { result, scanResult in
+            if case let .redundantProtocol(references, inherited) = scanResult.annotation {
+                result[scanResult.declaration] = (references, inherited)
             }
         }
     }