Fix redundant public accessibility for initializers & subscripts. (#308)

ileitch · web-flow · commit f8a7f31dc93f · 2021-05-27T19:28:02.000+02:00
diff --git a/Sources/PeripheryKit/Indexer/Declaration.swift b/Sources/PeripheryKit/Indexer/Declaration.swift
@@ -50,6 +50,14 @@ public final class Declaration {
             rawValue.hasPrefix("function")
         }
 
+        var isAccessibilityModifiableFunctionKind: Bool {
+            rawValue.hasPrefix("function.method")
+                || rawValue.hasPrefix("function.operator")
+                || rawValue == "function.subscript"
+                || rawValue == "function.free"
+                || rawValue == "function.constructor"
+        }
+
         static var variableKinds: Set<Kind> {
             Set(Kind.allCases.filter { $0.isVariableKind })
         }
@@ -58,6 +66,10 @@ public final class Declaration {
             rawValue.hasPrefix("var")
         }
 
+        var isAccessibilityModifiableVariableKind: Bool {
+            isVariableKind
+        }
+
         static var globalKinds: Set<Kind> = [
             .class,
             .protocol,
diff --git a/Sources/PeripheryKit/Indexer/SwiftIndexer.swift b/Sources/PeripheryKit/Indexer/SwiftIndexer.swift
@@ -214,16 +214,16 @@ public final class SwiftIndexer {
 
             file.importStatements = importVisitor.importStatements
             let propertyByLocation = propertyVisitor.resultsByLocation
-            let propertyByTypeLocation = propertyVisitor.resultsByTypeLocation
             let functionByLocation = functionVisitor.resultsByLocation
             let conformableByLocation = conformableVisitor.resultsByLocation
-            let propertyDeclarations = decls.filter { !$0.isImplicit && $0.kind.isVariableKind }
-            let functionDeclarations = decls.filter { !$0.isImplicit && $0.kind.isFunctionKind }
-            let conformableDeclarations = decls.filter { !$0.isImplicit && $0.kind.isDiscreteConformableKind }
+            let explicitDeclarations = decls.filter { !$0.isImplicit }
+            let propertyDeclarations = explicitDeclarations.filter { $0.kind.isAccessibilityModifiableVariableKind }
+            let functionDeclarations = explicitDeclarations.filter { $0.kind.isAccessibilityModifiableFunctionKind }
+            let conformableDeclarations = explicitDeclarations.filter { $0.kind.isDiscreteConformableKind }
 
             establishDeclarationHierarchy()
             makeProtocolPropertyAccessorsImplicit(for: decls)
-            associateDanglingReferences(for: decls, using: propertyByTypeLocation)
+            associateDanglingReferences(for: explicitDeclarations)
             identifyDeclaredPropertyTypes(for: propertyDeclarations, using: propertyByLocation)
             identifyPropertyReferenceRoles(for: propertyDeclarations, using: propertyByLocation)
             identifyFunctionReferenceRoles(for: functionDeclarations, using: functionByLocation)
@@ -297,34 +297,20 @@ public final class SwiftIndexer {
         // Workaround for https://bugs.swift.org/browse/SR-13766
         // Swift does not associate some type references with the containing declaration, resulting in references
         // with no clear parent.
-        private func associateDanglingReferences(for decls: [Declaration], using propertyByTypeLocation: [SourceLocation: [PropertyVisitor.Result]]) {
+        private func associateDanglingReferences(for declarations: [Declaration]) {
             guard !danglingReferences.isEmpty else { return }
 
-            let explicitDecls = decls.filter { !$0.isImplicit }
-            let declsByLocation = explicitDecls
-                .lazy
+            let declsByLocation = declarations
                 .reduce(into: [SourceLocation: [Declaration]]()) { (result, decl) in
                     result[decl.location, default: []].append(decl)
                 }
-            let declsByLine = explicitDecls
-                .lazy
+            let declsByLine = declarations
                 .reduce(into: [Int64: [Declaration]]()) { (result, decl) in
                     result[decl.location.line, default: []].append(decl)
                 }
 
             for ref in danglingReferences {
-                var propertyDecls: [Declaration]?
-
-                // First attempt to identify property declarations that may be associated with this reference.
-                if let propertyMetadatas = propertyByTypeLocation[ref.location] {
-                    for propertyMetadata in propertyMetadatas {
-                        if let decls = declsByLocation[propertyMetadata.location] {
-                            propertyDecls = decls
-                        }
-                    }
-                }
-
-                guard let candidateDecls = propertyDecls ??
+                guard let candidateDecls =
                         declsByLocation[ref.location] ??
                         declsByLine[ref.location.line] else { continue }
 
@@ -333,7 +319,11 @@ public final class SwiftIndexer {
                 // a decl without a parent, as the reference may be a related type of a class/struct/etc.
                 if let decl = candidateDecls.first(where: { $0.parent == nil }) {
                     associate(ref, with: decl)
-                } else if let decl = candidateDecls.first { // Fallback to using the first decl.
+                } else if let decl = candidateDecls.sorted().first {
+                    // Fallback to using the first declaration.
+                    // Sorting the declarations helps in the situation where the candidate declarations includes a
+                    // property/subscript, and a getter on the same line. The property/subscript is more likely to be
+                    // the declaration that should hold the references.
                     associate(ref, with: decl)
                 }
             }
diff --git a/Sources/PeripheryKit/Syntax/FunctionVisitor.swift b/Sources/PeripheryKit/Syntax/FunctionVisitor.swift
@@ -32,46 +32,72 @@ final class FunctionVisitor: PeripherySyntaxVisitor {
     }
 
     func visit(_ node: FunctionDeclSyntax) {
-        let location = sourceLocationBuilder.location(at: node.identifier.positionAfterSkippingLeadingTrivia)
-        var returnTypeLocations: Set<SourceLocation> = []
-        var parameterTypeLocations: Set<SourceLocation> = []
-        var genericParameterLocations: Set<SourceLocation> = []
-        var genericConformanceRequirementLocations: Set<SourceLocation> = []
-
-        if let returnTypeSyntax = node.signature.output?.returnType {
-            if let someReturnType = returnTypeSyntax.as(SomeTypeSyntax.self) {
-                returnTypeLocations = typeSyntaxInspector.typeLocations(for: someReturnType.baseType)
-            } else {
-                returnTypeLocations = typeSyntaxInspector.typeLocations(for: returnTypeSyntax)
-            }
-        }
+        results.append((sourceLocationBuilder.location(at: node.identifier.positionAfterSkippingLeadingTrivia),
+                        typeLocations(for: node.signature.input),
+                        typeLocations(for: node.signature.output),
+                        typeLocations(for: node.genericParameterClause),
+                        typeLocations(for: node.genericWhereClause)))
+    }
+
+    func visit(_ node: InitializerDeclSyntax) {
+        results.append((sourceLocationBuilder.location(at: node.initKeyword.positionAfterSkippingLeadingTrivia),
+                        typeLocations(for: node.parameters),
+                        [],
+                        typeLocations(for: node.genericParameterClause),
+                        typeLocations(for: node.genericWhereClause)))
+    }
 
-        for functionParameterSyntax in node.signature.input.parameterList {
-            if let typeSyntax = functionParameterSyntax.type {
-                parameterTypeLocations.formUnion(typeSyntaxInspector.typeLocations(for: typeSyntax))
+    func visit(_ node: SubscriptDeclSyntax) {
+        results.append((sourceLocationBuilder.location(at: node.subscriptKeyword.positionAfterSkippingLeadingTrivia),
+                        typeLocations(for: node.indices),
+                        typeLocations(for: node.result),
+                        typeLocations(for: node.genericParameterClause),
+                        typeLocations(for: node.genericWhereClause)))
+    }
+
+    // MARK: - Private
+
+    private func typeLocations(for parameterClause: ParameterClauseSyntax) -> Set<SourceLocation> {
+        return Set(parameterClause.parameterList.flatMap { parameter -> Set<SourceLocation> in
+            if let typeSyntax = parameter.type {
+                return typeSyntaxInspector.typeLocations(for: typeSyntax)
             }
+
+            return []
+        })
+    }
+
+    private func typeLocations(for returnClause: ReturnClauseSyntax?) -> Set<SourceLocation> {
+        guard let returnTypeSyntax = returnClause?.returnType else { return [] }
+
+        if let someReturnType = returnTypeSyntax.as(SomeTypeSyntax.self) {
+            return typeSyntaxInspector.typeLocations(for: someReturnType.baseType)
         }
 
-        if let genericParameterList = node.genericParameterClause?.genericParameterList {
-            for param in genericParameterList {
-                if let inheritedType = param.inheritedType {
-                    genericParameterLocations.formUnion(typeSyntaxInspector.typeLocations(for: inheritedType))
-                }
+        return typeSyntaxInspector.typeLocations(for: returnTypeSyntax)
+    }
+
+    private func typeLocations(for genericParameterClause: GenericParameterClauseSyntax?) -> Set<SourceLocation> {
+        guard let genericParameterClause = genericParameterClause else { return [] }
+
+        return Set(genericParameterClause.genericParameterList.flatMap { parameter -> Set<SourceLocation> in
+            if let inheritedType = parameter.inheritedType {
+                return typeSyntaxInspector.typeLocations(for: inheritedType)
             }
-        }
 
-        if let requirementList = node.genericWhereClause?.requirementList {
-            for requirement in requirementList {
-                if let conformanceRequirementType = requirement.body.as(ConformanceRequirementSyntax.self) {
-                    genericConformanceRequirementLocations.formUnion(typeSyntaxInspector.typeLocations(for: conformanceRequirementType.rightTypeIdentifier))
-                }
+            return []
+        })
+    }
+
+    private func typeLocations(for genericWhereClause: GenericWhereClauseSyntax?) -> Set<SourceLocation> {
+        guard let genericWhereClause = genericWhereClause else { return [] }
+
+        return Set(genericWhereClause.requirementList.flatMap { requirement -> Set<SourceLocation> in
+            if let conformanceRequirementType = requirement.body.as(ConformanceRequirementSyntax.self) {
+                return typeSyntaxInspector.typeLocations(for: conformanceRequirementType.rightTypeIdentifier)
             }
-        }
 
-        results.append((location,
-                        parameterTypeLocations,
-                        returnTypeLocations,
-                        genericParameterLocations,
-                        genericConformanceRequirementLocations))
+            return []
+        })
     }
 }
diff --git a/Sources/PeripheryKit/Syntax/PropertyVisitor.swift b/Sources/PeripheryKit/Syntax/PropertyVisitor.swift
@@ -19,12 +19,6 @@ final class PropertyVisitor: PeripherySyntaxVisitor {
     private let logger: Logger
     private(set) var results: [Result] = []
 
-    var resultsByTypeLocation: [SourceLocation: [Result]] {
-        results.reduce(into: [SourceLocation: [Result]]()) { (dict, result) in
-            result.typeLocations.forEach { dict[$0, default: []].append(result) }
-        }
-    }
-
     var resultsByLocation: [SourceLocation: Result] {
         results.reduce(into: [SourceLocation: Result]()) { (dict, result) in
             dict[result.location] = result
diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift
@@ -10,6 +10,11 @@ _ = PublicTypeUsedAsPublicPropertyTypeRetainer().retain1
 _ = PublicTypeUsedAsPublicPropertyTypeRetainer().retain2
 _ = PublicTypeUsedAsPublicPropertyTypeRetainer().retain3
 
+_ = PublicTypeUsedAsPublicInitializerParameterTypeRetainer()
+
+_ = PublicTypeUsedAsPublicSubscriptParameterTypeRetainer()[]
+_ = PublicTypeUsedAsPublicSubscriptReturnTypeRetainer()[]
+
 PublicTypeUsedAsPublicFunctionParameterTypeRetainer().retain1()
 PublicTypeUsedAsPublicFunctionParameterTypeRetainer().retain2()
 PublicTypeUsedAsPublicFunctionParameterTypeRetainer().retain3()
diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PublicTypeUsedAsPublicInitializerParameterType.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PublicTypeUsedAsPublicInitializerParameterType.swift
@@ -0,0 +1,7 @@
+import Foundation
+
+public class PublicTypeUsedAsPublicInitializerParameterType {}
+
+public class PublicTypeUsedAsPublicInitializerParameterTypeRetainer {
+    public init(_ cls: PublicTypeUsedAsPublicInitializerParameterType? = nil) {}
+}
diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PublicTypeUsedAsPublicSubscriptParameterType.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PublicTypeUsedAsPublicSubscriptParameterType.swift
@@ -0,0 +1,8 @@
+import Foundation
+
+public class PublicTypeUsedAsPublicSubscriptParameterType {}
+
+public class PublicTypeUsedAsPublicSubscriptParameterTypeRetainer {
+    public init() {}
+    public subscript(_ type: PublicTypeUsedAsPublicSubscriptParameterType? = nil) -> Int { 0 }
+}
diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PublicTypeUsedAsPublicSubscriptReturnType.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PublicTypeUsedAsPublicSubscriptReturnType.swift
@@ -0,0 +1,8 @@
+import Foundation
+
+public class PublicTypeUsedAsPublicSubscriptReturnType {}
+
+public class PublicTypeUsedAsPublicSubscriptReturnTypeRetainer {
+    public init() {}
+    public subscript(_ idx: Int = 0) -> PublicTypeUsedAsPublicSubscriptReturnType { .init() }
+}
diff --git a/Tests/AccessibilityTests/RedundantPublicAccessabilityTest.swift b/Tests/AccessibilityTests/RedundantPublicAccessabilityTest.swift
@@ -59,6 +59,10 @@ class RedundantPublicAccessabilityTest: SourceGraphTestCase {
         XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicPropertyArrayType"))
     }
 
+    func testPublicTypeUsedAsPublicInitializerParameterType() {
+        XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicInitializerParameterType"))
+    }
+
     func testPublicTypeUsedAsPublicFunctionParameterType() {
         XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicFunctionParameterType"))
         XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicFunctionParameterTypeClosureArgument"))
@@ -71,6 +75,14 @@ class RedundantPublicAccessabilityTest: SourceGraphTestCase {
         XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicFunctionReturnTypeClosureReturnType"))
     }
 
+    func testPublicTypeUsedAsPublicSubscriptParameterType() {
+        XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicSubscriptParameterType"))
+    }
+
+    func testPublicTypeUsedAsPublicSubscriptReturnType() {
+        XCTAssertNotRedundantPublicAccessibility((.class, "PublicTypeUsedAsPublicSubscriptReturnType"))
+    }
+
     func testPublicTypeUsedInPublicFunctionBody() {
         XCTAssertRedundantPublicAccessibility((.class, "PublicTypeUsedInPublicFunctionBody"))
     }
diff --git a/Tests/Fixtures/FunctionVisitorFixtures/FunctionVisitorFixture.swift b/Tests/Fixtures/FunctionVisitorFixtures/FunctionVisitorFixture.swift
@@ -21,3 +21,16 @@ func functionWithGenericArgument<T: StringProtocol & AnyObject>(_ t: T.Type) whe
 
 @available(macOS 10.15.0, *)
 func functionWithSomeReturnType() -> some StringProtocol { "" }
+
+class FixtureClass1 {
+    init(a: String, b: Int) {}
+}
+
+class FixtureClass2 {
+    init<T: StringProtocol & AnyObject>(_ t: T.Type) where T: RawRepresentable {}
+}
+
+class FixtureClass3 {
+    subscript(a: Int, b: String) -> Int { 0 }
+    subscript<T: StringProtocol & AnyObject>(_ t: T.Type) -> Int where T: RawRepresentable { 0 }
+}
diff --git a/Tests/PeripheryTests/Syntax/FunctionVisitorTest.swift b/Tests/PeripheryTests/Syntax/FunctionVisitorTest.swift
@@ -92,6 +92,62 @@ class FunctionVisitorTest: XCTestCase {
         XCTAssertTrue(result?.genericConformanceRequirementLocations.isEmpty ?? false)
     }
 
+    func testInitializerWithArguments() throws {
+        let result = results[fixtureLocation(line: 26, column: 5)]
+        XCTAssertEqual(result?.parameterTypeLocations, [
+            fixtureLocation(line: 26, column: 13),
+            fixtureLocation(line: 26, column: 24),
+        ])
+        XCTAssertTrue(result?.returnTypeLocations.isEmpty ?? false)
+        XCTAssertTrue(result?.genericParameterLocations.isEmpty ?? false)
+        XCTAssertTrue(result?.genericConformanceRequirementLocations.isEmpty ?? false)
+    }
+
+    func testInitializerWithGenericArguments() throws {
+        let result = results[fixtureLocation(line: 30, column: 5)]
+        XCTAssertEqual(result?.parameterTypeLocations, [
+            fixtureLocation(line: 30, column: 46)
+        ])
+        XCTAssertEqual(result?.genericParameterLocations, [
+            fixtureLocation(line: 30, column: 13),
+            fixtureLocation(line: 30, column: 30)
+        ])
+        XCTAssertEqual(result?.genericConformanceRequirementLocations, [
+            fixtureLocation(line: 30, column: 63),
+        ])
+        XCTAssertTrue(result?.returnTypeLocations.isEmpty ?? false)
+    }
+
+    func testSubscriptWithArguments() throws {
+        let result = results[fixtureLocation(line: 34, column: 5)]
+        XCTAssertEqual(result?.parameterTypeLocations, [
+            fixtureLocation(line: 34, column: 18),
+            fixtureLocation(line: 34, column: 26),
+        ])
+        XCTAssertEqual(result?.returnTypeLocations, [
+            fixtureLocation(line: 34, column: 37)
+        ])
+        XCTAssertTrue(result?.genericParameterLocations.isEmpty ?? false)
+        XCTAssertTrue(result?.genericConformanceRequirementLocations.isEmpty ?? false)
+    }
+
+    func testSubscriptWithGenericArguments() throws {
+        let result = results[fixtureLocation(line: 35, column: 5)]
+        XCTAssertEqual(result?.parameterTypeLocations, [
+            fixtureLocation(line: 35, column: 51)
+        ])
+        XCTAssertEqual(result?.returnTypeLocations, [
+            fixtureLocation(line: 35, column: 62)
+        ])
+        XCTAssertEqual(result?.genericParameterLocations, [
+            fixtureLocation(line: 35, column: 18),
+            fixtureLocation(line: 35, column: 35)
+        ])
+        XCTAssertEqual(result?.genericConformanceRequirementLocations, [
+            fixtureLocation(line: 35, column: 75),
+        ])
+    }
+
     // MARK: - Private
 
     private var fixturePath: SourceFile {
