From 29509a7cd3bfb616b6af07bc44d36694b37c0777 Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Wed, 16 Jun 2021 14:53:13 -0700 Subject: [PATCH 1/6] Verify that only targets that are manifest dependencies can be imported. This change introduces a build verification step that attempts to detect scenarios where a target contains an `import` of another target in the package without declaring the imported target as a dependency in the manifest. This is done via SwiftDriver's import-prescan capability which relies on libSwiftScan to quickly parse a target's sources and identify all `import`ed modules. Related to rdar://79423257 --- .../ImportOfMissingDependency/Package.swift | 20 +++++++++ .../Sources/A/A.swift | 4 ++ .../Sources/B/B.swift | 3 ++ .../Sources/B/main.swift | 1 + Sources/Build/BuildOperation.swift | 43 ++++++++++++++++++- ...dOperationBuildSystemDelegateHandler.swift | 21 +++++++++ Sources/Build/BuildPlan.swift | 22 ++++++---- Tests/CommandsTests/BuildToolTests.swift | 13 ++++++ 8 files changed, 117 insertions(+), 10 deletions(-) create mode 100644 Fixtures/Miscellaneous/ImportOfMissingDependency/Package.swift create mode 100644 Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/A/A.swift create mode 100644 Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/B.swift create mode 100644 Fixtures/Miscellaneous/ImportOfMissingDependency/Sources/B/main.swift 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..c82b4033b5f 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,51 @@ 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 { + for (target, commandLine) in description.swiftTargetScanArgs { + do { + guard let dependencies = description.targetDependencyMap[target] else { + // Skip target if no dependency information is present + continue + } + let resolver = try ArgsResolver(fileSystem: localFileSystem) + let executor = SPMSwiftDriverExecutor(resolver: resolver, + fileSystem: localFileSystem, + env: ProcessEnv.vars) + var driver = try Driver(args: commandLine, + diagnosticsEngine: diagnostics, + fileSystem: localFileSystem, + executor: executor) + let imports = try driver.performImportPrescan().imports + let targetDependenciesSet = Set(dependencies) + let nonDependencyTargetsSet = + Set(description.targetDependencyMap.keys.filter { !targetDependenciesSet.contains($0) }) + let importedTargetsMissingDependency = Set(imports).intersection(nonDependencyTargetsSet) + if let missedDependency = importedTargetsMissingDependency.first { + diagnostics.emit(warning: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.", location: nil) + } + } 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..02b3da61720 100644 --- a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift +++ b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift @@ -213,6 +213,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 +227,12 @@ public struct BuildDescription: Codable { /// The map of copy commands. let copyCommands: [BuildManifest.CmdName: LLBuildManifest.CopyTool] + /// 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]] + /// The built test products. public let builtTestProducts: [BuiltTestProduct] @@ -244,6 +251,20 @@ public struct BuildDescription: Codable { self.swiftFrontendCommands = swiftFrontendCommands self.testDiscoveryCommands = testDiscoveryCommands self.copyCommands = copyCommands + 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]] = [:] + 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.swiftCompiler.pathString] + } + self.swiftTargetScanArgs = targetCommandLines 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/Tests/CommandsTests/BuildToolTests.swift b/Tests/CommandsTests/BuildToolTests.swift index 44ebda93203..bb4be075688 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -75,6 +75,19 @@ final class BuildToolTests: CommandsTestCase { } } + func testImportOfMissedDepWarning() throws { + 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 + } + XCTAssertTrue(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) From ef4b0c98c8dc3ada8c7c2e0209754fe93c7cbf17 Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Mon, 28 Jun 2021 12:38:16 -0700 Subject: [PATCH 2/6] Skip target-import dependency verification procedure and test, if the host compiler does not support it. --- Sources/Build/BuildOperation.swift | 4 ++++ Tests/CommandsTests/BuildToolTests.swift | 3 +++ 2 files changed, 7 insertions(+) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index c82b4033b5f..f656f26e599 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -161,6 +161,10 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // 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 { + // Ensure the compiler supports the import-scan operation + guard SwiftTargetBuildDescription.checkSupportedFrontendFlags(flags: ["import-prescan"], fs: localFileSystem) else { + return + } for (target, commandLine) in description.swiftTargetScanArgs { do { guard let dependencies = description.targetDependencyMap[target] else { diff --git a/Tests/CommandsTests/BuildToolTests.swift b/Tests/CommandsTests/BuildToolTests.swift index bb4be075688..000809f0a9f 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -76,6 +76,9 @@ final class BuildToolTests: CommandsTestCase { } func testImportOfMissedDepWarning() throws { + #if swift(<5.5) + try XCTSkipIf(true, "skipping because host compiler doesn't support '-import-prescan'") + #endif fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in let fullPath = resolveSymlinks(path) XCTAssertThrowsError(try build([], packagePath: fullPath)) { error in From 08725a9cf83464e0bd84e9d14dc5b34b597b0e82 Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Wed, 30 Jun 2021 14:23:04 -0700 Subject: [PATCH 3/6] Skip targets that contain or depend-on generated code, because generated source-code may not be available at the time of scan. --- Sources/Build/BuildOperation.swift | 18 ++++++++++++++++-- ...ldOperationBuildSystemDelegateHandler.swift | 11 ++++++++++- 2 files changed, 26 insertions(+), 3 deletions(-) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index f656f26e599..63a81a97522 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -165,22 +165,36 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS guard SwiftTargetBuildDescription.checkSupportedFrontendFlags(flags: ["import-prescan"], fs: 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: diagnostics, + 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 targetDependenciesSet = Set(dependencies) let nonDependencyTargetsSet = Set(description.targetDependencyMap.keys.filter { !targetDependenciesSet.contains($0) }) let importedTargetsMissingDependency = Set(imports).intersection(nonDependencyTargetsSet) diff --git a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift index 02b3da61720..b0d04790b53 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 <<< """ ] } @@ -233,6 +232,9 @@ public struct BuildDescription: Codable { /// 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] @@ -256,6 +258,7 @@ public struct BuildDescription: Codable { $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 @@ -263,8 +266,14 @@ public struct BuildDescription: Codable { targetCommandLines[target.c99name] = try desc.emitCommandLine(scanInvocation: true) + ["-driver-use-frontend-path", plan.buildParameters.toolchain.swiftCompiler.pathString] + if desc.testDiscoveryTarget { + 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, From 01eb92d5bb6c4f04f05af3ccc40d0e806689d6e3 Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Fri, 16 Jul 2021 10:07:42 -0700 Subject: [PATCH 4/6] Add an opt-out from explicit target dependency import checking. `--disable-explicit-target-dependency-import-checking` prevents the verification code from being run as a just-in-case option to opt-out from this warning --- Sources/Build/BuildOperation.swift | 7 +++++-- .../BuildOperationBuildSystemDelegateHandler.swift | 5 +++++ Sources/Commands/Options.swift | 5 +++++ Sources/Commands/SwiftTool.swift | 1 + Sources/SPMBuildCore/BuildParameters.swift | 6 ++++++ Tests/CommandsTests/BuildToolTests.swift | 12 ++++++++++++ 6 files changed, 34 insertions(+), 2 deletions(-) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index 63a81a97522..246084cd95f 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -161,8 +161,11 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // 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 { + guard !description.disableExplicitTargetDependencyImportChecking else { + return + } // Ensure the compiler supports the import-scan operation - guard SwiftTargetBuildDescription.checkSupportedFrontendFlags(flags: ["import-prescan"], fs: localFileSystem) else { + guard SwiftTargetBuildDescription.checkSupportedFrontendFlags(flags: ["import-prescan"], fileSystem: localFileSystem) else { return } @@ -199,7 +202,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS Set(description.targetDependencyMap.keys.filter { !targetDependenciesSet.contains($0) }) let importedTargetsMissingDependency = Set(imports).intersection(nonDependencyTargetsSet) if let missedDependency = importedTargetsMissingDependency.first { - diagnostics.emit(warning: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.", location: nil) + self.observabilityScope.emit(warning: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.") } } catch { // The above verification is a best-effort attempt to warn the user about a potential manifest diff --git a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift index b0d04790b53..3a50145ed58 100644 --- a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift +++ b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift @@ -226,6 +226,10 @@ public struct BuildDescription: Codable { /// The map of copy commands. let copyCommands: [BuildManifest.CmdName: LLBuildManifest.CopyTool] + /// A flag that inidcates this build should skip checking whether targets only import + /// their explicitly-declared dependencies + let disableExplicitTargetDependencyImportChecking: Bool + /// Every target's set of dependencies. let targetDependencyMap: [TargetName: [TargetName]] @@ -253,6 +257,7 @@ public struct BuildDescription: Codable { self.swiftFrontendCommands = swiftFrontendCommands self.testDiscoveryCommands = testDiscoveryCommands self.copyCommands = copyCommands + self.disableExplicitTargetDependencyImportChecking = plan.buildParameters.disableExplicitTargetDependencyImportChecking self.targetDependencyMap = try plan.targets.reduce(into: [TargetName: [TargetName]]()) { let deps = try $1.target.recursiveTargetDependencies().map { $0.c99name } $0[$1.target.c99name] = deps diff --git a/Sources/Commands/Options.swift b/Sources/Commands/Options.swift index 32bd201ff89..470f90a1497 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 + @Flag() + var enableExplicitTargetDependencyImportChecking: Bool = false + /// Whether to use the explicit module build flow (with the integrated driver) @Flag(name: .customLong("experimental-explicit-module-build")) var useExplicitModuleBuild: Bool = false diff --git a/Sources/Commands/SwiftTool.swift b/Sources/Commands/SwiftTool.swift index 51cc9bdc6dc..44f0766d602 100644 --- a/Sources/Commands/SwiftTool.swift +++ b/Sources/Commands/SwiftTool.swift @@ -792,6 +792,7 @@ public class SwiftTool { enableParseableModuleInterfaces: options.build.shouldEnableParseableModuleInterfaces, emitSwiftModuleSeparately: options.build.emitSwiftModuleSeparately, useIntegratedSwiftDriver: options.build.useIntegratedSwiftDriver, + disableExplicitTargetDependencyImportChecking: build.disableExplicitTargetDependencyImportChecking, useExplicitModuleBuild: options.build.useExplicitModuleBuild, isXcodeBuildSystemEnabled: options.build.buildSystem == .xcode, forceTestDiscovery: options.build.enableTestDiscovery, // backwards compatibility, remove with --enable-test-discovery diff --git a/Sources/SPMBuildCore/BuildParameters.swift b/Sources/SPMBuildCore/BuildParameters.swift index d8c2c939190..324dc1fa93a 100644 --- a/Sources/SPMBuildCore/BuildParameters.swift +++ b/Sources/SPMBuildCore/BuildParameters.swift @@ -128,6 +128,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 skip checking whether targets only import + /// their explicitly-declared dependencies + public var disableExplicitTargetDependencyImportChecking: Bool + /// Whether to create dylibs for dynamic library products. public var shouldCreateDylibForDynamicProducts: Bool @@ -199,6 +203,7 @@ public struct BuildParameters: Encodable { isXcodeBuildSystemEnabled: Bool = false, enableTestability: Bool? = nil, forceTestDiscovery: Bool = false, + disableExplicitTargetDependencyImportChecking: Bool = false linkerDeadStrip: Bool = true, colorizedOutput: Bool = false, verboseOutput: Bool = false @@ -236,6 +241,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.disableExplicitTargetDependencyImportChecking = disableExplicitTargetDependencyImportChecking self.linkerDeadStrip = linkerDeadStrip self.colorizedOutput = colorizedOutput self.verboseOutput = verboseOutput diff --git a/Tests/CommandsTests/BuildToolTests.swift b/Tests/CommandsTests/BuildToolTests.swift index 000809f0a9f..940b2970da7 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -89,6 +89,18 @@ final class BuildToolTests: CommandsTestCase { XCTAssertTrue(stderr.contains("warning: Target A imports another target (B) in the package without declaring it a dependency.")) } } + + // Verify that the disable toggle works as expected + fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in + let fullPath = resolveSymlinks(path) + XCTAssertThrowsError(try build(["--disable-explicit-target-dependency-import-checking"], 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 { From a4114eb92d2d77fd85495ac5a0fb3617f0e5267e Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Tue, 26 Oct 2021 16:10:12 -0700 Subject: [PATCH 5/6] Flip the `--disable-explicit-target-dependency-import-checking` option to an opt-in `enable` option. While we work on optimizing incremental build performance of this check, it would be nice to have it as an opt-in option. --- Sources/Build/BuildOperation.swift | 2 +- .../BuildOperationBuildSystemDelegateHandler.swift | 10 +++++----- Sources/Commands/SwiftTool.swift | 2 +- Sources/SPMBuildCore/BuildParameters.swift | 8 ++++---- Tests/CommandsTests/BuildToolTests.swift | 8 ++++---- 5 files changed, 15 insertions(+), 15 deletions(-) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index 246084cd95f..53748f45892 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -161,7 +161,7 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // 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 { - guard !description.disableExplicitTargetDependencyImportChecking else { + guard description.enableExplicitTargetDependencyImportChecking else { return } // Ensure the compiler supports the import-scan operation diff --git a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift index 3a50145ed58..2f2b7ebca70 100644 --- a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift +++ b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift @@ -226,9 +226,9 @@ public struct BuildDescription: Codable { /// The map of copy commands. let copyCommands: [BuildManifest.CmdName: LLBuildManifest.CopyTool] - /// A flag that inidcates this build should skip checking whether targets only import + /// A flag that inidcates this build should perform a check for whether targets only import /// their explicitly-declared dependencies - let disableExplicitTargetDependencyImportChecking: Bool + let enableExplicitTargetDependencyImportChecking: Bool /// Every target's set of dependencies. let targetDependencyMap: [TargetName: [TargetName]] @@ -257,7 +257,7 @@ public struct BuildDescription: Codable { self.swiftFrontendCommands = swiftFrontendCommands self.testDiscoveryCommands = testDiscoveryCommands self.copyCommands = copyCommands - self.disableExplicitTargetDependencyImportChecking = plan.buildParameters.disableExplicitTargetDependencyImportChecking + self.enableExplicitTargetDependencyImportChecking = plan.buildParameters.enableExplicitTargetDependencyImportChecking self.targetDependencyMap = try plan.targets.reduce(into: [TargetName: [TargetName]]()) { let deps = try $1.target.recursiveTargetDependencies().map { $0.c99name } $0[$1.target.c99name] = deps @@ -270,8 +270,8 @@ public struct BuildDescription: Codable { } targetCommandLines[target.c99name] = try desc.emitCommandLine(scanInvocation: true) + ["-driver-use-frontend-path", - plan.buildParameters.toolchain.swiftCompiler.pathString] - if desc.testDiscoveryTarget { + plan.buildParameters.toolchain.swiftCompilerPath.pathString] + if desc.isTestDiscoveryTarget { generatedSourceTargets.append(target.c99name) } } diff --git a/Sources/Commands/SwiftTool.swift b/Sources/Commands/SwiftTool.swift index 44f0766d602..469ef64611a 100644 --- a/Sources/Commands/SwiftTool.swift +++ b/Sources/Commands/SwiftTool.swift @@ -792,10 +792,10 @@ public class SwiftTool { enableParseableModuleInterfaces: options.build.shouldEnableParseableModuleInterfaces, emitSwiftModuleSeparately: options.build.emitSwiftModuleSeparately, useIntegratedSwiftDriver: options.build.useIntegratedSwiftDriver, - disableExplicitTargetDependencyImportChecking: build.disableExplicitTargetDependencyImportChecking, useExplicitModuleBuild: options.build.useExplicitModuleBuild, isXcodeBuildSystemEnabled: options.build.buildSystem == .xcode, forceTestDiscovery: options.build.enableTestDiscovery, // backwards compatibility, remove with --enable-test-discovery + enableExplicitTargetDependencyImportChecking: options.build.enableExplicitTargetDependencyImportChecking, linkerDeadStrip: options.linker.linkerDeadStrip, verboseOutput: self.logLevel <= .info ) diff --git a/Sources/SPMBuildCore/BuildParameters.swift b/Sources/SPMBuildCore/BuildParameters.swift index 324dc1fa93a..22fdcd23525 100644 --- a/Sources/SPMBuildCore/BuildParameters.swift +++ b/Sources/SPMBuildCore/BuildParameters.swift @@ -128,9 +128,9 @@ 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 skip checking whether targets only import + /// A flag that inidcates this build should check whether targets only import /// their explicitly-declared dependencies - public var disableExplicitTargetDependencyImportChecking: Bool + public var enableExplicitTargetDependencyImportChecking: Bool /// Whether to create dylibs for dynamic library products. public var shouldCreateDylibForDynamicProducts: Bool @@ -203,7 +203,7 @@ public struct BuildParameters: Encodable { isXcodeBuildSystemEnabled: Bool = false, enableTestability: Bool? = nil, forceTestDiscovery: Bool = false, - disableExplicitTargetDependencyImportChecking: Bool = false + enableExplicitTargetDependencyImportChecking: Bool = false, linkerDeadStrip: Bool = true, colorizedOutput: Bool = false, verboseOutput: Bool = false @@ -241,7 +241,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.disableExplicitTargetDependencyImportChecking = disableExplicitTargetDependencyImportChecking + self.enableExplicitTargetDependencyImportChecking = enableExplicitTargetDependencyImportChecking self.linkerDeadStrip = linkerDeadStrip self.colorizedOutput = colorizedOutput self.verboseOutput = verboseOutput diff --git a/Tests/CommandsTests/BuildToolTests.swift b/Tests/CommandsTests/BuildToolTests.swift index 940b2970da7..112a8bfdb3e 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -79,9 +79,9 @@ final class BuildToolTests: CommandsTestCase { #if swift(<5.5) try XCTSkipIf(true, "skipping because host compiler doesn't support '-import-prescan'") #endif - fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in + try fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in let fullPath = resolveSymlinks(path) - XCTAssertThrowsError(try build([], packagePath: fullPath)) { error in + XCTAssertThrowsError(try build(["--enable-explicit-target-dependency-import-checking"], packagePath: fullPath)) { error in guard case SwiftPMProductError.executionFailure(_, _, let stderr) = error else { XCTFail() return @@ -91,9 +91,9 @@ final class BuildToolTests: CommandsTestCase { } // Verify that the disable toggle works as expected - fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in + try fixture(name: "Miscellaneous/ImportOfMissingDependency") { path in let fullPath = resolveSymlinks(path) - XCTAssertThrowsError(try build(["--disable-explicit-target-dependency-import-checking"], packagePath: fullPath)) { error in + XCTAssertThrowsError(try build([], packagePath: fullPath)) { error in guard case SwiftPMProductError.executionFailure(_, _, let stderr) = error else { XCTFail() return From e5020d8c133c59222bfb0f778a610e6dec7f2a8e Mon Sep 17 00:00:00 2001 From: Artem Chikin Date: Wed, 6 Jul 2022 11:54:16 -0700 Subject: [PATCH 6/6] Allow configuring the explicit import checking with either 'error', 'warning', or 'none' modes, with 'none' being the default. --- Sources/Build/BuildOperation.swift | 12 ++++++++++-- ...dOperationBuildSystemDelegateHandler.swift | 4 ++-- Sources/Commands/Options.swift | 10 ++++++++-- Sources/Commands/SwiftTool.swift | 15 ++++++++++++++- Sources/SPMBuildCore/BuildParameters.swift | 13 ++++++++++--- Tests/CommandsTests/BuildToolTests.swift | 19 +++++++++++++++++-- 6 files changed, 61 insertions(+), 12 deletions(-) diff --git a/Sources/Build/BuildOperation.swift b/Sources/Build/BuildOperation.swift index 53748f45892..366a5bd6839 100644 --- a/Sources/Build/BuildOperation.swift +++ b/Sources/Build/BuildOperation.swift @@ -161,7 +161,8 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS // 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 { - guard description.enableExplicitTargetDependencyImportChecking else { + let checkingMode = description.explicitTargetDependencyImportCheckingMode + guard checkingMode != .none else { return } // Ensure the compiler supports the import-scan operation @@ -202,7 +203,14 @@ public final class BuildOperation: PackageStructureDelegate, SPMBuildCore.BuildS Set(description.targetDependencyMap.keys.filter { !targetDependenciesSet.contains($0) }) let importedTargetsMissingDependency = Set(imports).intersection(nonDependencyTargetsSet) if let missedDependency = importedTargetsMissingDependency.first { - self.observabilityScope.emit(warning: "Target \(target) imports another target (\(missedDependency)) in the package without declaring it a dependency.") + 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 diff --git a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift index 2f2b7ebca70..3baf352c665 100644 --- a/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift +++ b/Sources/Build/BuildOperationBuildSystemDelegateHandler.swift @@ -228,7 +228,7 @@ public struct BuildDescription: Codable { /// A flag that inidcates this build should perform a check for whether targets only import /// their explicitly-declared dependencies - let enableExplicitTargetDependencyImportChecking: Bool + let explicitTargetDependencyImportCheckingMode: BuildParameters.TargetDependencyImportCheckingMode /// Every target's set of dependencies. let targetDependencyMap: [TargetName: [TargetName]] @@ -257,7 +257,7 @@ public struct BuildDescription: Codable { self.swiftFrontendCommands = swiftFrontendCommands self.testDiscoveryCommands = testDiscoveryCommands self.copyCommands = copyCommands - self.enableExplicitTargetDependencyImportChecking = plan.buildParameters.enableExplicitTargetDependencyImportChecking + 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 diff --git a/Sources/Commands/Options.swift b/Sources/Commands/Options.swift index 470f90a1497..b2c116dab22 100644 --- a/Sources/Commands/Options.swift +++ b/Sources/Commands/Options.swift @@ -337,8 +337,8 @@ struct BuildOptions: ParsableArguments { /// A flag that inidcates this build should check whether targets only import /// their explicitly-declared dependencies - @Flag() - var enableExplicitTargetDependencyImportChecking: Bool = false + @Option() + var explicitTargetDependencyImportCheck: TargetDependencyImportCheckingMode = .none /// Whether to use the explicit module build flow (with the integrated driver) @Flag(name: .customLong("experimental-explicit-module-build")) @@ -375,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 469ef64611a..cdf56bb3ca7 100644 --- a/Sources/Commands/SwiftTool.swift +++ b/Sources/Commands/SwiftTool.swift @@ -795,7 +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 - enableExplicitTargetDependencyImportChecking: options.build.enableExplicitTargetDependencyImportChecking, + explicitTargetDependencyImportCheckingMode: options.build.explicitTargetDependencyImportCheck.modeParameter, linkerDeadStrip: options.linker.linkerDeadStrip, verboseOutput: self.logLevel <= .info ) @@ -1188,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 22fdcd23525..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 @@ -130,7 +137,7 @@ public struct BuildParameters: Encodable { /// A flag that inidcates this build should check whether targets only import /// their explicitly-declared dependencies - public var enableExplicitTargetDependencyImportChecking: Bool + public var explicitTargetDependencyImportCheckingMode: TargetDependencyImportCheckingMode /// Whether to create dylibs for dynamic library products. public var shouldCreateDylibForDynamicProducts: Bool @@ -203,7 +210,7 @@ public struct BuildParameters: Encodable { isXcodeBuildSystemEnabled: Bool = false, enableTestability: Bool? = nil, forceTestDiscovery: Bool = false, - enableExplicitTargetDependencyImportChecking: Bool = false, + explicitTargetDependencyImportCheckingMode: TargetDependencyImportCheckingMode = .none, linkerDeadStrip: Bool = true, colorizedOutput: Bool = false, verboseOutput: Bool = false @@ -241,7 +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.enableExplicitTargetDependencyImportChecking = enableExplicitTargetDependencyImportChecking + 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 112a8bfdb3e..f6f4c2e6831 100644 --- a/Tests/CommandsTests/BuildToolTests.swift +++ b/Tests/CommandsTests/BuildToolTests.swift @@ -79,18 +79,33 @@ final class BuildToolTests: CommandsTestCase { #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(["--enable-explicit-target-dependency-import-checking"], packagePath: fullPath)) { error in + 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 that the disable toggle works as expected + // 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