Skip to content

Commit

Permalink
Fix redundant public accessibility analysis false-positive for retain…
Browse files Browse the repository at this point in the history 
…ed/ignored declarations, closes #688
  • Loading branch information
@ileitch
ileitch committed Dec 20, 2023
1 parent 5f5e7e5 commit fffa73d
Show file tree
Hide file tree
Showing 6 changed files with 83 additions and 49 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- Fix redundant public accessibility analysis false-positive for function parameter default values.
- Fix redundant public accessibility analysis false-positive for inherited and default associated types.
- Fix redundant public accessibility analysis false-positive for generic types used in the generic argument clause of a return type.
- Fix redundant public accessibility analysis false-positive for retained/ignored declarations.

## 2.17.1 (2023-12-04)

Expand Down
3 changes: 2 additions & 1 deletion Sources/PeripheryKit/ScanResultBuilder.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,8 @@ public struct ScanResultBuilder {
.filter {
!$0.declaration.isImplicit &&
!$0.declaration.kind.isAccessorKind &&
!graph.ignoredDeclarations.contains($0.declaration)
!graph.ignoredDeclarations.contains($0.declaration) &&
!graph.retainedDeclarations.contains($0.declaration)
}

return result
Expand Down
11 changes: 11 additions & 0 deletions ...sibilityTests/AccessibilityProject/Sources/TargetA/PublicComparableOperatorFunction.swift
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
import Foundation

public class PublicComparableOperatorFunction: Comparable {}

public func < (lhs: PublicComparableOperatorFunction, rhs: PublicComparableOperatorFunction) -> Bool {
true
}

public func == (lhs: PublicComparableOperatorFunction, rhs: PublicComparableOperatorFunction) -> Bool {
true
}
7 changes: 7 additions & 0 deletions Tests/AccessibilityTests/RedundantPublicAccessibilityTest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,4 +300,11 @@ class RedundantPublicAccessibilityTest: SourceGraphTestCase {

assertNotRedundantPublicAccessibility(.protocol("PublicInheritedAssociatedTypeDefaultType"))
}

func testPublicComparableOperatorFunction() {
Self.index()

assertNotRedundantPublicAccessibility(.functionOperatorInfix("<(_:_:)"))
assertNotRedundantPublicAccessibility(.functionOperatorInfix("==(_:_:)"))
}
}
48 changes: 12 additions & 36 deletions Tests/PeripheryTests/RetentionTest.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -91,9 +91,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {

func testSelfReferencedProperty() {
analyze() {
assertNotReferenced(.class("FixtureClass39")) {
self.assertNotReferenced(.varInstance("someVar"))
}
assertNotReferenced(.class("FixtureClass39"))
}
}

Expand All @@ -118,11 +116,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {

func testDeeplyNestedClassReferences() {
analyze() {
assertNotReferenced(.class("FixtureClass17")) {
self.assertNotReferenced(.class("FixtureClass18")) {
self.assertNotReferenced(.class("FixtureClass19"))
}
}
assertNotReferenced(.class("FixtureClass17"))
}
}

Expand Down Expand Up @@ -295,9 +289,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {

func testRetainedProtocolDoesNotRetainUnusedClass() {
analyze(retainPublic: true) {
assertNotReferenced(.class("FixtureClass57")) {
self.assertNotReferenced(.functionMethodInstance("protocolMethod()"))
}
assertNotReferenced(.class("FixtureClass57"))
assertReferenced(.protocol("FixtureProtocol57"))
}
}
Expand All @@ -307,9 +299,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {
assertReferenced(.protocol("FixtureProtocol200")) {
self.assertReferenced(.functionMethodInstance("protocolFunc()"))
}
assertNotReferenced(.class("FixtureClass200")) {
self.assertNotReferenced(.functionMethodInstance("protocolFunc()"))
}
assertNotReferenced(.class("FixtureClass200"))
assertNotReferenced(.class("FixtureClass201"))
}
}
Expand Down Expand Up @@ -794,10 +784,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {
func testDoesNotRetainDescendantsOfUnusedDeclaration() {
analyze(retainPublic: true) {
assertReferenced(.class("FixtureClass99Outer")) {
self.assertNotReferenced(.class("FixtureClass99")) {
self.assertNotReferenced(.functionMethodInstance("someMethod()"))
self.assertNotReferenced(.varInstance("someVar"))
}
self.assertNotReferenced(.class("FixtureClass99"))
}
}
}
Expand Down Expand Up @@ -1056,16 +1043,9 @@ final class RetentionTest: FixtureSourceGraphTestCase {

analyze(retainPublic: true) {
assertReferenced(.class("FixtureClass70")) {
self.assertNotReferenced(.varInstance("simpleUnreadVar"))
self.assertAssignOnlyProperty(.varInstance("simpleUnreadVar"))

self.assertNotReferenced(.varInstance("simpleUnreadShadowedVar"))
self.assertAssignOnlyProperty(.varInstance("simpleUnreadShadowedVar"))

self.assertNotReferenced(.varInstance("simpleUnreadVarAssignedMultiple"))
self.assertAssignOnlyProperty(.varInstance("simpleUnreadVarAssignedMultiple"))

self.assertNotReferenced(.varStatic("simpleStaticUnreadVar"))
self.assertAssignOnlyProperty(.varStatic("simpleStaticUnreadVar"))

self.assertReferenced(.varInstance("complexUnreadVar1"))
Expand Down Expand Up @@ -1116,7 +1096,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {
func testSimpleAssignOnlyPropertyNameConflict() {
analyze(retainPublic: true) {
assertReferenced(.class("FixtureClass131")) {
self.assertNotReferenced(.varInstance("someProperty"))
self.assertAssignOnlyProperty(.varInstance("someProperty"))
self.assertReferenced(.varStatic("someProperty"))
}
}
Expand All @@ -1138,11 +1118,11 @@ final class RetentionTest: FixtureSourceGraphTestCase {
self.assertReferenced(.varInstance("retainedAnyCancellable"))
#endif

self.assertNotReferenced(.varInstance("notRetainedSimpleProperty"))
self.assertNotReferenced(.varInstance("notRetainedModulePrefixedProperty"))
self.assertNotReferenced(.varInstance("notRetainedTupleProperty"))
self.assertNotReferenced(.varInstance("notRetainedDestructuredPropertyB"))
self.assertNotReferenced(.varInstance("notRetainedMultipleBindingPropertyB"))
self.assertAssignOnlyProperty(.varInstance("notRetainedSimpleProperty"))
self.assertAssignOnlyProperty(.varInstance("notRetainedModulePrefixedProperty"))
self.assertAssignOnlyProperty(.varInstance("notRetainedTupleProperty"))
self.assertAssignOnlyProperty(.varInstance("notRetainedDestructuredPropertyB"))
self.assertAssignOnlyProperty(.varInstance("notRetainedMultipleBindingPropertyB"))
}
}
}
Expand Down Expand Up @@ -1483,11 +1463,7 @@ final class RetentionTest: FixtureSourceGraphTestCase {

func testIgnoreUnusedParamInUnusedFunction() {
analyze() {
assertNotReferenced(.class("FixtureClass105")) {
self.assertNotReferenced(.functionMethodInstance("unused(param:)")) {
self.assertNotReferenced(.varParameter("param"))
}
}
assertNotReferenced(.class("FixtureClass105"))
}
}

Expand Down
62 changes: 50 additions & 12 deletions Tests/Shared/SourceGraphTestCase.swift
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,10 @@ import Shared
open class SourceGraphTestCase: XCTestCase {
static var driver: ProjectDriver!
static var configuration: Configuration!
static var graph = SourceGraph()

private static var graph = SourceGraph()
private static var allIndexedDeclarations: Set<Declaration> = []
private static var results: [ScanResult] = []

var configuration: Configuration { Self.configuration }

Expand Down Expand Up @@ -47,6 +48,7 @@ open class SourceGraphTestCase: XCTestCase {
try! Self.driver.index(graph: graph)
allIndexedDeclarations = graph.allDeclarations
try! SourceGraphMutatorRunner.perform(graph: graph)
results = ScanResultBuilder.build(for: graph)
}

func assertReferenced(_ description: DeclarationDescription, scopedAssertions: (() -> Void)? = nil, file: StaticString = #file, line: UInt = #line) {
Expand All @@ -61,22 +63,18 @@ open class SourceGraphTestCase: XCTestCase {
scopeStack.removeLast()
}

func assertNotReferenced(_ description: DeclarationDescription, scopedAssertions: (() -> Void)? = nil, file: StaticString = #file, line: UInt = #line) {
func assertNotReferenced(_ description: DeclarationDescription, file: StaticString = #file, line: UInt = #line) {
guard let declaration = materialize(description, file: file, line: line) else { return }

if !Self.graph.unusedDeclarations.contains(declaration) {
if !Self.results.unusedDeclarations.contains(declaration) {
XCTFail("Expected declaration to not be referenced: \(declaration)", file: file, line: line)
}

scopeStack.append(.declaration(declaration))
scopedAssertions?()
scopeStack.removeLast()
}

func assertRedundantProtocol(_ name: String, implementedBy conformances: DeclarationDescription..., file: StaticString = #file, line: UInt = #line) {
guard let declaration = materialize(.protocol(name), file: file, line: line) else { return }

if let references = Self.graph.redundantProtocols[declaration] {
if let references = Self.results.redundantProtocolDeclarations[declaration] {
let decls = references.compactMap { $0.parent }

for conformance in conformances {
Expand All @@ -92,7 +90,7 @@ open class SourceGraphTestCase: XCTestCase {
func assertNotRedundantProtocol(_ name: String, file: StaticString = #file, line: UInt = #line) {
guard let declaration = materialize(.protocol(name), file: file, line: line) else { return }

if Self.graph.redundantProtocols.keys.contains(declaration) {
if Self.results.redundantProtocolDeclarations.keys.contains(declaration) {
XCTFail("Expected '\(name)' to not be redundant.", file: file, line: line)
}
}
Expand All @@ -112,7 +110,7 @@ open class SourceGraphTestCase: XCTestCase {
func assertRedundantPublicAccessibility(_ description: DeclarationDescription, scopedAssertions: (() -> Void)? = nil, file: StaticString = #file, line: UInt = #line) {
guard let declaration = materialize(description, in: Self.allIndexedDeclarations, file: file, line: line) else { return }

if !Self.graph.redundantPublicAccessibility.keys.contains(declaration) {
if !Self.results.redundantPublicAccessibilityDeclarations.contains(declaration) {
XCTFail("Expected declaration to have redundant public accessibility: \(declaration)", file: file, line: line)
}

Expand All @@ -124,7 +122,7 @@ open class SourceGraphTestCase: XCTestCase {
func assertNotRedundantPublicAccessibility(_ description: DeclarationDescription, scopedAssertions: (() -> Void)? = nil, file: StaticString = #file, line: UInt = #line) {
guard let declaration = materialize(description, in: Self.allIndexedDeclarations, file: file, line: line) else { return }

if Self.graph.redundantPublicAccessibility.keys.contains(declaration) {
if Self.results.redundantPublicAccessibilityDeclarations.contains(declaration) {
XCTFail("Expected declaration to not have redundant public accessibility: \(declaration)", file: file, line: line)
}

Expand All @@ -144,7 +142,7 @@ open class SourceGraphTestCase: XCTestCase {
func assertAssignOnlyProperty(_ description: DeclarationDescription, scopedAssertions: (() -> Void)? = nil, file: StaticString = #file, line: UInt = #line) {
guard let declaration = materialize(description, file: file, line: line) else { return }

if !Self.graph.assignOnlyProperties.contains(declaration) {
if !Self.results.assignOnlyPropertyDeclarations.contains(declaration) {
XCTFail("Expected property to be assign-only: \(declaration)", file: file, line: line)
}

Expand Down Expand Up @@ -211,3 +209,43 @@ open class SourceGraphTestCase: XCTestCase {
}
}
}

private extension Array where Element == ScanResult {
var unusedDeclarations: Set<Declaration> {
compactMapSet {
if case .unused = $0.annotation {
return $0.declaration
}

return nil
}
}

var assignOnlyPropertyDeclarations: Set<Declaration> {
compactMapSet {
if case .assignOnlyProperty = $0.annotation {
return $0.declaration
}

return nil
}
}

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 redundantPublicAccessibilityDeclarations: Set<Declaration> {
compactMapSet {
if case .redundantPublicAccessibility = $0.annotation {
return $0.declaration
}

return nil
}
}
}

0 comments on commit fffa73d

Please sign in to comment.