From 626ad9732729a694c87a2b34bb074342fada1ce7 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Fri, 25 Jul 2025 09:17:01 -0700 Subject: [PATCH 01/30] Initial commit adding redundant internal and redundant fileprivate access --- Sources/BUILD.bazel | 2 + Sources/Configuration/Configuration.swift | 7 ++ Sources/Frontend/Commands/ScanCommand.swift | 8 ++ .../Results/OutputFormatter.swift | 10 +++ Sources/PeripheryKit/ScanResult.swift | 2 + Sources/PeripheryKit/ScanResultBuilder.swift | 12 ++- .../SourceGraph/Elements/Declaration.swift | 2 + .../Mutators/ExtensionReferenceBuilder.swift | 13 +++ ...undantFilePrivateAccessibilityMarker.swift | 70 ++++++++++++++++ ...RedundantInternalAccessibilityMarker.swift | 82 +++++++++++++++++++ Sources/SourceGraph/SourceGraph.swift | 21 +++++ .../SourceGraphMutatorRunner.swift | 4 + .../Sources/MainTarget/main.swift | 3 + .../TargetA/InternalPropertyExtension.swift | 9 ++ .../TargetA/InternalPropertyOwner.swift | 4 + .../InternalPropertyUsedInExtension.swift | 8 ++ .../InternalPropertyUserExtension.swift | 6 ++ .../NotRedundantFilePrivateComponents.swift | 28 +++++++ .../NotRedundantInternalClassComponents.swift | 22 +++++ ...ndantInternalClassComponents_Support.swift | 16 ++++ .../RedundantFilePrivateComponents.swift | 19 +++++ .../TargetA/RedundantInternalComponents.swift | 19 +++++ ...edundantFilePrivateAccessibilityTest.swift | 22 +++++ .../RedundantInternalAccessibilityTest.swift | 51 ++++++++++++ Tests/Shared/SourceGraphTestCase.swift | 68 +++++++++++++++ 25 files changed, 507 insertions(+), 1 deletion(-) create mode 100644 Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift create mode 100644 Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift create mode 100644 Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift create mode 100644 Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift diff --git a/Sources/BUILD.bazel b/Sources/BUILD.bazel index 0c4263aa4..7597606e4 100644 --- a/Sources/BUILD.bazel +++ b/Sources/BUILD.bazel @@ -75,6 +75,8 @@ swift_library( "SourceGraph/Mutators/ProtocolExtensionReferenceBuilder.swift", "SourceGraph/Mutators/PubliclyAccessibleRetainer.swift", "SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift", + "SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift", + "SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift", "SourceGraph/Mutators/RedundantProtocolMarker.swift", "SourceGraph/Mutators/ResultBuilderRetainer.swift", "SourceGraph/Mutators/StringInterpolationAppendInterpolationRetainer.swift", diff --git a/Sources/Configuration/Configuration.swift b/Sources/Configuration/Configuration.swift index f2551ccd7..82df94800 100644 --- a/Sources/Configuration/Configuration.swift +++ b/Sources/Configuration/Configuration.swift @@ -77,6 +77,12 @@ public final class Configuration { @Setting(key: "disable_redundant_public_analysis", defaultValue: false) public var disableRedundantPublicAnalysis: Bool + @Setting(key: "disable_redundant_internal_analysis", defaultValue: false) + public var disableRedundantInternalAnalysis: Bool + + @Setting(key: "disable_redundant_fileprivate_analysis", defaultValue: false) + public var disableRedundantFilePrivateAnalysis: Bool + @Setting(key: "disable_unused_import_analysis", defaultValue: false) public var disableUnusedImportAnalysis: Bool @@ -196,6 +202,7 @@ public final class Configuration { $project, $schemes, $excludeTargets, $excludeTests, $indexExclude, $reportExclude, $reportInclude, $outputFormat, $retainPublic, $retainFiles, $retainAssignOnlyProperties, $retainAssignOnlyPropertyTypes, $retainObjcAccessible, $retainObjcAnnotated, $retainUnusedProtocolFuncParams, $retainSwiftUIPreviews, $disableRedundantPublicAnalysis, + $disableRedundantInternalAnalysis, $disableRedundantFilePrivateAnalysis, $disableUnusedImportAnalysis, $retainUnusedImportedModules, $externalEncodableProtocols, $externalCodableProtocols, $externalTestCaseClasses, $verbose, $quiet, $disableUpdateCheck, $strict, $indexStorePath, $skipBuild, $skipSchemesValidation, $cleanBuild, $buildArguments, $xcodeListArguments, $relativeResults, $jsonPackageManifestPath, diff --git a/Sources/Frontend/Commands/ScanCommand.swift b/Sources/Frontend/Commands/ScanCommand.swift index ca253e12a..c1bdface6 100644 --- a/Sources/Frontend/Commands/ScanCommand.swift +++ b/Sources/Frontend/Commands/ScanCommand.swift @@ -60,6 +60,12 @@ struct ScanCommand: FrontendCommand { @Flag(help: "Disable identification of redundant public accessibility") var disableRedundantPublicAnalysis: Bool = defaultConfiguration.$disableRedundantPublicAnalysis.defaultValue + @Flag(help: "Disable identification of redundant internal accessibility") + var disableRedundantInternalAnalysis: Bool = defaultConfiguration.$disableRedundantInternalAnalysis.defaultValue + + @Flag(help: "Disable identification of redundant fileprivate accessibility") + var disableRedundantFilePrivateAnalysis: Bool = defaultConfiguration.$disableRedundantFilePrivateAnalysis.defaultValue + @Flag(help: "Disable identification of unused imports") var disableUnusedImportAnalysis: Bool = defaultConfiguration.$disableUnusedImportAnalysis.defaultValue @@ -173,6 +179,8 @@ struct ScanCommand: FrontendCommand { configuration.apply(\.$retainUnusedProtocolFuncParams, retainUnusedProtocolFuncParams) configuration.apply(\.$retainSwiftUIPreviews, retainSwiftUIPreviews) configuration.apply(\.$disableRedundantPublicAnalysis, disableRedundantPublicAnalysis) + configuration.apply(\.$disableRedundantInternalAnalysis, disableRedundantInternalAnalysis) + configuration.apply(\.$disableRedundantFilePrivateAnalysis, disableRedundantFilePrivateAnalysis) configuration.apply(\.$disableUnusedImportAnalysis, disableUnusedImportAnalysis) configuration.apply(\.$retainUnusedImportedModules, retainUnusedImportedModules) configuration.apply(\.$externalEncodableProtocols, externalEncodableProtocols) diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 925022c55..e5f2ccfc7 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -32,6 +32,10 @@ extension OutputFormatter { "redundantProtocol" case .redundantPublicAccessibility: "redundantPublicAccessibility" + case .redundantInternalAccessibility: + "redundantInternalAccessibility" + case .redundantFilePrivateAccessibility: + "redundantFilePrivateAccessibility" } } @@ -65,6 +69,12 @@ extension OutputFormatter { case let .redundantPublicAccessibility(modules): let modulesJoined = modules.sorted().joined(separator: ", ") description += " is declared public, but not used outside of \(modulesJoined)" + case let .redundantInternalAccessibility(files): + let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") + description += " is declared internal, but not used outside of \(filesJoined)" + case let .redundantFilePrivateAccessibility(files): + let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") + description += " is declared fileprivate, but not used outside of \(filesJoined)" } } else { description += "unused" diff --git a/Sources/PeripheryKit/ScanResult.swift b/Sources/PeripheryKit/ScanResult.swift index cc346d3c6..f672fa9e4 100644 --- a/Sources/PeripheryKit/ScanResult.swift +++ b/Sources/PeripheryKit/ScanResult.swift @@ -7,6 +7,8 @@ public struct ScanResult { case assignOnlyProperty case redundantProtocol(references: Set, inherited: Set) case redundantPublicAccessibility(modules: Set) + case redundantInternalAccessibility(files: Set) + case redundantFilePrivateAccessibility(files: Set) } let declaration: Declaration diff --git a/Sources/PeripheryKit/ScanResultBuilder.swift b/Sources/PeripheryKit/ScanResultBuilder.swift index fc3d1a053..92a190899 100644 --- a/Sources/PeripheryKit/ScanResultBuilder.swift +++ b/Sources/PeripheryKit/ScanResultBuilder.swift @@ -9,6 +9,8 @@ public enum ScanResultBuilder { .union(graph.unusedModuleImports) let redundantProtocols = graph.redundantProtocols.filter { !removableDeclarations.contains($0.0) } let redundantPublicAccessibility = graph.redundantPublicAccessibility.filter { !removableDeclarations.contains($0.0) } + let redundantInternalAccessibility = graph.redundantInternalAccessibility.filter { !removableDeclarations.contains($0.0) } + let redundantFilePrivateAccessibility = graph.redundantFilePrivateAccessibility.filter { !removableDeclarations.contains($0.0) } let annotatedRemovableDeclarations: [ScanResult] = removableDeclarations.flatMap { removableDeclaration in var extensionResults = [ScanResult]() @@ -40,10 +42,18 @@ public enum ScanResultBuilder { let annotatedRedundantPublicAccessibility: [ScanResult] = redundantPublicAccessibility.map { .init(declaration: $0.0, annotation: .redundantPublicAccessibility(modules: $0.1)) } + let annotatedRedundantInternalAccessibility: [ScanResult] = redundantInternalAccessibility.map { + .init(declaration: $0.0, annotation: .redundantInternalAccessibility(files: $0.1)) + } + let annotatedRedundantFilePrivateAccessibility: [ScanResult] = redundantFilePrivateAccessibility.map { + .init(declaration: $0.0, annotation: .redundantFilePrivateAccessibility(files: $0.1)) + } let allAnnotatedDeclarations = annotatedRemovableDeclarations + annotatedAssignOnlyProperties + annotatedRedundantProtocols + - annotatedRedundantPublicAccessibility + annotatedRedundantPublicAccessibility + + annotatedRedundantInternalAccessibility + + annotatedRedundantFilePrivateAccessibility return allAnnotatedDeclarations .filter { diff --git a/Sources/SourceGraph/Elements/Declaration.swift b/Sources/SourceGraph/Elements/Declaration.swift index 25d2b0fd5..dee73a4ea 100644 --- a/Sources/SourceGraph/Elements/Declaration.swift +++ b/Sources/SourceGraph/Elements/Declaration.swift @@ -231,6 +231,7 @@ public final class Declaration { public var related: Set = [] public var isImplicit: Bool = false public var isObjcAccessible: Bool = false + public var referencedFiles: Set private let hashValueCache: Int @@ -290,6 +291,7 @@ public final class Declaration { self.kind = kind self.usrs = usrs self.location = location + self.referencedFiles = [location.file] hashValueCache = usrs.hashValue } diff --git a/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift b/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift index 61f082937..eed58f819 100644 --- a/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift +++ b/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift @@ -34,6 +34,19 @@ final class ExtensionReferenceBuilder: SourceGraphMutator { extendedDeclaration.references.formUnion(extensionDeclaration.references) extendedDeclaration.related.formUnion(extensionDeclaration.related) + // Add the extension's file to the extended declaration's referencedFiles set. + extendedDeclaration.referencedFiles.insert(extensionDeclaration.location.file) + // Propagate the extension's file to all member declarations being merged. + for member in extensionDeclaration.declarations { + member.referencedFiles.insert(extensionDeclaration.location.file) + } + // Also update referencedFiles of existing members that are referenced in this extension. + for reference in extensionDeclaration.references { + if let referencedDecl = graph.declaration(withUsr: reference.usr) { + referencedDecl.referencedFiles.insert(extensionDeclaration.location.file) + } + } + extensionDeclaration.declarations.forEach { $0.parent = extendedDeclaration } extensionDeclaration.references.forEach { $0.parent = extendedDeclaration } extensionDeclaration.related.forEach { $0.parent = extendedDeclaration } diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift new file mode 100644 index 000000000..d7ae24202 --- /dev/null +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -0,0 +1,70 @@ +import Configuration +import Shared + +final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { + private let graph: SourceGraph + private let configuration: Configuration + + required init(graph: SourceGraph, configuration: Configuration, swiftVersion _: SwiftVersion) { + self.graph = graph + self.configuration = configuration + } + + func mutate() throws { + guard !configuration.disableRedundantFilePrivateAnalysis else { return } + + let nonExtensionKinds = graph.rootDeclarations.filter { !$0.kind.isExtensionKind } + let extensionKinds = graph.rootDeclarations.filter(\.kind.isExtensionKind) + + for decl in nonExtensionKinds { + try validate(decl) + } + + for decl in extensionKinds { + try validateExtension(decl) + } + } + + // MARK: - Private + + private func validate(_ decl: Declaration) throws { + if decl.accessibility.isExplicitly(.fileprivate) { + if !graph.isRetained(decl), !isReferencedOutsideFile(decl) { + mark(decl) + markExplicitFilePrivateDescendentDeclarations(from: decl) + } + } else { + markExplicitFilePrivateDescendentDeclarations(from: decl) + } + } + + private func validateExtension(_ decl: Declaration) throws { + if decl.accessibility.isExplicitly(.fileprivate) { + if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), + graph.redundantFilePrivateAccessibility.keys.contains(extendedDecl) { + mark(decl) + } + } + } + + private func mark(_ decl: Declaration) { + guard !graph.isRetained(decl) else { return } + graph.markRedundantFilePrivateAccessibility(decl, file: decl.location.file) + } + + private func markExplicitFilePrivateDescendentDeclarations(from decl: Declaration) { + for descDecl in descendentFilePrivateDeclarations(from: decl) { + mark(descDecl) + } + } + + private func isReferencedOutsideFile(_ decl: Declaration) -> Bool { + let referenceFiles = graph.references(to: decl).map(\.location.file) + return referenceFiles.contains { $0 != decl.location.file } + } + + private func descendentFilePrivateDeclarations(from decl: Declaration) -> Set { + let filePrivateDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.fileprivate) } + return filePrivateDeclarations.flatMapSet { descendentFilePrivateDeclarations(from: $0) }.union(filePrivateDeclarations) + } +} \ No newline at end of file diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift new file mode 100644 index 000000000..7a9b55811 --- /dev/null +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -0,0 +1,82 @@ +import Configuration +import Shared + +final class RedundantInternalAccessibilityMarker: SourceGraphMutator { + private let graph: SourceGraph + private let configuration: Configuration + + required init(graph: SourceGraph, configuration: Configuration, swiftVersion _: SwiftVersion) { + self.graph = graph + self.configuration = configuration + } + + func mutate() throws { + guard !configuration.disableRedundantInternalAnalysis else { return } + + let nonExtensionKinds = graph.rootDeclarations.filter { !$0.kind.isExtensionKind } + let extensionKinds = graph.rootDeclarations.filter(\.kind.isExtensionKind) + + for decl in nonExtensionKinds { + try validate(decl) + } + + for decl in extensionKinds { + try validateExtension(decl) + } + } + + // MARK: - Private + + private func validate(_ decl: Declaration) throws { + if decl.accessibility.isExplicitly(.internal) { + if !graph.isRetained(decl) { + let isReferencedOutside = isReferencedOutsideFile(decl) + if !isReferencedOutside { + mark(decl) + markExplicitInternalDescendentDeclarations(from: decl) + } + } + } else { + markExplicitInternalDescendentDeclarations(from: decl) + } + } + + private func validateExtension(_ decl: Declaration) throws { + if decl.accessibility.isExplicitly(.internal) { + if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), + graph.redundantInternalAccessibility.keys.contains(extendedDecl) { + mark(decl) + } + } + } + + private func mark(_ decl: Declaration) { + guard !graph.isRetained(decl) else { return } + graph.markRedundantInternalAccessibility(decl, file: decl.location.file) + } + + private func markExplicitInternalDescendentDeclarations(from decl: Declaration) { + for descDecl in descendentInternalDeclarations(from: decl) { + if !graph.isRetained(descDecl) { + let isReferencedOutside = isReferencedOutsideFile(descDecl) + if !isReferencedOutside { + mark(descDecl) + } + } + } + } + + private func isReferencedOutsideFile(_ decl: Declaration) -> Bool { + // Use graph.references(to: decl) to get all references to this declaration + let allReferences = graph.references(to: decl) + let referenceFiles = allReferences.map(\.location.file) + + let result = referenceFiles.contains { $0 != decl.location.file } + return result + } + + private func descendentInternalDeclarations(from decl: Declaration) -> Set { + let internalDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.internal) } + return internalDeclarations.flatMapSet { descendentInternalDeclarations(from: $0) }.union(internalDeclarations) + } +} \ No newline at end of file diff --git a/Sources/SourceGraph/SourceGraph.swift b/Sources/SourceGraph/SourceGraph.swift index 4df7fcef2..a8142e485 100644 --- a/Sources/SourceGraph/SourceGraph.swift +++ b/Sources/SourceGraph/SourceGraph.swift @@ -9,6 +9,8 @@ public final class SourceGraph { public private(set) var redundantProtocols: [Declaration: (references: Set, inherited: Set)] = [:] public private(set) var rootDeclarations: Set = [] public private(set) var redundantPublicAccessibility: [Declaration: Set] = [:] + public private(set) var redundantInternalAccessibility: [Declaration: Set] = [:] + public private(set) var redundantFilePrivateAccessibility: [Declaration: Set] = [:] public private(set) var rootReferences: Set = [] public private(set) var allReferences: Set = [] public private(set) var retainedDeclarations: Set = [] @@ -85,6 +87,22 @@ public final class SourceGraph { _ = redundantPublicAccessibility.removeValue(forKey: declaration) } + func markRedundantInternalAccessibility(_ declaration: Declaration, file: SourceFile) { + redundantInternalAccessibility[declaration, default: []].insert(file) + } + + func unmarkRedundantInternalAccessibility(_ declaration: Declaration) { + _ = redundantInternalAccessibility.removeValue(forKey: declaration) + } + + func markRedundantFilePrivateAccessibility(_ declaration: Declaration, file: SourceFile) { + redundantFilePrivateAccessibility[declaration, default: []].insert(file) + } + + func unmarkRedundantFilePrivateAccessibility(_ declaration: Declaration) { + _ = redundantFilePrivateAccessibility.removeValue(forKey: declaration) + } + func markIgnored(_ declaration: Declaration) { _ = ignoredDeclarations.insert(declaration) } @@ -143,6 +161,9 @@ public final class SourceGraph { public func add(_ reference: Reference) { _ = allReferences.insert(reference) allReferencesByUsr[reference.usr, default: []].insert(reference) + if let decl = declaration(withUsr: reference.usr) { + decl.referencedFiles.insert(reference.location.file) + } } public func add(_ references: Set) { diff --git a/Sources/SourceGraph/SourceGraphMutatorRunner.swift b/Sources/SourceGraph/SourceGraphMutatorRunner.swift index 93aaf963c..c94e544a1 100644 --- a/Sources/SourceGraph/SourceGraphMutatorRunner.swift +++ b/Sources/SourceGraph/SourceGraphMutatorRunner.swift @@ -17,6 +17,10 @@ public final class SourceGraphMutatorRunner { // Must come before ProtocolExtensionReferenceBuilder because it removes references // from the extension to the protocol, thus making them appear to be unknown. ExtensionReferenceBuilder.self, + // Must come after ExtensionReferenceBuilder so that it can detect redundant accessibility + // on properties that are used in extensions. + RedundantInternalAccessibilityMarker.self, + RedundantFilePrivateAccessibilityMarker.self, // Must come before ProtocolConformanceReferenceBuilder because it removes references to // conformed protocols, which CodingKeyEnumReferenceBuilder needs to inspect before removal. // It must also come after ExtensionReferenceBuilder as some types may declare conformance diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index 534fe6931..0abad4294 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -79,3 +79,6 @@ inlinableFunction() // Associated types _ = PublicInheritedAssociatedTypeClass().items _ = PublicInheritedAssociatedTypeDefaultTypeClass().items + +// Internal accessibility tests +// _ = InternalPropertyUsedInExtension() // Commented out for now diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift new file mode 100644 index 000000000..3b72a5366 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift @@ -0,0 +1,9 @@ +// InternalPropertyExtension.swift +// Extension that uses the internal property from InternalPropertyUsedInExtension +// This should prevent the property from being flagged as redundant + +extension InternalPropertyUsedInExtension { + func useProperty() { + print(propertyUsedInExtension) // This reference should prevent propertyUsedInExtension from being flagged as redundant + } +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift new file mode 100644 index 000000000..c4969afc8 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift @@ -0,0 +1,4 @@ +// InternalPropertyOwner.swift +internal class InternalPropertyOwner { + internal var value: Int = 42 +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift new file mode 100644 index 000000000..6c89161f3 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift @@ -0,0 +1,8 @@ +// InternalPropertyUsedInExtension.swift +// Tests the case where an internal property is used in an extension in a different file +// This should NOT be flagged as redundant + +internal class InternalPropertyUsedInExtension { + internal var propertyUsedInExtension: String = "test" + internal var propertyOnlyUsedInSameFile: String = "test" // This should be flagged as redundant +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift new file mode 100644 index 000000000..aa70c5878 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift @@ -0,0 +1,6 @@ +// InternalPropertyUserExtension.swift +extension InternalPropertyOwner { + func printValue() { + print(value) + } +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift new file mode 100644 index 000000000..e63e71124 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift @@ -0,0 +1,28 @@ +// NotRedundantFilePrivateComponents.swift +// Tests for fileprivate classes/members that should NOT be flagged as redundant + +fileprivate class NotRedundantFilePrivateClass { + fileprivate func usedFilePrivateMethod() {} + func publicMethodCallingFilePrivate() { + usedFilePrivateMethod() + } +} + +fileprivate struct NotRedundantFilePrivateStruct { + fileprivate var usedFilePrivateProperty: Int = 0 + func useProperty() -> Int { + return usedFilePrivateProperty + } +} + +fileprivate enum NotRedundantFilePrivateEnum { + case usedCase + func useCase() -> NotRedundantFilePrivateEnum { + return .usedCase + } +} + +// Used by main.swift to ensure these are referenced +class NotRedundantFilePrivateComponents { + public init() {} +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift new file mode 100644 index 000000000..46a830c94 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift @@ -0,0 +1,22 @@ +// NotRedundantInternalClassComponents.swift +// Tests for internal classes/members that should NOT be flagged as redundant + +class NotRedundantInternalClassComponents { + public init() {} + + internal func usedInternalMethod() {} +} + +internal struct NotRedundantInternalStruct { + internal var usedInternalProperty: Int = 0 + func useProperty() -> Int { + return usedInternalProperty + } +} + +internal enum NotRedundantInternalEnum { + case usedCase + func useCase() -> NotRedundantInternalEnum { + return .usedCase + } +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift new file mode 100644 index 000000000..edc6add65 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift @@ -0,0 +1,16 @@ +// NotRedundantInternalClassComponents_Support.swift +// Support types/usages for NotRedundantInternalClassComponents + +internal class NotRedundantInternalClass_Support { + internal func helper() {} +} + +// Used by main.swift to ensure these are referenced +class NotRedundantInternalClassComponents_Support { + public init() {} + + func useInternalMethod() { + let cls = NotRedundantInternalClassComponents() + cls.usedInternalMethod() + } +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift new file mode 100644 index 000000000..70dc1f199 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift @@ -0,0 +1,19 @@ +// RedundantFilePrivateComponents.swift +// Tests for fileprivate classes/members that should be flagged as redundant + +fileprivate class RedundantFilePrivateClass { + fileprivate func unusedFilePrivateMethod() {} +} + +fileprivate struct RedundantFilePrivateStruct { + fileprivate var unusedFilePrivateProperty: Int = 0 +} + +fileprivate enum RedundantFilePrivateEnum { + case unusedCase +} + +// Used by main.swift to ensure these are referenced +class RedundantFilePrivateComponents { + public init() {} +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift new file mode 100644 index 000000000..74d1cdbbf --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift @@ -0,0 +1,19 @@ +// RedundantInternalComponents.swift +// Tests for internal classes/members that should be flagged as redundant + +internal class RedundantInternalClass { + internal func unusedInternalMethod() {} +} + +internal struct RedundantInternalStruct { + internal var unusedInternalProperty: Int = 0 +} + +internal enum RedundantInternalEnum { + case unusedCase +} + +// Used by main.swift to ensure these are referenced +class RedundantInternalClassComponents { + public init() {} +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift new file mode 100644 index 000000000..5a92acf0a --- /dev/null +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -0,0 +1,22 @@ +import Configuration +@testable import TestShared +import XCTest + +final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { + override static func setUp() { + super.setUp() + build(projectPath: AccessibilityProjectPath) + } + + func testRedundantFilePrivateClass() { + // This should be flagged as redundant + index() + assertRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) + } + + func testNotRedundantFilePrivateClass() { + // This should NOT be flagged as redundant + index() + assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) + } +} \ No newline at end of file diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift new file mode 100644 index 000000000..98ec00cfd --- /dev/null +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -0,0 +1,51 @@ +import Configuration +@testable import TestShared +import XCTest + +final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { + override static func setUp() { + super.setUp() + build(projectPath: AccessibilityProjectPath) + } + + func testInternalPropertyUsedInExtensionInOtherFile() { + // This should NOT be flagged as redundant + // Tests the case where an internal property is used in an extension in a different file + // Like delayedClearBoatHistory in AppState.swift used in AppState+BoatTracking.swift + index() + + // InternalPropertyUsedInExtension.propertyUsedInExtension should NOT be flagged as redundant + // because it's used in InternalPropertyExtension.swift + assertNotRedundantInternalAccessibility(.varInstance("propertyUsedInExtension")) + } + + func testInternalPropertyUsedOnlyInSameFile() { + // This should be flagged as redundant + // Tests the case where an internal property is only used within its own file + index() + + // InternalPropertyUsedInExtension.propertyOnlyUsedInSameFile should be flagged as redundant + // because it's only used within InternalPropertyUsedInExtension.swift + assertRedundantInternalAccessibility(.varInstance("propertyOnlyUsedInSameFile")) + } + + func testInternalPropertyUsedInMultipleFiles() { + // This should NOT be flagged as redundant + // Tests the case where an internal property is used across multiple files + index() + + // This test would need additional setup with multiple files + // For now, we'll test that the existing NotRedundantInternalClassComponents work + assertNotRedundantInternalAccessibility(.class("NotRedundantInternalClass")) + } + + func testInternalMethodUsedInExtension() { + // This should NOT be flagged as redundant + // Tests the case where an internal method is used in an extension + index() + + // This test would need additional setup with methods in extensions + // For now, we'll test that the existing NotRedundantInternalClassComponents work + assertNotRedundantInternalAccessibility(.functionMethodInstance("usedInternalMethod()")) + } +} \ No newline at end of file diff --git a/Tests/Shared/SourceGraphTestCase.swift b/Tests/Shared/SourceGraphTestCase.swift index 6cceed00e..b730202d3 100644 --- a/Tests/Shared/SourceGraphTestCase.swift +++ b/Tests/Shared/SourceGraphTestCase.swift @@ -174,6 +174,54 @@ open class SourceGraphTestCase: XCTestCase { scopeStack.removeLast() } + func assertRedundantInternalAccessibility(_ 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.results.redundantInternalAccessibilityDeclarations.contains(declaration) { + XCTFail("Expected declaration to have redundant internal accessibility: \(declaration)", file: file, line: line) + } + + scopeStack.append(.declaration(declaration)) + scopedAssertions?() + scopeStack.removeLast() + } + + func assertNotRedundantInternalAccessibility(_ 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.results.redundantInternalAccessibilityDeclarations.contains(declaration) { + XCTFail("Expected declaration to not have redundant internal accessibility: \(declaration)", file: file, line: line) + } + + scopeStack.append(.declaration(declaration)) + scopedAssertions?() + scopeStack.removeLast() + } + + func assertRedundantFilePrivateAccessibility(_ 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.results.redundantFilePrivateAccessibilityDeclarations.contains(declaration) { + XCTFail("Expected declaration to have redundant fileprivate accessibility: \(declaration)", file: file, line: line) + } + + scopeStack.append(.declaration(declaration)) + scopedAssertions?() + scopeStack.removeLast() + } + + func assertNotRedundantFilePrivateAccessibility(_ 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.results.redundantFilePrivateAccessibilityDeclarations.contains(declaration) { + XCTFail("Expected declaration to not have redundant fileprivate accessibility: \(declaration)", file: file, line: line) + } + + scopeStack.append(.declaration(declaration)) + scopedAssertions?() + scopeStack.removeLast() + } + func assertUsedParameter(_ name: String, file: StaticString = #file, line: UInt = #line) { let declaration = materialize(.varParameter(name), fail: false, file: file, line: line) @@ -309,4 +357,24 @@ private extension [ScanResult] { return nil } } + + var redundantInternalAccessibilityDeclarations: Set { + compactMapSet { + if case .redundantInternalAccessibility = $0.annotation { + return $0.declaration + } + + return nil + } + } + + var redundantFilePrivateAccessibilityDeclarations: Set { + compactMapSet { + if case .redundantFilePrivateAccessibility = $0.annotation { + return $0.declaration + } + + return nil + } + } } From bd369a8e2cd737d92febccaf3a4ee91e30185e09 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 15 Dec 2025 10:12:23 -0800 Subject: [PATCH 02/30] Update Sources/PeripheryKit/Results/OutputFormatter.swift Co-authored-by: Ian Leitch --- Sources/PeripheryKit/Results/OutputFormatter.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 30d379c9a..24cee5f52 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -74,7 +74,7 @@ extension OutputFormatter { description += " is declared internal, but not used outside of \(filesJoined)" case let .redundantFilePrivateAccessibility(files): let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") - description += " is declared fileprivate, but not used outside of \(filesJoined)" + description += " is declared fileprivate, but not used outside its enclosing scope in \(filesJoined)" } } else { description += "unused" From 3e13ecbfce1bb0d7a75cf5e240e04250a677abf5 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 15 Dec 2025 10:16:19 -0800 Subject: [PATCH 03/30] remove some stuff Ian noticed in my PR --- Sources/SourceGraph/SourceGraph.swift | 8 -------- .../AccessibilityProject/Sources/MainTarget/main.swift | 2 +- .../RedundantInternalAccessibilityTest.swift | 1 - 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/Sources/SourceGraph/SourceGraph.swift b/Sources/SourceGraph/SourceGraph.swift index 8976c7660..9c1dbf71b 100644 --- a/Sources/SourceGraph/SourceGraph.swift +++ b/Sources/SourceGraph/SourceGraph.swift @@ -91,18 +91,10 @@ public final class SourceGraph { redundantInternalAccessibility[declaration, default: []].insert(file) } - func unmarkRedundantInternalAccessibility(_ declaration: Declaration) { - _ = redundantInternalAccessibility.removeValue(forKey: declaration) - } - func markRedundantFilePrivateAccessibility(_ declaration: Declaration, file: SourceFile) { redundantFilePrivateAccessibility[declaration, default: []].insert(file) } - func unmarkRedundantFilePrivateAccessibility(_ declaration: Declaration) { - _ = redundantFilePrivateAccessibility.removeValue(forKey: declaration) - } - func markIgnored(_ declaration: Declaration) { _ = ignoredDeclarations.insert(declaration) } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index 0abad4294..617ba9545 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -81,4 +81,4 @@ _ = PublicInheritedAssociatedTypeClass().items _ = PublicInheritedAssociatedTypeDefaultTypeClass().items // Internal accessibility tests -// _ = InternalPropertyUsedInExtension() // Commented out for now +_ = InternalPropertyUsedInExtension() diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 98ec00cfd..26c34014a 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -11,7 +11,6 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { func testInternalPropertyUsedInExtensionInOtherFile() { // This should NOT be flagged as redundant // Tests the case where an internal property is used in an extension in a different file - // Like delayedClearBoatHistory in AppState.swift used in AppState+BoatTracking.swift index() // InternalPropertyUsedInExtension.propertyUsedInExtension should NOT be flagged as redundant From 262b3f97035302a76ce9ee73b4bbe8032e68733e Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 15 Dec 2025 10:28:56 -0800 Subject: [PATCH 04/30] =?UTF-8?q?remove=20=E2=80=98referencedFiles?= =?UTF-8?q?=E2=80=99=20which=20I=E2=80=99m=20no=20longer=20needing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- Sources/SourceGraph/Elements/Declaration.swift | 2 -- .../Mutators/ExtensionReferenceBuilder.swift | 13 ------------- Sources/SourceGraph/SourceGraph.swift | 3 --- 3 files changed, 18 deletions(-) diff --git a/Sources/SourceGraph/Elements/Declaration.swift b/Sources/SourceGraph/Elements/Declaration.swift index dee73a4ea..25d2b0fd5 100644 --- a/Sources/SourceGraph/Elements/Declaration.swift +++ b/Sources/SourceGraph/Elements/Declaration.swift @@ -231,7 +231,6 @@ public final class Declaration { public var related: Set = [] public var isImplicit: Bool = false public var isObjcAccessible: Bool = false - public var referencedFiles: Set private let hashValueCache: Int @@ -291,7 +290,6 @@ public final class Declaration { self.kind = kind self.usrs = usrs self.location = location - self.referencedFiles = [location.file] hashValueCache = usrs.hashValue } diff --git a/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift b/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift index eed58f819..61f082937 100644 --- a/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift +++ b/Sources/SourceGraph/Mutators/ExtensionReferenceBuilder.swift @@ -34,19 +34,6 @@ final class ExtensionReferenceBuilder: SourceGraphMutator { extendedDeclaration.references.formUnion(extensionDeclaration.references) extendedDeclaration.related.formUnion(extensionDeclaration.related) - // Add the extension's file to the extended declaration's referencedFiles set. - extendedDeclaration.referencedFiles.insert(extensionDeclaration.location.file) - // Propagate the extension's file to all member declarations being merged. - for member in extensionDeclaration.declarations { - member.referencedFiles.insert(extensionDeclaration.location.file) - } - // Also update referencedFiles of existing members that are referenced in this extension. - for reference in extensionDeclaration.references { - if let referencedDecl = graph.declaration(withUsr: reference.usr) { - referencedDecl.referencedFiles.insert(extensionDeclaration.location.file) - } - } - extensionDeclaration.declarations.forEach { $0.parent = extendedDeclaration } extensionDeclaration.references.forEach { $0.parent = extendedDeclaration } extensionDeclaration.related.forEach { $0.parent = extendedDeclaration } diff --git a/Sources/SourceGraph/SourceGraph.swift b/Sources/SourceGraph/SourceGraph.swift index 9c1dbf71b..659fc0f5a 100644 --- a/Sources/SourceGraph/SourceGraph.swift +++ b/Sources/SourceGraph/SourceGraph.swift @@ -153,9 +153,6 @@ public final class SourceGraph { public func add(_ reference: Reference) { _ = allReferences.insert(reference) allReferencesByUsr[reference.usr, default: []].insert(reference) - if let decl = declaration(withUsr: reference.usr) { - decl.referencedFiles.insert(reference.location.file) - } } public func add(_ references: Set) { From 431ac492a7ed93c557578971dd91f0b2910b6e2c Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 15 Dec 2025 12:47:42 -0800 Subject: [PATCH 05/30] Redoing tests - WIP - moving into same module and rearranging --- .../Sources/MainTarget/main.swift | 3 -- .../NotRedundantFilePrivateComponents.swift | 28 ------------------- .../RedundantFilePrivateComponents.swift | 19 ------------- .../TargetA => }/InternalPropertyOwner.swift | 0 ... => InternalPropertyOwner_Extension.swift} | 0 .../InternalPropertyUsedInExtension.swift | 6 +++- ...alPropertyUsedInExtension_Extension.swift} | 4 +-- .../NotRedundantInternalClassComponents.swift | 4 +-- ...ndantInternalClassComponents_Support.swift | 0 ...edundantFilePrivateAccessibilityTest.swift | 17 ++++++++++- .../RedundantInternalAccessibilityTest.swift | 6 +++- .../RedundantInternalComponents.swift | 0 12 files changed, 30 insertions(+), 57 deletions(-) delete mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift delete mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA => }/InternalPropertyOwner.swift (100%) rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift => InternalPropertyOwner_Extension.swift} (100%) rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA => }/InternalPropertyUsedInExtension.swift (81%) rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift => InternalPropertyUsedInExtension_Extension.swift} (90%) rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA => }/NotRedundantInternalClassComponents.swift (92%) rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA => }/NotRedundantInternalClassComponents_Support.swift (100%) rename Tests/AccessibilityTests/{AccessibilityProject/Sources/TargetA => }/RedundantInternalComponents.swift (100%) diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index 617ba9545..534fe6931 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -79,6 +79,3 @@ inlinableFunction() // Associated types _ = PublicInheritedAssociatedTypeClass().items _ = PublicInheritedAssociatedTypeDefaultTypeClass().items - -// Internal accessibility tests -_ = InternalPropertyUsedInExtension() diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift deleted file mode 100644 index e63e71124..000000000 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateComponents.swift +++ /dev/null @@ -1,28 +0,0 @@ -// NotRedundantFilePrivateComponents.swift -// Tests for fileprivate classes/members that should NOT be flagged as redundant - -fileprivate class NotRedundantFilePrivateClass { - fileprivate func usedFilePrivateMethod() {} - func publicMethodCallingFilePrivate() { - usedFilePrivateMethod() - } -} - -fileprivate struct NotRedundantFilePrivateStruct { - fileprivate var usedFilePrivateProperty: Int = 0 - func useProperty() -> Int { - return usedFilePrivateProperty - } -} - -fileprivate enum NotRedundantFilePrivateEnum { - case usedCase - func useCase() -> NotRedundantFilePrivateEnum { - return .usedCase - } -} - -// Used by main.swift to ensure these are referenced -class NotRedundantFilePrivateComponents { - public init() {} -} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift deleted file mode 100644 index 70dc1f199..000000000 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateComponents.swift +++ /dev/null @@ -1,19 +0,0 @@ -// RedundantFilePrivateComponents.swift -// Tests for fileprivate classes/members that should be flagged as redundant - -fileprivate class RedundantFilePrivateClass { - fileprivate func unusedFilePrivateMethod() {} -} - -fileprivate struct RedundantFilePrivateStruct { - fileprivate var unusedFilePrivateProperty: Int = 0 -} - -fileprivate enum RedundantFilePrivateEnum { - case unusedCase -} - -// Used by main.swift to ensure these are referenced -class RedundantFilePrivateComponents { - public init() {} -} \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift b/Tests/AccessibilityTests/InternalPropertyOwner.swift similarity index 100% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift rename to Tests/AccessibilityTests/InternalPropertyOwner.swift diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift b/Tests/AccessibilityTests/InternalPropertyOwner_Extension.swift similarity index 100% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUserExtension.swift rename to Tests/AccessibilityTests/InternalPropertyOwner_Extension.swift diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift b/Tests/AccessibilityTests/InternalPropertyUsedInExtension.swift similarity index 81% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift rename to Tests/AccessibilityTests/InternalPropertyUsedInExtension.swift index 6c89161f3..95bf724c2 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift +++ b/Tests/AccessibilityTests/InternalPropertyUsedInExtension.swift @@ -5,4 +5,8 @@ internal class InternalPropertyUsedInExtension { internal var propertyUsedInExtension: String = "test" internal var propertyOnlyUsedInSameFile: String = "test" // This should be flagged as redundant -} \ No newline at end of file + + func useSameFileProperty() { + print(propertyOnlyUsedInSameFile) + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift b/Tests/AccessibilityTests/InternalPropertyUsedInExtension_Extension.swift similarity index 90% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift rename to Tests/AccessibilityTests/InternalPropertyUsedInExtension_Extension.swift index 3b72a5366..48f8bba34 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyExtension.swift +++ b/Tests/AccessibilityTests/InternalPropertyUsedInExtension_Extension.swift @@ -3,7 +3,7 @@ // This should prevent the property from being flagged as redundant extension InternalPropertyUsedInExtension { - func useProperty() { + func usePropertyInExtension() { print(propertyUsedInExtension) // This reference should prevent propertyUsedInExtension from being flagged as redundant } -} \ No newline at end of file +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift b/Tests/AccessibilityTests/NotRedundantInternalClassComponents.swift similarity index 92% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift rename to Tests/AccessibilityTests/NotRedundantInternalClassComponents.swift index 46a830c94..4735a2f47 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift +++ b/Tests/AccessibilityTests/NotRedundantInternalClassComponents.swift @@ -9,7 +9,7 @@ class NotRedundantInternalClassComponents { internal struct NotRedundantInternalStruct { internal var usedInternalProperty: Int = 0 - func useProperty() -> Int { + func useInternalProperty() -> Int { return usedInternalProperty } } @@ -19,4 +19,4 @@ internal enum NotRedundantInternalEnum { func useCase() -> NotRedundantInternalEnum { return .usedCase } -} \ No newline at end of file +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift b/Tests/AccessibilityTests/NotRedundantInternalClassComponents_Support.swift similarity index 100% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift rename to Tests/AccessibilityTests/NotRedundantInternalClassComponents_Support.swift diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift index 5a92acf0a..d7d9cd253 100644 --- a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -5,6 +5,7 @@ import XCTest final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { override static func setUp() { super.setUp() + _ = RedundantFilePrivateClass() build(projectPath: AccessibilityProjectPath) } @@ -17,6 +18,20 @@ final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { func testNotRedundantFilePrivateClass() { // This should NOT be flagged as redundant index() + NotRedundantFilePrivateClass.staticMethodCallingFilePrivate() assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) } -} \ No newline at end of file +} + +fileprivate class NotRedundantFilePrivateClass { + fileprivate static func usedFilePrivateMethod() {} + + static func staticMethodCallingFilePrivate() { + usedFilePrivateMethod() + } +} + +fileprivate class RedundantFilePrivateClass { + fileprivate func unusedFilePrivateMethod() {} +} + diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 26c34014a..606978083 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -18,6 +18,10 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.varInstance("propertyUsedInExtension")) } +/* +failed - Expected declaration to have redundant internal accessibility: +Declaration(var.instance, 'propertyOnlyUsedInSameFile', explicit, internal, [internal], [], [], [s:7TargetA31InternalPropertyUsedInExtensionC012propertyOnlydE8SameFileSSvp], InternalPropertyUsedInExtension.swift:7:18) +*/ func testInternalPropertyUsedOnlyInSameFile() { // This should be flagged as redundant // Tests the case where an internal property is only used within its own file @@ -47,4 +51,4 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { // For now, we'll test that the existing NotRedundantInternalClassComponents work assertNotRedundantInternalAccessibility(.functionMethodInstance("usedInternalMethod()")) } -} \ No newline at end of file +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift b/Tests/AccessibilityTests/RedundantInternalComponents.swift similarity index 100% rename from Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift rename to Tests/AccessibilityTests/RedundantInternalComponents.swift From 17e0ee800c636b5ded2384e981877cee0f05c035 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 15 Dec 2025 13:59:27 -0800 Subject: [PATCH 06/30] Redo tests, found bug in source graph --- ...undantFilePrivateAccessibilityMarker.swift | 14 +++++++-- ...RedundantInternalAccessibilityMarker.swift | 14 +++++++-- .../Sources/MainTarget/main.swift | 2 ++ .../TargetA}/InternalPropertyOwner.swift | 0 .../InternalPropertyOwner_Extension.swift | 0 .../InternalPropertyUsedInExtension.swift | 8 +++++ ...nalPropertyUsedInExtension_Extension.swift | 0 .../NotRedundantFilePrivateClass.swift | 23 ++++++++++++++ .../NotRedundantInternalClassComponents.swift | 4 +-- ...ndantInternalClassComponents_Support.swift | 2 +- .../TargetA/RedundantFilePrivateClass.swift | 31 +++++++++++++++++++ .../RedundantInternalComponents.swift | 0 ...edundantFilePrivateAccessibilityTest.swift | 20 ++---------- 13 files changed, 91 insertions(+), 27 deletions(-) rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/InternalPropertyOwner.swift (100%) rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/InternalPropertyOwner_Extension.swift (100%) rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/InternalPropertyUsedInExtension.swift (69%) rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/InternalPropertyUsedInExtension_Extension.swift (100%) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateClass.swift rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/NotRedundantInternalClassComponents.swift (91%) rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/NotRedundantInternalClassComponents_Support.swift (87%) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift rename Tests/AccessibilityTests/{ => AccessibilityProject/Sources/TargetA}/RedundantInternalComponents.swift (100%) diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift index d7ae24202..ad9d82bda 100644 --- a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -31,11 +31,19 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { if decl.accessibility.isExplicitly(.fileprivate) { if !graph.isRetained(decl), !isReferencedOutsideFile(decl) { mark(decl) - markExplicitFilePrivateDescendentDeclarations(from: decl) } - } else { - markExplicitFilePrivateDescendentDeclarations(from: decl) } + + /* + Always check descendents, even if parent is not redundant. + + A parent declaration may be used outside its file (making it not redundant), + while still having child declarations that are only used within the same file + (making those children redundant). For example, a class used cross-file may have + a fileprivate property only referenced within the same file - that property should + be flagged as redundant even though the parent class is not. + */ + markExplicitFilePrivateDescendentDeclarations(from: decl) } private func validateExtension(_ decl: Declaration) throws { diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 7a9b55811..62c5ffdbc 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -33,12 +33,20 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let isReferencedOutside = isReferencedOutsideFile(decl) if !isReferencedOutside { mark(decl) - markExplicitInternalDescendentDeclarations(from: decl) } } - } else { - markExplicitInternalDescendentDeclarations(from: decl) } + + /* + Always check descendents, even if parent is not redundant. + + A parent declaration may be used outside its file (making it not redundant), + while still having child declarations that are only used within the same file + (making those children redundant). For example, a class used cross-file may have + an internal property only referenced within the same file - that property should + be flagged as redundant even though the parent class is not. + */ + markExplicitInternalDescendentDeclarations(from: decl) } private func validateExtension(_ decl: Declaration) throws { diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index 534fe6931..dfc0d83c1 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -5,6 +5,8 @@ PublicDeclarationInInternalParentRetainer().retain() PublicExtensionOnRedundantPublicKindRetainer().retain() IgnoreCommentCommandRetainer().retain() IgnoreAllCommentCommandRetainer().retain() +RedundantFilePrivateClassRetainer().retain() +InternalPropertyUsedInExtensionRetainer().retain() _ = PublicTypeUsedAsPublicInitializerParameterTypeRetainer() diff --git a/Tests/AccessibilityTests/InternalPropertyOwner.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift similarity index 100% rename from Tests/AccessibilityTests/InternalPropertyOwner.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner.swift diff --git a/Tests/AccessibilityTests/InternalPropertyOwner_Extension.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner_Extension.swift similarity index 100% rename from Tests/AccessibilityTests/InternalPropertyOwner_Extension.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyOwner_Extension.swift diff --git a/Tests/AccessibilityTests/InternalPropertyUsedInExtension.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift similarity index 69% rename from Tests/AccessibilityTests/InternalPropertyUsedInExtension.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift index 95bf724c2..25dc53e71 100644 --- a/Tests/AccessibilityTests/InternalPropertyUsedInExtension.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension.swift @@ -9,4 +9,12 @@ internal class InternalPropertyUsedInExtension { func useSameFileProperty() { print(propertyOnlyUsedInSameFile) } +} + +public class InternalPropertyUsedInExtensionRetainer { + public init() {} + public func retain() { + let instance = InternalPropertyUsedInExtension() + instance.useSameFileProperty() + } } diff --git a/Tests/AccessibilityTests/InternalPropertyUsedInExtension_Extension.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension_Extension.swift similarity index 100% rename from Tests/AccessibilityTests/InternalPropertyUsedInExtension_Extension.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalPropertyUsedInExtension_Extension.swift diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateClass.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateClass.swift new file mode 100644 index 000000000..aabe2452b --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivateClass.swift @@ -0,0 +1,23 @@ +/* + NotRedundantFilePrivateClass.swift + Tests that a fileprivate class is NOT flagged as redundant when it's used from a different type in the same file +*/ + +fileprivate class NotRedundantFilePrivateClass { + fileprivate static func usedFilePrivateMethod() {} + + static func staticMethodCallingFilePrivate() { + usedFilePrivateMethod() + } +} + +/* + This separate type accesses NotRedundantFilePrivateClass. + Since they're in the same file, fileprivate allows the access. + If NotRedundantFilePrivateClass were private instead, this would fail to compile. +*/ +class NotRedundantFilePrivateClassUser { + func useFilePrivateClass() { + NotRedundantFilePrivateClass.staticMethodCallingFilePrivate() + } +} diff --git a/Tests/AccessibilityTests/NotRedundantInternalClassComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift similarity index 91% rename from Tests/AccessibilityTests/NotRedundantInternalClassComponents.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift index 4735a2f47..c4f852e72 100644 --- a/Tests/AccessibilityTests/NotRedundantInternalClassComponents.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift @@ -1,9 +1,9 @@ // NotRedundantInternalClassComponents.swift // Tests for internal classes/members that should NOT be flagged as redundant -class NotRedundantInternalClassComponents { +class NotRedundantInternalClass { public init() {} - + internal func usedInternalMethod() {} } diff --git a/Tests/AccessibilityTests/NotRedundantInternalClassComponents_Support.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift similarity index 87% rename from Tests/AccessibilityTests/NotRedundantInternalClassComponents_Support.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift index edc6add65..055f1e21f 100644 --- a/Tests/AccessibilityTests/NotRedundantInternalClassComponents_Support.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift @@ -10,7 +10,7 @@ class NotRedundantInternalClassComponents_Support { public init() {} func useInternalMethod() { - let cls = NotRedundantInternalClassComponents() + let cls = NotRedundantInternalClass() cls.usedInternalMethod() } } \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift new file mode 100644 index 000000000..8aaf98996 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift @@ -0,0 +1,31 @@ +/* + RedundantFilePrivateClass.swift + Tests that a fileprivate class is flagged as redundant when it's only referenced from within its own declaration +*/ + +fileprivate class RedundantFilePrivateClass { + fileprivate func someMethod() { + // Method that does something + } + + /* + This internal method creates a self-reference, ensuring the class is "used" + but only within its own declaration scope, making fileprivate redundant (could be private) + */ + func createInstance() -> RedundantFilePrivateClass { + RedundantFilePrivateClass() + } +} + +/* + This retainer ensures the file is indexed and calls a method on the class. + The fileprivate class is used, but only within the same file and not by other declarations, + making the fileprivate access level redundant (could be private). +*/ +public class RedundantFilePrivateClassRetainer { + public init() {} + public func retain() { + let instance = RedundantFilePrivateClass() + _ = instance.createInstance() + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift similarity index 100% rename from Tests/AccessibilityTests/RedundantInternalComponents.swift rename to Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift index d7d9cd253..1d1063498 100644 --- a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -5,33 +5,17 @@ import XCTest final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { override static func setUp() { super.setUp() - _ = RedundantFilePrivateClass() build(projectPath: AccessibilityProjectPath) } - + func testRedundantFilePrivateClass() { - // This should be flagged as redundant index() assertRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) } - + func testNotRedundantFilePrivateClass() { - // This should NOT be flagged as redundant index() - NotRedundantFilePrivateClass.staticMethodCallingFilePrivate() assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) } } - -fileprivate class NotRedundantFilePrivateClass { - fileprivate static func usedFilePrivateMethod() {} - - static func staticMethodCallingFilePrivate() { - usedFilePrivateMethod() - } -} - -fileprivate class RedundantFilePrivateClass { - fileprivate func unusedFilePrivateMethod() {} -} From 882e58d4d95665bd061bf516350be66cab1225b6 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 15 Dec 2025 14:29:08 -0800 Subject: [PATCH 07/30] Remove files that got moved --- ...edundantFilePrivateAccessibilityTest.swift | 21 -------- .../RedundantInternalAccessibilityTest.swift | 54 ------------------- 2 files changed, 75 deletions(-) delete mode 100644 Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift delete mode 100644 Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift deleted file mode 100644 index 1d1063498..000000000 --- a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift +++ /dev/null @@ -1,21 +0,0 @@ -import Configuration -@testable import TestShared -import XCTest - -final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { - override static func setUp() { - super.setUp() - build(projectPath: AccessibilityProjectPath) - } - - func testRedundantFilePrivateClass() { - index() - assertRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) - } - - func testNotRedundantFilePrivateClass() { - index() - assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) - } -} - diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift deleted file mode 100644 index 606978083..000000000 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ /dev/null @@ -1,54 +0,0 @@ -import Configuration -@testable import TestShared -import XCTest - -final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { - override static func setUp() { - super.setUp() - build(projectPath: AccessibilityProjectPath) - } - - func testInternalPropertyUsedInExtensionInOtherFile() { - // This should NOT be flagged as redundant - // Tests the case where an internal property is used in an extension in a different file - index() - - // InternalPropertyUsedInExtension.propertyUsedInExtension should NOT be flagged as redundant - // because it's used in InternalPropertyExtension.swift - assertNotRedundantInternalAccessibility(.varInstance("propertyUsedInExtension")) - } - -/* -failed - Expected declaration to have redundant internal accessibility: -Declaration(var.instance, 'propertyOnlyUsedInSameFile', explicit, internal, [internal], [], [], [s:7TargetA31InternalPropertyUsedInExtensionC012propertyOnlydE8SameFileSSvp], InternalPropertyUsedInExtension.swift:7:18) -*/ - func testInternalPropertyUsedOnlyInSameFile() { - // This should be flagged as redundant - // Tests the case where an internal property is only used within its own file - index() - - // InternalPropertyUsedInExtension.propertyOnlyUsedInSameFile should be flagged as redundant - // because it's only used within InternalPropertyUsedInExtension.swift - assertRedundantInternalAccessibility(.varInstance("propertyOnlyUsedInSameFile")) - } - - func testInternalPropertyUsedInMultipleFiles() { - // This should NOT be flagged as redundant - // Tests the case where an internal property is used across multiple files - index() - - // This test would need additional setup with multiple files - // For now, we'll test that the existing NotRedundantInternalClassComponents work - assertNotRedundantInternalAccessibility(.class("NotRedundantInternalClass")) - } - - func testInternalMethodUsedInExtension() { - // This should NOT be flagged as redundant - // Tests the case where an internal method is used in an extension - index() - - // This test would need additional setup with methods in extensions - // For now, we'll test that the existing NotRedundantInternalClassComponents work - assertNotRedundantInternalAccessibility(.functionMethodInstance("usedInternalMethod()")) - } -} From 425e438a2f55a6d2a2291b25583e16cb11c8e561 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Tue, 30 Dec 2025 12:17:33 -0800 Subject: [PATCH 08/30] Update OutputFormatter to match format found in new changes upstream --- Sources/PeripheryKit/Results/OutputFormatter.swift | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 24cee5f52..d16dd9ac8 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -68,14 +68,14 @@ extension OutputFormatter { } case let .redundantPublicAccessibility(modules): let modulesJoined = modules.sorted().joined(separator: ", ") - description += " is declared public, but not used outside of \(modulesJoined)" - case let .redundantInternalAccessibility(files): + description += "Redundant public accessibility for \(kindDisplayName) '\(name)' (not used outside of \(modulesJoined))" + case let .redundantInternalAccessibility(files): let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") - description += " is declared internal, but not used outside of \(filesJoined)" + description += "Redundant internal accessibility for \(kindDisplayName) '\(name)' (not used outside of \(filesJoined))" case let .redundantFilePrivateAccessibility(files): let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") - description += " is declared fileprivate, but not used outside its enclosing scope in \(filesJoined)" - } + description += "Redundant fileprivate accessibility for \(kindDisplayName) '\(name)' (not used outside of \(filesJoined))" + } } else { description += "unused" } From 0c71d320642701842430939fc5841d44ed4a1460 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Tue, 30 Dec 2025 13:23:53 -0800 Subject: [PATCH 09/30] Revert "Remove files that got moved" This reverts commit 882e58d4d95665bd061bf516350be66cab1225b6. --- ...edundantFilePrivateAccessibilityTest.swift | 21 ++++++++ .../RedundantInternalAccessibilityTest.swift | 54 +++++++++++++++++++ 2 files changed, 75 insertions(+) create mode 100644 Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift create mode 100644 Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift new file mode 100644 index 000000000..1d1063498 --- /dev/null +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -0,0 +1,21 @@ +import Configuration +@testable import TestShared +import XCTest + +final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { + override static func setUp() { + super.setUp() + build(projectPath: AccessibilityProjectPath) + } + + func testRedundantFilePrivateClass() { + index() + assertRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) + } + + func testNotRedundantFilePrivateClass() { + index() + assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) + } +} + diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift new file mode 100644 index 000000000..606978083 --- /dev/null +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -0,0 +1,54 @@ +import Configuration +@testable import TestShared +import XCTest + +final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { + override static func setUp() { + super.setUp() + build(projectPath: AccessibilityProjectPath) + } + + func testInternalPropertyUsedInExtensionInOtherFile() { + // This should NOT be flagged as redundant + // Tests the case where an internal property is used in an extension in a different file + index() + + // InternalPropertyUsedInExtension.propertyUsedInExtension should NOT be flagged as redundant + // because it's used in InternalPropertyExtension.swift + assertNotRedundantInternalAccessibility(.varInstance("propertyUsedInExtension")) + } + +/* +failed - Expected declaration to have redundant internal accessibility: +Declaration(var.instance, 'propertyOnlyUsedInSameFile', explicit, internal, [internal], [], [], [s:7TargetA31InternalPropertyUsedInExtensionC012propertyOnlydE8SameFileSSvp], InternalPropertyUsedInExtension.swift:7:18) +*/ + func testInternalPropertyUsedOnlyInSameFile() { + // This should be flagged as redundant + // Tests the case where an internal property is only used within its own file + index() + + // InternalPropertyUsedInExtension.propertyOnlyUsedInSameFile should be flagged as redundant + // because it's only used within InternalPropertyUsedInExtension.swift + assertRedundantInternalAccessibility(.varInstance("propertyOnlyUsedInSameFile")) + } + + func testInternalPropertyUsedInMultipleFiles() { + // This should NOT be flagged as redundant + // Tests the case where an internal property is used across multiple files + index() + + // This test would need additional setup with multiple files + // For now, we'll test that the existing NotRedundantInternalClassComponents work + assertNotRedundantInternalAccessibility(.class("NotRedundantInternalClass")) + } + + func testInternalMethodUsedInExtension() { + // This should NOT be flagged as redundant + // Tests the case where an internal method is used in an extension + index() + + // This test would need additional setup with methods in extensions + // For now, we'll test that the existing NotRedundantInternalClassComponents work + assertNotRedundantInternalAccessibility(.functionMethodInstance("usedInternalMethod()")) + } +} From 5ad3a8562d66bc12401acab3ad026718563f0ac6 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Tue, 30 Dec 2025 13:29:05 -0800 Subject: [PATCH 10/30] remove old inline failure warning --- .../RedundantInternalAccessibilityTest.swift | 4 ---- 1 file changed, 4 deletions(-) diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 606978083..1b806cfe6 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -18,10 +18,6 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.varInstance("propertyUsedInExtension")) } -/* -failed - Expected declaration to have redundant internal accessibility: -Declaration(var.instance, 'propertyOnlyUsedInSameFile', explicit, internal, [internal], [], [], [s:7TargetA31InternalPropertyUsedInExtensionC012propertyOnlydE8SameFileSSvp], InternalPropertyUsedInExtension.swift:7:18) -*/ func testInternalPropertyUsedOnlyInSameFile() { // This should be flagged as redundant // Tests the case where an internal property is only used within its own file From 0543aa1f9b8a12fc55f1dd1a288fe67a95102f24 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Tue, 30 Dec 2025 19:53:13 -0800 Subject: [PATCH 11/30] try to deal with most of the warnings from `mise r scan` --- Sources/Extensions/FilePath+Glob.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Extensions/FilePath+Glob.swift b/Sources/Extensions/FilePath+Glob.swift index bdc0b52d2..0194d98ad 100644 --- a/Sources/Extensions/FilePath+Glob.swift +++ b/Sources/Extensions/FilePath+Glob.swift @@ -28,7 +28,7 @@ private class Glob { private let excludedDirectories: [String] private var isDirectoryCache: [String: Bool] = [:] - fileprivate var paths: Set = [] + internal var paths: Set = [] init( pattern: String, From bde0395fe1552cfe570a9b3d026c8e0067052c0a Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:07:43 -0800 Subject: [PATCH 12/30] take out specifier completely to make `mise` happy --- Sources/Extensions/FilePath+Glob.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Extensions/FilePath+Glob.swift b/Sources/Extensions/FilePath+Glob.swift index 0194d98ad..390d961fe 100644 --- a/Sources/Extensions/FilePath+Glob.swift +++ b/Sources/Extensions/FilePath+Glob.swift @@ -28,7 +28,7 @@ private class Glob { private let excludedDirectories: [String] private var isDirectoryCache: [String: Bool] = [:] - internal var paths: Set = [] + var paths: Set = [] init( pattern: String, From 62859d136be2999eda95b6f187cd774ee6980aaa Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 1 Jan 2026 13:11:35 -0800 Subject: [PATCH 13/30] After running `mise r lint` and `mise r gen-bazel-rules` --- Sources/BUILD.bazel | 2 +- .../Results/OutputFormatter.swift | 4 ++-- ...undantFilePrivateAccessibilityMarker.swift | 19 +++++++-------- ...RedundantInternalAccessibilityMarker.swift | 23 ++++++++++--------- ...edundantFilePrivateAccessibilityTest.swift | 1 - .../RedundantInternalAccessibilityTest.swift | 18 +++++++-------- 6 files changed, 34 insertions(+), 33 deletions(-) diff --git a/Sources/BUILD.bazel b/Sources/BUILD.bazel index e17c06b7e..b6648e20c 100644 --- a/Sources/BUILD.bazel +++ b/Sources/BUILD.bazel @@ -77,8 +77,8 @@ swift_library( "SourceGraph/Mutators/ProtocolExtensionReferenceBuilder.swift", "SourceGraph/Mutators/PubliclyAccessibleRetainer.swift", "SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift", - "SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift", "SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift", + "SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift", "SourceGraph/Mutators/RedundantProtocolMarker.swift", "SourceGraph/Mutators/ResultBuilderRetainer.swift", "SourceGraph/Mutators/StringInterpolationAppendInterpolationRetainer.swift", diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 1e9f68274..fcc6d5368 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -68,10 +68,10 @@ extension OutputFormatter { let modulesJoined = modules.sorted().joined(separator: ", ") description += "Redundant public accessibility for \(kindDisplayName) '\(name)' (not used outside of \(modulesJoined))" case let .redundantInternalAccessibility(files): - let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") + let filesJoined = files.sorted { $0.path.string < $1.path.string }.map(\.path.string).joined(separator: ", ") description += "Redundant internal accessibility for \(kindDisplayName) '\(name)' (not used outside of \(filesJoined))" case let .redundantFilePrivateAccessibility(files): - let filesJoined = files.sorted { $0.path.string < $1.path.string }.map { $0.path.string }.joined(separator: ", ") + let filesJoined = files.sorted { $0.path.string < $1.path.string }.map(\.path.string).joined(separator: ", ") description += "Redundant fileprivate accessibility for \(kindDisplayName) '\(name)' (not used outside of \(filesJoined))" } } else { diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift index ad9d82bda..738adbe72 100644 --- a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -35,21 +35,22 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { } /* - Always check descendents, even if parent is not redundant. + Always check descendents, even if parent is not redundant. - A parent declaration may be used outside its file (making it not redundant), - while still having child declarations that are only used within the same file - (making those children redundant). For example, a class used cross-file may have - a fileprivate property only referenced within the same file - that property should - be flagged as redundant even though the parent class is not. - */ + A parent declaration may be used outside its file (making it not redundant), + while still having child declarations that are only used within the same file + (making those children redundant). For example, a class used cross-file may have + a fileprivate property only referenced within the same file - that property should + be flagged as redundant even though the parent class is not. + */ markExplicitFilePrivateDescendentDeclarations(from: decl) } private func validateExtension(_ decl: Declaration) throws { if decl.accessibility.isExplicitly(.fileprivate) { if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), - graph.redundantFilePrivateAccessibility.keys.contains(extendedDecl) { + graph.redundantFilePrivateAccessibility.keys.contains(extendedDecl) + { mark(decl) } } @@ -75,4 +76,4 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { let filePrivateDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.fileprivate) } return filePrivateDeclarations.flatMapSet { descendentFilePrivateDeclarations(from: $0) }.union(filePrivateDeclarations) } -} \ No newline at end of file +} diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 62c5ffdbc..75a165816 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -38,21 +38,22 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { } /* - Always check descendents, even if parent is not redundant. - - A parent declaration may be used outside its file (making it not redundant), - while still having child declarations that are only used within the same file - (making those children redundant). For example, a class used cross-file may have - an internal property only referenced within the same file - that property should - be flagged as redundant even though the parent class is not. - */ + Always check descendents, even if parent is not redundant. + + A parent declaration may be used outside its file (making it not redundant), + while still having child declarations that are only used within the same file + (making those children redundant). For example, a class used cross-file may have + an internal property only referenced within the same file - that property should + be flagged as redundant even though the parent class is not. + */ markExplicitInternalDescendentDeclarations(from: decl) } private func validateExtension(_ decl: Declaration) throws { if decl.accessibility.isExplicitly(.internal) { if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), - graph.redundantInternalAccessibility.keys.contains(extendedDecl) { + graph.redundantInternalAccessibility.keys.contains(extendedDecl) + { mark(decl) } } @@ -78,7 +79,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // Use graph.references(to: decl) to get all references to this declaration let allReferences = graph.references(to: decl) let referenceFiles = allReferences.map(\.location.file) - + let result = referenceFiles.contains { $0 != decl.location.file } return result } @@ -87,4 +88,4 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let internalDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.internal) } return internalDeclarations.flatMapSet { descendentInternalDeclarations(from: $0) }.union(internalDeclarations) } -} \ No newline at end of file +} diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift index 1d1063498..45bf4614c 100644 --- a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -18,4 +18,3 @@ final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) } } - diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 1b806cfe6..3f91e497d 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -7,44 +7,44 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { super.setUp() build(projectPath: AccessibilityProjectPath) } - + func testInternalPropertyUsedInExtensionInOtherFile() { // This should NOT be flagged as redundant // Tests the case where an internal property is used in an extension in a different file index() - + // InternalPropertyUsedInExtension.propertyUsedInExtension should NOT be flagged as redundant // because it's used in InternalPropertyExtension.swift assertNotRedundantInternalAccessibility(.varInstance("propertyUsedInExtension")) } - + func testInternalPropertyUsedOnlyInSameFile() { // This should be flagged as redundant // Tests the case where an internal property is only used within its own file index() - + // InternalPropertyUsedInExtension.propertyOnlyUsedInSameFile should be flagged as redundant // because it's only used within InternalPropertyUsedInExtension.swift assertRedundantInternalAccessibility(.varInstance("propertyOnlyUsedInSameFile")) } - + func testInternalPropertyUsedInMultipleFiles() { // This should NOT be flagged as redundant // Tests the case where an internal property is used across multiple files index() - + // This test would need additional setup with multiple files // For now, we'll test that the existing NotRedundantInternalClassComponents work assertNotRedundantInternalAccessibility(.class("NotRedundantInternalClass")) } - + func testInternalMethodUsedInExtension() { // This should NOT be flagged as redundant // Tests the case where an internal method is used in an extension index() - + // This test would need additional setup with methods in extensions // For now, we'll test that the existing NotRedundantInternalClassComponents work assertNotRedundantInternalAccessibility(.functionMethodInstance("usedInternalMethod()")) } -} +} From 99b7fe9e5099f9f08d973e286e2deecc50540d79 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 1 Jan 2026 15:25:37 -0800 Subject: [PATCH 14/30] restore paths to be fileprivate, since we were getting false positive. Fix the scanning logic for that, and update the tests to reflect that. --- Sources/Extensions/FilePath+Glob.swift | 2 +- ...undantFilePrivateAccessibilityMarker.swift | 74 ++++++++++++++++++- ...antFilePrivatePropertyInPrivateClass.swift | 37 ++++++++++ .../TargetA/RedundantFilePrivateClass.swift | 8 +- ...edundantFilePrivateAccessibilityTest.swift | 33 ++++++++- Tests/Shared/SourceGraphTestCase.swift | 12 --- 6 files changed, 146 insertions(+), 20 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivatePropertyInPrivateClass.swift diff --git a/Sources/Extensions/FilePath+Glob.swift b/Sources/Extensions/FilePath+Glob.swift index 390d961fe..bdc0b52d2 100644 --- a/Sources/Extensions/FilePath+Glob.swift +++ b/Sources/Extensions/FilePath+Glob.swift @@ -28,7 +28,7 @@ private class Glob { private let excludedDirectories: [String] private var isDirectoryCache: [String: Bool] = [:] - var paths: Set = [] + fileprivate var paths: Set = [] init( pattern: String, diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift index 738adbe72..981640013 100644 --- a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -29,7 +29,7 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { private func validate(_ decl: Declaration) throws { if decl.accessibility.isExplicitly(.fileprivate) { - if !graph.isRetained(decl), !isReferencedOutsideFile(decl) { + if !graph.isRetained(decl), !isReferencedOutsideFile(decl), !isReferencedFromDifferentTypeInSameFile(decl) { mark(decl) } } @@ -63,7 +63,9 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { private func markExplicitFilePrivateDescendentDeclarations(from decl: Declaration) { for descDecl in descendentFilePrivateDeclarations(from: decl) { - mark(descDecl) + if !graph.isRetained(descDecl), !isReferencedOutsideFile(descDecl), !isReferencedFromDifferentTypeInSameFile(descDecl) { + mark(descDecl) + } } } @@ -76,4 +78,72 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { let filePrivateDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.fileprivate) } return filePrivateDeclarations.flatMapSet { descendentFilePrivateDeclarations(from: $0) }.union(filePrivateDeclarations) } + + /** + Finds the top-level type declaration by walking up the parent chain. + Returns the outermost type that contains the given declaration. + */ + private func topLevelType(of decl: Declaration) -> Declaration? { + let baseTypeKinds: Set = [.class, .struct, .enum, .protocol] + let typeKinds = baseTypeKinds.union(Declaration.Kind.extensionKinds) + let ancestors = [decl] + Array(decl.ancestralDeclarations) + return ancestors.last { typeDecl in + guard typeKinds.contains(typeDecl.kind) else { return false } + guard let parent = typeDecl.parent else { return true } + return !typeKinds.contains(parent.kind) + } + } + + /** + Gets the logical type for comparison purposes. + For extensions of types in the SAME FILE, treats the extension as the extended type. + For extensions of types in DIFFERENT FILES (like extending external types), + treats the extension as its own distinct type for the purpose of this file. + */ + private func logicalType(of decl: Declaration, inFile file: SourceFile) -> Declaration? { + if decl.kind.isExtensionKind { + if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), + extendedDecl.location.file == file + { + return extendedDecl + } + return decl + } + return decl + } + + /** + Checks if a declaration is referenced from a different type in the same file. + Returns true if any same-file reference comes from a different logical type, + indicating that fileprivate access is necessary. + + Even for top-level declarations, private and fileprivate are different: + - private: only accessible within the declaration itself and its extensions in the same file + - fileprivate: accessible from anywhere in the same file + */ + private func isReferencedFromDifferentTypeInSameFile(_ decl: Declaration) -> Bool { + let file = decl.location.file + let sameFileReferences = graph.references(to: decl).filter { $0.location.file == file } + + guard let declTopLevel = topLevelType(of: decl) else { + return false + } + + let declLogicalType = logicalType(of: declTopLevel, inFile: file) + + for ref in sameFileReferences { + guard let refParent = ref.parent, + let refTopLevel = topLevelType(of: refParent) + else { + continue + } + + let refLogicalType = logicalType(of: refTopLevel, inFile: file) + + if declLogicalType !== refLogicalType { + return true + } + } + return false + } } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivatePropertyInPrivateClass.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivatePropertyInPrivateClass.swift new file mode 100644 index 000000000..3673f0db4 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantFilePrivatePropertyInPrivateClass.swift @@ -0,0 +1,37 @@ +/* + NotRedundantFilePrivatePropertyInPrivateClass.swift + Tests that a fileprivate property in a private class is NOT flagged as redundant + when accessed from a different type (extension) in the same file. + + This replicates the pattern from FilePath+Glob.swift where: + - A private class has a fileprivate property + - An extension of a different type accesses that property + - The fileprivate modifier is necessary for cross-type same-file access +*/ + +public extension SomePublicType { + static func accessPrivateClass() -> Set { + PrivateClassWithFilePrivateProperty().filePrivatePaths + } +} + +private class PrivateClassWithFilePrivateProperty { + fileprivate var filePrivatePaths: Set = ["path1", "path2"] + + func someMethod() { + _ = filePrivatePaths.count + } +} + +/* + This retainer ensures the extension method is used, making the file indexed. + The key test is that filePrivatePaths is accessed from the SomePublicType extension, + which is a different type than PrivateClassWithFilePrivateProperty. +*/ +public struct SomePublicType { + public init() {} + + public func retain() { + _ = SomePublicType.accessPrivateClass() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift index 8aaf98996..5b4d5e3f3 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantFilePrivateClass.swift @@ -1,6 +1,7 @@ /* RedundantFilePrivateClass.swift - Tests that a fileprivate class is flagged as redundant when it's only referenced from within its own declaration + This file tests a case where fileprivate is actually NOT redundant because the + class is accessed from a different type (RedundantFilePrivateClassRetainer). */ fileprivate class RedundantFilePrivateClass { @@ -18,9 +19,8 @@ fileprivate class RedundantFilePrivateClass { } /* - This retainer ensures the file is indexed and calls a method on the class. - The fileprivate class is used, but only within the same file and not by other declarations, - making the fileprivate access level redundant (could be private). + This retainer accesses RedundantFilePrivateClass from a different type, + which means fileprivate is necessary (NOT redundant). */ public class RedundantFilePrivateClassRetainer { public init() {} diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift index 45bf4614c..936c7e5d6 100644 --- a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -8,13 +8,44 @@ final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { build(projectPath: AccessibilityProjectPath) } + /* + Tests that a fileprivate class is NOT flagged as redundant when accessed + from a different type in the same file. + + In Swift, private and fileprivate have distinct meanings even for top-level declarations: + - private: accessible only within the lexical scope and its extensions in the same file + - fileprivate: accessible from anywhere in the same file + + Since RedundantFilePrivateClass is accessed from RedundantFilePrivateClassRetainer + (a different type), fileprivate is the minimum access level required. Changing it to + private would prevent RedundantFilePrivateClassRetainer from accessing it. + */ func testRedundantFilePrivateClass() { index() - assertRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) + assertNotRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) } func testNotRedundantFilePrivateClass() { index() assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) } + + func testNotRedundantFilePrivatePropertyInPrivateClass() { + index() + assertNotRedundantFilePrivateAccessibility(.varInstance("filePrivatePaths")) + } + + /* + NOTE: the opposite of the above, e.g. a function called "assertRedundantFilePrivateAccessibility()", + is intentionally not tested here. + + After fixing the bug where cross-type same-file references were incorrectly + flagged as redundant, there are no meaningful test cases for truly redundant + fileprivate declarations. Here's why: + + - Fileprivate exists specifically for same-file cross-type access + - A fileprivate declaration used only within its own type should be private + - A fileprivate declaration with no references at all is marked as "unused", not + "redundant fileprivate" + */ } diff --git a/Tests/Shared/SourceGraphTestCase.swift b/Tests/Shared/SourceGraphTestCase.swift index 0d28bf25a..0ef2ff313 100644 --- a/Tests/Shared/SourceGraphTestCase.swift +++ b/Tests/Shared/SourceGraphTestCase.swift @@ -200,18 +200,6 @@ open class SourceGraphTestCase: XCTestCase { scopeStack.removeLast() } - func assertRedundantFilePrivateAccessibility(_ 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.results.redundantFilePrivateAccessibilityDeclarations.contains(declaration) { - XCTFail("Expected declaration to have redundant fileprivate accessibility: \(declaration)", file: file, line: line) - } - - scopeStack.append(.declaration(declaration)) - scopedAssertions?() - scopeStack.removeLast() - } - func assertNotRedundantFilePrivateAccessibility(_ 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 } From 06ca954e5e0b8853b0b97199b0456cea45479c77 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 1 Jan 2026 18:06:54 -0800 Subject: [PATCH 15/30] readme update --- README.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/README.md b/README.md index 10474ff32..7f146338c 100644 --- a/README.md +++ b/README.md @@ -28,6 +28,8 @@ - [Enumerations](#enumerations) - [Assign-only Properties](#assign-only-properties) - [Redundant Public Accessibility](#redundant-public-accessibility) + - [Redundant Internal Accessibility](#redundant-internal-accessibility) + - [Redundant Fileprivate Accessibility](#redundant-fileprivate-accessibility) - [Unused Imports](#unused-imports) - [Objective-C](#objective-c) - [Codable](#codable) @@ -291,6 +293,18 @@ Declarations that are marked `public` yet are not referenced from outside their This analysis can be disabled with `--disable-redundant-public-analysis`. +### Redundant Internal Accessibility + +Declarations that are marked `internal` (or are unmarked, since this is Swift's default access level), yet are not referenced outside the file they're defined in are identified as having redundant internal accessibility. In this scenario, the declaration could be marked `private` or `fileprivate`. Reducing the visibility of declarations — encapsulation — helps with code maintainability and can improve compilation performance. + +This analysis can be disabled with `--disable-redundant-internal-analysis`. + +### Redundant Fileprivate Accessibility + +Declarations that are marked `fileprivate` yet are not accessed from other types within the same file are identified as having redundant fileprivate accessibility. If a `fileprivate` declaration is only used within its own type, it should be marked `private` instead. Reducing the visibility of declarations helps with code maintainability and makes access boundaries clearer. + +This analysis can be disabled with `--disable-redundant-fileprivate-analysis`. + ### Unused Imports Periphery can only detect unused imports of targets it has scanned. It cannot detect unused imports of other targets because the Swift source files are unavailable and uses of `@_exported` cannot be observed. `@_exported` is problematic because it changes the public interface of a target such that the declarations exported by the target are no longer necessarily declared by the imported target. For example, the `Foundation` target exports `Dispatch`, among other targets. If any given source file imports `Foundation` and references `DispatchQueue` but no other declarations from `Foundation`, then the `Foundation` import cannot be removed as it would also make the `DispatchQueue` type unavailable. To avoid false positives, therefore, Periphery only detects unused imports of targets it has scanned. From bbc79900e06ef0339593916c7f951230678b3cd8 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 1 Jan 2026 18:18:58 -0800 Subject: [PATCH 16/30] A bit more documentation --- ...RedundantFilePrivateAccessibilityMarker.swift | 16 ++++++++++++++++ .../RedundantInternalAccessibilityMarker.swift | 11 +++++++++++ 2 files changed, 27 insertions(+) diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift index 981640013..cdf40dbab 100644 --- a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -1,6 +1,22 @@ import Configuration import Shared +/** + Identifies declarations explicitly marked `fileprivate` that don't actually need file-level access. + + Swift's `fileprivate` exists specifically to allow access from other types within the same file. + If a `fileprivate` declaration is only accessed within its own type (not from other types in + the same file), it should be marked `private` instead. + + This mutator is more complex than RedundantInternalAccessibilityMarker because it must: + - Distinguish between access from the same type vs. different types in the same file + - Handle extensions of types (both same-file and cross-file extensions) + - Walk the type hierarchy to find the top-level containing type for comparison + + The key insight: `private` and `fileprivate` differ in that `private` is accessible only within + the declaration and its extensions in the same file, while `fileprivate` is accessible from + anywhere in the same file. + */ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { private let graph: SourceGraph private let configuration: Configuration diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 75a165816..da2d87d35 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -1,6 +1,17 @@ import Configuration import Shared +/** + Identifies declarations explicitly marked `internal` that are not referenced outside + the file they're defined in. + + Since `internal` is Swift's default access level, declarations that are only used within + their defining file should be marked `private` or `fileprivate` instead. This improves + encapsulation and can help with compilation performance. + + This mutator follows the same pattern as RedundantPublicAccessibilityMarker but checks + for file-scoped usage instead of module-scoped usage. + */ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { private let graph: SourceGraph private let configuration: Configuration From d4483b00ed3a845cdd86c2b80036e00fcc60a946 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Wed, 7 Jan 2026 11:06:16 -0800 Subject: [PATCH 17/30] Handle implicit internal, fix false positives and false negatives, refactor checking --- Sources/BUILD.bazel | 1 + Sources/Configuration/Configuration.swift | 5 +- Sources/Frontend/Commands/ScanCommand.swift | 4 + .../Results/OutputFormatter.swift | 12 +- Sources/PeripheryKit/ScanResult.swift | 4 +- Sources/PeripheryKit/ScanResultBuilder.swift | 4 +- .../RedundantAccessibilityMarkerShared.swift | 55 ++++ ...antExplicitPublicAccessibilityMarker.swift | 5 +- ...undantFilePrivateAccessibilityMarker.swift | 128 +++++--- ...RedundantInternalAccessibilityMarker.swift | 309 +++++++++++++++--- Sources/SourceGraph/SourceGraph.swift | 23 +- .../Sources/MainTarget/main.swift | 9 + ...ternalSuggestingPrivateVsFileprivate.swift | 46 +++ .../TargetA/NestedTypeAccessibility.swift | 43 +++ .../NotRedundantInternalClassComponents.swift | 5 + ...ndantInternalClassComponents_Support.swift | 13 +- .../PropertyWrapperAccessibility.swift | 49 +++ .../ProtocolRequirementAccessibility.swift | 41 +++ .../TargetA/RedundantInternalComponents.swift | 65 +++- .../TrulyRedundantFilePrivateMembers.swift | 38 +++ ...edundantFilePrivateAccessibilityTest.swift | 55 ++-- .../RedundantInternalAccessibilityTest.swift | 137 +++++++- Tests/Shared/SourceGraphTestCase.swift | 31 +- 23 files changed, 937 insertions(+), 145 deletions(-) create mode 100644 Sources/SourceGraph/Mutators/RedundantAccessibilityMarkerShared.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalSuggestingPrivateVsFileprivate.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NestedTypeAccessibility.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PropertyWrapperAccessibility.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ProtocolRequirementAccessibility.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TrulyRedundantFilePrivateMembers.swift diff --git a/Sources/BUILD.bazel b/Sources/BUILD.bazel index 4e4d594f7..c0a481d31 100644 --- a/Sources/BUILD.bazel +++ b/Sources/BUILD.bazel @@ -77,6 +77,7 @@ swift_library( "SourceGraph/Mutators/ProtocolConformanceReferenceBuilder.swift", "SourceGraph/Mutators/ProtocolExtensionReferenceBuilder.swift", "SourceGraph/Mutators/PubliclyAccessibleRetainer.swift", + "SourceGraph/Mutators/RedundantAccessibilityMarkerShared.swift", "SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift", "SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift", "SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift", diff --git a/Sources/Configuration/Configuration.swift b/Sources/Configuration/Configuration.swift index 1b9cbb792..f05b208aa 100644 --- a/Sources/Configuration/Configuration.swift +++ b/Sources/Configuration/Configuration.swift @@ -86,6 +86,9 @@ public final class Configuration { @Setting(key: "disable_redundant_fileprivate_analysis", defaultValue: false) public var disableRedundantFilePrivateAnalysis: Bool + @Setting(key: "show_nested_redundant_accessibility", defaultValue: false) + public var showNestedRedundantAccessibility: Bool + @Setting(key: "disable_unused_import_analysis", defaultValue: false) public var disableUnusedImportAnalysis: Bool @@ -219,7 +222,7 @@ public final class Configuration { $project, $schemes, $excludeTargets, $excludeTests, $indexExclude, $reportExclude, $reportInclude, $outputFormat, $retainPublic, $noRetainSPI, $retainFiles, $retainAssignOnlyProperties, $retainAssignOnlyPropertyTypes, $retainObjcAccessible, $retainObjcAnnotated, $retainUnusedProtocolFuncParams, $retainSwiftUIPreviews, $disableRedundantPublicAnalysis, - $disableRedundantInternalAnalysis, $disableRedundantFilePrivateAnalysis, + $disableRedundantInternalAnalysis, $disableRedundantFilePrivateAnalysis, $showNestedRedundantAccessibility, $disableUnusedImportAnalysis, $retainUnusedImportedModules, $externalEncodableProtocols, $externalCodableProtocols, $externalTestCaseClasses, $verbose, $quiet, $color, $disableUpdateCheck, $strict, $indexStorePath, $skipBuild, $skipSchemesValidation, $cleanBuild, $buildArguments, $xcodeListArguments, $relativeResults, diff --git a/Sources/Frontend/Commands/ScanCommand.swift b/Sources/Frontend/Commands/ScanCommand.swift index 130fff2a4..3314b897b 100644 --- a/Sources/Frontend/Commands/ScanCommand.swift +++ b/Sources/Frontend/Commands/ScanCommand.swift @@ -69,6 +69,9 @@ struct ScanCommand: ParsableCommand { @Flag(help: "Disable identification of redundant fileprivate accessibility") var disableRedundantFilePrivateAnalysis: Bool = defaultConfiguration.$disableRedundantFilePrivateAnalysis.defaultValue + @Flag(help: "Show redundant internal/fileprivate accessibility warnings for nested declarations even when the containing type is already flagged") + var showNestedRedundantAccessibility: Bool = defaultConfiguration.$showNestedRedundantAccessibility.defaultValue + @Flag(help: "Disable identification of unused imports") var disableUnusedImportAnalysis: Bool = defaultConfiguration.$disableUnusedImportAnalysis.defaultValue @@ -195,6 +198,7 @@ struct ScanCommand: ParsableCommand { configuration.apply(\.$disableRedundantPublicAnalysis, disableRedundantPublicAnalysis) configuration.apply(\.$disableRedundantInternalAnalysis, disableRedundantInternalAnalysis) configuration.apply(\.$disableRedundantFilePrivateAnalysis, disableRedundantFilePrivateAnalysis) + configuration.apply(\.$showNestedRedundantAccessibility, showNestedRedundantAccessibility) configuration.apply(\.$disableUnusedImportAnalysis, disableUnusedImportAnalysis) configuration.apply(\.$retainUnusedImportedModules, retainUnusedImportedModules) configuration.apply(\.$externalEncodableProtocols, externalEncodableProtocols) diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 26be58f7c..491bafad7 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -68,12 +68,12 @@ extension OutputFormatter { case let .redundantPublicAccessibility(modules): let modulesJoined = modules.sorted().joined(separator: ", ") description += "Redundant public accessibility for \(kindDisplayName) '\(name)' (not used outside of \(modulesJoined))" - case let .redundantInternalAccessibility(files): - let filesJoined = files.sorted { $0.path.string < $1.path.string }.map(\.path.string).joined(separator: ", ") - description += "Redundant internal accessibility for \(kindDisplayName) '\(name)' (not used outside of \(filesJoined))" - case let .redundantFilePrivateAccessibility(files): - let filesJoined = files.sorted { $0.path.string < $1.path.string }.map(\.path.string).joined(separator: ", ") - description += "Redundant fileprivate accessibility for \(kindDisplayName) '\(name)' (not used outside of \(filesJoined))" + case let .redundantInternalAccessibility(_, suggestedAccessibility): + let accessibilityText = suggestedAccessibility?.rawValue ?? "private/fileprivate" + description += "Redundant internal accessibility for \(kindDisplayName) '\(name)' (not used outside of file; can be \(accessibilityText)" + case let .redundantFilePrivateAccessibility(_, containingTypeName): + let context = containingTypeName.map { "only used within \($0)" } ?? "not used outside of file" + description += "Redundant fileprivate accessibility for \(kindDisplayName) '\(name)' (\(context); can be private)" } } else { description += "Unused" diff --git a/Sources/PeripheryKit/ScanResult.swift b/Sources/PeripheryKit/ScanResult.swift index f672fa9e4..d53841de8 100644 --- a/Sources/PeripheryKit/ScanResult.swift +++ b/Sources/PeripheryKit/ScanResult.swift @@ -7,8 +7,8 @@ public struct ScanResult { case assignOnlyProperty case redundantProtocol(references: Set, inherited: Set) case redundantPublicAccessibility(modules: Set) - case redundantInternalAccessibility(files: Set) - case redundantFilePrivateAccessibility(files: Set) + case redundantInternalAccessibility(files: Set, suggestedAccessibility: Accessibility?) + case redundantFilePrivateAccessibility(files: Set, containingTypeName: String?) } let declaration: Declaration diff --git a/Sources/PeripheryKit/ScanResultBuilder.swift b/Sources/PeripheryKit/ScanResultBuilder.swift index 92a190899..e9b49d0d9 100644 --- a/Sources/PeripheryKit/ScanResultBuilder.swift +++ b/Sources/PeripheryKit/ScanResultBuilder.swift @@ -43,10 +43,10 @@ public enum ScanResultBuilder { .init(declaration: $0.0, annotation: .redundantPublicAccessibility(modules: $0.1)) } let annotatedRedundantInternalAccessibility: [ScanResult] = redundantInternalAccessibility.map { - .init(declaration: $0.0, annotation: .redundantInternalAccessibility(files: $0.1)) + .init(declaration: $0.0, annotation: .redundantInternalAccessibility(files: $0.1.files, suggestedAccessibility: $0.1.suggestedAccessibility)) } let annotatedRedundantFilePrivateAccessibility: [ScanResult] = redundantFilePrivateAccessibility.map { - .init(declaration: $0.0, annotation: .redundantFilePrivateAccessibility(files: $0.1)) + .init(declaration: $0.0, annotation: .redundantFilePrivateAccessibility(files: $0.1.files, containingTypeName: $0.1.containingTypeName)) } let allAnnotatedDeclarations = annotatedRemovableDeclarations + annotatedAssignOnlyProperties + diff --git a/Sources/SourceGraph/Mutators/RedundantAccessibilityMarkerShared.swift b/Sources/SourceGraph/Mutators/RedundantAccessibilityMarkerShared.swift new file mode 100644 index 000000000..894de5361 --- /dev/null +++ b/Sources/SourceGraph/Mutators/RedundantAccessibilityMarkerShared.swift @@ -0,0 +1,55 @@ +// Shared utilities for redundant accessibility analysis mutators. + +extension Declaration { + /// Checks if this declaration is referenced outside its defining file. + /// This is a common check used by multiple accessibility markers to determine + /// if a declaration needs file-level or module-level accessibility. + func isReferencedOutsideFile(graph: SourceGraph) -> Bool { + graph.references(to: self).map(\.location.file).contains { $0 != location.file } + } + + /// Generic recursive descendent declaration finder with filtering. + /// Recursively traverses the declaration tree and returns all descendants matching the predicate. + /// Used by all accessibility markers to find declarations with specific accessibility levels. + func descendentDeclarations(matching predicate: (Declaration) -> Bool) -> Set { + let matchingDeclarations = declarations.filter(predicate) + return matchingDeclarations + .flatMapSet { $0.descendentDeclarations(matching: predicate) } + .union(matchingDeclarations) + } + + /// Checks if any ancestor declaration is marked as redundant in the given accessibility map. + /// Used by accessibility markers to suppress nested warnings when a containing type is already flagged. + /// This avoids redundant warnings since fixing the parent's accessibility fixes the children too. + func isAnyAncestorMarked(in accessibilityMap: [Declaration: Any]) -> Bool { + var current = parent + var visited: Set = [] + + while let currentParent = current { + guard !visited.contains(currentParent) else { + return false + } + + visited.insert(currentParent) + + if accessibilityMap[currentParent] != nil { + return true + } + current = currentParent.parent + } + return false + } + + /// Counts the number of ancestors for this declaration. + /// Used for sorting declarations by depth to ensure parents are marked before children, + /// which is important for nested redundant accessibility suppression logic. + var ancestorCount: Int { + var count = 0 + var current = parent + while current != nil { + count += 1 + current = current?.parent + } + return count + } +} diff --git a/Sources/SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift index 5e6f71eaa..5f50ade08 100644 --- a/Sources/SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantExplicitPublicAccessibilityMarker.swift @@ -184,7 +184,8 @@ final class RedundantExplicitPublicAccessibilityMarker: SourceGraphMutator { } private func descendentPublicDeclarations(from decl: Declaration) -> Set { - let publicDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.public) } - return publicDeclarations.flatMapSet { descendentPublicDeclarations(from: $0) }.union(publicDeclarations) + decl.descendentDeclarations(matching: { + !$0.isImplicit && $0.accessibility.isExplicitly(.public) + }) } } diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift index cdf40dbab..fa118fb46 100644 --- a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -1,22 +1,20 @@ import Configuration import Shared -/** - Identifies declarations explicitly marked `fileprivate` that don't actually need file-level access. - - Swift's `fileprivate` exists specifically to allow access from other types within the same file. - If a `fileprivate` declaration is only accessed within its own type (not from other types in - the same file), it should be marked `private` instead. - - This mutator is more complex than RedundantInternalAccessibilityMarker because it must: - - Distinguish between access from the same type vs. different types in the same file - - Handle extensions of types (both same-file and cross-file extensions) - - Walk the type hierarchy to find the top-level containing type for comparison - - The key insight: `private` and `fileprivate` differ in that `private` is accessible only within - the declaration and its extensions in the same file, while `fileprivate` is accessible from - anywhere in the same file. - */ +/// Identifies declarations explicitly marked `fileprivate` that don't actually need file-level access. +/// +/// Swift's `fileprivate` exists specifically to allow access from other types within the same file. +/// If a `fileprivate` declaration is only accessed within its own type (not from other types in +/// the same file), it should be marked `private` instead. +/// +/// This mutator is more complex than RedundantInternalAccessibilityMarker because it must: +/// - Distinguish between access from the same type vs. different types in the same file +/// - Handle extensions of types (both same-file and cross-file extensions) +/// - Walk the type hierarchy to find the top-level containing type for comparison +/// +/// The key insight: `private` and `fileprivate` differ in that `private` is accessible only within +/// the declaration and its extensions in the same file, while `fileprivate` is accessible from +/// anywhere in the same file. final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { private let graph: SourceGraph private let configuration: Configuration @@ -45,20 +43,21 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { private func validate(_ decl: Declaration) throws { if decl.accessibility.isExplicitly(.fileprivate) { - if !graph.isRetained(decl), !isReferencedOutsideFile(decl), !isReferencedFromDifferentTypeInSameFile(decl) { + if !graph.isRetained(decl), + !decl.isReferencedOutsideFile(graph: graph), + !isReferencedFromDifferentTypeInSameFile(decl) + { mark(decl) } } - /* - Always check descendents, even if parent is not redundant. - - A parent declaration may be used outside its file (making it not redundant), - while still having child declarations that are only used within the same file - (making those children redundant). For example, a class used cross-file may have - a fileprivate property only referenced within the same file - that property should - be flagged as redundant even though the parent class is not. - */ + // Always check descendants, even if parent is not redundant. + // + // A parent declaration may be used outside its file (making it not redundant), + // while still having child declarations that are only used within the same file + // (making those children redundant). For example, a class used cross-file may have + // a fileprivate property only referenced within the same file - that property should + // be flagged as redundant even though the parent class is not. markExplicitFilePrivateDescendentDeclarations(from: decl) } @@ -74,31 +73,44 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { private func mark(_ decl: Declaration) { guard !graph.isRetained(decl) else { return } - graph.markRedundantFilePrivateAccessibility(decl, file: decl.location.file) + + // Unless explicitly requested, skip marking nested declarations when an ancestor is already marked. + // This avoids redundant warnings since fixing the parent's accessibility fixes the children too. + if !configuration.showNestedRedundantAccessibility, + decl.isAnyAncestorMarked(in: graph.redundantFilePrivateAccessibility) + { + return + } + + let containingTypeName = containingTypeName(for: decl) + graph.markRedundantFilePrivateAccessibility(decl, file: decl.location.file, containingTypeName: containingTypeName) } private func markExplicitFilePrivateDescendentDeclarations(from decl: Declaration) { - for descDecl in descendentFilePrivateDeclarations(from: decl) { - if !graph.isRetained(descDecl), !isReferencedOutsideFile(descDecl), !isReferencedFromDifferentTypeInSameFile(descDecl) { + // Sort descendants by their depth to ensure parents are marked before children. + // This is important for the nested redundant accessibility suppression logic. + let descendants = descendentFilePrivateDeclarations(from: decl).sorted { decl1, decl2 in + decl1.ancestorCount < decl2.ancestorCount + } + + for descDecl in descendants { + if !graph.isRetained(descDecl), + !descDecl.isReferencedOutsideFile(graph: graph), + !isReferencedFromDifferentTypeInSameFile(descDecl) + { mark(descDecl) } } } - private func isReferencedOutsideFile(_ decl: Declaration) -> Bool { - let referenceFiles = graph.references(to: decl).map(\.location.file) - return referenceFiles.contains { $0 != decl.location.file } - } - private func descendentFilePrivateDeclarations(from decl: Declaration) -> Set { - let filePrivateDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.fileprivate) } - return filePrivateDeclarations.flatMapSet { descendentFilePrivateDeclarations(from: $0) }.union(filePrivateDeclarations) + decl.descendentDeclarations(matching: { + !$0.isImplicit && $0.accessibility.isExplicitly(.fileprivate) + }) } - /** - Finds the top-level type declaration by walking up the parent chain. - Returns the outermost type that contains the given declaration. - */ + /// Finds the top-level type declaration by walking up the parent chain. + /// Returns the outermost type that contains the given declaration. private func topLevelType(of decl: Declaration) -> Declaration? { let baseTypeKinds: Set = [.class, .struct, .enum, .protocol] let typeKinds = baseTypeKinds.union(Declaration.Kind.extensionKinds) @@ -106,16 +118,15 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { return ancestors.last { typeDecl in guard typeKinds.contains(typeDecl.kind) else { return false } guard let parent = typeDecl.parent else { return true } + return !typeKinds.contains(parent.kind) } } - /** - Gets the logical type for comparison purposes. - For extensions of types in the SAME FILE, treats the extension as the extended type. - For extensions of types in DIFFERENT FILES (like extending external types), - treats the extension as its own distinct type for the purpose of this file. - */ + /// Gets the logical type for comparison purposes. + /// For extensions of types in the SAME FILE, treats the extension as the extended type. + /// For extensions of types in DIFFERENT FILES (like extending external types), + /// treats the extension as its own distinct type for the purpose of this file. private func logicalType(of decl: Declaration, inFile file: SourceFile) -> Declaration? { if decl.kind.isExtensionKind { if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), @@ -128,15 +139,24 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { return decl } - /** - Checks if a declaration is referenced from a different type in the same file. - Returns true if any same-file reference comes from a different logical type, - indicating that fileprivate access is necessary. + /// Extracts a display name for the containing type of a declaration. + /// + /// Returns a string like "class Foo" or "struct Bar" that identifies the type + /// containing the declaration. Returns nil for top-level declarations. + private func containingTypeName(for decl: Declaration) -> String? { + guard let topLevel = topLevelType(of: decl) else { return nil } + guard let name = topLevel.name else { return nil } + + return "\(topLevel.kind.displayName) \(name)" + } - Even for top-level declarations, private and fileprivate are different: - - private: only accessible within the declaration itself and its extensions in the same file - - fileprivate: accessible from anywhere in the same file - */ + /// Checks if a declaration is referenced from a different type in the same file. + /// Returns true if any same-file reference comes from a different logical type, + /// indicating that fileprivate access is necessary. + /// + /// Even for top-level declarations, private and fileprivate are different: + /// - private: only accessible within the declaration itself and its extensions in the same file + /// - fileprivate: accessible from anywhere in the same file private func isReferencedFromDifferentTypeInSameFile(_ decl: Declaration) -> Bool { let file = decl.location.file let sameFileReferences = graph.references(to: decl).filter { $0.location.file == file } diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index da2d87d35..23bb9c373 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -1,17 +1,15 @@ import Configuration import Shared -/** - Identifies declarations explicitly marked `internal` that are not referenced outside - the file they're defined in. - - Since `internal` is Swift's default access level, declarations that are only used within - their defining file should be marked `private` or `fileprivate` instead. This improves - encapsulation and can help with compilation performance. - - This mutator follows the same pattern as RedundantPublicAccessibilityMarker but checks - for file-scoped usage instead of module-scoped usage. - */ +/// Identifies `internal` declarations (implicit, or explicitly marked) that are not referenced outside +/// the file they're defined in. +/// +/// Since `internal` is Swift's default access level, declarations that are only used within +/// their defining file should be marked `private` or `fileprivate` instead. This improves +/// encapsulation and can help with compilation performance. +/// +/// This mutator follows the same pattern as RedundantPublicAccessibilityMarker but checks +/// for file-scoped usage instead of module-scoped usage. final class RedundantInternalAccessibilityMarker: SourceGraphMutator { private let graph: SourceGraph private let configuration: Configuration @@ -39,29 +37,27 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // MARK: - Private private func validate(_ decl: Declaration) throws { - if decl.accessibility.isExplicitly(.internal) { - if !graph.isRetained(decl) { - let isReferencedOutside = isReferencedOutsideFile(decl) + if decl.accessibility.value == .internal { + if !graph.isRetained(decl), !shouldSkipMarking(decl) { + let isReferencedOutside = decl.isReferencedOutsideFile(graph: graph) if !isReferencedOutside { mark(decl) } } } - /* - Always check descendents, even if parent is not redundant. - - A parent declaration may be used outside its file (making it not redundant), - while still having child declarations that are only used within the same file - (making those children redundant). For example, a class used cross-file may have - an internal property only referenced within the same file - that property should - be flagged as redundant even though the parent class is not. - */ - markExplicitInternalDescendentDeclarations(from: decl) + // Always check descendants, even if parent is not redundant. + // + // A parent declaration may be used outside its file (making it not redundant), + // while still having child declarations that are only used within the same file + // (making those children redundant). For example, a class used cross-file may have + // an internal property only referenced within the same file - that property should + // be flagged as redundant even though the parent class is not. + markInternalDescendentDeclarations(from: decl) } private func validateExtension(_ decl: Declaration) throws { - if decl.accessibility.isExplicitly(.internal) { + if decl.accessibility.value == .internal { if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), graph.redundantInternalAccessibility.keys.contains(extendedDecl) { @@ -72,13 +68,52 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { private func mark(_ decl: Declaration) { guard !graph.isRetained(decl) else { return } - graph.markRedundantInternalAccessibility(decl, file: decl.location.file) + + // Unless explicitly requested, skip marking nested declarations when an ancestor is already marked. + // This avoids redundant warnings since fixing the parent's accessibility fixes the children too. + if !configuration.showNestedRedundantAccessibility, + decl.isAnyAncestorMarked(in: graph.redundantInternalAccessibility) + { + return + } + + // Determine the suggested accessibility level. + // For top-level declarations, fileprivate is equivalent to private, so we pass nil + // to indicate the ambiguity in the output message. + // If the declaration is referenced from different types in the same file, + // it needs fileprivate. Otherwise, private is sufficient. + let isTopLevel = decl.parent == nil + let suggestedAccessibility: Accessibility? = isTopLevel ? nil : (isReferencedFromDifferentTypeInSameFile(decl) ? .fileprivate : .private) + + // Check if the parent's accessibility already constrains this member. + // If the parent is `private`, the member is already effectively `private`. + // If the parent is `fileprivate` and we would suggest `fileprivate`, it's already constrained. + // Marking these would be misleading since changing them would actually increase visibility. + if let maxAccessibility = effectiveMaximumAccessibility(for: decl), + let suggestedAccessibility + { + let accessibilityOrder: [Accessibility] = [.private, .fileprivate, .internal, .public, .open] + let maxIndex = accessibilityOrder.firstIndex(of: maxAccessibility) ?? 0 + let suggestedIndex = accessibilityOrder.firstIndex(of: suggestedAccessibility) ?? 0 + + if suggestedIndex >= maxIndex { + return + } + } + + graph.markRedundantInternalAccessibility(decl, file: decl.location.file, suggestedAccessibility: suggestedAccessibility) } - private func markExplicitInternalDescendentDeclarations(from decl: Declaration) { - for descDecl in descendentInternalDeclarations(from: decl) { - if !graph.isRetained(descDecl) { - let isReferencedOutside = isReferencedOutsideFile(descDecl) + private func markInternalDescendentDeclarations(from decl: Declaration) { + // Sort descendants by their depth to ensure parents are marked before children. + // This is important for the nested redundant accessibility suppression logic. + let descendants = descendentInternalDeclarations(from: decl).sorted { decl1, decl2 in + decl1.ancestorCount < decl2.ancestorCount + } + + for descDecl in descendants { + if !graph.isRetained(descDecl), !shouldSkipMarking(descDecl) { + let isReferencedOutside = descDecl.isReferencedOutsideFile(graph: graph) if !isReferencedOutside { mark(descDecl) } @@ -86,17 +121,215 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { } } - private func isReferencedOutsideFile(_ decl: Declaration) -> Bool { - // Use graph.references(to: decl) to get all references to this declaration - let allReferences = graph.references(to: decl) - let referenceFiles = allReferences.map(\.location.file) + /// Determines if a declaration should be skipped from redundant internal marking. + /// + /// Declarations are skipped if: + /// - They should be skipped from all accessibility analysis (generic type params, implicit decls) + /// - They are protocol requirements (must maintain accessibility for protocol conformance) + /// - They are part of a property wrapper's API (must be accessible to wrapper users) + private func shouldSkipMarking(_ decl: Declaration) -> Bool { + if shouldSkipAccessibilityAnalysis(for: decl) { + return true + } - let result = referenceFiles.contains { $0 != decl.location.file } - return result + if isProtocolRequirement(decl) { + return true + } + + if isPropertyWrapperMember(decl) { + return true + } + + return false } private func descendentInternalDeclarations(from decl: Declaration) -> Set { - let internalDeclarations = decl.declarations.filter { !$0.isImplicit && $0.accessibility.isExplicitly(.internal) } - return internalDeclarations.flatMapSet { descendentInternalDeclarations(from: $0) }.union(internalDeclarations) + decl.descendentDeclarations(matching: { + $0.accessibility.value == .internal + }) + } + + // MARK: - Internal Accessibility Analysis Helpers + + /// Determines if a declaration should be skipped from accessibility analysis entirely. + /// + /// This helper is specific to internal accessibility analysis, checking conditions + /// that make a declaration ineligible for redundant internal marking. + private func shouldSkipAccessibilityAnalysis(for decl: Declaration) -> Bool { + // Generic type parameters must match their container's accessibility. + if decl.kind == .genericTypeParam { return true } + + // Skip implicit (compiler-generated) declarations. + if decl.isImplicit { return true } + + // Deinitializers cannot have explicit access modifiers in Swift. + if decl.kind == .functionDestructor { return true } + + // Override methods must be at least as accessible as what they override. + if decl.isOverride { return true } + + return false + } + + /// Checks if a declaration is a protocol requirement or protocol conformance. + /// + /// Protocol requirements must maintain sufficient accessibility to fulfill the protocol + /// contract, even if only referenced within the same file. This is critical for internal + /// accessibility analysis to avoid marking protocol implementations as redundant. + private func isProtocolRequirement(_ decl: Declaration) -> Bool { + // Case 1: Direct protocol requirement - parent is a protocol. + if let parent = decl.parent, parent.kind == .protocol { + return true + } + + // Case 2: Protocol conformance - this declaration implements a protocol requirement. + // + // When a type conforms to a protocol, Swift's indexer creates "related" references + // from the conforming declaration to the protocol requirement. If this declaration + // has any related references pointing to protocol members with matching names, + // it's implementing a protocol requirement. + let relatedReferences = graph.references(to: decl).filter(\.isRelated) + for ref in relatedReferences { + if let protocolDecl = graph.declaration(withUsr: ref.usr), + protocolDecl.kind.isProtocolMemberKind || protocolDecl.kind == .associatedtype + { + return true + } + } + + // Alternative check: Look for related references FROM this declaration + // to protocol members. The ProtocolConformanceReferenceBuilder inverts + // these relationships, so we might find them either direction. + for ref in decl.related where ref.kind.isProtocolMemberConformingKind { + if let referencedDecl = graph.declaration(withUsr: ref.usr), + let referencedParent = referencedDecl.parent, + referencedParent.kind == .protocol + { + return true + } + } + + return false + } + + /// Checks if a declaration is part of a property wrapper's public API. + /// + /// Property wrappers require certain members to be accessible even for internal + /// accessibility analysis. This prevents marking essential property wrapper components + /// as redundant when they're only referenced within the same file. + private func isPropertyWrapperMember(_ decl: Declaration) -> Bool { + guard let parent = decl.parent else { return false } + + // Check if parent type has @propertyWrapper attribute. + let isPropertyWrapper = parent.attributes.contains { $0.name == "propertyWrapper" } + guard isPropertyWrapper else { return false } + + // Property wrapper initializers are part of the API. + if decl.kind == .functionConstructor { return true } + + // wrappedValue and projectedValue are part of the API. + if decl.kind == .varInstance { + let propertyWrapperSpecialProperties = ["wrappedValue", "projectedValue"] + if propertyWrapperSpecialProperties.contains(decl.name ?? "") { + return true + } + } + + // Typealiases used in method/init signatures must be accessible. + // If this is a typealias and it's referenced by a function/init in the same + // property wrapper type, it's part of the API. + if decl.kind == .typealias { + let siblings = parent.declarations + let hasFunctionReference = siblings.contains { sibling in + sibling.kind.isFunctionKind && sibling.references.contains { ref in + ref.kind == .typealias && decl.usrs.contains(ref.usr) + } + } + if hasFunctionReference { + return true + } + } + + return false + } + + /// Determines the effective maximum accessibility a member can have based on its parent's accessibility. + /// + /// In Swift, a member's effective accessibility is constrained by its parent. This helper + /// ensures internal accessibility analysis respects these constraints when suggesting + /// more restrictive access levels. + private func effectiveMaximumAccessibility(for decl: Declaration) -> Accessibility? { + guard let parent = decl.parent else { return nil } + + let parentAccessibility = parent.accessibility.value + + switch parentAccessibility { + case .private: + return .private + case .fileprivate: + return .fileprivate + case .internal: + return .internal + case .public, .open: + return nil + } + } + + /// Checks if a declaration is referenced from a different type in the same file. + /// + /// For internal accessibility analysis, this determines whether to suggest `fileprivate` + /// versus `private` when a declaration is only used within its file. + private func isReferencedFromDifferentTypeInSameFile(_ decl: Declaration) -> Bool { + let file = decl.location.file + let sameFileReferences = graph.references(to: decl).filter { $0.location.file == file } + + guard let declTopLevel = topLevelType(of: decl) else { + return false + } + + let declLogicalType = logicalType(of: declTopLevel, inFile: file) + + for ref in sameFileReferences { + guard let refParent = ref.parent, + let refTopLevel = topLevelType(of: refParent) + else { + continue + } + + let refLogicalType = logicalType(of: refTopLevel, inFile: file) + + if declLogicalType !== refLogicalType { + return true + } + } + return false + } + + // Finds the top-level type declaration by walking up the parent chain. + private func topLevelType(of decl: Declaration) -> Declaration? { + let baseTypeKinds: Set = [.class, .struct, .enum, .protocol] + let typeKinds = baseTypeKinds.union(Declaration.Kind.extensionKinds) + let ancestors = [decl] + Array(decl.ancestralDeclarations) + return ancestors.last { typeDecl in + guard typeKinds.contains(typeDecl.kind) else { return false } + guard let parent = typeDecl.parent else { return true } + + return !typeKinds.contains(parent.kind) + } + } + + // Gets the logical type for comparison purposes when analyzing internal accessibility. + // For extensions of types in the SAME FILE, treats the extension as the extended type. + // For extensions of types in DIFFERENT FILES, treats the extension as its own distinct type. + private func logicalType(of decl: Declaration, inFile file: SourceFile) -> Declaration? { + if decl.kind.isExtensionKind { + if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), + extendedDecl.location.file == file + { + return extendedDecl + } + return decl + } + return decl } } diff --git a/Sources/SourceGraph/SourceGraph.swift b/Sources/SourceGraph/SourceGraph.swift index 485fccf25..350b5c94c 100644 --- a/Sources/SourceGraph/SourceGraph.swift +++ b/Sources/SourceGraph/SourceGraph.swift @@ -9,8 +9,8 @@ public final class SourceGraph { public private(set) var redundantProtocols: [Declaration: (references: Set, inherited: Set)] = [:] public private(set) var rootDeclarations: Set = [] public private(set) var redundantPublicAccessibility: [Declaration: Set] = [:] - public private(set) var redundantInternalAccessibility: [Declaration: Set] = [:] - public private(set) var redundantFilePrivateAccessibility: [Declaration: Set] = [:] + public private(set) var redundantInternalAccessibility: [Declaration: (files: Set, suggestedAccessibility: Accessibility?)] = [:] + public private(set) var redundantFilePrivateAccessibility: [Declaration: (files: Set, containingTypeName: String?)] = [:] public private(set) var rootReferences: Set = [] public private(set) var allReferences: Set = [] public private(set) var retainedDeclarations: Set = [] @@ -87,12 +87,23 @@ public final class SourceGraph { _ = redundantPublicAccessibility.removeValue(forKey: declaration) } - func markRedundantInternalAccessibility(_ declaration: Declaration, file: SourceFile) { - redundantInternalAccessibility[declaration, default: []].insert(file) + func markRedundantInternalAccessibility(_ declaration: Declaration, file: SourceFile, suggestedAccessibility: Accessibility?) { + if let existing = redundantInternalAccessibility[declaration] { + var files = existing.files + files.insert(file) + redundantInternalAccessibility[declaration] = (files: files, suggestedAccessibility: existing.suggestedAccessibility) + } else { + redundantInternalAccessibility[declaration] = (files: [file], suggestedAccessibility: suggestedAccessibility) + } } - func markRedundantFilePrivateAccessibility(_ declaration: Declaration, file: SourceFile) { - redundantFilePrivateAccessibility[declaration, default: []].insert(file) + func markRedundantFilePrivateAccessibility(_ declaration: Declaration, file: SourceFile, containingTypeName: String?) { + if var existing = redundantFilePrivateAccessibility[declaration] { + existing.files.insert(file) + redundantFilePrivateAccessibility[declaration] = existing + } else { + redundantFilePrivateAccessibility[declaration] = (files: [file], containingTypeName: containingTypeName) + } } func markIgnored(_ declaration: Declaration) { diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index dfc0d83c1..6b6e194c6 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -81,3 +81,12 @@ inlinableFunction() // Associated types _ = PublicInheritedAssociatedTypeClass().items _ = PublicInheritedAssociatedTypeDefaultTypeClass().items + +// Redundant accessibility tests +TrulyRedundantFilePrivateMembersRetainer().retain() +ProtocolRequirementAccessibilityRetainer().retain() +PropertyWrapperAccessibilityRetainer().retain() +NestedTypeAccessibilityRetainer().retain() +InternalSuggestingPrivateVsFileprivateRetainer().retain() +ImplicitlyInternalRetainer().retain() +NotRedundantInternalClassComponents_Support().useImplicitlyInternalStruct() diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalSuggestingPrivateVsFileprivate.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalSuggestingPrivateVsFileprivate.swift new file mode 100644 index 000000000..5e01a29d2 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalSuggestingPrivateVsFileprivate.swift @@ -0,0 +1,46 @@ +/* + InternalSuggestingPrivateVsFileprivate.swift + Tests that internal declarations correctly suggest private vs fileprivate + based on whether they're accessed from different types in the same file. +*/ + +// Class with internal property only used within its own type - should suggest private. +public class ClassWithInternalPropertySuggestingPrivate { + // Should be flagged as redundant internal (can be 'private'). + internal var usedOnlyInOwnType: Int = 0 + + public init() {} + + public func useProperty() { + _ = usedOnlyInOwnType + } +} + +// Two classes where one accesses the other's internal property - should suggest fileprivate. +public class ClassA { + // Should be flagged as redundant internal (can be 'fileprivate'). + internal var sharedWithClassB: String = "" + + public init() {} +} + +public class ClassB { + public init() {} + + public func accessClassA(_ a: ClassA) { + _ = a.sharedWithClassB + } +} + +// Used by main.swift to ensure these are referenced. +public class InternalSuggestingPrivateVsFileprivateRetainer { + public init() {} + public func retain() { + let obj1 = ClassWithInternalPropertySuggestingPrivate() + obj1.useProperty() + + let a = ClassA() + let b = ClassB() + b.accessClassA(a) + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NestedTypeAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NestedTypeAccessibility.swift new file mode 100644 index 000000000..a5e185759 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NestedTypeAccessibility.swift @@ -0,0 +1,43 @@ +/* + NestedTypeAccessibility.swift + Tests nested type accessibility analysis and redundancy suppression. +*/ + +/* Public class with internal nested types that are only used within the file. */ +public class OuterClassWithNestedTypes { + /* Should be flagged as redundant (not used outside file). */ + internal struct NestedStruct { + /* Should NOT be flagged if parent is already flagged (nested redundancy suppression). */ + internal var nestedProperty: Int = 0 + + /* Should NOT be flagged if parent is already flagged (nested redundancy suppression). */ + internal func nestedMethod() {} + } + + /* Should be flagged as redundant (not used outside file). */ + internal class NestedClass { + /* Should NOT be flagged if parent is already flagged (nested redundancy suppression). */ + internal var anotherProperty: String = "" + } + + internal var nested: NestedStruct = .init() + + public init() {} + + public func useNested() { + _ = nested.nestedProperty + nested.nestedMethod() + + let nc = NestedClass() + _ = nc.anotherProperty + } +} + +/* Used by main.swift to ensure these are referenced. */ +public class NestedTypeAccessibilityRetainer { + public init() {} + public func retain() { + let obj = OuterClassWithNestedTypes() + obj.useNested() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift index c4f852e72..5a6d60677 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents.swift @@ -19,4 +19,9 @@ internal enum NotRedundantInternalEnum { func useCase() -> NotRedundantInternalEnum { return .usedCase } +} + +// Test case for implicitly internal declaration used from another file - should NOT be flagged. +struct ImplicitlyInternalStructUsedFromAnotherFile { + var implicitlyInternalProperty: Int = 42 } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift index 055f1e21f..87e672250 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/NotRedundantInternalClassComponents_Support.swift @@ -5,12 +5,17 @@ internal class NotRedundantInternalClass_Support { internal func helper() {} } -// Used by main.swift to ensure these are referenced -class NotRedundantInternalClassComponents_Support { +// Used by main.swift to ensure these are referenced. +public class NotRedundantInternalClassComponents_Support { public init() {} - - func useInternalMethod() { + + public func useInternalMethod() { let cls = NotRedundantInternalClass() cls.usedInternalMethod() } + + public func useImplicitlyInternalStruct() { + let s = ImplicitlyInternalStructUsedFromAnotherFile() + _ = s.implicitlyInternalProperty + } } \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PropertyWrapperAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PropertyWrapperAccessibility.swift new file mode 100644 index 000000000..add913064 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/PropertyWrapperAccessibility.swift @@ -0,0 +1,49 @@ +/* + PropertyWrapperAccessibility.swift + Tests that property wrapper members are NOT flagged as redundant. +*/ + +/* Property wrapper with internal accessibility. */ +@propertyWrapper +internal struct InternalPropertyWrapper { + private var value: T + + /* Should NOT be flagged - wrappedValue is part of property wrapper API. */ + internal var wrappedValue: T { + get { value } + set { value = newValue } + } + + /* Should NOT be flagged - projectedValue is part of property wrapper API. */ + internal var projectedValue: InternalPropertyWrapper { + self + } + + /* Should NOT be flagged - init is part of property wrapper API. */ + internal init(wrappedValue: T) { + value = wrappedValue + } + + /* This typealias is used in the init signature, so it's part of the API. */ + internal typealias Value = T +} + +/* Class using the property wrapper. */ +internal class ClassUsingPropertyWrapper { + @InternalPropertyWrapper + var wrappedProperty: String = "test" + + func access() { + _ = wrappedProperty + _ = $wrappedProperty + } +} + +/* Used by main.swift to ensure these are referenced. */ +public class PropertyWrapperAccessibilityRetainer { + public init() {} + public func retain() { + let obj = ClassUsingPropertyWrapper() + obj.access() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ProtocolRequirementAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ProtocolRequirementAccessibility.swift new file mode 100644 index 000000000..20e214bc4 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ProtocolRequirementAccessibility.swift @@ -0,0 +1,41 @@ +/* + ProtocolRequirementAccessibility.swift + Tests that protocol requirements and conformances are NOT flagged as redundant. +*/ + +/* Internal protocol with internal requirements. */ +internal protocol InternalProtocolWithRequirements { + func requiredMethod() + var requiredProperty: Int { get } +} + +/* Class conforming to the internal protocol. */ +internal class ConformingToInternalProtocol: InternalProtocolWithRequirements { + /* Should NOT be flagged - this implements a protocol requirement. */ + func requiredMethod() {} + + /* Should NOT be flagged - this implements a protocol requirement. */ + var requiredProperty: Int { 42 } +} + +/* Protocol with fileprivate requirement (within same file). */ +fileprivate protocol FilePrivateProtocol { + func filePrivateRequirement() +} + +/* Extension conforming to fileprivate protocol. */ +extension ConformingToInternalProtocol: FilePrivateProtocol { + /* Should NOT be flagged - implements protocol requirement. */ + func filePrivateRequirement() {} +} + +/* Used by main.swift to ensure these are referenced. */ +public class ProtocolRequirementAccessibilityRetainer { + public init() {} + public func retain() { + let obj = ConformingToInternalProtocol() + obj.requiredMethod() + _ = obj.requiredProperty + obj.filePrivateRequirement() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift index 74d1cdbbf..e45953e9d 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/RedundantInternalComponents.swift @@ -13,7 +13,70 @@ internal enum RedundantInternalEnum { case unusedCase } -// Used by main.swift to ensure these are referenced +// Test case for implicitly internal declaration that is redundant. +class ImplicitlyInternalClassUsedOnlyInSameFile { + var implicitlyInternalProperty: String = "test" + + func useProperty() { + _ = implicitlyInternalProperty + } +} + +// Retainer to ensure ImplicitlyInternalClassUsedOnlyInSameFile is used in the same file. +public class ImplicitlyInternalRetainer { + public init() {} + + public func retain() { + let obj = ImplicitlyInternalClassUsedOnlyInSameFile() + obj.useProperty() + _ = obj.implicitlyInternalProperty + } +} + +/* + Test case for members of private/fileprivate containers. + Members of a private class are already effectively private, + so marking them as redundant internal would be misleading. + */ +private class PrivateContainerClass { + // This should NOT be flagged - already constrained by parent's private accessibility. + func method() {} + + // This should NOT be flagged - already constrained by parent's private accessibility. + var property: Int = 0 +} + +fileprivate class FileprivateContainerClass { + // This should NOT be flagged - already constrained by parent's fileprivate accessibility. + func method() {} + + // This should NOT be flagged - already constrained by parent's fileprivate accessibility. + var property: Int = 0 +} + +/* + Test case for deinit - should not be flagged. + Deinitializers cannot have explicit access modifiers in Swift. + */ +class ClassWithDeinit { + // This should NOT be flagged - deinit cannot have access modifiers. + deinit {} +} + +/* + Test case for override methods - should not be flagged. + Override methods must be at least as accessible as what they override. + */ +class BaseClass { + func overridableMethod() {} +} + +class DerivedClass: BaseClass { + // This should NOT be flagged - override methods have accessibility constraints. + override func overridableMethod() {} +} + +// Used by main.swift to ensure these are referenced. class RedundantInternalClassComponents { public init() {} } \ No newline at end of file diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TrulyRedundantFilePrivateMembers.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TrulyRedundantFilePrivateMembers.swift new file mode 100644 index 000000000..7c98a6e82 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TrulyRedundantFilePrivateMembers.swift @@ -0,0 +1,38 @@ +/* + TrulyRedundantFilePrivateMembers.swift + Tests cases where fileprivate is truly redundant and should be private. +*/ + +/* This class has a fileprivate method that is only used within the class itself. */ +class ClassWithRedundantFilePrivateMethod { + /* Should be flagged as redundant - only used within the same type (can be private). */ + fileprivate func helper() -> Int { + 42 + } + + func publicMethod() -> Int { + helper() + } +} + +/* This struct has a fileprivate property only accessed within its own type. */ +struct StructWithRedundantFilePrivateProperty { + /* Should be flagged as redundant - only used within the same type (can be private). */ + fileprivate var internalState: String = "" + + func access() -> String { + internalState + } +} + +/* Used by main.swift to ensure these are referenced. */ +public class TrulyRedundantFilePrivateMembersRetainer { + public init() {} + public func retain() { + let obj = ClassWithRedundantFilePrivateMethod() + _ = obj.publicMethod() + + let s = StructWithRedundantFilePrivateProperty() + _ = s.access() + } +} diff --git a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift index 936c7e5d6..272238585 100644 --- a/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantFilePrivateAccessibilityTest.swift @@ -8,18 +8,16 @@ final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { build(projectPath: AccessibilityProjectPath) } - /* - Tests that a fileprivate class is NOT flagged as redundant when accessed - from a different type in the same file. - - In Swift, private and fileprivate have distinct meanings even for top-level declarations: - - private: accessible only within the lexical scope and its extensions in the same file - - fileprivate: accessible from anywhere in the same file - - Since RedundantFilePrivateClass is accessed from RedundantFilePrivateClassRetainer - (a different type), fileprivate is the minimum access level required. Changing it to - private would prevent RedundantFilePrivateClassRetainer from accessing it. - */ + // Tests that a fileprivate class is NOT flagged as redundant when accessed + // from a different type in the same file. + // + // In Swift, private and fileprivate have distinct meanings even for top-level declarations: + // - private: accessible only within the lexical scope and its extensions in the same file + // - fileprivate: accessible from anywhere in the same file + // + // Since RedundantFilePrivateClass is accessed from RedundantFilePrivateClassRetainer + // (a different type), fileprivate is the minimum access level required. Changing it to + // private would prevent RedundantFilePrivateClassRetainer from accessing it. func testRedundantFilePrivateClass() { index() assertNotRedundantFilePrivateAccessibility(.class("RedundantFilePrivateClass")) @@ -30,22 +28,31 @@ final class RedundantFilePrivateAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantFilePrivateAccessibility(.class("NotRedundantFilePrivateClass")) } + // Tests that a fileprivate property inside a private class is NOT flagged as redundant. + // This is valid because fileprivate expands the property's visibility beyond the private + // container, making it accessible to other types in the same file. The private class + // restricts access to the file, but fileprivate allows the property to be more visible + // than its container within that scope. func testNotRedundantFilePrivatePropertyInPrivateClass() { index() assertNotRedundantFilePrivateAccessibility(.varInstance("filePrivatePaths")) } - /* - NOTE: the opposite of the above, e.g. a function called "assertRedundantFilePrivateAccessibility()", - is intentionally not tested here. - - After fixing the bug where cross-type same-file references were incorrectly - flagged as redundant, there are no meaningful test cases for truly redundant - fileprivate declarations. Here's why: + func testTrulyRedundantFilePrivateMethod() { + // A fileprivate method only used within its own type should be private. + index() + assertRedundantFilePrivateAccessibility( + .functionMethodInstance("helper()", line: 9), + containingTypeName: "class ClassWithRedundantFilePrivateMethod" + ) + } - - Fileprivate exists specifically for same-file cross-type access - - A fileprivate declaration used only within its own type should be private - - A fileprivate declaration with no references at all is marked as "unused", not - "redundant fileprivate" - */ + func testTrulyRedundantFilePrivateProperty() { + // A fileprivate property only used within its own type should be private. + index() + assertRedundantFilePrivateAccessibility( + .varInstance("internalState"), + containingTypeName: "struct StructWithRedundantFilePrivateProperty" + ) + } } diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 3f91e497d..106e8595b 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -39,12 +39,141 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { } func testInternalMethodUsedInExtension() { - // This should NOT be flagged as redundant - // Tests the case where an internal method is used in an extension + // This should NOT be flagged as redundant. index() - // This test would need additional setup with methods in extensions - // For now, we'll test that the existing NotRedundantInternalClassComponents work assertNotRedundantInternalAccessibility(.functionMethodInstance("usedInternalMethod()")) } + + /// Tests that members of a private class are not flagged as redundant internal. + /// + /// In Swift, members of a private class are already effectively private due to + /// the parent's accessibility constraint. Suggesting to change them to fileprivate + /// would actually increase their visibility, which is incorrect. + func testMembersOfPrivateClassNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.functionMethodInstance("method()", line: 43)) + assertNotRedundantInternalAccessibility(.varInstance("property", line: 46)) + } + + /// Tests that members of a fileprivate class are not flagged as redundant internal + /// when they would be suggested to be fileprivate. + /// + /// In Swift, members of a fileprivate class are already constrained to fileprivate + /// accessibility. Suggesting to mark them as fileprivate would be redundant. + func testMembersOfFileprivateClassNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.functionMethodInstance("method()", line: 51)) + assertNotRedundantInternalAccessibility(.varInstance("property", line: 54)) + } + + /// Tests that deinit is not flagged as redundant internal. + /// + /// In Swift, deinitializers cannot have explicit access modifiers. + /// They always match the accessibility of their enclosing type. + func testDeinitNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.functionDestructor("deinit", line: 63)) + } + + /// Tests that override methods are not flagged as redundant internal. + /// + /// Override methods must be at least as accessible as the method they override, + /// so their accessibility is constrained by the base method. + func testOverrideMethodNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.functionMethodInstance("overridableMethod()", line: 76)) + } + + /// Tests that protocol requirements are not flagged as redundant internal. + /// + /// Methods and properties that implement protocol requirements must maintain + /// sufficient accessibility to fulfill the protocol contract. + func testProtocolRequirementsNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.functionMethodInstance("requiredMethod()")) + assertNotRedundantInternalAccessibility(.varInstance("requiredProperty")) + } + + /// Tests that fileprivate protocol conformances are not flagged. + func testFilePrivateProtocolConformanceNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.functionMethodInstance("filePrivateRequirement()")) + } + + /// Tests that property wrapper members are not flagged as redundant internal. + /// + /// Property wrappers require certain members (init, wrappedValue, projectedValue) + /// to be accessible as part of their API contract. + func testPropertyWrapperMembersNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.varInstance("wrappedValue")) + assertNotRedundantInternalAccessibility(.varInstance("projectedValue")) + assertNotRedundantInternalAccessibility(.functionConstructor("init(wrappedValue:)")) + } + + /// Tests internal suggesting private. + /// + /// An internal property only used within its own type should be private. + func testInternalSuggestingPrivate() { + index() + + assertRedundantInternalAccessibility( + .varInstance("usedOnlyInOwnType"), + suggestedAccessibility: .private + ) + } + + /// Tests internal suggesting fileprivate. + /// + /// An internal property used from a different type in the same file should be fileprivate. + func testInternalSuggestingFileprivate() { + index() + + assertRedundantInternalAccessibility( + .varInstance("sharedWithClassB"), + suggestedAccessibility: .fileprivate + ) + } + + /// Tests nested type redundancy with parent already flagged. + /// + /// When nested types are flagged as redundant, their nested members should be + /// suppressed to avoid noise (unless showNestedRedundantAccessibility is enabled). + func testNestedTypeRedundancy() { + index() + + assertRedundantInternalAccessibility(.struct("NestedStruct")) + assertRedundantInternalAccessibility(.class("NestedClass")) + assertRedundantInternalAccessibility(.varInstance("nested")) + } + + /// Tests that implicitly internal declarations (no access modifier) are flagged as redundant + /// when only used within the same file. + /// + /// This ensures the analyzer handles both explicit 'internal' keyword and implicit internal + /// (default accessibility) correctly in positive cases. + func testImplicitlyInternalRedundant() { + index() + + assertRedundantInternalAccessibility(.class("ImplicitlyInternalClassUsedOnlyInSameFile")) + } + + /// Tests that implicitly internal declarations (no access modifier) are NOT flagged as redundant + /// when used from another file. + /// + /// This ensures the analyzer handles both explicit 'internal' keyword and implicit internal + /// (default accessibility) correctly in negative cases. + func testImplicitlyInternalNotRedundant() { + index() + + assertNotRedundantInternalAccessibility(.struct("ImplicitlyInternalStructUsedFromAnotherFile")) + } } diff --git a/Tests/Shared/SourceGraphTestCase.swift b/Tests/Shared/SourceGraphTestCase.swift index fb03811b8..2a8b1e79e 100644 --- a/Tests/Shared/SourceGraphTestCase.swift +++ b/Tests/Shared/SourceGraphTestCase.swift @@ -177,13 +177,22 @@ open class SourceGraphTestCase: XCTestCase { scopeStack.removeLast() } - func assertRedundantInternalAccessibility(_ description: DeclarationDescription, scopedAssertions: (() -> Void)? = nil, file: StaticString = #file, line: UInt = #line) { + func assertRedundantInternalAccessibility(_ description: DeclarationDescription, suggestedAccessibility: Accessibility? = nil, 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.results.redundantInternalAccessibilityDeclarations.contains(declaration) { XCTFail("Expected declaration to have redundant internal accessibility: \(declaration)", file: file, line: line) } + if let suggestedAccessibility { + if let info = Self.graph.redundantInternalAccessibility[declaration] { + if info.suggestedAccessibility != suggestedAccessibility { + let actualText = info.suggestedAccessibility?.rawValue ?? "nil" + XCTFail("Expected suggested accessibility to be '\(suggestedAccessibility.rawValue)', but got '\(actualText)': \(declaration)", file: file, line: line) + } + } + } + scopeStack.append(.declaration(declaration)) scopedAssertions?() scopeStack.removeLast() @@ -201,6 +210,26 @@ open class SourceGraphTestCase: XCTestCase { scopeStack.removeLast() } + func assertRedundantFilePrivateAccessibility(_ description: DeclarationDescription, containingTypeName: String? = nil, 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.results.redundantFilePrivateAccessibilityDeclarations.contains(declaration) { + XCTFail("Expected declaration to have redundant fileprivate accessibility: \(declaration)", file: file, line: line) + } + + if let containingTypeName { + if let info = Self.graph.redundantFilePrivateAccessibility[declaration] { + if info.containingTypeName != containingTypeName { + XCTFail("Expected containing type name to be '\(containingTypeName)', but got '\(info.containingTypeName ?? "nil")': \(declaration)", file: file, line: line) + } + } + } + + scopeStack.append(.declaration(declaration)) + scopedAssertions?() + scopeStack.removeLast() + } + func assertNotRedundantFilePrivateAccessibility(_ 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 } From 35357b8ef4e1951ee959ebbf3fb7ec68bf02ec4b Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Wed, 7 Jan 2026 12:06:09 -0800 Subject: [PATCH 18/30] Fix accessibility warnings throughout the code base so that `mise r scan` results in no warnings --- Sources/Configuration/Configuration.swift | 4 +- Sources/Frontend/Commands/ScanCommand.swift | 102 +++++++++--------- Sources/Frontend/Project.swift | 2 +- Sources/Frontend/main.swift | 4 +- Sources/Indexer/InfoPlistIndexer.swift | 2 +- Sources/Indexer/XCDataModelIndexer.swift | 2 +- Sources/Indexer/XibIndexer.swift | 2 +- Sources/Logger/Logger.swift | 20 ++-- .../ProjectDrivers/GenericProjectDriver.swift | 2 +- Sources/ProjectDrivers/SPMProjectDriver.swift | 2 +- .../ProjectDrivers/XcodeProjectDriver.swift | 2 +- Sources/Shared/JobPool.swift | 2 +- Sources/Shared/SetupGuide.swift | 2 +- Sources/Shared/Shell.swift | 2 +- Sources/Shared/SwiftVersion.swift | 2 +- .../Elements/DeclarationAttribute.swift | 2 +- Sources/SourceGraph/Elements/Reference.swift | 2 +- .../Mutators/EnumCaseReferenceBuilder.swift | 2 +- Sources/SourceGraph/SourceGraph.swift | 2 +- Sources/SyntaxAnalysis/CommentCommand.swift | 2 +- .../DeclarationSyntaxVisitor.swift | 2 +- .../MultiplexingSyntaxVisitor.swift | 4 +- .../SyntaxAnalysis/TypeSyntaxInspector.swift | 2 +- .../UnusedParameterParser.swift | 2 +- Sources/XcodeSupport/XcodeTarget.swift | 2 +- 25 files changed, 87 insertions(+), 87 deletions(-) diff --git a/Sources/Configuration/Configuration.swift b/Sources/Configuration/Configuration.swift index f05b208aa..85cca0573 100644 --- a/Sources/Configuration/Configuration.swift +++ b/Sources/Configuration/Configuration.swift @@ -218,7 +218,7 @@ public final class Configuration { // MARK: - Private - lazy var settings: [any AbstractSetting] = [ + private lazy var settings: [any AbstractSetting] = [ $project, $schemes, $excludeTargets, $excludeTests, $indexExclude, $reportExclude, $reportInclude, $outputFormat, $retainPublic, $noRetainSPI, $retainFiles, $retainAssignOnlyProperties, $retainAssignOnlyPropertyTypes, $retainObjcAccessible, $retainObjcAnnotated, $retainUnusedProtocolFuncParams, $retainSwiftUIPreviews, $disableRedundantPublicAnalysis, @@ -249,7 +249,7 @@ public final class Configuration { } } -protocol AbstractSetting { +private protocol AbstractSetting { associatedtype Value var key: String { get } diff --git a/Sources/Frontend/Commands/ScanCommand.swift b/Sources/Frontend/Commands/ScanCommand.swift index 3314b897b..73a2080df 100644 --- a/Sources/Frontend/Commands/ScanCommand.swift +++ b/Sources/Frontend/Commands/ScanCommand.swift @@ -13,157 +13,157 @@ struct ScanCommand: ParsableCommand { ) @Argument(help: "Arguments following '--' will be passed to the underlying build tool, which is either 'swift build' or 'xcodebuild' depending on your project") - var buildArguments: [String] = defaultConfiguration.$buildArguments.defaultValue + private var buildArguments: [String] = defaultConfiguration.$buildArguments.defaultValue @Flag(help: "Enable guided setup") - var setup: Bool = defaultConfiguration.guidedSetup + private var setup: Bool = defaultConfiguration.guidedSetup @Option(help: "Path to the root directory of your project") - var projectRoot: FilePath = projectRootDefault + private var projectRoot: FilePath = projectRootDefault @Option(help: "Path to configuration file. By default Periphery will look for .periphery.yml in the current directory") - var config: FilePath? + private var config: FilePath? @Option(help: "Path to your project's .xcodeproj or .xcworkspace") - var project: FilePath? + private var project: FilePath? @Option(parsing: .upToNextOption, help: "Schemes to build. All targets built by these schemes will be scanned") - var schemes: [String] = defaultConfiguration.$schemes.defaultValue + private var schemes: [String] = defaultConfiguration.$schemes.defaultValue @Option(help: "Output format") - var format: OutputFormat = defaultConfiguration.$outputFormat.defaultValue + private var format: OutputFormat = defaultConfiguration.$outputFormat.defaultValue @Flag(help: "Exclude test targets from indexing") - var excludeTests: Bool = defaultConfiguration.$excludeTests.defaultValue + private var excludeTests: Bool = defaultConfiguration.$excludeTests.defaultValue @Option(parsing: .upToNextOption, help: "Targets to exclude from indexing") - var excludeTargets: [String] = defaultConfiguration.$excludeTargets.defaultValue + private var excludeTargets: [String] = defaultConfiguration.$excludeTargets.defaultValue @Option(parsing: .upToNextOption, help: "Source file globs to exclude from indexing") - var indexExclude: [String] = defaultConfiguration.$indexExclude.defaultValue + private var indexExclude: [String] = defaultConfiguration.$indexExclude.defaultValue @Option(parsing: .upToNextOption, help: "Source file globs to exclude from the results. Note that this option is purely cosmetic, these files will still be indexed") - var reportExclude: [String] = defaultConfiguration.$reportExclude.defaultValue + private var reportExclude: [String] = defaultConfiguration.$reportExclude.defaultValue @Option(parsing: .upToNextOption, help: "Source file globs to include in the results. This option supersedes '--report-exclude'. Note that this option is purely cosmetic, these files will still be indexed") - var reportInclude: [String] = defaultConfiguration.$reportInclude.defaultValue + private var reportInclude: [String] = defaultConfiguration.$reportInclude.defaultValue @Option(parsing: .upToNextOption, help: "Source file globs for which all containing declarations will be retained") - var retainFiles: [String] = defaultConfiguration.$retainFiles.defaultValue + private var retainFiles: [String] = defaultConfiguration.$retainFiles.defaultValue @Option(parsing: .upToNextOption, help: "Index store paths. Implies '--skip-build'") - var indexStorePath: [FilePath] = defaultConfiguration.$indexStorePath.defaultValue + private var indexStorePath: [FilePath] = defaultConfiguration.$indexStorePath.defaultValue @Flag(help: "Retain all public declarations, recommended for framework/library projects") - var retainPublic: Bool = defaultConfiguration.$retainPublic.defaultValue + private var retainPublic: Bool = defaultConfiguration.$retainPublic.defaultValue @Option(parsing: .upToNextOption, help: "Public SPIs (System Programming Interfaces) to check for unused code even when '--retain-public' is enabled") - var noRetainSPI: [String] = defaultConfiguration.$noRetainSPI.defaultValue + private var noRetainSPI: [String] = defaultConfiguration.$noRetainSPI.defaultValue @Flag(help: "Disable identification of redundant public accessibility") - var disableRedundantPublicAnalysis: Bool = defaultConfiguration.$disableRedundantPublicAnalysis.defaultValue + private var disableRedundantPublicAnalysis: Bool = defaultConfiguration.$disableRedundantPublicAnalysis.defaultValue @Flag(help: "Disable identification of redundant internal accessibility") - var disableRedundantInternalAnalysis: Bool = defaultConfiguration.$disableRedundantInternalAnalysis.defaultValue + private var disableRedundantInternalAnalysis: Bool = defaultConfiguration.$disableRedundantInternalAnalysis.defaultValue @Flag(help: "Disable identification of redundant fileprivate accessibility") - var disableRedundantFilePrivateAnalysis: Bool = defaultConfiguration.$disableRedundantFilePrivateAnalysis.defaultValue + private var disableRedundantFilePrivateAnalysis: Bool = defaultConfiguration.$disableRedundantFilePrivateAnalysis.defaultValue @Flag(help: "Show redundant internal/fileprivate accessibility warnings for nested declarations even when the containing type is already flagged") - var showNestedRedundantAccessibility: Bool = defaultConfiguration.$showNestedRedundantAccessibility.defaultValue + private var showNestedRedundantAccessibility: Bool = defaultConfiguration.$showNestedRedundantAccessibility.defaultValue @Flag(help: "Disable identification of unused imports") - var disableUnusedImportAnalysis: Bool = defaultConfiguration.$disableUnusedImportAnalysis.defaultValue + private var disableUnusedImportAnalysis: Bool = defaultConfiguration.$disableUnusedImportAnalysis.defaultValue @Option(parsing: .upToNextOption, help: "Names of unused imported modules to retain") - var retainUnusedImportedModules: [String] = defaultConfiguration.$retainUnusedImportedModules.defaultValue + private var retainUnusedImportedModules: [String] = defaultConfiguration.$retainUnusedImportedModules.defaultValue @Flag(help: "Retain properties that are assigned, but never used") - var retainAssignOnlyProperties: Bool = defaultConfiguration.$retainAssignOnlyProperties.defaultValue + private var retainAssignOnlyProperties: Bool = defaultConfiguration.$retainAssignOnlyProperties.defaultValue @Option(parsing: .upToNextOption, help: "Property types to retain if the property is assigned, but never read") - var retainAssignOnlyPropertyTypes: [String] = defaultConfiguration.$retainAssignOnlyPropertyTypes.defaultValue + private var retainAssignOnlyPropertyTypes: [String] = defaultConfiguration.$retainAssignOnlyPropertyTypes.defaultValue @Option(parsing: .upToNextOption, help: "Names of external protocols that inherit Encodable. Properties and CodingKey enums of types conforming to these protocols will be retained") - var externalEncodableProtocols: [String] = defaultConfiguration.$externalEncodableProtocols.defaultValue + private var externalEncodableProtocols: [String] = defaultConfiguration.$externalEncodableProtocols.defaultValue @Option(parsing: .upToNextOption, help: "Names of external protocols that inherit Codable. Properties and CodingKey enums of types conforming to these protocols will be retained") - var externalCodableProtocols: [String] = defaultConfiguration.$externalCodableProtocols.defaultValue + private var externalCodableProtocols: [String] = defaultConfiguration.$externalCodableProtocols.defaultValue @Option(parsing: .upToNextOption, help: "Names of XCTestCase subclasses that reside in external targets") - var externalTestCaseClasses: [String] = defaultConfiguration.$externalTestCaseClasses.defaultValue + private var externalTestCaseClasses: [String] = defaultConfiguration.$externalTestCaseClasses.defaultValue @Flag(help: "Retain declarations that are exposed to Objective-C implicitly by inheriting NSObject classes, or explicitly with the @objc and @objcMembers attributes") - var retainObjcAccessible: Bool = defaultConfiguration.$retainObjcAccessible.defaultValue + private var retainObjcAccessible: Bool = defaultConfiguration.$retainObjcAccessible.defaultValue @Flag(help: "Retain declarations that are exposed to Objective-C explicitly with the @objc and @objcMembers attributes") - var retainObjcAnnotated: Bool = defaultConfiguration.$retainObjcAnnotated.defaultValue + private var retainObjcAnnotated: Bool = defaultConfiguration.$retainObjcAnnotated.defaultValue @Flag(help: "Retain unused protocol function parameters, even if the parameter is unused in all conforming functions") - var retainUnusedProtocolFuncParams: Bool = defaultConfiguration.$retainUnusedProtocolFuncParams.defaultValue + private var retainUnusedProtocolFuncParams: Bool = defaultConfiguration.$retainUnusedProtocolFuncParams.defaultValue @Flag(help: "Retain SwiftUI previews") - var retainSwiftUIPreviews: Bool = defaultConfiguration.$retainSwiftUIPreviews.defaultValue + private var retainSwiftUIPreviews: Bool = defaultConfiguration.$retainSwiftUIPreviews.defaultValue @Flag(help: "Retain properties on Codable types (including Encodable and Decodable)") - var retainCodableProperties: Bool = defaultConfiguration.$retainCodableProperties.defaultValue + private var retainCodableProperties: Bool = defaultConfiguration.$retainCodableProperties.defaultValue @Flag(help: "Retain properties on Encodable types only") - var retainEncodableProperties: Bool = defaultConfiguration.$retainEncodableProperties.defaultValue + private var retainEncodableProperties: Bool = defaultConfiguration.$retainEncodableProperties.defaultValue @Flag(help: "Clean existing build artifacts before building") - var cleanBuild: Bool = defaultConfiguration.$cleanBuild.defaultValue + private var cleanBuild: Bool = defaultConfiguration.$cleanBuild.defaultValue @Flag(help: "Skip the project build step") - var skipBuild: Bool = defaultConfiguration.$skipBuild.defaultValue + private var skipBuild: Bool = defaultConfiguration.$skipBuild.defaultValue @Flag(help: "Skip schemes validation") - var skipSchemesValidation: Bool = defaultConfiguration.$skipSchemesValidation.defaultValue + private var skipSchemesValidation: Bool = defaultConfiguration.$skipSchemesValidation.defaultValue @Flag(help: "Output result paths relative to the current directory") - var relativeResults: Bool = defaultConfiguration.$relativeResults.defaultValue + private var relativeResults: Bool = defaultConfiguration.$relativeResults.defaultValue @Flag(help: "Exit with non-zero status if any unused code is found") - var strict: Bool = defaultConfiguration.$strict.defaultValue + private var strict: Bool = defaultConfiguration.$strict.defaultValue @Flag(help: "Disable checking for updates") - var disableUpdateCheck: Bool = defaultConfiguration.$disableUpdateCheck.defaultValue + private var disableUpdateCheck: Bool = defaultConfiguration.$disableUpdateCheck.defaultValue @Flag(help: "Enable verbose logging") - var verbose: Bool = defaultConfiguration.$verbose.defaultValue + private var verbose: Bool = defaultConfiguration.$verbose.defaultValue @Flag(help: "Only output results") - var quiet: Bool = defaultConfiguration.$quiet.defaultValue + private var quiet: Bool = defaultConfiguration.$quiet.defaultValue @Option(help: "Colored output mode") - var color: ColorOption = defaultConfiguration.$color.defaultValue + private var color: ColorOption = defaultConfiguration.$color.defaultValue @Flag(name: .customLong("no-color"), help: .hidden) - var noColor: Bool = false + private var noColor: Bool = false @Option(help: "JSON package manifest path (obtained using `swift package describe --type json` or manually)") - var jsonPackageManifestPath: FilePath? + private var jsonPackageManifestPath: FilePath? @Option(help: "Baseline file path used to filter results") - var baseline: FilePath? + private var baseline: FilePath? @Option(help: "Baseline file path where results are written. Pass the same path to '--baseline' in subsequent scans to exclude the results recorded in the baseline.") - var writeBaseline: FilePath? + private var writeBaseline: FilePath? @Option(help: "File path where formatted results are written.") - var writeResults: FilePath? + private var writeResults: FilePath? @Option(help: "Project configuration for non-Apple build systems") - var genericProjectConfig: FilePath? + private var genericProjectConfig: FilePath? @Flag(help: "Enable Bazel project mode") - var bazel: Bool = defaultConfiguration.$bazel.defaultValue + private var bazel: Bool = defaultConfiguration.$bazel.defaultValue @Option(help: "Filter pattern applied to the Bazel top-level targets query") - var bazelFilter: String? + private var bazelFilter: String? @Option(help: "Path to a global index store populated by Bazel. If provided, will be used instead of individual module stores.") - var bazelIndexStore: FilePath? + private var bazelIndexStore: FilePath? private static let defaultConfiguration = Configuration() diff --git a/Sources/Frontend/Project.swift b/Sources/Frontend/Project.swift index 60b694714..13909fbd4 100644 --- a/Sources/Frontend/Project.swift +++ b/Sources/Frontend/Project.swift @@ -6,7 +6,7 @@ import Shared import SystemPackage final class Project { - let kind: ProjectKind + private let kind: ProjectKind private let configuration: Configuration private let shell: Shell diff --git a/Sources/Frontend/main.swift b/Sources/Frontend/main.swift index 052f3bf39..9ee79aed5 100644 --- a/Sources/Frontend/main.swift +++ b/Sources/Frontend/main.swift @@ -3,7 +3,7 @@ import Foundation // When stdout is a pipe, enable line buffering so output is flushed after each // newline rather than block-buffered, ensuring timely output to the consumer. -var info = stat() +private var info = stat() fstat(STDOUT_FILENO, &info) if (info.st_mode & S_IFMT) == S_IFIFO { @@ -11,7 +11,7 @@ if (info.st_mode & S_IFMT) == S_IFIFO { setlinebuf(stderr) } -struct PeripheryCommand: ParsableCommand { +private struct PeripheryCommand: ParsableCommand { static let configuration = CommandConfiguration( commandName: "periphery", subcommands: [ diff --git a/Sources/Indexer/InfoPlistIndexer.swift b/Sources/Indexer/InfoPlistIndexer.swift index e6f9049c1..cb0f1addb 100644 --- a/Sources/Indexer/InfoPlistIndexer.swift +++ b/Sources/Indexer/InfoPlistIndexer.swift @@ -5,7 +5,7 @@ import SourceGraph import SystemPackage final class InfoPlistIndexer: Indexer { - enum PlistError: Error { + private enum PlistError: Error { case failedToParse(path: FilePath, underlyingError: Error) } diff --git a/Sources/Indexer/XCDataModelIndexer.swift b/Sources/Indexer/XCDataModelIndexer.swift index 00cc0496c..3d3a44a95 100644 --- a/Sources/Indexer/XCDataModelIndexer.swift +++ b/Sources/Indexer/XCDataModelIndexer.swift @@ -5,7 +5,7 @@ import SourceGraph import SystemPackage final class XCDataModelIndexer: Indexer { - enum XCDataModelError: Error { + private enum XCDataModelError: Error { case failedToParse(path: FilePath, underlyingError: Error) } diff --git a/Sources/Indexer/XibIndexer.swift b/Sources/Indexer/XibIndexer.swift index 0cf837be0..8c456c361 100644 --- a/Sources/Indexer/XibIndexer.swift +++ b/Sources/Indexer/XibIndexer.swift @@ -5,7 +5,7 @@ import SourceGraph import SystemPackage final class XibIndexer: Indexer { - enum XibError: Error { + private enum XibError: Error { case failedToParse(path: FilePath, underlyingError: Error) } diff --git a/Sources/Logger/Logger.swift b/Sources/Logger/Logger.swift index 4ff721a6e..d6afb5913 100644 --- a/Sources/Logger/Logger.swift +++ b/Sources/Logger/Logger.swift @@ -28,13 +28,13 @@ public enum LoggerColorMode: Sendable { } public struct Logger: Sendable { - let outputQueue: DispatchQueue - let quiet: Bool - let verbose: Bool - let colorMode: LoggerColorMode + private let outputQueue: DispatchQueue + private let quiet: Bool + private let verbose: Bool + private let colorMode: LoggerColorMode #if canImport(os) - let signposter = OSSignposter() + private let signposter = OSSignposter() #endif public var isColoredOutputEnabled: Bool { @@ -115,7 +115,7 @@ public struct Logger: Sendable { // MARK: - Private - func log(_ line: String, output: UnsafeMutablePointer) { + private func log(_ line: String, output: UnsafeMutablePointer) { _ = outputQueue.sync { fputs(line + "\n", output) } } @@ -132,8 +132,8 @@ public struct Logger: Sendable { } public struct ContextualLogger: Sendable { - let logger: Logger - let context: String + fileprivate let logger: Logger + fileprivate let context: String public func contextualized(with innerContext: String) -> ContextualLogger { logger.contextualized(with: "\(context):\(innerContext)") @@ -154,8 +154,8 @@ public struct ContextualLogger: Sendable { #if canImport(os) public struct SignpostInterval { - let name: StaticString - let state: OSSignpostIntervalState + fileprivate let name: StaticString + fileprivate let state: OSSignpostIntervalState } #else public struct SignpostInterval { diff --git a/Sources/ProjectDrivers/GenericProjectDriver.swift b/Sources/ProjectDrivers/GenericProjectDriver.swift index 6807e3643..4a166b95b 100644 --- a/Sources/ProjectDrivers/GenericProjectDriver.swift +++ b/Sources/ProjectDrivers/GenericProjectDriver.swift @@ -7,7 +7,7 @@ import SwiftIndexStore import SystemPackage public final class GenericProjectDriver { - struct GenericConfig: Decodable { + private struct GenericConfig: Decodable { let indexstores: Set let plists: Set let xibs: Set diff --git a/Sources/ProjectDrivers/SPMProjectDriver.swift b/Sources/ProjectDrivers/SPMProjectDriver.swift index 5b54afdfe..f95049035 100644 --- a/Sources/ProjectDrivers/SPMProjectDriver.swift +++ b/Sources/ProjectDrivers/SPMProjectDriver.swift @@ -20,7 +20,7 @@ public final class SPMProjectDriver { self.init(pkg: pkg, configuration: configuration, logger: logger) } - init(pkg: SPM.Package, configuration: Configuration, logger: Logger) { + private init(pkg: SPM.Package, configuration: Configuration, logger: Logger) { self.pkg = pkg self.configuration = configuration self.logger = logger diff --git a/Sources/ProjectDrivers/XcodeProjectDriver.swift b/Sources/ProjectDrivers/XcodeProjectDriver.swift index 943fe363c..058708c8f 100644 --- a/Sources/ProjectDrivers/XcodeProjectDriver.swift +++ b/Sources/ProjectDrivers/XcodeProjectDriver.swift @@ -79,7 +79,7 @@ ) } - init( + private init( logger: Logger, configuration: Configuration, xcodebuild: Xcodebuild, diff --git a/Sources/Shared/JobPool.swift b/Sources/Shared/JobPool.swift index ae527d431..2d2b38eaf 100644 --- a/Sources/Shared/JobPool.swift +++ b/Sources/Shared/JobPool.swift @@ -2,7 +2,7 @@ import Foundation import Synchronization public struct JobPool { - let jobs: [Job] + private let jobs: [Job] public init(jobs: [Job]) { self.jobs = jobs diff --git a/Sources/Shared/SetupGuide.swift b/Sources/Shared/SetupGuide.swift index bc10dde2a..35790554d 100644 --- a/Sources/Shared/SetupGuide.swift +++ b/Sources/Shared/SetupGuide.swift @@ -26,7 +26,7 @@ open class SetupGuideHelpers { self.logger = logger } - func display(options: [String]) { + private func display(options: [String]) { let maxPaddingCount = String(options.count).count for (index, option) in options.enumerated() { diff --git a/Sources/Shared/Shell.swift b/Sources/Shared/Shell.swift index 0f7c1667a..0d89148ce 100644 --- a/Sources/Shared/Shell.swift +++ b/Sources/Shared/Shell.swift @@ -2,7 +2,7 @@ import Foundation import Logger import Synchronization -final class ShellProcessStore: Sendable { +private final class ShellProcessStore: Sendable { private let processes = Mutex>([]) func interruptRunning() { diff --git a/Sources/Shared/SwiftVersion.swift b/Sources/Shared/SwiftVersion.swift index 4a4210fb4..e1ef6bee1 100644 --- a/Sources/Shared/SwiftVersion.swift +++ b/Sources/Shared/SwiftVersion.swift @@ -2,7 +2,7 @@ import Extensions import Foundation public struct SwiftVersion { - static let minimumVersion = "5.10" + private static let minimumVersion = "5.10" public let version: VersionString public let fullVersion: String diff --git a/Sources/SourceGraph/Elements/DeclarationAttribute.swift b/Sources/SourceGraph/Elements/DeclarationAttribute.swift index d8e1b1b93..832cb976b 100644 --- a/Sources/SourceGraph/Elements/DeclarationAttribute.swift +++ b/Sources/SourceGraph/Elements/DeclarationAttribute.swift @@ -1,6 +1,6 @@ public struct DeclarationAttribute: Hashable, CustomStringConvertible { let name: String - let arguments: String? + private let arguments: String? public init(name: String, arguments: String?) { self.name = name diff --git a/Sources/SourceGraph/Elements/Reference.swift b/Sources/SourceGraph/Elements/Reference.swift index 013a16946..b30ee1239 100644 --- a/Sources/SourceGraph/Elements/Reference.swift +++ b/Sources/SourceGraph/Elements/Reference.swift @@ -66,7 +66,7 @@ extension Reference: CustomStringConvertible { return "\(referenceType)(\(descriptionParts.joined(separator: ", ")))" } - var descriptionParts: [String] { + private var descriptionParts: [String] { let formattedName = name != nil ? "'\(name!)'" : "nil" return [kind.rawValue, formattedName, "'\(usr)'", role.rawValue, location.shortDescription] diff --git a/Sources/SourceGraph/Mutators/EnumCaseReferenceBuilder.swift b/Sources/SourceGraph/Mutators/EnumCaseReferenceBuilder.swift index 79414df2a..bb7a8f2f1 100644 --- a/Sources/SourceGraph/Mutators/EnumCaseReferenceBuilder.swift +++ b/Sources/SourceGraph/Mutators/EnumCaseReferenceBuilder.swift @@ -33,7 +33,7 @@ final class EnumCaseReferenceBuilder: SourceGraphMutator { // MARK: - Private - func isRawRepresentable(_ enumDeclaration: Declaration) -> Bool { + private func isRawRepresentable(_ enumDeclaration: Declaration) -> Bool { // If the enum has a related struct it's very likely to be raw representable, // and thus is dynamic in nature. diff --git a/Sources/SourceGraph/SourceGraph.swift b/Sources/SourceGraph/SourceGraph.swift index 350b5c94c..bf35e8b9e 100644 --- a/Sources/SourceGraph/SourceGraph.swift +++ b/Sources/SourceGraph/SourceGraph.swift @@ -281,7 +281,7 @@ public final class SourceGraph { inheritedTypeReferences(of: decl).compactMap { declaration(withUsr: $0.usr) } } - func immediateSubclasses(of decl: Declaration) -> Set { + private func immediateSubclasses(of decl: Declaration) -> Set { references(to: decl) .filter { $0.isRelated && $0.kind == .class } .flatMap { $0.parent?.usrs ?? [] } diff --git a/Sources/SyntaxAnalysis/CommentCommand.swift b/Sources/SyntaxAnalysis/CommentCommand.swift index 1c0dec7c1..6cf22fe29 100644 --- a/Sources/SyntaxAnalysis/CommentCommand.swift +++ b/Sources/SyntaxAnalysis/CommentCommand.swift @@ -27,7 +27,7 @@ extension CommentCommand { } } - static func parse(_ rawCommand: String) -> Self? { + private static func parse(_ rawCommand: String) -> Self? { if rawCommand == "ignore" { return .ignore } else if rawCommand == "ignore:all" { diff --git a/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift b/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift index 704cb748f..3f1ea112a 100644 --- a/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift +++ b/Sources/SyntaxAnalysis/DeclarationSyntaxVisitor.swift @@ -222,7 +222,7 @@ public final class DeclarationSyntaxVisitor: PeripherySyntaxVisitor { } } - func visitVariableTupleBinding(node: VariableDeclSyntax, pattern: TuplePatternSyntax, typeTuple: TupleTypeElementListSyntax?, initializerTuple: LabeledExprListSyntax?) { + private func visitVariableTupleBinding(node: VariableDeclSyntax, pattern: TuplePatternSyntax, typeTuple: TupleTypeElementListSyntax?, initializerTuple: LabeledExprListSyntax?) { let elements = Array(pattern.elements) let types: [TupleTypeElementSyntax?] = typeTuple?.map(\.self) ?? Array(repeating: nil, count: elements.count) let initializers: [LabeledExprSyntax?] = initializerTuple?.map(\.self) ?? Array(repeating: nil, count: elements.count) diff --git a/Sources/SyntaxAnalysis/MultiplexingSyntaxVisitor.swift b/Sources/SyntaxAnalysis/MultiplexingSyntaxVisitor.swift index 9222f86c0..5de68831c 100644 --- a/Sources/SyntaxAnalysis/MultiplexingSyntaxVisitor.swift +++ b/Sources/SyntaxAnalysis/MultiplexingSyntaxVisitor.swift @@ -95,8 +95,8 @@ public final class MultiplexingSyntaxVisitor: SyntaxVisitor { public let sourceFile: SourceFile public let syntax: SourceFileSyntax public let locationConverter: SourceLocationConverter - let sourceLocationBuilder: SourceLocationBuilder - let swiftVersion: SwiftVersion + private let sourceLocationBuilder: SourceLocationBuilder + private let swiftVersion: SwiftVersion private var visitors: [PeripherySyntaxVisitor] = [] diff --git a/Sources/SyntaxAnalysis/TypeSyntaxInspector.swift b/Sources/SyntaxAnalysis/TypeSyntaxInspector.swift index 370f60970..2403963a4 100644 --- a/Sources/SyntaxAnalysis/TypeSyntaxInspector.swift +++ b/Sources/SyntaxAnalysis/TypeSyntaxInspector.swift @@ -28,7 +28,7 @@ struct TypeSyntaxInspector { // MARK: - Private - func types(for typeSyntax: TypeSyntax) -> Set { + private func types(for typeSyntax: TypeSyntax) -> Set { if let identifierType = typeSyntax.as(IdentifierTypeSyntax.self) { // Simple type. var result: Set = identifierType.genericArgumentClause?.arguments.flatMapSet { diff --git a/Sources/SyntaxAnalysis/UnusedParameterParser.swift b/Sources/SyntaxAnalysis/UnusedParameterParser.swift index cf7250f4f..d6b7ec947 100644 --- a/Sources/SyntaxAnalysis/UnusedParameterParser.swift +++ b/Sources/SyntaxAnalysis/UnusedParameterParser.swift @@ -131,7 +131,7 @@ struct UnusedParameterParser { self.parseProtocols = parseProtocols } - func parse() -> [Function] { + private func parse() -> [Function] { parse(node: syntax, collecting: Function.self) } diff --git a/Sources/XcodeSupport/XcodeTarget.swift b/Sources/XcodeSupport/XcodeTarget.swift index bb184b94f..89eb06d68 100644 --- a/Sources/XcodeSupport/XcodeTarget.swift +++ b/Sources/XcodeSupport/XcodeTarget.swift @@ -4,7 +4,7 @@ import SystemPackage import XcodeProj public final class XcodeTarget { - let project: XcodeProject + private let project: XcodeProject private let target: PBXTarget private var files: [ProjectFileKind: Set] = [:] From 9868cddc89120f5924022faee75efb81f22221a4 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Wed, 7 Jan 2026 13:44:27 -0800 Subject: [PATCH 19/30] Fix false positives caught by CI/Bazel building This commit fixes false positive warnings where internal declarations were incorrectly flagged as "not used outside of file" when they were actually being used in test files via @testable import. This only appeared using the --bazel flag on github. Fix a linux-only warning. Also, handle @usableFromInline to avoid false positives. Updated bazel.json and linux-bazel.json to suppress 3 new Bazel-specific false positives. When Bazel builds modules independently, each module's index store doesn't capture cross-module usage, leading to false "unused" warnings. The baseline suppresses these known artifacts while preserving detection of genuine issues. Updated the build script to clean SwiftPM cache before build to prevent occasional CI failure. --- .github/workflows/test.yml | 4 ++ MODULE.bazel.lock | 2 +- .../Results/OutputFormatter.swift | 4 +- ...RedundantInternalAccessibilityMarker.swift | 7 +++ .../TargetA/InternalTestableImportUsage.swift | 11 +++++ .../InternalTestableImportUsage_Support.swift | 10 ++++ .../UsableFromInlineAccessibility.swift | 46 +++++++++++++++++++ .../InternalTestableImportTest.swift | 14 ++++++ .../RedundantInternalAccessibilityTest.swift | 45 ++++++++++++++++++ baselines/bazel.json | 5 +- baselines/linux-bazel.json | 5 +- 11 files changed, 149 insertions(+), 4 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage_Support.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/UsableFromInlineAccessibility.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Tests/TestTarget/InternalTestableImportTest.swift diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 3f423c240..247141704 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -125,6 +125,8 @@ jobs: run: ${{ env.swift_build }} - name: Scan run: ${{ env.periphery_scan }} ${{ matrix.baseline && format('--baseline baselines/{0}', matrix.baseline) || '' }} + - name: Clean SwiftPM cache + run: rm -rf ~/Library/Caches/org.swift.swiftpm - name: Test run: ${{ env.swift_test }} linux: @@ -172,5 +174,7 @@ jobs: run: ${{ env.swift_build }} - name: Scan run: ${{ env.periphery_scan }} --baseline baselines/linux.json + - name: Clean SwiftPM cache + run: rm -rf ~/.cache/org.swift.swiftpm - name: Test run: ${{ env.swift_test }} diff --git a/MODULE.bazel.lock b/MODULE.bazel.lock index 11fa27a61..8621f3a50 100644 --- a/MODULE.bazel.lock +++ b/MODULE.bazel.lock @@ -193,7 +193,7 @@ "//bazel:generated.bzl%generated": { "general": { "bzlTransitiveDigest": "nMR2FBcoRPImVocN9DNOnm2NQWyTbJPu7SHJgAXsLFw=", - "usagesDigest": "Hdmxc+LdckmZqHgeSNbQlCU0uQaBARRWkeCpjro+Xhk=", + "usagesDigest": "S5zTCGsw3nugb0U3s6bPvFYl04dzqTMrLLUzzabunO8=", "recordedFileInputs": {}, "recordedDirentsInputs": {}, "envVariables": {}, diff --git a/Sources/PeripheryKit/Results/OutputFormatter.swift b/Sources/PeripheryKit/Results/OutputFormatter.swift index 491bafad7..fdc335bf7 100644 --- a/Sources/PeripheryKit/Results/OutputFormatter.swift +++ b/Sources/PeripheryKit/Results/OutputFormatter.swift @@ -70,7 +70,9 @@ extension OutputFormatter { description += "Redundant public accessibility for \(kindDisplayName) '\(name)' (not used outside of \(modulesJoined))" case let .redundantInternalAccessibility(_, suggestedAccessibility): let accessibilityText = suggestedAccessibility?.rawValue ?? "private/fileprivate" - description += "Redundant internal accessibility for \(kindDisplayName) '\(name)' (not used outside of file; can be \(accessibilityText)" + description += "Redundant internal accessibility for \(kindDisplayName) '\(name)' (not used outside of file; can be \(accessibilityText))" + // Hint: if we wanted to output the USR for helping build bazel.json, we could also output: + // result.declaration.usrs.joined(separator: ", ") case let .redundantFilePrivateAccessibility(_, containingTypeName): let context = containingTypeName.map { "only used within \($0)" } ?? "not used outside of file" description += "Redundant fileprivate accessibility for \(kindDisplayName) '\(name)' (\(context); can be private)" diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 23bb9c373..c87b8670f 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -168,6 +168,13 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // Override methods must be at least as accessible as what they override. if decl.isOverride { return true } + // Declarations with @usableFromInline must remain internal (or package). + // This attribute allows internal declarations to be inlined into client code, + // requiring them to maintain internal visibility. + if decl.attributes.contains(where: { $0.name == "usableFromInline" }) { + return true + } + return false } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage.swift new file mode 100644 index 000000000..a8d44bd3e --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage.swift @@ -0,0 +1,11 @@ +import Foundation + +// Should NOT be flagged as redundant - used from test via @testable import +internal class InternalUsedOnlyInTest { + internal func testOnlyMethod() {} +} + +// Should NOT be flagged - used from both test AND production code +internal class InternalUsedInBoth { + internal func sharedMethod() {} +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage_Support.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage_Support.swift new file mode 100644 index 000000000..e6c6cc4e8 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTestableImportUsage_Support.swift @@ -0,0 +1,10 @@ +import Foundation + +// This file uses InternalUsedInBoth from production code within the same module +// to ensure it's not flagged as redundant internal (since it's used both in production and tests) + +struct InternalTestableImportRetainer { + func retain() { + _ = InternalUsedInBoth().sharedMethod() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/UsableFromInlineAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/UsableFromInlineAccessibility.swift new file mode 100644 index 000000000..50689f712 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/UsableFromInlineAccessibility.swift @@ -0,0 +1,46 @@ +// Test file for @usableFromInline attribute handling + +public struct PublicContainer { + // This should NOT be flagged as redundant internal because it has @usableFromInline. + // @usableFromInline requires the declaration to remain internal (or package) so it can + // be inlined into client code. + @usableFromInline + internal init() {} + + // This should NOT be flagged as redundant internal because of @usableFromInline. + @usableFromInline + internal func inlinableHelper() -> Int { + 42 + } + + // This should NOT be flagged as redundant internal because of @usableFromInline. + @usableFromInline + internal var inlinableProperty: String { + "value" + } + + // This should NOT be flagged as redundant internal. + // Even though it's only used in this file, @usableFromInline means it could be + // inlined into client code that imports this module. + @usableFromInline + internal static func inlinableStaticMethod() -> Bool { + true + } + + // Public method that uses the @usableFromInline members + @inlinable + public func publicInlinableMethod() -> String { + "\(inlinableProperty): \(inlinableHelper())" + } +} + +// This SHOULD be flagged as redundant internal - no @usableFromInline attribute +// and only used within the same file in a non-inlinable private function. +internal func regularInternalMethod() -> String { + PublicContainer.inlinableStaticMethod().description +} + +// Use the regular internal method within the same file +private func useRegularMethod() { + _ = regularInternalMethod() +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Tests/TestTarget/InternalTestableImportTest.swift b/Tests/AccessibilityTests/AccessibilityProject/Tests/TestTarget/InternalTestableImportTest.swift new file mode 100644 index 000000000..09d5901e9 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Tests/TestTarget/InternalTestableImportTest.swift @@ -0,0 +1,14 @@ +import Foundation +import XCTest +@testable import TargetA + +class InternalTestableImportTest: XCTestCase { + func testInternalAccess() { + // Access internal declarations via @testable import + let obj1 = InternalUsedOnlyInTest() + obj1.testOnlyMethod() + + let obj2 = InternalUsedInBoth() + obj2.sharedMethod() + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 106e8595b..0bf6f93fb 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -176,4 +176,49 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.struct("ImplicitlyInternalStructUsedFromAnotherFile")) } + + /// Tests that internal declarations accessed from test files via @testable import + /// are NOT flagged as redundant internal. + /// + /// This verifies that @testable import references count as legitimate cross-file usage, + /// preventing false positives when test files access internal members. Since tests ARE + /// using these internal members from a different file, they require internal accessibility. + func testInternalUsedViaTestableImportNotFlagged() { + index() + + // InternalUsedOnlyInTest should NOT be flagged because it IS used from + // a test file (different file) via @testable import + assertNotRedundantInternalAccessibility(.class("InternalUsedOnlyInTest")) + } + + /// Tests that internal declarations used from production code in the same module + /// are NOT flagged as redundant (baseline behavior verification). + func testInternalUsedInProductionNotFlagged() { + index() + + // InternalUsedInBoth should NOT be flagged because it's used from + // production code (InternalTestableImportUsage_Support.swift) within the same module + assertNotRedundantInternalAccessibility(.class("InternalUsedInBoth")) + } + + /// Tests that declarations with @usableFromInline are NOT flagged as redundant internal. + /// + /// The @usableFromInline attribute allows internal declarations to be inlined into + /// client code, requiring them to maintain internal (or package) visibility. Marking + /// them as fileprivate or private would cause a compiler error because @usableFromInline + /// is incompatible with those access levels. + /// + /// This test verifies the fix for a build error on Linux where @usableFromInline + /// declarations were incorrectly flagged as redundant internal, and changing them + /// to fileprivate caused: "@usableFromInline attribute can only be applied to + /// internal or package declarations". + func testUsableFromInlineNotFlagged() { + index() + + // All @usableFromInline members should NOT be flagged, even if only used in same file + assertNotRedundantInternalAccessibility(.functionConstructor("init()")) + assertNotRedundantInternalAccessibility(.functionMethodInstance("inlinableHelper()")) + assertNotRedundantInternalAccessibility(.varInstance("inlinableProperty")) + assertNotRedundantInternalAccessibility(.functionMethodStatic("inlinableStaticMethod()")) + } } diff --git a/baselines/bazel.json b/baselines/bazel.json index 33393450d..eb6740271 100644 --- a/baselines/bazel.json +++ b/baselines/bazel.json @@ -7,7 +7,10 @@ "s:13ConfigurationAAC5resetyyF", "s:13SystemPackage8FilePathV10ExtensionsE5chdir7closureyyyKXE_tKF", "s:14SyntaxAnalysis21UnusedParameterParserV5parse4file0F9ProtocolsSayAA8FunctionVG11SourceGraph0J4FileC_SbtKFZ", - "s:14SyntaxAnalysis8FunctionV8fullNameSSvp" + "s:14SyntaxAnalysis8FunctionV8fullNameSSvp", + "s:14SyntaxAnalysis23UnusedParameterAnalyzerC7analyze8functionShyAA0D0VGAA8FunctionV_tF", + "s:14SyntaxAnalysis9ParameterV5labelAC8PartKindOvp", + "s:14SyntaxAnalysis9ParameterV8location11SourceGraph8LocationCvp" ] } } \ No newline at end of file diff --git a/baselines/linux-bazel.json b/baselines/linux-bazel.json index c54f8ef14..6563e1f8d 100644 --- a/baselines/linux-bazel.json +++ b/baselines/linux-bazel.json @@ -11,7 +11,10 @@ "s:6Shared17SetupGuideHelpersC6select8multipleAA0B9SelectionOSaySSG_tF", "s:SS10ExtensionsE17withEscapedQuotesSSvp", "s:SS10ExtensionsE4djb2Sivp", - "s:SS10ExtensionsE7djb2HexSSvp" + "s:SS10ExtensionsE7djb2HexSSvp", + "s:14SyntaxAnalysis23UnusedParameterAnalyzerC7analyze8functionShyAA0D0VGAA8FunctionV_tF", + "s:14SyntaxAnalysis9ParameterV5labelAC8PartKindOvp", + "s:14SyntaxAnalysis9ParameterV8location11SourceGraph8LocationCvp" ] } } From 0a1f5d6f4b19ec4b5a84b290b80cbcff4c64deeb Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Sun, 11 Jan 2026 22:41:13 -0800 Subject: [PATCH 20/30] Make new function be private, conforming to new code --- Sources/SourceGraph/SourceGraphDebugger.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/SourceGraph/SourceGraphDebugger.swift b/Sources/SourceGraph/SourceGraphDebugger.swift index e62f6ddba..10c78d958 100644 --- a/Sources/SourceGraph/SourceGraphDebugger.swift +++ b/Sources/SourceGraph/SourceGraphDebugger.swift @@ -14,7 +14,7 @@ final class SourceGraphDebugger { // MARK: - Private - func describe(_ declarations: [Declaration]) { + private func describe(_ declarations: [Declaration]) { for (index, declaration) in declarations.enumerated() { describe(declaration) From c9338c65c18ce4aba67c52fa1c678557cdb454ec Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Tue, 20 Jan 2026 10:46:18 -0800 Subject: [PATCH 21/30] Update to work with upstream changes, fixing external protocol check and adding test cases. --- ...RedundantInternalAccessibilityMarker.swift | 27 ++++++++++++------- ...rnalProtocolConformanceAccessibility.swift | 27 +++++++++++++++++++ .../RedundantInternalAccessibilityTest.swift | 25 +++++++++++++++++ baselines/bazel.json | 3 ++- baselines/linux-bazel.json | 3 ++- 5 files changed, 73 insertions(+), 12 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolConformanceAccessibility.swift diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index c87b8670f..b9fa6da60 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -195,7 +195,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // from the conforming declaration to the protocol requirement. If this declaration // has any related references pointing to protocol members with matching names, // it's implementing a protocol requirement. - let relatedReferences = graph.references(to: decl).filter(\.isRelated) + let relatedReferences = graph.references(to: decl).filter { $0.kind == .related } for ref in relatedReferences { if let protocolDecl = graph.declaration(withUsr: ref.usr), protocolDecl.kind.isProtocolMemberKind || protocolDecl.kind == .associatedtype @@ -204,14 +204,21 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { } } - // Alternative check: Look for related references FROM this declaration - // to protocol members. The ProtocolConformanceReferenceBuilder inverts - // these relationships, so we might find them either direction. - for ref in decl.related where ref.kind.isProtocolMemberConformingKind { - if let referencedDecl = graph.declaration(withUsr: ref.usr), - let referencedParent = referencedDecl.parent, - referencedParent.kind == .protocol - { + // Case 3: Check for .related references FROM this declaration to protocol members. + // This covers both internal AND external protocol conformances. + for ref in decl.related where ref.declarationKind.isProtocolMemberConformingKind { + if let referencedDecl = graph.declaration(withUsr: ref.usr) { + // Internal protocol: verify the referenced declaration's parent is a protocol. + if let referencedParent = referencedDecl.parent, + referencedParent.kind == .protocol + { + return true + } + } else if ref.name == decl.name { + // External protocol: the declaration doesn't exist in our graph, + // but the indexer created a .related reference with a protocol member kind + // AND the names match. This means this declaration implements an external + // protocol requirement. return true } } @@ -249,7 +256,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let siblings = parent.declarations let hasFunctionReference = siblings.contains { sibling in sibling.kind.isFunctionKind && sibling.references.contains { ref in - ref.kind == .typealias && decl.usrs.contains(ref.usr) + ref.declarationKind == .typealias && decl.usrs.contains(ref.usr) } } if hasFunctionReference { diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolConformanceAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolConformanceAccessibility.swift new file mode 100644 index 000000000..77458b0db --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolConformanceAccessibility.swift @@ -0,0 +1,27 @@ +/* + ExternalProtocolConformanceAccessibility.swift + Tests that types conforming to external protocols are NOT flagged as redundant internal. +*/ + +import ExternalTarget + +// Internal struct conforming to external protocol - should NOT be flagged. +internal struct InternalStructConformingToExternalProtocol: ExternalProtocol { + func someExternalProtocolMethod() {} +} + +// Implicitly internal class conforming to external protocol - should NOT be flagged. +class ImplicitlyInternalClassConformingToExternalProtocol: ExternalProtocol { + func someExternalProtocolMethod() {} +} + +// Used to ensure these types are referenced. +public class ExternalProtocolConformanceRetainer { + public init() {} + public func retain() { + let s = InternalStructConformingToExternalProtocol() + s.someExternalProtocolMethod() + let c = ImplicitlyInternalClassConformingToExternalProtocol() + c.someExternalProtocolMethod() + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 0bf6f93fb..0a6393ccb 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -221,4 +221,29 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.varInstance("inlinableProperty")) assertNotRedundantInternalAccessibility(.functionMethodStatic("inlinableStaticMethod()")) } + + /// Tests that internal types conforming to external protocols are NOT flagged as redundant. + /// + /// When a type conforms to an external protocol (from another module), its protocol + /// requirement implementations must maintain their accessibility to fulfill the + /// protocol contract. This prevents false positives like those seen with + /// CheckUpdateCommand, ScanCommand, etc. implementing ArgumentParser's ParsableCommand. + func testExternalProtocolConformanceNotFlagged() { + index() + + // Internal struct conforming to ExternalProtocol should NOT be flagged + assertNotRedundantInternalAccessibility(.struct("InternalStructConformingToExternalProtocol")) + // The protocol requirement implementation should NOT be flagged (line 10 in fixture) + assertNotRedundantInternalAccessibility(.functionMethodInstance("someExternalProtocolMethod()", line: 10)) + } + + /// Tests that implicitly internal types conforming to external protocols are NOT flagged. + func testImplicitlyInternalExternalProtocolConformanceNotFlagged() { + index() + + // Implicitly internal class conforming to ExternalProtocol should NOT be flagged + assertNotRedundantInternalAccessibility(.class("ImplicitlyInternalClassConformingToExternalProtocol")) + // The protocol requirement implementation should NOT be flagged (line 15 in fixture) + assertNotRedundantInternalAccessibility(.functionMethodInstance("someExternalProtocolMethod()", line: 15)) + } } diff --git a/baselines/bazel.json b/baselines/bazel.json index 013b41612..80417a573 100644 --- a/baselines/bazel.json +++ b/baselines/bazel.json @@ -7,7 +7,8 @@ "s:14SyntaxAnalysis8FunctionV8fullNameSSvp", "s:14SyntaxAnalysis23UnusedParameterAnalyzerC7analyze8functionShyAA0D0VGAA8FunctionV_tF", "s:14SyntaxAnalysis9ParameterV5labelAC8PartKindOvp", - "s:14SyntaxAnalysis9ParameterV8location11SourceGraph8LocationCvp" + "s:14SyntaxAnalysis9ParameterV8location11SourceGraph8LocationCvp", + "s:11SourceGraph32InterfaceBuilderPropertyRetainerC19swiftNameToSelectoryS2SFZ" ] } } diff --git a/baselines/linux-bazel.json b/baselines/linux-bazel.json index c5cd8fe94..4aadd93bf 100644 --- a/baselines/linux-bazel.json +++ b/baselines/linux-bazel.json @@ -15,7 +15,8 @@ "s:SS10ExtensionsE7djb2HexSSvp", "s:14SyntaxAnalysis23UnusedParameterAnalyzerC7analyze8functionShyAA0D0VGAA8FunctionV_tF", "s:14SyntaxAnalysis9ParameterV5labelAC8PartKindOvp", - "s:14SyntaxAnalysis9ParameterV8location11SourceGraph8LocationCvp" + "s:14SyntaxAnalysis9ParameterV8location11SourceGraph8LocationCvp", + "s:11SourceGraph32InterfaceBuilderPropertyRetainerC19swiftNameToSelectoryS2SFZ" ] } } From 7c26750a259f57f78288faf94d33aa73792ea72f Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Sat, 24 Jan 2026 08:51:30 -0800 Subject: [PATCH 22/30] Make sure that internal types used as return types of functions called from other files are NOT flagged as redundant internal --- ...RedundantInternalAccessibilityMarker.swift | 106 ++++++- .../Sources/MainTarget/main.swift | 3 + .../TargetA/InternalTypeAsReturnType.swift | 25 ++ .../InternalTypeAsReturnType_Consumer.swift | 11 + ...nalTypeTransitivelyExposedInSameFile.swift | 37 +++ .../TargetA/TransitiveAccessExposure.swift | 267 ++++++++++++++++++ .../TransitiveAccessExposure_Consumer.swift | 94 ++++++ .../RedundantInternalAccessibilityTest.swift | 88 ++++++ 8 files changed, 628 insertions(+), 3 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType_Consumer.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeTransitivelyExposedInSameFile.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure_Consumer.swift diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index b9fa6da60..449d43e98 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -40,7 +40,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { if decl.accessibility.value == .internal { if !graph.isRetained(decl), !shouldSkipMarking(decl) { let isReferencedOutside = decl.isReferencedOutsideFile(graph: graph) - if !isReferencedOutside { + if !isReferencedOutside, !isTransitivelyExposedOutsideFile(decl) { mark(decl) } } @@ -82,8 +82,11 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // to indicate the ambiguity in the output message. // If the declaration is referenced from different types in the same file, // it needs fileprivate. Otherwise, private is sufficient. + // Also check transitive exposure: if the type is used as return/parameter type of a + // function called from a different type in the same file, it needs fileprivate. let isTopLevel = decl.parent == nil - let suggestedAccessibility: Accessibility? = isTopLevel ? nil : (isReferencedFromDifferentTypeInSameFile(decl) ? .fileprivate : .private) + let needsFileprivate = isReferencedFromDifferentTypeInSameFile(decl) || isTransitivelyExposedFromDifferentTypeInSameFile(decl) + let suggestedAccessibility: Accessibility? = isTopLevel ? nil : (needsFileprivate ? .fileprivate : .private) // Check if the parent's accessibility already constrains this member. // If the parent is `private`, the member is already effectively `private`. @@ -114,7 +117,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { for descDecl in descendants { if !graph.isRetained(descDecl), !shouldSkipMarking(descDecl) { let isReferencedOutside = descDecl.isReferencedOutsideFile(graph: graph) - if !isReferencedOutside { + if !isReferencedOutside, !isTransitivelyExposedOutsideFile(descDecl) { mark(descDecl) } } @@ -346,4 +349,101 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { } return decl } + + /// Checks if a type is transitively exposed outside its file through an API signature. + /// + /// A type is transitively exposed when it appears in the signature of a function, property, + /// or initializer (as return type, parameter type, etc.) where that API is referenced from + /// a different file. Even if the type itself is never directly referenced outside its file, + /// it must remain `internal` (not `fileprivate` or `private`) if it's part of an API that + /// is used from other files. + /// + /// For example, if `ScanProgress` is the return type of `runFullScanWithStreaming()`, and + /// that function is called from another file, then `ScanProgress` is transitively exposed + /// and should not be marked as redundantly internal. + private func isTransitivelyExposedOutsideFile(_ decl: Declaration) -> Bool { + let refs = graph.references(to: decl) + + for ref in refs { + // Check if this reference is in an API signature role (return type, parameter type, etc.) + guard ref.role.isPubliclyExposable else { continue } + + // Get the parent declaration (the function/property that uses this type in its signature) + guard let parent = ref.parent else { continue } + + // Check if that parent API is referenced from outside this file + if parent.isReferencedOutsideFile(graph: graph) { + return true + } + + // For properties, also check if the containing type is used from outside the file. + // When code accesses `obj.property`, the property's type is exposed even if + // `property` itself isn't directly referenced from outside. + if parent.kind.isVariableKind { + if let containingType = parent.parent, + containingType.isReferencedOutsideFile(graph: graph) + { + return true + } + } + } + + return false + } + + /// Checks if a type is transitively exposed from a different type within the same file. + /// + /// This is similar to `isTransitivelyExposedOutsideFile`, but checks for exposure within + /// the same file from a different type. When a type is used as a return type or parameter + /// type of a function that is called from a different type in the same file, the type + /// needs to be at least `fileprivate` (not `private`). + /// + /// For example: + /// ```swift + /// class ClassA { + /// enum Status { case active } // Only directly referenced in ClassA + /// func getStatus() -> Status { .active } // Called from ClassB + /// } + /// class ClassB { + /// func use() { _ = ClassA().getStatus() } // Uses Status transitively + /// } + /// ``` + /// Here, `Status` should be suggested as `fileprivate`, not `private`. + private func isTransitivelyExposedFromDifferentTypeInSameFile(_ decl: Declaration) -> Bool { + let file = decl.location.file + let refs = graph.references(to: decl) + + guard let declTopLevel = topLevelType(of: decl) else { + return false + } + + let declLogicalType = logicalType(of: declTopLevel, inFile: file) + + for ref in refs { + // Check if this reference is in an API signature role (return type, parameter type, etc.) + guard ref.role.isPubliclyExposable else { continue } + + // Get the parent declaration (the function/property that uses this type in its signature) + guard let parent = ref.parent else { continue } + + // Check references to that parent from the same file + let parentRefs = graph.references(to: parent).filter { $0.location.file == file } + + for parentRef in parentRefs { + guard let refParent = parentRef.parent, + let refTopLevel = topLevelType(of: refParent) + else { + continue + } + + let refLogicalType = logicalType(of: refTopLevel, inFile: file) + + if declLogicalType !== refLogicalType { + return true + } + } + } + + return false + } } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index 6b6e194c6..b4e21dfb0 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -90,3 +90,6 @@ NestedTypeAccessibilityRetainer().retain() InternalSuggestingPrivateVsFileprivateRetainer().retain() ImplicitlyInternalRetainer().retain() NotRedundantInternalClassComponents_Support().useImplicitlyInternalStruct() +InternalTypeAsReturnTypeRetainer().retain() +InternalTypeTransitivelyExposedInSameFileRetainer().retain() +TransitiveAccessExposureRetainer().retain() diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType.swift new file mode 100644 index 000000000..608c2f6e7 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType.swift @@ -0,0 +1,25 @@ +// InternalTypeAsReturnType.swift +// Tests for internal types used as return types of functions called from other files. +// These should NOT be flagged as redundant internal. + +// This internal enum is used as the return type of a function that is called from another file. +// Even though InternalReturnTypeEnum is never directly referenced outside this file, +// it is transitively exposed through the function's return type. +internal enum InternalReturnTypeEnum { + case value +} + +internal class InternalReturnTypeContainer { + func getEnum() -> InternalReturnTypeEnum { + return .value + } +} + +public class InternalTypeAsReturnTypeRetainer { + public init() {} + + public func retain() { + let container = InternalReturnTypeContainer() + _ = container.getEnum() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType_Consumer.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType_Consumer.swift new file mode 100644 index 000000000..728aebddf --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeAsReturnType_Consumer.swift @@ -0,0 +1,11 @@ +// InternalTypeAsReturnType_Consumer.swift +// Consumer that calls the function with internal return type from a different file. +// This creates the transitive exposure of InternalReturnTypeEnum. + +class InternalReturnTypeConsumer { + func consume() { + let container = InternalReturnTypeContainer() + // This call uses InternalReturnTypeEnum transitively through the return type + let _ = container.getEnum() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeTransitivelyExposedInSameFile.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeTransitivelyExposedInSameFile.swift new file mode 100644 index 000000000..0f7c1d645 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/InternalTypeTransitivelyExposedInSameFile.swift @@ -0,0 +1,37 @@ +// InternalTypeTransitivelyExposedInSameFile.swift +// Tests for internal types that are transitively exposed within the same file +// but from a different type. These should suggest fileprivate, not private. + +class TransitiveExposureClassA { + // This enum is only directly referenced within ClassA, but it's the return type + // of getStatus() which IS called from ClassB in the same file. + // It should be suggested as fileprivate (not private) because ClassB uses it transitively. + internal enum TransitivelyExposedStatus { + case active + case inactive + } + + func getStatus() -> TransitivelyExposedStatus { + .active + } +} + +class TransitiveExposureClassB { + func checkStatus() { + let a = TransitiveExposureClassA() + // This call uses TransitivelyExposedStatus transitively through the return type + let _ = a.getStatus() + } +} + +// Retainer that only uses ClassB (not ClassA.getStatus() directly) +// This ensures getStatus() is only referenced from within this file +public class InternalTypeTransitivelyExposedInSameFileRetainer { + public init() {} + + public func retain() { + // Only call checkStatus(), not getStatus() directly + // So getStatus() is only referenced from checkStatus() in this same file + TransitiveExposureClassB().checkStatus() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure.swift new file mode 100644 index 000000000..61dad9c51 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure.swift @@ -0,0 +1,267 @@ +// TransitiveAccessExposure.swift +// Comprehensive test cases for transitive access exposure scenarios. +// Each scenario shows a type that must have sufficient access level because +// it is exposed through another declaration's signature. +// +// For each case: +// - The inner type is marked with the minimum required access level +// - A comment shows what error would occur if we lowered the access level +// +// NOTE: This file focuses on CROSS-FILE exposure scenarios (internal types). +// Same-file scenarios (fileprivate types) are handled in a separate file. + +// MARK: - 1. Function Parameter Type Exposure + +// Scenario 1a: Internal type used as parameter of internal function called from another file +// If ParameterTypeA were fileprivate: +// "Method must be declared fileprivate because its parameter uses a fileprivate type" +internal struct ParameterTypeA { + var value: Int = 0 +} + +class ParameterExposureContainer { + // This function is called from TransitiveAccessExposure_Consumer.swift + func processParameter(_ param: ParameterTypeA) { + _ = param.value + } +} + +// MARK: - 2. Function Return Type Exposure + +// Scenario 2a: Internal type used as return type of function called from another file +// If ReturnTypeA were fileprivate: +// "Function must be declared fileprivate because its result uses a fileprivate type" +internal enum ReturnTypeA { + case success + case failure +} + +class ReturnTypeExposureContainer { + // This function is called from TransitiveAccessExposure_Consumer.swift + func getResult() -> ReturnTypeA { + .success + } +} + +// MARK: - 3. Property Type Exposure + +// Scenario 3a: Internal type used as property type, property accessed from another file +// If PropertyTypeA were fileprivate: +// "Property must be declared fileprivate because its type uses a fileprivate type" +internal struct PropertyTypeA { + var data: String = "" +} + +class PropertyTypeExposureContainer { + // This property is accessed from TransitiveAccessExposure_Consumer.swift + var exposedProperty: PropertyTypeA = PropertyTypeA() +} + +// MARK: - 4. Generic Constraint Exposure + +// Scenario 4a: Internal protocol used as generic constraint, function called from another file +// If GenericConstraintProtocolA were fileprivate: +// "Generic parameter 'T' cannot be declared internal because it uses a fileprivate type in its requirement" +internal protocol GenericConstraintProtocolA { + var identifier: String { get } +} + +internal struct GenericConstraintConformingTypeA: GenericConstraintProtocolA { + var identifier: String = "A" +} + +class GenericConstraintExposureContainer { + // This function is called from TransitiveAccessExposure_Consumer.swift + func processGeneric(_ item: T) -> String { + item.identifier + } +} + +// MARK: - 5. Protocol Requirement Exposure +// Note: Protocol requirements expose types in their signatures + +// Scenario 5a: Internal type used in protocol requirement parameter +// If ProtocolRequirementTypeA were fileprivate: +// "Method in an internal protocol cannot use a fileprivate type" +internal struct ProtocolRequirementTypeA { + var payload: Int = 0 +} + +internal protocol ProtocolWithRequirementA { + func process(input: ProtocolRequirementTypeA) +} + +class ProtocolRequirementExposureContainer: ProtocolWithRequirementA { + func process(input: ProtocolRequirementTypeA) { + _ = input.payload + } +} + +// MARK: - 6. Enum Associated Value Exposure + +// Scenario 6a: Internal type used as enum associated value, enum used from another file +// If EnumAssociatedTypeA were fileprivate: +// "Enum case in an internal enum uses a fileprivate type" +internal struct EnumAssociatedTypeA { + var content: String = "" +} + +internal enum EnumWithAssociatedValueA { + case success(EnumAssociatedTypeA) + case failure(Error) +} + +class EnumAssociatedValueExposureContainer { + // This function is called from TransitiveAccessExposure_Consumer.swift + func getEnumValue() -> EnumWithAssociatedValueA { + .success(EnumAssociatedTypeA(content: "data")) + } +} + +// MARK: - 7. Typealias Exposure + +// Scenario 7a: Internal type aliased by internal typealias, used from another file +// If TypealiasTargetTypeA were fileprivate: +// "Type alias cannot be declared internal because it uses a fileprivate type" +internal struct TypealiasTargetTypeA { + var value: Double = 0.0 +} + +internal typealias AliasedTypeA = TypealiasTargetTypeA + +class TypealiasExposureContainer { + // This property uses the typealias and is accessed from TransitiveAccessExposure_Consumer.swift + var aliasedProperty: AliasedTypeA = AliasedTypeA() +} + +// MARK: - 8. Subscript Exposure + +// Scenario 8a: Internal type used as subscript parameter, subscript accessed from another file +// If SubscriptKeyTypeA were fileprivate: +// "Subscript cannot be declared internal because its parameter uses a fileprivate type" +internal struct SubscriptKeyTypeA: Hashable { + var key: String = "" +} + +class SubscriptExposureContainer { + private var storage: [SubscriptKeyTypeA: Int] = [:] + + // This subscript is accessed from TransitiveAccessExposure_Consumer.swift + subscript(key: SubscriptKeyTypeA) -> Int { + get { storage[key] ?? 0 } + set { storage[key] = newValue } + } +} + +// Scenario 8b: Internal type used as subscript return type +// If SubscriptReturnTypeA were fileprivate: +// "Subscript cannot be declared internal because its element type uses a fileprivate type" +internal struct SubscriptReturnTypeA { + var data: String = "" +} + +class SubscriptReturnTypeExposureContainer { + private var items: [SubscriptReturnTypeA] = [] + + // This subscript is accessed from TransitiveAccessExposure_Consumer.swift + subscript(index: Int) -> SubscriptReturnTypeA { + items.indices.contains(index) ? items[index] : SubscriptReturnTypeA() + } +} + +// MARK: - 9. Default Argument Exposure + +// Scenario 9a: Internal type used in default argument, function called from another file +// If DefaultArgTypeA were fileprivate: +// "Default argument value of internal function uses a fileprivate type" +internal struct DefaultArgTypeA { + var config: String = "default" + static let defaultValue = DefaultArgTypeA() +} + +class DefaultArgumentExposureContainer { + // This function is called from TransitiveAccessExposure_Consumer.swift + func processWithDefault(config: DefaultArgTypeA = DefaultArgTypeA.defaultValue) { + _ = config.config + } +} + +// MARK: - 10. Initializer Parameter Exposure + +// Scenario 10a: Internal type used as initializer parameter, init called from another file +// If InitParamTypeA were fileprivate: +// "Initializer cannot be declared internal because its parameter uses a fileprivate type" +internal struct InitParamTypeA { + var setting: Bool = false +} + +class InitializerExposureContainer { + let config: InitParamTypeA + + // This initializer is called from TransitiveAccessExposure_Consumer.swift + init(config: InitParamTypeA) { + self.config = config + } +} + +// MARK: - 11. Closure Type Exposure + +// Scenario 11a: Internal type used in closure parameter/return, closure accessed from another file +// If ClosureParamTypeA were fileprivate: +// "Property cannot be declared internal because its type uses a fileprivate type" +internal struct ClosureParamTypeA { + var input: Int = 0 +} + +internal struct ClosureReturnTypeA { + var output: Int = 0 +} + +class ClosureTypeExposureContainer { + // This closure property is accessed from TransitiveAccessExposure_Consumer.swift + var transformer: (ClosureParamTypeA) -> ClosureReturnTypeA = { param in + ClosureReturnTypeA(output: param.input * 2) + } +} + +// MARK: - Retainer class to ensure all code is exercised + +public class TransitiveAccessExposureRetainer { + public init() {} + + public func retain() { + // 1. Parameter exposure + _ = ParameterExposureContainer() + + // 2. Return type exposure + _ = ReturnTypeExposureContainer() + + // 3. Property type exposure + _ = PropertyTypeExposureContainer() + + // 4. Generic constraint exposure + _ = GenericConstraintExposureContainer() + + // 5. Protocol requirement exposure + _ = ProtocolRequirementExposureContainer() + + // 6. Enum associated value exposure + _ = EnumAssociatedValueExposureContainer() + + // 7. Typealias exposure + _ = TypealiasExposureContainer() + + // 8. Subscript exposure + _ = SubscriptExposureContainer() + _ = SubscriptReturnTypeExposureContainer() + + // 9. Default argument exposure + _ = DefaultArgumentExposureContainer() + + // 10. Initializer exposure + _ = InitializerExposureContainer(config: InitParamTypeA()) + + // 11. Closure type exposure + _ = ClosureTypeExposureContainer() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure_Consumer.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure_Consumer.swift new file mode 100644 index 000000000..ca2c400f6 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/TransitiveAccessExposure_Consumer.swift @@ -0,0 +1,94 @@ +// TransitiveAccessExposure_Consumer.swift +// Consumer file that references declarations from TransitiveAccessExposure.swift +// This creates cross-file transitive exposure of the inner types. + +class TransitiveAccessExposureConsumer { + // 1. Parameter exposure - calls function with ParameterTypeA parameter + func consumeParameterExposure() { + let container = ParameterExposureContainer() + container.processParameter(ParameterTypeA(value: 42)) + } + + // 2. Return type exposure - calls function returning ReturnTypeA + func consumeReturnTypeExposure() { + let container = ReturnTypeExposureContainer() + let result = container.getResult() + switch result { + case .success: break + case .failure: break + } + } + + // 3. Property type exposure - accesses property of PropertyTypeA type + func consumePropertyTypeExposure() { + let container = PropertyTypeExposureContainer() + _ = container.exposedProperty + } + + // 4. Generic constraint exposure - calls generic function constrained by GenericConstraintProtocolA + func consumeGenericConstraintExposure() { + let container = GenericConstraintExposureContainer() + let item = GenericConstraintConformingTypeA() + _ = container.processGeneric(item) + } + + // 5. Protocol requirement exposure - uses conforming type + func consumeProtocolRequirementExposure() { + let container = ProtocolRequirementExposureContainer() + container.process(input: ProtocolRequirementTypeA(payload: 100)) + } + + // 6. Enum associated value exposure - uses enum with associated type + func consumeEnumAssociatedValueExposure() { + let container = EnumAssociatedValueExposureContainer() + let value = container.getEnumValue() + switch value { + case .success(let data): _ = data.content + case .failure: break + } + } + + // 7. Typealias exposure - uses typealias property + func consumeTypealiasExposure() { + let container = TypealiasExposureContainer() + _ = container.aliasedProperty + } + + // 8a. Subscript key exposure - uses subscript with SubscriptKeyTypeA + func consumeSubscriptKeyExposure() { + var container = SubscriptExposureContainer() + let key = SubscriptKeyTypeA(key: "test") + container[key] = 10 + _ = container[key] + } + + // 8b. Subscript return type exposure - uses subscript returning SubscriptReturnTypeA + func consumeSubscriptReturnTypeExposure() { + let container = SubscriptReturnTypeExposureContainer() + let item: SubscriptReturnTypeA = container[0] + _ = item.data + } + + // 9. Default argument exposure - calls function with default argument + func consumeDefaultArgumentExposure() { + let container = DefaultArgumentExposureContainer() + // Calling with default argument + container.processWithDefault() + // Calling with explicit argument + container.processWithDefault(config: DefaultArgTypeA(config: "custom")) + } + + // 10. Initializer exposure - calls initializer with InitParamTypeA + func consumeInitializerExposure() { + let config = InitParamTypeA(setting: true) + _ = InitializerExposureContainer(config: config) + } + + // 11. Closure type exposure - uses closure with exposed types + func consumeClosureTypeExposure() { + let container = ClosureTypeExposureContainer() + let input = ClosureParamTypeA(input: 5) + let output = container.transformer(input) + _ = output.output + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 0a6393ccb..ed32c419a 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -246,4 +246,92 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { // The protocol requirement implementation should NOT be flagged (line 15 in fixture) assertNotRedundantInternalAccessibility(.functionMethodInstance("someExternalProtocolMethod()", line: 15)) } + + /// Tests that top-level internal types used only within the same file are flagged + /// as redundant internal. + /// + /// For top-level declarations, private and fileprivate are equivalent, so the + /// suggested accessibility is nil (ambiguous). Nested types are suppressed when + /// their parent is already flagged to reduce noise. + func testInternalTypeTransitivelyExposedInSameFileSuggestsFileprivate() { + index() + + // TransitiveExposureClassA is only used within its file (from ClassB), so it + // should be flagged as redundant internal. Since it's top-level, the suggestion + // is nil (private and fileprivate are equivalent for top-level declarations). + assertRedundantInternalAccessibility(.class("TransitiveExposureClassA")) + + // TransitivelyExposedStatus is suppressed because its parent (ClassA) is already + // flagged. This is by design to reduce noise - fixing the parent is sufficient. + } + + // MARK: - Transitive Access Exposure Tests + + // These tests verify that Periphery does NOT incorrectly flag internal types + // that are transitively exposed through API signatures when those APIs are + // called from other files. See TransitiveAccessExposure.swift for fixtures. + + /// Tests that internal types used in function/method signatures are NOT flagged + /// when the function is called from another file. + /// + /// Covers: parameter types, return types, default argument types, initializer parameters, + /// and closure types in properties. + func testTransitiveExposureThroughFunctionSignatures() { + index() + + // Parameter types: ParameterTypeA used in processParameter() + assertNotRedundantInternalAccessibility(.struct("ParameterTypeA")) + + // Return types: ReturnTypeA returned by getResult(), InternalReturnTypeEnum returned by getEnum() + assertNotRedundantInternalAccessibility(.enum("ReturnTypeA")) + assertNotRedundantInternalAccessibility(.enum("InternalReturnTypeEnum")) + + // Default argument types: DefaultArgTypeA used in processWithDefault() + assertNotRedundantInternalAccessibility(.struct("DefaultArgTypeA")) + + // Initializer parameters: InitParamTypeA used in init(config:) + assertNotRedundantInternalAccessibility(.struct("InitParamTypeA")) + + // Closure types: ClosureParamTypeA/ClosureReturnTypeA in transformer property + assertNotRedundantInternalAccessibility(.struct("ClosureParamTypeA")) + assertNotRedundantInternalAccessibility(.struct("ClosureReturnTypeA")) + } + + /// Tests that internal types used in property and subscript signatures are NOT flagged + /// when accessed from another file. + /// + /// Covers: property types, subscript parameter types, subscript return types. + func testTransitiveExposureThroughPropertyAndSubscriptSignatures() { + index() + + // Property types: PropertyTypeA used in exposedProperty + assertNotRedundantInternalAccessibility(.struct("PropertyTypeA")) + + // Subscript parameter types: SubscriptKeyTypeA used in subscript(key:) + assertNotRedundantInternalAccessibility(.struct("SubscriptKeyTypeA")) + + // Subscript return types: SubscriptReturnTypeA returned by subscript + assertNotRedundantInternalAccessibility(.struct("SubscriptReturnTypeA")) + } + + /// Tests that internal types used in generic constraints, protocol requirements, + /// enum associated values, and typealiases are NOT flagged when exposed from another file. + /// + /// Covers: generic constraint protocols, protocol requirement types, enum associated + /// value types, typealias target types. + func testTransitiveExposureThroughTypeSystemConstructs() { + index() + + // Generic constraints: GenericConstraintProtocolA constrains processGeneric() + assertNotRedundantInternalAccessibility(.protocol("GenericConstraintProtocolA")) + + // Protocol requirement types: ProtocolRequirementTypeA in ProtocolWithRequirementA + assertNotRedundantInternalAccessibility(.struct("ProtocolRequirementTypeA")) + + // Enum associated values: EnumAssociatedTypeA in EnumWithAssociatedValueA + assertNotRedundantInternalAccessibility(.struct("EnumAssociatedTypeA")) + + // Typealias targets: TypealiasTargetTypeA aliased by AliasedTypeA + assertNotRedundantInternalAccessibility(.struct("TypealiasTargetTypeA")) + } } From ecaee8711dfea91e8bdf1f82db220a78ef5df6ee Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Sat, 24 Jan 2026 19:12:08 -0800 Subject: [PATCH 23/30] =?UTF-8?q?don=E2=80=99t=20mark=20enum=20cases=20as?= =?UTF-8?q?=20needing=20to=20be=20private;=20fix=20fileprivate=20detection?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Note: periphery ought to identify enum cases that are unused --- ...RedundantInternalAccessibilityMarker.swift | 175 +++++++++++++++--- .../TargetA/EnumCaseAccessibility.swift | 32 ++++ .../StructMemberwiseInitAccessibility.swift | 53 ++++++ .../RedundantInternalAccessibilityTest.swift | 36 ++++ 4 files changed, 270 insertions(+), 26 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/EnumCaseAccessibility.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StructMemberwiseInitAccessibility.swift diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 449d43e98..000a361b3 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -78,15 +78,25 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { } // Determine the suggested accessibility level. - // For top-level declarations, fileprivate is equivalent to private, so we pass nil - // to indicate the ambiguity in the output message. // If the declaration is referenced from different types in the same file, // it needs fileprivate. Otherwise, private is sufficient. // Also check transitive exposure: if the type is used as return/parameter type of a // function called from a different type in the same file, it needs fileprivate. + // Additionally, types used in protocol requirement signatures need fileprivate even + // at top level (private would make them inaccessible from the protocol method). let isTopLevel = decl.parent == nil - let needsFileprivate = isReferencedFromDifferentTypeInSameFile(decl) || isTransitivelyExposedFromDifferentTypeInSameFile(decl) - let suggestedAccessibility: Accessibility? = isTopLevel ? nil : (needsFileprivate ? .fileprivate : .private) + let needsFileprivate = isReferencedFromDifferentTypeInSameFile(decl) || + isTransitivelyExposedFromDifferentTypeInSameFile(decl) || + isUsedInProtocolRequirementSignature(decl) + + // For top-level declarations where private and fileprivate would both work, + // we pass nil to indicate the ambiguity. But if fileprivate is specifically needed + // (e.g., the type is used in a protocol requirement signature), we suggest fileprivate. + let suggestedAccessibility: Accessibility? = if isTopLevel, !needsFileprivate { + nil + } else { + needsFileprivate ? .fileprivate : .private + } // Check if the parent's accessibility already constrains this member. // If the parent is `private`, the member is already effectively `private`. @@ -130,6 +140,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { /// - They should be skipped from all accessibility analysis (generic type params, implicit decls) /// - They are protocol requirements (must maintain accessibility for protocol conformance) /// - They are part of a property wrapper's API (must be accessible to wrapper users) + /// - They are struct stored properties used in an implicit memberwise initializer private func shouldSkipMarking(_ decl: Declaration) -> Bool { if shouldSkipAccessibilityAnalysis(for: decl) { return true @@ -143,6 +154,10 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } + if isStructMemberwiseInitProperty(decl) { + return true + } + return false } @@ -168,6 +183,10 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // Deinitializers cannot have explicit access modifiers in Swift. if decl.kind == .functionDestructor { return true } + // Enum cases cannot have explicit access modifiers in Swift. + // They inherit the accessibility of their containing enum. + if decl.kind == .enumelement { return true } + // Override methods must be at least as accessible as what they override. if decl.isOverride { return true } @@ -270,6 +289,46 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return false } + /// Checks if a declaration is a stored property that's part of a struct's implicit memberwise + /// initializer AND that initializer is used from outside the file. + /// + /// Struct stored properties that are parameters to the implicit memberwise initializer + /// must maintain sufficient accessibility for that initializer to work when called from + /// other files. If the memberwise init is only used within the same file, the properties + /// can still be marked as redundantly internal with a suggestion of fileprivate. + private func isStructMemberwiseInitProperty(_ decl: Declaration) -> Bool { + guard decl.kind == .varInstance, + let parent = decl.parent, + parent.kind == .struct + else { return false } + + // Check if the struct has an implicit memberwise initializer that includes this property. + let implicitInits = parent.declarations.filter { $0.kind == .functionConstructor && $0.isImplicit } + + for implicitInit in implicitInits { + guard let initName = implicitInit.name, + let propertyName = decl.name + else { continue } + + // Parse the init parameter names from the init signature (e.g., "init(foo:bar:)") + let parameterNames = initName + .dropFirst("init(".count) + .dropLast(")".count) + .split(separator: ":") + .map(String.init) + + // Only skip if this property is part of the memberwise init AND + // the init is used from outside the file. + if parameterNames.contains(propertyName), + implicitInit.isReferencedOutsideFile(graph: graph) + { + return true + } + } + + return false + } + /// Determines the effective maximum accessibility a member can have based on its parent's accessibility. /// /// In Swift, a member's effective accessibility is constrained by its parent. This helper @@ -296,24 +355,29 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { /// /// For internal accessibility analysis, this determines whether to suggest `fileprivate` /// versus `private` when a declaration is only used within its file. + /// + /// This uses the **immediate containing type** (not the top-level type) because: + /// - For nested types like `OuterStruct.InnerStruct`, a member of `InnerStruct` that's + /// accessed from code inside `OuterStruct` (but outside `InnerStruct`) needs `fileprivate` + /// - Using top-level type would incorrectly see both as belonging to `OuterStruct` private func isReferencedFromDifferentTypeInSameFile(_ decl: Declaration) -> Bool { let file = decl.location.file let sameFileReferences = graph.references(to: decl).filter { $0.location.file == file } - guard let declTopLevel = topLevelType(of: decl) else { + guard let declContainingType = immediateContainingType(of: decl) else { return false } - let declLogicalType = logicalType(of: declTopLevel, inFile: file) + let declLogicalType = logicalType(of: declContainingType, inFile: file) for ref in sameFileReferences { guard let refParent = ref.parent, - let refTopLevel = topLevelType(of: refParent) + let refContainingType = immediateContainingType(of: refParent) else { continue } - let refLogicalType = logicalType(of: refTopLevel, inFile: file) + let refLogicalType = logicalType(of: refContainingType, inFile: file) if declLogicalType !== refLogicalType { return true @@ -322,22 +386,40 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return false } - // Finds the top-level type declaration by walking up the parent chain. - private func topLevelType(of decl: Declaration) -> Declaration? { + /// Finds the immediate containing type of a declaration. + /// + /// For members (properties, methods, etc.), this returns their containing type. + /// For nested types, this returns the type that contains them (the outer type). + /// For top-level types, this returns the type itself (they are their own container). + private func immediateContainingType(of decl: Declaration) -> Declaration? { let baseTypeKinds: Set = [.class, .struct, .enum, .protocol] let typeKinds = baseTypeKinds.union(Declaration.Kind.extensionKinds) - let ancestors = [decl] + Array(decl.ancestralDeclarations) - return ancestors.last { typeDecl in - guard typeKinds.contains(typeDecl.kind) else { return false } - guard let parent = typeDecl.parent else { return true } - return !typeKinds.contains(parent.kind) + // For types, check if they have a parent type (nested type case). + // If so, return the parent type. If not (top-level), return the type itself. + if typeKinds.contains(decl.kind) { + if let parent = decl.parent, typeKinds.contains(parent.kind) { + return parent + } + return decl } + + // Walk up the parent chain to find the first containing type + var current = decl.parent + while let parent = current { + if typeKinds.contains(parent.kind) { + return parent + } + current = parent.parent + } + + return nil } - // Gets the logical type for comparison purposes when analyzing internal accessibility. - // For extensions of types in the SAME FILE, treats the extension as the extended type. - // For extensions of types in DIFFERENT FILES, treats the extension as its own distinct type. + /// Gets the logical type for comparison purposes when analyzing internal accessibility. + /// + /// For extensions of types in the SAME FILE, treats the extension as the extended type. + /// For extensions of types in DIFFERENT FILES, treats the extension as its own distinct type. private func logicalType(of decl: Declaration, inFile file: SourceFile) -> Declaration? { if decl.kind.isExtensionKind { if let extendedDecl = try? graph.extendedDeclaration(forExtension: decl), @@ -376,10 +458,17 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } - // For properties, also check if the containing type is used from outside the file. - // When code accesses `obj.property`, the property's type is exposed even if - // `property` itself isn't directly referenced from outside. - if parent.kind.isVariableKind { + // For properties, also check if they could be accessed from outside the file + // through their containing type. The property's type is exposed when: + // 1. The property is internal (accessible from outside the file) + // 2. The containing type is used from outside the file + // 3. The property is not actually referenced from outside (already checked above) + // If the property is already referenced from outside, we would have returned + // true at line 472. This check catches cases where the property COULD be + // accessed but hasn't been yet. + if parent.kind.isVariableKind, + parent.accessibility.value == .internal || parent.accessibility.isAccessibleCrossModule + { if let containingType = parent.parent, containingType.isReferencedOutsideFile(graph: graph) { @@ -413,11 +502,11 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let file = decl.location.file let refs = graph.references(to: decl) - guard let declTopLevel = topLevelType(of: decl) else { + guard let declContainingType = immediateContainingType(of: decl) else { return false } - let declLogicalType = logicalType(of: declTopLevel, inFile: file) + let declLogicalType = logicalType(of: declContainingType, inFile: file) for ref in refs { // Check if this reference is in an API signature role (return type, parameter type, etc.) @@ -431,12 +520,12 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { for parentRef in parentRefs { guard let refParent = parentRef.parent, - let refTopLevel = topLevelType(of: refParent) + let refContainingType = immediateContainingType(of: refParent) else { continue } - let refLogicalType = logicalType(of: refTopLevel, inFile: file) + let refLogicalType = logicalType(of: refContainingType, inFile: file) if declLogicalType !== refLogicalType { return true @@ -446,4 +535,38 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return false } + + /// Checks if a type is used in a protocol requirement's signature (return type, parameter type, etc.). + /// + /// When a type is used as the return type or parameter type of a protocol requirement method, + /// the type must be at least `fileprivate` - making it `private` would cause a compiler error + /// because the protocol method's signature would expose an inaccessible type. + /// + /// For example, in NSViewRepresentable: + /// ```swift + /// class FocusableNSView: NSView { ... } // Used as return type of makeNSView + /// + /// struct FocusClaimingView: NSViewRepresentable { + /// func makeNSView(context: Context) -> FocusableNSView { ... } + /// } + /// ``` + /// Here, `FocusableNSView` cannot be `private` because it's exposed through `makeNSView`'s signature. + private func isUsedInProtocolRequirementSignature(_ decl: Declaration) -> Bool { + let refs = graph.references(to: decl) + + for ref in refs { + // Check if this reference is in an API signature role (return type, parameter type, etc.) + guard ref.role.isPubliclyExposable else { continue } + + // Get the parent declaration (the function/property that uses this type in its signature) + guard let parent = ref.parent else { continue } + + // Check if that parent is a protocol requirement + if isProtocolRequirement(parent) { + return true + } + } + + return false + } } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/EnumCaseAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/EnumCaseAccessibility.swift new file mode 100644 index 000000000..588d71a10 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/EnumCaseAccessibility.swift @@ -0,0 +1,32 @@ +/* + EnumCaseAccessibility.swift + Tests that enum cases are NOT flagged as redundant internal. + + Enum cases cannot have explicit access modifiers in Swift - they inherit + the accessibility of their containing enum. Suggesting to make them private + or fileprivate would cause a syntax error. +*/ + +// Internal enum with cases only used in this file - should NOT flag the cases. +internal enum InternalEnumWithCasesUsedOnlyInSameFile { + case usedCase + case anotherUsedCase +} + +// Public enum with internal cases - cases should NOT be flagged. +public enum PublicEnumWithInternalCases { + case internalCase + case anotherInternalCase +} + +// Usage within the file to exercise the enum cases. +public class EnumCaseAccessibilityRetainer { + public init() {} + + public func retain() { + _ = InternalEnumWithCasesUsedOnlyInSameFile.usedCase + _ = InternalEnumWithCasesUsedOnlyInSameFile.anotherUsedCase + _ = PublicEnumWithInternalCases.internalCase + _ = PublicEnumWithInternalCases.anotherInternalCase + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StructMemberwiseInitAccessibility.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StructMemberwiseInitAccessibility.swift new file mode 100644 index 000000000..4d2ed9fe7 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StructMemberwiseInitAccessibility.swift @@ -0,0 +1,53 @@ +/* + StructMemberwiseInitAccessibility.swift + Tests that struct stored properties used in implicit memberwise initializers + are NOT flagged as redundant internal. + + When a struct relies on its implicit memberwise initializer, the stored + properties that are parameters to that initializer are part of the struct's + public API and must maintain their accessibility. +*/ + +// Struct with internal stored properties used in memberwise init. +// These properties should NOT be flagged as redundant internal. +public struct StructWithMemberwiseInit { + internal var memberwiseProperty1: String + internal var memberwiseProperty2: Int + internal let memberwiseConstant: Bool + + // No explicit init - relies on implicit memberwise init +} + +// Struct with mixed properties - some in memberwise init, some not. +public struct StructWithMixedProperties { + internal var memberwiseProperty: String + + // Computed property - not part of memberwise init + internal var computedProperty: String { memberwiseProperty.uppercased() } + + // Property with default value - still part of memberwise init + internal var propertyWithDefault: Int = 42 + + // No explicit init - relies on implicit memberwise init +} + +// Usage within the file to exercise the structs. +public class StructMemberwiseInitAccessibilityRetainer { + public init() {} + + public func retain() { + // Using memberwise initializer + let s1 = StructWithMemberwiseInit( + memberwiseProperty1: "hello", + memberwiseProperty2: 42, + memberwiseConstant: true + ) + _ = s1 + + let s2 = StructWithMixedProperties( + memberwiseProperty: "world", + propertyWithDefault: 100 + ) + _ = s2.computedProperty + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index ed32c419a..a7c163d85 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -334,4 +334,40 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { // Typealias targets: TypealiasTargetTypeA aliased by AliasedTypeA assertNotRedundantInternalAccessibility(.struct("TypealiasTargetTypeA")) } + + // MARK: - Enum Case Tests + + /// Tests that enum cases are NOT flagged as redundant internal. + /// + /// Enum cases cannot have explicit access modifiers in Swift - they always + /// inherit the accessibility of their containing enum. Suggesting to make + /// them private or fileprivate would cause a syntax error. + func testEnumCasesNotFlagged() { + index() + + // Enum cases should never be flagged + assertNotRedundantInternalAccessibility(.enumelement("usedCase")) + assertNotRedundantInternalAccessibility(.enumelement("anotherUsedCase")) + assertNotRedundantInternalAccessibility(.enumelement("internalCase")) + assertNotRedundantInternalAccessibility(.enumelement("anotherInternalCase")) + } + + // MARK: - Struct Memberwise Initializer Tests + + /// Tests that struct stored properties used in implicit memberwise initializers + /// are NOT flagged as redundant internal. + /// + /// Stored properties that are parameters to a struct's memberwise initializer + /// are part of the struct's public API. Even if they're only directly accessed + /// within the same file, they must remain accessible for the initializer. + func testStructMemberwiseInitPropertiesNotFlagged() { + index() + + // Properties used in memberwise init should not be flagged + assertNotRedundantInternalAccessibility(.varInstance("memberwiseProperty1")) + assertNotRedundantInternalAccessibility(.varInstance("memberwiseProperty2")) + assertNotRedundantInternalAccessibility(.varInstance("memberwiseConstant")) + assertNotRedundantInternalAccessibility(.varInstance("memberwiseProperty")) + assertNotRedundantInternalAccessibility(.varInstance("propertyWithDefault")) + } } From cc11547d5632868c2d8612da172154d0fae0b582 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Sat, 24 Jan 2026 19:29:49 -0800 Subject: [PATCH 24/30] Fix checking against an external protocol --- ...RedundantInternalAccessibilityMarker.swift | 58 +++++++++++++++++++ .../ExternalProtocolSignatureType.swift | 38 ++++++++++++ .../RedundantInternalAccessibilityTest.swift | 17 ++++++ 3 files changed, 113 insertions(+) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolSignatureType.swift diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 000a361b3..bf1414ce0 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -158,6 +158,10 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } + if isUsedInExternalProtocolRequirementSignature(decl) { + return true + } + return false } @@ -569,4 +573,58 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return false } + + /// Checks if a type is used in the signature of a method that conforms to an external protocol. + /// + /// Types used in external protocol requirement signatures must remain `internal` because: + /// 1. The method implementing the protocol requirement can't be more restrictive than the protocol + /// 2. The types used in its signature must be at least as accessible as the method + /// + /// For example, with NSViewRepresentable: + /// ```swift + /// class FocusableNSView: NSView { ... } // Must stay internal! + /// + /// struct FocusClaimingView: NSViewRepresentable { + /// func makeNSView(context: Context) -> FocusableNSView { ... } // Protocol requirement + /// func updateNSView(_ nsView: FocusableNSView, context: Context) { ... } // Protocol requirement + /// } + /// ``` + /// Here, `FocusableNSView` cannot be made `fileprivate` or `private` because the protocol + /// methods that use it must remain at the protocol's required accessibility level. + private func isUsedInExternalProtocolRequirementSignature(_ decl: Declaration) -> Bool { + let refs = graph.references(to: decl) + + for ref in refs { + // Check if this reference is in an API signature role (return type, parameter type, etc.) + guard ref.role.isPubliclyExposable else { continue } + + // Get the parent declaration (the function/property that uses this type in its signature) + guard let parent = ref.parent else { continue } + + // Check if that parent conforms to an external protocol requirement + if isExternalProtocolRequirement(parent) { + return true + } + } + + return false + } + + /// Checks if a declaration implements an external protocol requirement. + /// + /// External protocols are those defined outside our codebase (e.g., NSViewRepresentable, + /// Codable, etc.). When a declaration implements such a protocol, its accessibility is + /// constrained by the protocol. + private func isExternalProtocolRequirement(_ decl: Declaration) -> Bool { + // Check for .related references FROM this declaration to protocol members + // where the protocol is external (not in our graph). + for ref in decl.related where ref.declarationKind.isProtocolMemberConformingKind { + // If we can't find the declaration in our graph, it's external + if graph.declaration(withUsr: ref.usr) == nil, ref.name == decl.name { + return true + } + } + + return false + } } diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolSignatureType.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolSignatureType.swift new file mode 100644 index 000000000..f6214d34d --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/ExternalProtocolSignatureType.swift @@ -0,0 +1,38 @@ +/* + ExternalProtocolSignatureType.swift + Tests that types used in external protocol requirement signatures are NOT + flagged as redundant internal. + + When a type is used as the return type or parameter type of a method that + implements an external protocol requirement (like NSViewRepresentable.makeNSView), + the type must remain internal because the protocol method can't be more + restrictive than the protocol requires. + + This mimics patterns like: + - NSViewRepresentable/UIViewRepresentable returning custom NSView/UIView subclasses + - Codable types with custom coding containers +*/ + +import Foundation + +// This type is used as the return type of an external protocol requirement. +// It should NOT be flagged as redundant internal because Equatable.== must +// remain internal, and its parameter types must be at least as accessible. +internal struct TypeUsedInExternalProtocolSignature: Equatable { + internal var value: Int + + // The == function is an external protocol requirement (from Equatable). + // Since this struct conforms to Equatable, the == method's parameter type + // (this struct) must remain internal. +} + +// Usage to retain the type +public class ExternalProtocolSignatureTypeRetainer { + public init() {} + + public func retain() { + let a = TypeUsedInExternalProtocolSignature(value: 1) + let b = TypeUsedInExternalProtocolSignature(value: 2) + _ = a == b + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index a7c163d85..a8cf790f1 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -370,4 +370,21 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.varInstance("memberwiseProperty")) assertNotRedundantInternalAccessibility(.varInstance("propertyWithDefault")) } + + // MARK: - External Protocol Requirement Signature Tests + + /// Tests that types used in external protocol requirement signatures are NOT flagged. + /// + /// When a type is used as a return type or parameter type of a method that implements + /// an external protocol (like Equatable, NSViewRepresentable, etc.), the type must + /// remain internal because the protocol method's accessibility is constrained by the + /// protocol, and its signature types must be at least as accessible. + func testTypeUsedInExternalProtocolSignatureNotFlagged() { + index() + + // TypeUsedInExternalProtocolSignature conforms to Equatable, which means + // it's used as the parameter type for the == operator (an external protocol + // requirement). It should NOT be flagged as redundant internal. + assertNotRedundantInternalAccessibility(.struct("TypeUsedInExternalProtocolSignature")) + } } From c368910da0591feb237afb418afb39f15643610a Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Sat, 24 Jan 2026 20:57:12 -0800 Subject: [PATCH 25/30] stored property type transitive exposure --- ...RedundantInternalAccessibilityMarker.swift | 74 ++++++++++++++++--- .../TargetA/StoredPropertyTypeExposure.swift | 71 ++++++++++++++++++ .../StoredPropertyTypeExposure_Consumer.swift | 24 ++++++ .../RedundantInternalAccessibilityTest.swift | 41 ++++++++++ 4 files changed, 201 insertions(+), 9 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure_Consumer.swift diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index bf1414ce0..284cb0f45 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -447,7 +447,20 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { /// For example, if `ScanProgress` is the return type of `runFullScanWithStreaming()`, and /// that function is called from another file, then `ScanProgress` is transitively exposed /// and should not be marked as redundantly internal. + /// + /// This check is recursive: if TypeA is used as a property type in Container, and Container + /// is used as a property type in OuterContainer, and OuterContainer is referenced from + /// outside the file, then TypeA is transitively exposed through the chain. private func isTransitivelyExposedOutsideFile(_ decl: Declaration) -> Bool { + var visited: Set = [] + return isTransitivelyExposedOutsideFileRecursive(decl, visited: &visited) + } + + private func isTransitivelyExposedOutsideFileRecursive(_ decl: Declaration, visited: inout Set) -> Bool { + let id = ObjectIdentifier(decl) + guard !visited.contains(id) else { return false } + visited.insert(id) + let refs = graph.references(to: decl) for ref in refs { @@ -465,18 +478,20 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // For properties, also check if they could be accessed from outside the file // through their containing type. The property's type is exposed when: // 1. The property is internal (accessible from outside the file) - // 2. The containing type is used from outside the file + // 2. The containing type is used from outside the file OR transitively exposed // 3. The property is not actually referenced from outside (already checked above) - // If the property is already referenced from outside, we would have returned - // true at line 472. This check catches cases where the property COULD be - // accessed but hasn't been yet. if parent.kind.isVariableKind, parent.accessibility.value == .internal || parent.accessibility.isAccessibleCrossModule { - if let containingType = parent.parent, - containingType.isReferencedOutsideFile(graph: graph) - { - return true + if let containingType = parent.parent { + // Direct reference from outside the file + if containingType.isReferencedOutsideFile(graph: graph) { + return true + } + // Recursive: the containing type itself is transitively exposed + if isTransitivelyExposedOutsideFileRecursive(containingType, visited: &visited) { + return true + } } } } @@ -502,15 +517,39 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { /// } /// ``` /// Here, `Status` should be suggested as `fileprivate`, not `private`. + /// + /// This check is recursive: if TypeA is used as a property type in Container, and Container + /// is used as a property type in another type that is accessed from a different type in + /// the same file, then TypeA needs fileprivate. private func isTransitivelyExposedFromDifferentTypeInSameFile(_ decl: Declaration) -> Bool { let file = decl.location.file - let refs = graph.references(to: decl) guard let declContainingType = immediateContainingType(of: decl) else { return false } let declLogicalType = logicalType(of: declContainingType, inFile: file) + var visited: Set = [] + + return isTransitivelyExposedFromDifferentTypeInSameFileRecursive( + decl, + declLogicalType: declLogicalType, + file: file, + visited: &visited + ) + } + + private func isTransitivelyExposedFromDifferentTypeInSameFileRecursive( + _ decl: Declaration, + declLogicalType: Declaration?, + file: SourceFile, + visited: inout Set + ) -> Bool { + let id = ObjectIdentifier(decl) + guard !visited.contains(id) else { return false } + visited.insert(id) + + let refs = graph.references(to: decl) for ref in refs { // Check if this reference is in an API signature role (return type, parameter type, etc.) @@ -535,6 +574,23 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } } + + // For properties, also check if the containing type is transitively exposed + // from a different type in the same file + if parent.kind.isVariableKind, + parent.accessibility.value == .internal || parent.accessibility.isAccessibleCrossModule + { + if let containingType = parent.parent { + if isTransitivelyExposedFromDifferentTypeInSameFileRecursive( + containingType, + declLogicalType: declLogicalType, + file: file, + visited: &visited + ) { + return true + } + } + } } return false diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure.swift new file mode 100644 index 000000000..4b8add0e7 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure.swift @@ -0,0 +1,71 @@ +// StoredPropertyTypeExposure.swift +// Test cases for types used as stored property types that are transitively exposed. +// +// When a type T is used as a property type in a struct/class C, and C is instantiated +// from another file, T is transitively exposed and should NOT be flagged as redundant internal. + +// MARK: - Simple Property Type Exposure + +// StoredPropertyRole is used as the property type in StoredPropertyContainer. +// StoredPropertyContainer is instantiated from StoredPropertyTypeExposure_Consumer.swift. +// Therefore StoredPropertyRole is transitively exposed and should NOT be flagged. +internal enum StoredPropertyRole { + case primary + case secondary +} + +internal struct StoredPropertyContainer { + let role: StoredPropertyRole +} + +// MARK: - Nested Type as Property Type + +// NestedPhase is a nested enum used as a property type in its containing class. +// ClassWithNestedType is instantiated from StoredPropertyTypeExposure_Consumer.swift. +// Therefore NestedPhase is transitively exposed and should NOT be flagged. +internal class ClassWithNestedType { + internal enum NestedPhase { + case idle + case running + case completed + } + + var phase: NestedPhase = .idle + + func advance() { + switch phase { + case .idle: phase = .running + case .running: phase = .completed + case .completed: break + } + } +} + +// MARK: - Chained Property Type Exposure + +// InnerType is used in MiddleContainer, which is used in OuterContainer. +// OuterContainer is instantiated from StoredPropertyTypeExposure_Consumer.swift. +// Therefore both InnerType and MiddleContainer are transitively exposed. +internal struct InnerType { + var value: Int = 0 +} + +internal struct MiddleContainer { + var inner: InnerType +} + +internal struct OuterContainer { + var middle: MiddleContainer +} + +// MARK: - Retainer class to ensure all code is exercised + +public class StoredPropertyTypeExposureRetainer { + public init() {} + + public func use() { + _ = StoredPropertyContainer(role: .primary) + _ = ClassWithNestedType() + _ = OuterContainer(middle: MiddleContainer(inner: InnerType(value: 42))) + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure_Consumer.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure_Consumer.swift new file mode 100644 index 000000000..9b18f4920 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/StoredPropertyTypeExposure_Consumer.swift @@ -0,0 +1,24 @@ +// StoredPropertyTypeExposure_Consumer.swift +// Consumer file that references types from StoredPropertyTypeExposure.swift, +// creating cross-file transitive exposure of the property types. + +class StoredPropertyTypeExposureConsumer { + // Uses StoredPropertyContainer, which transitively exposes StoredPropertyRole + func consumeSimplePropertyType() { + let container = StoredPropertyContainer(role: .primary) + _ = container.role + } + + // Uses ClassWithNestedType, which transitively exposes NestedPhase + func consumeNestedType() { + let obj = ClassWithNestedType() + obj.advance() + _ = obj.phase + } + + // Uses OuterContainer, which transitively exposes MiddleContainer and InnerType + func consumeChainedPropertyTypes() { + let outer = OuterContainer(middle: MiddleContainer(inner: InnerType(value: 100))) + _ = outer.middle.inner.value + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index a8cf790f1..10f967bd9 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -387,4 +387,45 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { // requirement). It should NOT be flagged as redundant internal. assertNotRedundantInternalAccessibility(.struct("TypeUsedInExternalProtocolSignature")) } + + // MARK: - Stored Property Type Transitive Exposure Tests + + /// Tests that types used as stored property types are NOT flagged when the containing + /// type is instantiated from another file. + /// + /// When a type T is used as a property type in struct/class C, and C is used from + /// outside the file, T is transitively exposed and must remain internal. + func testStoredPropertyTypeNotFlagged() { + index() + + // StoredPropertyRole is used as the type of StoredPropertyContainer.role + // StoredPropertyContainer is instantiated from StoredPropertyTypeExposure_Consumer.swift + assertNotRedundantInternalAccessibility(.enum("StoredPropertyRole")) + assertNotRedundantInternalAccessibility(.struct("StoredPropertyContainer")) + } + + /// Tests that nested types used as property types are NOT flagged when the containing + /// type is instantiated from another file. + func testNestedTypeAsPropertyTypeNotFlagged() { + index() + + // NestedPhase is used as the type of ClassWithNestedType.phase + // ClassWithNestedType is instantiated from StoredPropertyTypeExposure_Consumer.swift + assertNotRedundantInternalAccessibility(.enum("NestedPhase")) + assertNotRedundantInternalAccessibility(.class("ClassWithNestedType")) + } + + /// Tests that types in a chain of property types are NOT flagged when the outermost + /// type is instantiated from another file. + /// + /// This tests the recursive transitive exposure check: InnerType -> MiddleContainer -> OuterContainer + func testChainedPropertyTypeExposureNotFlagged() { + index() + + // InnerType is used in MiddleContainer, which is used in OuterContainer + // OuterContainer is instantiated from StoredPropertyTypeExposure_Consumer.swift + assertNotRedundantInternalAccessibility(.struct("InnerType")) + assertNotRedundantInternalAccessibility(.struct("MiddleContainer")) + assertNotRedundantInternalAccessibility(.struct("OuterContainer")) + } } From b13d41a6899f17ff832d4b44c89364976e3536e1 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Sun, 25 Jan 2026 19:18:17 -0800 Subject: [PATCH 26/30] Fix more problems with same file --- ...RedundantInternalAccessibilityMarker.swift | 118 ++++++++++++++++-- .../TargetA/SameFileMemberwiseInit.swift | 20 +++ .../SameFileMemberwiseInit_Consumer.swift | 6 + .../TargetA/SameFileTypeConstraint.swift | 59 +++++++++ .../SameFileTypeConstraint_Consumer.swift | 15 +++ .../RedundantInternalAccessibilityTest.swift | 60 +++++++++ 6 files changed, 266 insertions(+), 12 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit_Consumer.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint_Consumer.swift diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 284cb0f45..9078aa62d 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -162,6 +162,11 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } + // Check if type is constrained by same-file type usage + if isConstrainedBySameFileTypeUsage(decl) { + return true + } + return false } @@ -294,19 +299,14 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { } /// Checks if a declaration is a stored property that's part of a struct's implicit memberwise - /// initializer AND that initializer is used from outside the file. - /// - /// Struct stored properties that are parameters to the implicit memberwise initializer - /// must maintain sufficient accessibility for that initializer to work when called from - /// other files. If the memberwise init is only used within the same file, the properties - /// can still be marked as redundantly internal with a suggestion of fileprivate. + /// initializer AND that initializer is used (either from outside the file OR from within + /// the same file when the struct must remain internal). private func isStructMemberwiseInitProperty(_ decl: Declaration) -> Bool { guard decl.kind == .varInstance, let parent = decl.parent, parent.kind == .struct else { return false } - // Check if the struct has an implicit memberwise initializer that includes this property. let implicitInits = parent.declarations.filter { $0.kind == .functionConstructor && $0.isImplicit } for implicitInit in implicitInits { @@ -314,18 +314,110 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let propertyName = decl.name else { continue } - // Parse the init parameter names from the init signature (e.g., "init(foo:bar:)") let parameterNames = initName .dropFirst("init(".count) .dropLast(")".count) .split(separator: ":") .map(String.init) - // Only skip if this property is part of the memberwise init AND - // the init is used from outside the file. - if parameterNames.contains(propertyName), - implicitInit.isReferencedOutsideFile(graph: graph) + guard parameterNames.contains(propertyName) else { continue } + + // Case 1: Init referenced outside file -> properties must stay internal + if implicitInit.isReferencedOutsideFile(graph: graph) { + return true + } + + // Case 2: Init referenced in same file AND struct must remain internal + // (because it's used outside file or transitively exposed) + let hasAnyReference = !graph.references(to: implicitInit).isEmpty + if hasAnyReference { + if parent.isReferencedOutsideFile(graph: graph) { + return true + } + if isTransitivelyExposedOutsideFile(parent) { + return true + } + } + } + + return false + } + + /// Checks if a type is constrained by being used in the signature of another + /// internal declaration (in the same file) that must remain internal. + /// + /// In Swift, types used in a declaration's signature must be at least as accessible + /// as that declaration. If TypeA is used as a property type, return type, parameter + /// type, or generic constraint in TypeB/MemberB, and TypeB must remain internal + /// (because it's referenced outside the file or transitively exposed), then TypeA + /// cannot be made fileprivate/private. + /// + /// This complements isTransitivelyExposedOutsideFile() which handles cross-file + /// scenarios. This function handles same-file scenarios where the constraint chain + /// exists entirely within one file. + private func isConstrainedBySameFileTypeUsage(_ decl: Declaration) -> Bool { + let typeKinds: Set = [.enum, .struct, .class, .protocol] + guard typeKinds.contains(decl.kind) else { return false } + + var visited: Set = [] + return isConstrainedBySameFileTypeUsageRecursive(decl, visited: &visited) + } + + private func isConstrainedBySameFileTypeUsageRecursive( + _ decl: Declaration, + visited: inout Set + ) -> Bool { + let id = ObjectIdentifier(decl) + guard !visited.contains(id) else { return false } + + visited.insert(id) + + let typeKinds: Set = [.enum, .struct, .class, .protocol] + let file = decl.location.file + let refs = graph.references(to: decl) + + for ref in refs { + // Check if this reference is in ANY publicly exposable role + // (property type, return type, parameter type, generic constraint, etc.) + guard ref.role.isPubliclyExposable else { continue } + + // Must be in the same file (cross-file is handled by isTransitivelyExposedOutsideFile) + guard ref.location.file == file else { continue } + + // Get the declaration that uses this type in its signature + guard let usingDecl = ref.parent else { continue } + + // Find the containing type of that declaration + let containingType: Declaration? + if typeKinds.contains(usingDecl.kind) || usingDecl.kind.isExtensionKind { + // The using declaration IS a type (e.g., conformedType, inheritedType) + containingType = usingDecl + } else if let parent = usingDecl.parent, + typeKinds.contains(parent.kind) || parent.kind.isExtensionKind { + // The using declaration is a member of a type + containingType = parent + } else { + continue + } + + guard let containingType else { continue } + + // Check if the containing type must remain internal + guard containingType.accessibility.value == .internal else { continue } + + // If the containing type is referenced outside the file, our type is constrained + if containingType.isReferencedOutsideFile(graph: graph) { + return true + } + + // If the containing type is transitively exposed outside the file + if isTransitivelyExposedOutsideFile(containingType) { + return true + } + + // Recursive: if the containing type is itself constrained + if isConstrainedBySameFileTypeUsageRecursive(containingType, visited: &visited) { return true } } @@ -459,6 +551,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { private func isTransitivelyExposedOutsideFileRecursive(_ decl: Declaration, visited: inout Set) -> Bool { let id = ObjectIdentifier(decl) guard !visited.contains(id) else { return false } + visited.insert(id) let refs = graph.references(to: decl) @@ -547,6 +640,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { ) -> Bool { let id = ObjectIdentifier(decl) guard !visited.contains(id) else { return false } + visited.insert(id) let refs = graph.references(to: decl) diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit.swift new file mode 100644 index 000000000..bd2107cf2 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit.swift @@ -0,0 +1,20 @@ +// Tests that struct memberwise init properties are NOT flagged when the init +// is used within the same file AND the struct is part of a type hierarchy +// that must remain internal. + +internal struct SameFileMemberwiseStruct { + let field1: String + let field2: Int +} + +internal struct SameFileOuterStruct { + let inner: SameFileMemberwiseStruct +} + +public class SameFileMemberwiseInitRetainer { + public init() {} + public func use() { + let inner = SameFileMemberwiseStruct(field1: "test", field2: 42) + _ = SameFileOuterStruct(inner: inner) + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit_Consumer.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit_Consumer.swift new file mode 100644 index 000000000..44b018e85 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileMemberwiseInit_Consumer.swift @@ -0,0 +1,6 @@ +class SameFileMemberwiseInitConsumer { + func consume() { + let inner = SameFileMemberwiseStruct(field1: "test", field2: 42) + _ = SameFileOuterStruct(inner: inner) + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint.swift new file mode 100644 index 000000000..edd864004 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint.swift @@ -0,0 +1,59 @@ +// Tests that types used in signatures of other internal types are NOT flagged +// when the containing type must remain internal. + +// MARK: - Property Type Constraint + +internal enum SameFileConstrainedEnum { + case one + case two +} + +internal struct SameFileConstrainingStruct { + let enumValue: SameFileConstrainedEnum +} + +// MARK: - Return Type Constraint + +internal struct SameFileReturnType { + var value: Int = 0 +} + +internal class SameFileClassWithReturnType { + func getReturnType() -> SameFileReturnType { + SameFileReturnType(value: 42) + } +} + +// MARK: - Parameter Type Constraint + +internal struct SameFileParamType { + var data: String = "" +} + +internal class SameFileClassWithParamType { + func process(_ param: SameFileParamType) { + _ = param.data + } +} + +// MARK: - Generic Constraint + +internal protocol SameFileConstraintProtocol { + var id: String { get } +} + +internal class SameFileClassWithGenericConstraint { + func process(_ item: T) -> String { + item.id + } +} + +public class SameFileTypeConstraintRetainer { + public init() {} + public func use() { + _ = SameFileConstrainingStruct(enumValue: .one) + _ = SameFileClassWithReturnType() + _ = SameFileClassWithParamType() + _ = SameFileClassWithGenericConstraint() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint_Consumer.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint_Consumer.swift new file mode 100644 index 000000000..b09551f67 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/SameFileTypeConstraint_Consumer.swift @@ -0,0 +1,15 @@ +class SameFileTypeConstraintConsumer { + func consumePropertyType() { + _ = SameFileConstrainingStruct(enumValue: .one) + } + + func consumeReturnType() { + let obj = SameFileClassWithReturnType() + _ = obj.getReturnType() + } + + func consumeParamType() { + let obj = SameFileClassWithParamType() + obj.process(SameFileParamType(data: "test")) + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 10f967bd9..5cf6d6712 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -428,4 +428,64 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.struct("MiddleContainer")) assertNotRedundantInternalAccessibility(.struct("OuterContainer")) } + + // MARK: - Same-File Type Constraint Tests + + /// Tests that types used as property types in internal types are NOT flagged + /// when the containing type must remain internal. + func testSameFilePropertyTypeConstraintNotFlagged() { + index() + + // SameFileConstrainedEnum is used as property type in SameFileConstrainingStruct + // SameFileConstrainingStruct is used from another file + assertNotRedundantInternalAccessibility(.enum("SameFileConstrainedEnum")) + assertNotRedundantInternalAccessibility(.struct("SameFileConstrainingStruct")) + } + + /// Tests that types used as return types are NOT flagged when the containing + /// type must remain internal. + func testSameFileReturnTypeConstraintNotFlagged() { + index() + + // SameFileReturnType is returned by SameFileClassWithReturnType.getReturnType() + // SameFileClassWithReturnType is used from another file + assertNotRedundantInternalAccessibility(.struct("SameFileReturnType")) + assertNotRedundantInternalAccessibility(.class("SameFileClassWithReturnType")) + } + + /// Tests that types used as parameter types are NOT flagged when the containing + /// type must remain internal. + func testSameFileParamTypeConstraintNotFlagged() { + index() + + // SameFileParamType is a parameter to SameFileClassWithParamType.process() + // SameFileClassWithParamType is used from another file + assertNotRedundantInternalAccessibility(.struct("SameFileParamType")) + assertNotRedundantInternalAccessibility(.class("SameFileClassWithParamType")) + } + + /// Tests that protocols used as generic constraints are NOT flagged when the + /// containing type must remain internal. + func testSameFileGenericConstraintNotFlagged() { + index() + + // SameFileConstraintProtocol is used as generic constraint in SameFileClassWithGenericConstraint + // SameFileClassWithGenericConstraint is used from another file + assertNotRedundantInternalAccessibility(.protocol("SameFileConstraintProtocol")) + assertNotRedundantInternalAccessibility(.class("SameFileClassWithGenericConstraint")) + } + + // MARK: - Same-File Memberwise Init Tests + + /// Tests that struct memberwise init properties are NOT flagged when the struct + /// is instantiated in the same file AND the struct must remain internal due to + /// being used as a property type in another struct that's used from outside. + func testSameFileMemberwiseInitPropertiesNotFlagged() { + index() + + // SameFileMemberwiseStruct is used as property type in SameFileOuterStruct + // SameFileOuterStruct is used from another file + assertNotRedundantInternalAccessibility(.struct("SameFileMemberwiseStruct")) + assertNotRedundantInternalAccessibility(.struct("SameFileOuterStruct")) + } } From c5f28ee4e7a0ab603a7a2c66c1b6f97b5d0d2aa3 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Mon, 26 Jan 2026 14:13:15 -0800 Subject: [PATCH 27/30] fix warning we found for newly introduced upstream property --- Sources/Frontend/Commands/ScanCommand.swift | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Sources/Frontend/Commands/ScanCommand.swift b/Sources/Frontend/Commands/ScanCommand.swift index 601334721..e7f77169e 100644 --- a/Sources/Frontend/Commands/ScanCommand.swift +++ b/Sources/Frontend/Commands/ScanCommand.swift @@ -76,7 +76,7 @@ struct ScanCommand: ParsableCommand { private var disableUnusedImportAnalysis: Bool = defaultConfiguration.$disableUnusedImportAnalysis.defaultValue @Flag(inversion: .prefixedNo, help: "Report superfluous ignore comments") - var superfluousIgnoreComments: Bool = defaultConfiguration.$superfluousIgnoreComments.defaultValue + private var superfluousIgnoreComments: Bool = defaultConfiguration.$superfluousIgnoreComments.defaultValue @Option(parsing: .upToNextOption, help: "Names of unused imported modules to retain") private var retainUnusedImportedModules: [String] = defaultConfiguration.$retainUnusedImportedModules.defaultValue From 5c0a85297c096c1edf6b66ade614117d8600e656 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 29 Jan 2026 10:40:31 -0800 Subject: [PATCH 28/30] =?UTF-8?q?Don=E2=80=99t=20mark=20#Preview=20blocks?= =?UTF-8?q?=20as=20fileprivate?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...RedundantInternalAccessibilityMarker.swift | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 9078aa62d..eafc3c8b4 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -141,6 +141,7 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { /// - They are protocol requirements (must maintain accessibility for protocol conformance) /// - They are part of a property wrapper's API (must be accessible to wrapper users) /// - They are struct stored properties used in an implicit memberwise initializer + /// - They are referenced by a `#Preview` macro expansion (when retainSwiftUIPreviews is enabled) private func shouldSkipMarking(_ decl: Declaration) -> Bool { if shouldSkipAccessibilityAnalysis(for: decl) { return true @@ -167,6 +168,11 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } + // Skip declarations referenced by #Preview macro expansions + if isReferencedByPreviewMacro(decl) { + return true + } + return false } @@ -425,6 +431,26 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return false } + /// Checks if a declaration is referenced by a `#Preview` macro expansion. + /// + /// When `retainSwiftUIPreviews` is enabled, `#Preview` macros are retained. However, the + /// implicit declarations generated by these macros cannot have access modifiers. Types + /// referenced only by `#Preview` must remain `internal` — making them `fileprivate` or + /// `private` would break the preview because the macro expansion needs to access them. + private func isReferencedByPreviewMacro(_ decl: Declaration) -> Bool { + guard configuration.retainSwiftUIPreviews else { return false } + let previewRegistryUsr = "s:21DeveloperToolsSupport15PreviewRegistryP" + for ref in graph.references(to: decl) { + guard let parent = ref.parent, parent.isImplicit else { continue } + // Check if this implicit parent references PreviewRegistry, + // which identifies it as a #Preview macro expansion. + if parent.references.contains(where: { $0.usr == previewRegistryUsr }) { + return true + } + } + return false + } + /// Determines the effective maximum accessibility a member can have based on its parent's accessibility. /// /// In Swift, a member's effective accessibility is constrained by its parent. This helper From 98d125ef5fd91ed84e6ed6f39cf69d09bdc4dc03 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 29 Jan 2026 13:07:33 -0800 Subject: [PATCH 29/30] For when retainSwiftUIPreviews, fix the detection of #Preview macro --- .../Mutators/RedundantInternalAccessibilityMarker.swift | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index eafc3c8b4..838aad79c 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -439,12 +439,15 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { /// `private` would break the preview because the macro expansion needs to access them. private func isReferencedByPreviewMacro(_ decl: Declaration) -> Bool { guard configuration.retainSwiftUIPreviews else { return false } + let previewRegistryUsr = "s:21DeveloperToolsSupport15PreviewRegistryP" + for ref in graph.references(to: decl) { guard let parent = ref.parent, parent.isImplicit else { continue } - // Check if this implicit parent references PreviewRegistry, + + // Check if this implicit parent has a related reference to PreviewRegistry, // which identifies it as a #Preview macro expansion. - if parent.references.contains(where: { $0.usr == previewRegistryUsr }) { + if parent.related.contains(where: { $0.usr == previewRegistryUsr }) { return true } } From 8fe4d35c3138ab8d23ad5ea2ff66b21a0fe948c0 Mon Sep 17 00:00:00 2001 From: Dan Wood <207080+danwood@users.noreply.github.com> Date: Thu, 29 Jan 2026 14:02:34 -0800 Subject: [PATCH 30/30] solve some obscure false positive corner cases --- ...undantFilePrivateAccessibilityMarker.swift | 8 ++-- ...RedundantInternalAccessibilityMarker.swift | 45 +++++++++++++++---- .../Sources/MainTarget/main.swift | 2 + ...emberwiseInitCalledFromDifferentType.swift | 27 +++++++++++ .../MethodCalledFromFreeFunction.swift | 29 ++++++++++++ .../RedundantInternalAccessibilityTest.swift | 34 ++++++++++++++ 6 files changed, 132 insertions(+), 13 deletions(-) create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MemberwiseInitCalledFromDifferentType.swift create mode 100644 Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MethodCalledFromFreeFunction.swift diff --git a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift index fa118fb46..aac807fa0 100644 --- a/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantFilePrivateAccessibilityMarker.swift @@ -168,10 +168,10 @@ final class RedundantFilePrivateAccessibilityMarker: SourceGraphMutator { let declLogicalType = logicalType(of: declTopLevel, inFile: file) for ref in sameFileReferences { - guard let refParent = ref.parent, - let refTopLevel = topLevelType(of: refParent) - else { - continue + guard let refParent = ref.parent else { continue } + guard let refTopLevel = topLevelType(of: refParent) else { + // Reference from a free function or top-level code — no containing type. + return true } let refLogicalType = logicalType(of: refTopLevel, inFile: file) diff --git a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift index 838aad79c..942441bc1 100644 --- a/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift +++ b/Sources/SourceGraph/Mutators/RedundantInternalAccessibilityMarker.swift @@ -335,7 +335,8 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { // Case 2: Init referenced in same file AND struct must remain internal // (because it's used outside file or transitively exposed) - let hasAnyReference = !graph.references(to: implicitInit).isEmpty + let initReferences = graph.references(to: implicitInit) + let hasAnyReference = !initReferences.isEmpty if hasAnyReference { if parent.isReferencedOutsideFile(graph: graph) { return true @@ -344,6 +345,31 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { return true } } + + // Case 3: Init referenced from a different type (or free function) in the same file. + // The properties need at least fileprivate access, so skip marking them as private. + let file = parent.location.file + let sameFileInitRefs = initReferences.filter { $0.location.file == file } + for ref in sameFileInitRefs { + guard let refParent = ref.parent else { continue } + guard let refContainingType = immediateContainingType(of: refParent) else { + // Called from a free function or top-level code + return true + } + + let parentLogicalType = logicalType(of: parent, inFile: file) + let refLogicalType = logicalType(of: refContainingType, inFile: file) + if parentLogicalType !== refLogicalType { + return true + } + } + + // Case 4: Init is referenced AND the parent struct is referenced by a #Preview macro. + // Preview macro expansions generate implicit types that cannot have access modifiers, + // so the properties must remain accessible. + if hasAnyReference, isReferencedByPreviewMacro(parent) { + return true + } } return false @@ -496,10 +522,11 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let declLogicalType = logicalType(of: declContainingType, inFile: file) for ref in sameFileReferences { - guard let refParent = ref.parent, - let refContainingType = immediateContainingType(of: refParent) - else { - continue + guard let refParent = ref.parent else { continue } + guard let refContainingType = immediateContainingType(of: refParent) else { + // Reference from a free function or top-level code — no containing type. + // This is effectively a different type, so fileprivate is needed. + return true } let refLogicalType = logicalType(of: refContainingType, inFile: file) @@ -685,10 +712,10 @@ final class RedundantInternalAccessibilityMarker: SourceGraphMutator { let parentRefs = graph.references(to: parent).filter { $0.location.file == file } for parentRef in parentRefs { - guard let refParent = parentRef.parent, - let refContainingType = immediateContainingType(of: refParent) - else { - continue + guard let refParent = parentRef.parent else { continue } + guard let refContainingType = immediateContainingType(of: refParent) else { + // Reference from a free function or top-level code — no containing type. + return true } let refLogicalType = logicalType(of: refContainingType, inFile: file) diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift index b4e21dfb0..00e4b6e49 100644 --- a/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/MainTarget/main.swift @@ -93,3 +93,5 @@ NotRedundantInternalClassComponents_Support().useImplicitlyInternalStruct() InternalTypeAsReturnTypeRetainer().retain() InternalTypeTransitivelyExposedInSameFileRetainer().retain() TransitiveAccessExposureRetainer().retain() +MethodCalledFromFreeFunctionRetainer().retain() +MemberwiseInitCalledFromDifferentTypeRetainer().retain() diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MemberwiseInitCalledFromDifferentType.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MemberwiseInitCalledFromDifferentType.swift new file mode 100644 index 000000000..bf3d6af88 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MemberwiseInitCalledFromDifferentType.swift @@ -0,0 +1,27 @@ +/* + MemberwiseInitCalledFromDifferentType.swift + Tests that struct properties are NOT flagged as redundant internal when the + struct's implicit memberwise initializer is called from a different type + in the same file. + + When a ViewModifier or another type creates a struct using its memberwise init, + the properties need at least fileprivate access and should not be flagged. +*/ + +struct MemberwiseInitStruct { + var crossTypeProperty1: String + var crossTypeProperty2: Int +} + +class MemberwiseInitCaller { + func create() -> MemberwiseInitStruct { + MemberwiseInitStruct(crossTypeProperty1: "hello", crossTypeProperty2: 42) + } +} + +public class MemberwiseInitCalledFromDifferentTypeRetainer { + public init() {} + public func retain() { + _ = MemberwiseInitCaller().create() + } +} diff --git a/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MethodCalledFromFreeFunction.swift b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MethodCalledFromFreeFunction.swift new file mode 100644 index 000000000..0244c1fb4 --- /dev/null +++ b/Tests/AccessibilityTests/AccessibilityProject/Sources/TargetA/MethodCalledFromFreeFunction.swift @@ -0,0 +1,29 @@ +/* + MethodCalledFromFreeFunction.swift + Tests that type members are NOT flagged as private when they are called + from a free function in the same file. + + A free function has no containing type, so accessing a type's member from + a free function requires at least fileprivate access, not private. +*/ + +public class ClassWithMethodCalledFromFreeFunction { + public init() {} + + func methodCalledFromFreeFunction() -> String { "result" } + var propertyUsedFromFreeFunction: Int = 0 +} + +func freeFunctionCallingMethod() { + let obj = ClassWithMethodCalledFromFreeFunction() + _ = obj.methodCalledFromFreeFunction() + _ = obj.propertyUsedFromFreeFunction +} + +// Retainer to ensure the free function is used +public class MethodCalledFromFreeFunctionRetainer { + public init() {} + public func retain() { + freeFunctionCallingMethod() + } +} diff --git a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift index 5cf6d6712..ef16222c1 100644 --- a/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift +++ b/Tests/AccessibilityTests/RedundantInternalAccessibilityTest.swift @@ -475,6 +475,40 @@ final class RedundantInternalAccessibilityTest: SPMSourceGraphTestCase { assertNotRedundantInternalAccessibility(.class("SameFileClassWithGenericConstraint")) } + // MARK: - Memberwise Init Called From Different Type Tests + + /// Tests that struct properties are NOT flagged as redundant internal when + /// the struct's memberwise initializer is called from a different type in + /// the same file (e.g., a ViewModifier creating a struct). + func testMemberwiseInitCalledFromDifferentTypeNotFlagged() { + index() + + assertNotRedundantInternalAccessibility(.varInstance("crossTypeProperty1")) + assertNotRedundantInternalAccessibility(.varInstance("crossTypeProperty2")) + } + + // MARK: - Method Called From Free Function Tests + + /// Tests that type members are NOT flagged as private when called from + /// a free function in the same file. + /// + /// Free functions have no containing type, so members accessed from them + /// need at least fileprivate, not private. + func testMethodCalledFromFreeFunctionNotFlaggedAsPrivate() { + index() + + // Members called from a free function should suggest fileprivate, not private, + // because a free function has no containing type. + assertRedundantInternalAccessibility( + .functionMethodInstance("methodCalledFromFreeFunction()"), + suggestedAccessibility: .fileprivate + ) + assertRedundantInternalAccessibility( + .varInstance("propertyUsedFromFreeFunction"), + suggestedAccessibility: .fileprivate + ) + } + // MARK: - Same-File Memberwise Init Tests /// Tests that struct memberwise init properties are NOT flagged when the struct