diff --git a/Fixtures/Miscellaneous/ImportOfMissingDependency/Package.swift b/Fixtures/Miscellaneous/ImportOfMissingDependency/Package.swift new file mode 100644 index 00000000000..fcf79d6a61c --- /dev/null +++ b/Fixtures/Miscellaneous/ImportOfMissingDependency/Package.swift @@ -0,0 +1,20 @@ +// swift-tools-version:5.1 +import PackageDescription + +let package = Package( + name: "VerificationTestPackage", + products: [ + .executable(name: "BExec", targets: ["B"]), + ], + dependencies: [ + + ], + targets: [ + .target( + name: "A", + dependencies: []), + .target( + name: "B", + dependencies: []), + ] +) diff --git a/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/A/A.swift b/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/A/A.swift new file mode 100644 index 00000000000..992a085263c --- /dev/null +++ b/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/A/A.swift @@ -0,0 +1,4 @@ +import B +public func bar(x: Int) -> Int { + return 11 + foo(x: x) +} diff --git a/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/B.swift b/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/B.swift new file mode 100644 index 00000000000..4da1a2e51cd --- /dev/null +++ b/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/B.swift @@ -0,0 +1,3 @@ +public func foo(x: Int) -> Int { + return 11 + x +} diff --git a/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/main.swift b/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/main.swift new file mode 100644 index 00000000000..5364a821f33 --- /dev/null +++ b/Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/main.swift @@ -0,0 +1 @@ +print(baz(x: 11)) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index 04e25731509..366a5bd6839 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -25,6 +25,8 @@ import class TSCUtility.MultiLineNinjaProgressAnimation import class TSCUtility.NinjaProgressAnimation import protocol TSCUtility.ProgressAnimationProtocol +@_implementationOnly import SwiftDriver + public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildSystem, BuildErrorAdviceProvider { /// The delegate used by the build system. @@ -156,12 +158,80 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS buildSystem?.cancel() } + // Emit a warning if a target imports another target in this build + // without specifying it as a dependency in the manifest + private func verifyTargetImports(in description: BuildDescription) throws { + let checkingMode = description.explicitTargetDependencyImportCheckingMode + guard checkingMode != .none else { + return + } + // Ensure the compiler supports the import-scan operation + guard SwiftTargetBuildDescription.checkSupportedFrontendFlags(flags: ["import-prescan"], fileSystem: localFileSystem) else { + return + } + + for (target, commandLine) in description.swiftTargetScanArgs { + do { + guard let dependencies = description.targetDependencyMap[target] else { + // Skip target if no dependency information is present + continue + } + let targetDependenciesSet = Set(dependencies) + guard !description.generatedSourceTargetSet.contains(target), + targetDependenciesSet.intersection(description.generatedSourceTargetSet).isEmpty else { + // Skip targets which contain, or depend-on-targets, with generated source-code. + // Such as test discovery targets and targets with plugins. + continue + } + let resolver = try ArgsResolver(fileSystem: localFileSystem) + let executor = SPMSwiftDriverExecutor(resolver: resolver, + fileSystem: localFileSystem, + env: ProcessEnv.vars) + + let consumeDiagnostics: DiagnosticsEngine = DiagnosticsEngine(handlers: []) + var driver = try Driver(args: commandLine, + diagnosticsEngine: consumeDiagnostics, + fileSystem: localFileSystem, + executor: executor) + guard !consumeDiagnostics.hasErrors else { + // If we could not init the driver with this command, something went wrong, + // proceed without checking this target. + continue + } + let imports = try driver.performImportPrescan().imports + let nonDependencyTargetsSet = + Set(description.targetDependencyMap.keys.filter { !targetDependenciesSet.contains($0) }) + let importedTargetsMissingDependency = Set(imports).intersection(nonDependencyTargetsSet) + if let missedDependency = importedTargetsMissingDependency.first { + switch checkingMode { + case .error: + self.observabilityScope.emit(error: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.") + case .warn: + self.observabilityScope.emit(warning: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.") + case .none: + fatalError("Explicit import checking is disabled.") + } + } + } catch { + // The above verification is a best-effort attempt to warn the user about a potential manifest + // error. If something went wrong during the import-prescan, proceed silently. + return + } + } + } + /// Perform a build using the given build description and subset. public func build(subset: BuildSubset) throws { + let buildStartTime = DispatchTime.now() // Get the build description (either a cached one or newly created). - let buildDescription = try self.getBuildDescription() + + // Get the build description + let buildDescription = try getBuildDescription() + + // Verify dependency imports on the described targers + try verifyTargetImports(in: buildDescription) // Create the build system. let buildSystem = try self.createBuildSystem(buildDescription: buildDescription) diff --git a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift index 1a9d74580ab..3baf352c665 100644 --- a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift +++ b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift @@ -98,7 +98,6 @@ final class TestDiscoveryCommand: CustomLLBuildCommand { let className = iterator.key stream <<< indent(8) <<< "testCase(\(className).__allTests__\(className)),\n" } - stream <<< """ ] } @@ -213,6 +212,7 @@ private final class InProcessTool: Tool { public struct BuildDescription: Codable { public typealias CommandName = String public typealias TargetName = String + public typealias CommandLineFlag = String /// The Swift compiler invocation targets. let swiftCommands: [BuildManifest.CmdName : SwiftCompilerTool] @@ -226,6 +226,19 @@ public struct BuildDescription: Codable { /// The map of copy commands. let copyCommands: [BuildManifest.CmdName: LLBuildManifest.CopyTool] + /// A flag that inidcates this build should perform a check for whether targets only import + /// their explicitly-declared dependencies + let explicitTargetDependencyImportCheckingMode: BuildParameters.TargetDependencyImportCheckingMode + + /// Every target's set of dependencies. + let targetDependencyMap: [TargetName: [TargetName]] + + /// A full swift driver command-line invocation used to dependency-scan a given Swift target + let swiftTargetScanArgs: [TargetName: [CommandLineFlag]] + + /// A set of all targets with generated source + let generatedSourceTargetSet: Set + /// The built test products. public let builtTestProducts: [BuiltTestProduct] @@ -244,6 +257,28 @@ public struct BuildDescription: Codable { self.swiftFrontendCommands = swiftFrontendCommands self.testDiscoveryCommands = testDiscoveryCommands self.copyCommands = copyCommands + self.explicitTargetDependencyImportCheckingMode = plan.buildParameters.explicitTargetDependencyImportCheckingMode + self.targetDependencyMap = try plan.targets.reduce(into: [TargetName: [TargetName]]()) { + let deps = try $1.target.recursiveTargetDependencies().map { $0.c99name } + $0[$1.target.c99name] = deps + } + var targetCommandLines: [TargetName: [CommandLineFlag]] = [:] + var generatedSourceTargets: [TargetName] = [] + for (target, description) in plan.targetMap { + guard case .swift(let desc) = description else { + continue + } + targetCommandLines[target.c99name] = + try desc.emitCommandLine(scanInvocation: true) + ["-driver-use-frontend-path", + plan.buildParameters.toolchain.swiftCompilerPath.pathString] + if desc.isTestDiscoveryTarget { + generatedSourceTargets.append(target.c99name) + } + } + generatedSourceTargets.append(contentsOf: plan.graph.allTargets.filter {$0.type == .plugin} + .map { $0.c99name }) + self.swiftTargetScanArgs = targetCommandLines + self.generatedSourceTargetSet = Set(generatedSourceTargets) self.builtTestProducts = plan.buildProducts.filter{ $0.product.type == .test }.map { desc in return BuiltTestProduct( productName: desc.product.name, diff --git a/Sources/Build/BuildPlan.swift b/Sources/Build/BuildPlan.swift index 7b54240881f..c05da3e5c0f 100644 --- a/Sources/Build/BuildPlan.swift +++ b/Sources/Build/BuildPlan.swift @@ -889,23 +889,27 @@ public final class SwiftTargetBuildDescription { return args } - public func emitCommandLine() throws -> [String] { + /// When `scanInvocation` argument is set to `true`, omit the side-effect producing arguments + /// such as emitting a module or supplementary outputs. + public func emitCommandLine(scanInvocation: Bool = false) throws -> [String] { var result: [String] = [] result.append(buildParameters.toolchain.swiftCompilerPath.pathString) result.append("-module-name") result.append(target.c99name) - result.append("-emit-dependencies") + if !scanInvocation { + result.append("-emit-dependencies") - // FIXME: Do we always have a module? - result.append("-emit-module") - result.append("-emit-module-path") - result.append(moduleOutputPath.pathString) + // FIXME: Do we always have a module? + result.append("-emit-module") + result.append("-emit-module-path") + result.append(moduleOutputPath.pathString) - result.append("-output-file-map") - // FIXME: Eliminate side effect. - result.append(try writeOutputFileMap().pathString) + result.append("-output-file-map") + // FIXME: Eliminate side effect. + result.append(try writeOutputFileMap().pathString) + } if buildParameters.useWholeModuleOptimization { result.append("-whole-module-optimization") diff --git a/Sources/Commands/Options.swift b/Sources/Commands/Options.swift index 32bd201ff89..b2c116dab22 100644 --- a/Sources/Commands/Options.swift +++ b/Sources/Commands/Options.swift @@ -335,6 +335,11 @@ struct BuildOptions: ParsableArguments { @Flag() var useIntegratedSwiftDriver: Bool = false + /// A flag that inidcates this build should check whether targets only import + /// their explicitly-declared dependencies + @Option() + var explicitTargetDependencyImportCheck: TargetDependencyImportCheckingMode = .none + /// Whether to use the explicit module build flow (with the integrated driver) @Flag(name: .customLong("experimental-explicit-module-build")) var useExplicitModuleBuild: Bool = false @@ -370,6 +375,12 @@ struct BuildOptions: ParsableArguments { case native case xcode } + + enum TargetDependencyImportCheckingMode : String, Codable, ExpressibleByArgument { + case none + case warn + case error + } } struct LinkerOptions: ParsableArguments { diff --git a/Sources/Commands/SwiftTool.swift b/Sources/Commands/SwiftTool.swift index 51cc9bdc6dc..cdf56bb3ca7 100644 --- a/Sources/Commands/SwiftTool.swift +++ b/Sources/Commands/SwiftTool.swift @@ -795,6 +795,7 @@ public class SwiftTool { useExplicitModuleBuild: options.build.useExplicitModuleBuild, isXcodeBuildSystemEnabled: options.build.buildSystem == .xcode, forceTestDiscovery: options.build.enableTestDiscovery, // backwards compatibility, remove with --enable-test-discovery + explicitTargetDependencyImportCheckingMode: options.build.explicitTargetDependencyImportCheck.modeParameter, linkerDeadStrip: options.linker.linkerDeadStrip, verboseOutput: self.logLevel <= .info ) @@ -1187,6 +1188,19 @@ extension BuildOptions.StoreMode { } } +extension BuildOptions.TargetDependencyImportCheckingMode { + fileprivate var modeParameter: BuildParameters.TargetDependencyImportCheckingMode { + switch self { + case .none: + return .none + case .warn: + return .warn + case .error: + return .error + } + } +} + extension Basics.Diagnostic.Severity { fileprivate var isVerbose: Bool { return self <= .info diff --git a/Sources/SPMBuildCore/BuildParameters.swift b/Sources/SPMBuildCore/BuildParameters.swift index d8c2c939190..295734fa666 100644 --- a/Sources/SPMBuildCore/BuildParameters.swift +++ b/Sources/SPMBuildCore/BuildParameters.swift @@ -72,6 +72,13 @@ public struct BuildParameters: Encodable { } } + /// A mode for explicit import checking + public enum TargetDependencyImportCheckingMode : Codable { + case none + case warn + case error + } + /// The path to the data directory. public var dataPath: AbsolutePath @@ -128,6 +135,10 @@ public struct BuildParameters: Encodable { /// Whether to use the explicit module build flow (with the integrated driver) public var useExplicitModuleBuild: Bool + /// A flag that inidcates this build should check whether targets only import + /// their explicitly-declared dependencies + public var explicitTargetDependencyImportCheckingMode: TargetDependencyImportCheckingMode + /// Whether to create dylibs for dynamic library products. public var shouldCreateDylibForDynamicProducts: Bool @@ -199,6 +210,7 @@ public struct BuildParameters: Encodable { isXcodeBuildSystemEnabled: Bool = false, enableTestability: Bool? = nil, forceTestDiscovery: Bool = false, + explicitTargetDependencyImportCheckingMode: TargetDependencyImportCheckingMode = .none, linkerDeadStrip: Bool = true, colorizedOutput: Bool = false, verboseOutput: Bool = false @@ -236,6 +248,7 @@ public struct BuildParameters: Encodable { self.enableTestability = enableTestability ?? (.debug == configuration) // decide if to enable the use of test manifests based on platform. this is likely to change in the future self.testDiscoveryStrategy = triple.isDarwin() ? .objectiveC : .manifest(generate: forceTestDiscovery) + self.explicitTargetDependencyImportCheckingMode = explicitTargetDependencyImportCheckingMode self.linkerDeadStrip = linkerDeadStrip self.colorizedOutput = colorizedOutput self.verboseOutput = verboseOutput diff --git a/Tests/CommandsTests/BuildToolTests.swift b/Tests/CommandsTests/BuildToolTests.swift index 44ebda93203..f6f4c2e6831 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -75,6 +75,49 @@ final class BuildToolTests: CommandsTestCase { } } + func testImportOfMissedDepWarning() throws { + #if swift(<5.5) + try XCTSkipIf(true, "skipping because host compiler doesn't support '-import-prescan'") + #endif + // Verify the warning flow + try fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in + let fullPath = resolveSymlinks(path) + XCTAssertThrowsError(try build(["--explicit-target-dependency-import-check=warn"], packagePath: fullPath)) { error in + guard case SwiftPMProductError.executionFailure(_, _, let stderr) = error else { + XCTFail() + return + } + + XCTAssertTrue(stderr.contains("warning: Target A imports another target (B) in the package without declaring it a dependency.")) + } + } + + // Verify the error flow + try fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in + let fullPath = resolveSymlinks(path) + XCTAssertThrowsError(try build(["--explicit-target-dependency-import-check=error"], packagePath: fullPath)) { error in + guard case SwiftPMProductError.executionFailure(_, _, let stderr) = error else { + XCTFail() + return + } + + XCTAssertTrue(stderr.contains("error: Target A imports another target (B) in the package without declaring it a dependency.")) + } + } + + // Verify that the default does not run the check + try fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in + let fullPath = resolveSymlinks(path) + XCTAssertThrowsError(try build([], packagePath: fullPath)) { error in + guard case SwiftPMProductError.executionFailure(_, _, let stderr) = error else { + XCTFail() + return + } + XCTAssertFalse(stderr.contains("warning: Target A imports another target (B) in the package without declaring it a dependency.")) + } + } + } + func testBinPathAndSymlink() throws { try fixture(name: "ValidLayouts/SingleModule/ExecutableNew") { fixturePath in let fullPath = resolveSymlinks(fixturePath)