From a57b01824a2fd88354b58d42bba3545a98eead08 Mon Sep 17 00:00:00 2001 From: Thomas Catterall Date: Thu, 18 Dec 2025 13:41:57 -0500 Subject: [PATCH] Fix false positives for SPM packages using custom path: argument MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Problem: The tool produced false positives for SPM packages that use custom path: arguments in their target definitions. When Sources/TargetName didn't exist, it fell back to scanning ALL of Sources/ and attributed every file to that target. Example: A package with: .target(name: "MyModule", path: "Sources/Core") .target(name: "MyModuleUI", path: "Sources/UI") Would look for Sources/MyModule, not find it, fall back to Sources/, scan all files, and report UI code as violations for MyModule. Solution: 1. Added `path: String?` property to SwiftPackageTarget to store custom paths from target definitions 2. Updated PackageSwiftFileVisitor to parse the path: argument from SPM target function calls using SwiftSyntax 3. Modified PackagesParser to: - Use the explicit path from Package.swift when available - Normalize paths (strip leading/trailing slashes) to prevent double-slash issues - Throw `customPathNotFound` error if custom path doesn't exist (prevents silent false positives from scanning zero files) - Only fall back to convention (Sources/TargetName) when no custom path is specified Tests: - Added CustomPathPackage fixture with path: "Sources/Core" for target - Added Sources/UI/ with undeclared import to verify files outside custom path are NOT scanned (negative test case) - Added testTarget with custom path (CustomTests/) to verify .testTarget() works correctly with path: argument - Added InvalidCustomPathPackage fixture to test customPathNotFound error 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude --- .../SPMParsing/PackageSwiftFileVisitor.swift | 7 ++- Sources/SPMParsing/PackagesParser.swift | 28 ++++++++- Sources/SPMParsing/SwiftPackageTarget.swift | 1 + .../DiagramBuilder/DiagramBuilderTests.swift | 9 ++- .../CustomTests/TestFile.swift | 2 + .../Sources/Core/CoreFile.swift | 2 + .../CustomPathPackage/Sources/UI/UIFile.swift | 2 + .../Example/CustomPathPackage/Package.swift | 21 +++++++ .../InvalidCustomPathPackage/Package.swift | 16 +++++ .../SPMParsing/PackagesParserTests.swift | 59 +++++++++++++++++++ 10 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/CustomTests/TestFile.swift create mode 100644 Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/Core/CoreFile.swift create mode 100644 Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/UI/UIFile.swift create mode 100644 Tests/SwiftImportChecksTests/Example/CustomPathPackage/Package.swift create mode 100644 Tests/SwiftImportChecksTests/Example/InvalidCustomPathPackage/Package.swift diff --git a/Sources/SPMParsing/PackageSwiftFileVisitor.swift b/Sources/SPMParsing/PackageSwiftFileVisitor.swift index 08ebdef..ef68250 100644 --- a/Sources/SPMParsing/PackageSwiftFileVisitor.swift +++ b/Sources/SPMParsing/PackageSwiftFileVisitor.swift @@ -55,6 +55,10 @@ final class PackageSwiftFileVisitor: SyntaxVisitor { targetType = .regular } + let pathArgument = targetCall.argumentList.first(where: { $0.label?.text == "path" })? + .expression.as(StringLiteralExprSyntax.self)?.segments.description + .trimmingCharacters(in: .punctuationCharacters) + let dependenciesArray = targetCall.argumentList.first(where: { $0.label?.text == "dependencies" })?.expression.as(ArrayExprSyntax.self)?.elements let dependencies = dependenciesArray?.compactMap { element -> String? in if let stringLiteral = element.expression.as(StringLiteralExprSyntax.self) { @@ -76,7 +80,8 @@ final class PackageSwiftFileVisitor: SyntaxVisitor { type: targetType, dependencies: dependenciesSet, duplicateDependencies: findDuplicateDependencies(dependencies), - layerNumber: layers[targetName] + layerNumber: layers[targetName], + path: pathArgument ) targets.append(target) } diff --git a/Sources/SPMParsing/PackagesParser.swift b/Sources/SPMParsing/PackagesParser.swift index fd9ad0d..ed2be27 100644 --- a/Sources/SPMParsing/PackagesParser.swift +++ b/Sources/SPMParsing/PackagesParser.swift @@ -45,9 +45,28 @@ final class PackagesParser { guard target.duplicateDependencies.isEmpty else { throw PackagesParser.Error.duplicateDependencies(targetName: target.name, dependencies: target.duplicateDependencies) } - var swiftFilesPath = path + "/" + package.name + target.type.intermediatePath + target.name - if !FileManager.default.fileExists(atPath: swiftFilesPath) { - swiftFilesPath = path + "/" + package.name + target.type.intermediatePath + var swiftFilesPath: String + if let customPath = target.path { + // Normalize path: strip leading/trailing slashes to avoid double-slash issues + let normalizedPath = customPath + .trimmingCharacters(in: CharacterSet(charactersIn: "/")) + // Use explicit path from Package.swift + swiftFilesPath = path + "/" + package.name + "/" + normalizedPath + // Error if custom path doesn't exist - don't silently scan nothing + guard FileManager.default.fileExists(atPath: swiftFilesPath) else { + throw PackagesParser.Error.customPathNotFound( + targetName: target.name, + path: customPath, + resolvedPath: swiftFilesPath + ) + } + } else { + // Fall back to convention: Sources/TargetName or Tests/TargetName + swiftFilesPath = path + "/" + package.name + target.type.intermediatePath + target.name + // Only fall back to parent directory if convention path doesn't exist + if !FileManager.default.fileExists(atPath: swiftFilesPath) { + swiftFilesPath = path + "/" + package.name + target.type.intermediatePath + } } let swiftFilesParser = SwiftFilesParser( rootURL: URL(fileURLWithPath: swiftFilesPath), @@ -95,6 +114,7 @@ extension PackagesParser { enum Error: Swift.Error, CustomStringConvertible, Equatable { case failedToParsePackage(path: String) case duplicateDependencies(targetName: String, dependencies: [String]) + case customPathNotFound(targetName: String, path: String, resolvedPath: String) var description: String { switch self { @@ -102,6 +122,8 @@ extension PackagesParser { "Failed to parse Package.swift at path: \(path)" case .duplicateDependencies(let targetName, let dependencies): "❌ Target \(targetName) has duplicate dependencies: \(dependencies.joined(separator: ", "))" + case .customPathNotFound(let targetName, let path, let resolvedPath): + "❌ Target \(targetName) specifies path: \"\(path)\" but directory not found at: \(resolvedPath)" } } } diff --git a/Sources/SPMParsing/SwiftPackageTarget.swift b/Sources/SPMParsing/SwiftPackageTarget.swift index 6f43e04..1ab0e3c 100644 --- a/Sources/SPMParsing/SwiftPackageTarget.swift +++ b/Sources/SPMParsing/SwiftPackageTarget.swift @@ -11,4 +11,5 @@ struct SwiftPackageTarget: Equatable { let dependencies: Set let duplicateDependencies: [String] let layerNumber: Int? + let path: String? } diff --git a/Tests/SwiftImportChecksTests/DiagramBuilder/DiagramBuilderTests.swift b/Tests/SwiftImportChecksTests/DiagramBuilder/DiagramBuilderTests.swift index 91d403c..d482dd7 100644 --- a/Tests/SwiftImportChecksTests/DiagramBuilder/DiagramBuilderTests.swift +++ b/Tests/SwiftImportChecksTests/DiagramBuilder/DiagramBuilderTests.swift @@ -137,7 +137,8 @@ struct DiagramBuilderTests { type: .test, dependencies: [], duplicateDependencies: [], - layerNumber: nil + layerNumber: nil, + path: nil ) ] ) @@ -173,7 +174,8 @@ struct DiagramBuilderTests { type: .regular, dependencies: [], duplicateDependencies: [], - layerNumber: nil + layerNumber: nil, + path: nil ) ] ) @@ -212,7 +214,8 @@ struct DiagramBuilderTests { type: .regular, dependencies: [], duplicateDependencies: [], - layerNumber: 0 + layerNumber: 0, + path: nil ) ] ) diff --git a/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/CustomTests/TestFile.swift b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/CustomTests/TestFile.swift new file mode 100644 index 0000000..d07392e --- /dev/null +++ b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/CustomTests/TestFile.swift @@ -0,0 +1,2 @@ +import Foundation +import XCTest diff --git a/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/Core/CoreFile.swift b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/Core/CoreFile.swift new file mode 100644 index 0000000..17e29c6 --- /dev/null +++ b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/Core/CoreFile.swift @@ -0,0 +1,2 @@ +import Foundation +import CoreDependency diff --git a/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/UI/UIFile.swift b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/UI/UIFile.swift new file mode 100644 index 0000000..964a316 --- /dev/null +++ b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/CustomPathPackage/Sources/UI/UIFile.swift @@ -0,0 +1,2 @@ +import Foundation +import UndeclaredDependency diff --git a/Tests/SwiftImportChecksTests/Example/CustomPathPackage/Package.swift b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/Package.swift new file mode 100644 index 0000000..c1060ce --- /dev/null +++ b/Tests/SwiftImportChecksTests/Example/CustomPathPackage/Package.swift @@ -0,0 +1,21 @@ +// swift-tools-version: 6.0 +// The swift-tools-version declares the minimum version of Swift required to build this package. + +import PackageDescription + +let package = Package( + name: "CustomPathPackage", + dependencies: [], + targets: [ + .target( + name: "TestModule", + dependencies: ["CoreDependency"], + path: "Sources/Core" + ), + .testTarget( + name: "TestModuleTests", + dependencies: ["XCTest"], + path: "CustomTests" + ) + ] +) diff --git a/Tests/SwiftImportChecksTests/Example/InvalidCustomPathPackage/Package.swift b/Tests/SwiftImportChecksTests/Example/InvalidCustomPathPackage/Package.swift new file mode 100644 index 0000000..bae6245 --- /dev/null +++ b/Tests/SwiftImportChecksTests/Example/InvalidCustomPathPackage/Package.swift @@ -0,0 +1,16 @@ +// swift-tools-version: 6.0 +// The swift-tools-version declares the minimum version of Swift required to build this package. + +import PackageDescription + +let package = Package( + name: "InvalidPathPackage", + dependencies: [], + targets: [ + .target( + name: "MissingModule", + dependencies: [], + path: "Sources/DoesNotExist" + ) + ] +) diff --git a/Tests/SwiftImportChecksTests/SPMParsing/PackagesParserTests.swift b/Tests/SwiftImportChecksTests/SPMParsing/PackagesParserTests.swift index b820772..792866f 100644 --- a/Tests/SwiftImportChecksTests/SPMParsing/PackagesParserTests.swift +++ b/Tests/SwiftImportChecksTests/SPMParsing/PackagesParserTests.swift @@ -144,6 +144,63 @@ struct PackagesParserTests { ) #expect(messages == expectedMessages) } + + @Test("test parsePackages given valid path with custom path argument uses correct target path") + func parsePackagesGivenValidPathWithCustomPathArgument() throws { + // Given + // This test verifies that: + // 1. Files in Sources/Core are scanned for TestModule (has CoreDependency import) + // 2. Files in Sources/UI are NOT scanned (has UndeclaredDependency import that would fail) + // 3. Files in CustomTests are scanned for TestModuleTests (testTarget with custom path) + let path: String = URL.Mock.customPathPackageFileDir.relativePath + var messages: [String] = [] + let expectedMessages: [String] = [ + "Package: CustomPathPackage Target: TestModule - Type: regular", + "✅ All imports for target TestModule are explicit", + "Package: CustomPathPackage Target: TestModuleTests - Type: test", + "✅ All imports for target TestModuleTests are explicit" + ] + let sut = makeSUT(path: path) + + // When + try sut.parsePackages( + configs: configs, + verbose: verbose, + print: { messages.append($0) } + ) + + // Then + // If Sources/UI was incorrectly scanned, this would fail with UndeclaredDependency error + #expect(messages == expectedMessages) + } + + @Test("test parsePackages given custom path that does not exist throws error") + func parsePackagesGivenCustomPathNotFoundThrowsError() throws { + // Given + let path: String = URL.Mock.invalidCustomPathPackageFileDir.relativePath + var messages: [String] = [] + let expectedMessages: [String] = [ + "Package: InvalidPathPackage Target: MissingModule - Type: regular" + ] + let sut = makeSUT(path: path) + + // When, Then + #expect( + throws: PackagesParser.Error.customPathNotFound( + targetName: "MissingModule", + path: "Sources/DoesNotExist", + resolvedPath: path + "/InvalidPathPackage/Sources/DoesNotExist" + ), + performing: { + try sut.parsePackages( + configs: configs, + verbose: verbose, + print: { messages.append($0) } + ) + } + ) + #expect(messages == expectedMessages) + } } extension PackagesParserTests { @@ -162,5 +219,7 @@ private extension URL { static let secondPackageFileDir = Bundle.module.url(forResource: "Example/SecondPackage/Package", withExtension: "swift")!.deletingLastPathComponent() static let duplicatesPackageFileDir = Bundle.module.url(forResource: "Example/DuplicatesPackage/Package", withExtension: "swift")!.deletingLastPathComponent() static let failurePackageFileDir = Bundle.module.url(forResource: "Example/FailurePackage/Package", withExtension: "swift")!.deletingLastPathComponent() + static let customPathPackageFileDir = Bundle.module.url(forResource: "Example/CustomPathPackage/Package", withExtension: "swift")!.deletingLastPathComponent() + static let invalidCustomPathPackageFileDir = Bundle.module.url(forResource: "Example/InvalidCustomPathPackage/Package", withExtension: "swift")!.deletingLastPathComponent() } }