reduce use of preconditions and asserts (#4169)

motivation: better robustness

changes: replace use of preconditions and asserts with throwing guards

Author: tomerd

Sources/Build/BuildPlan.swift

@@ -278,7 +278,10 @@ public final class ClangTargetBuildDescription {
 
     /// Create a new target description with target and build parameters.
     init(target: ResolvedTarget, toolsVersion: ToolsVersion, buildParameters: BuildParameters, fileSystem: FileSystem) throws {
-        assert(target.underlyingTarget is ClangTarget, "underlying target type mismatch \(target)")
+        guard target.underlyingTarget is ClangTarget else {
+            throw InternalError("underlying target type mismatch \(target)")
+        }
+
         self.fileSystem = fileSystem
         self.target = target
         self.toolsVersion = toolsVersion
@@ -637,7 +640,9 @@ public final class SwiftTargetBuildDescription {
         isTestDiscoveryTarget: Bool = false,
         fileSystem: FileSystem
     ) throws {
-        assert(target.underlyingTarget is SwiftTarget, "underlying target type mismatch \(target)")
+        guard target.underlyingTarget is SwiftTarget else {
+            throw InternalError("underlying target type mismatch \(target)")
+        }
         self.target = target
         self.toolsVersion = toolsVersion
         self.buildParameters = buildParameters
@@ -870,7 +875,9 @@ public final class SwiftTargetBuildDescription {
 
     /// Command-line for emitting just the Swift module.
     public func emitModuleCommandLine() throws -> [String] {
-        assert(buildParameters.emitSwiftModuleSeparately)
+        guard buildParameters.emitSwiftModuleSeparately else {
+            throw InternalError("expecting emitSwiftModuleSeparately in build parameters")
+        }
 
         var result: [String] = []
         result.append(buildParameters.toolchain.swiftCompiler.pathString)
@@ -915,7 +922,9 @@ public final class SwiftTargetBuildDescription {
     ///
     /// Note: This doesn't emit the module.
     public func emitObjectsCommandLine() throws -> [String] {
-        assert(buildParameters.emitSwiftModuleSeparately)
+        guard buildParameters.emitSwiftModuleSeparately else {
+            throw InternalError("expecting emitSwiftModuleSeparately in build parameters")
+        }
 
         var result: [String] = []
         result.append(buildParameters.toolchain.swiftCompiler.pathString)
@@ -1183,8 +1192,11 @@ public final class ProductBuildDescription {
     private let observabilityScope: ObservabilityScope
 
     /// Create a build description for a product.
-    init(product: ResolvedProduct, toolsVersion: ToolsVersion, buildParameters: BuildParameters, fileSystem: FileSystem, observabilityScope: ObservabilityScope) {
-        assert(product.type != .library(.automatic), "Automatic type libraries should not be described.")
+    init(product: ResolvedProduct, toolsVersion: ToolsVersion, buildParameters: BuildParameters, fileSystem: FileSystem, observabilityScope: ObservabilityScope) throws {
+        guard product.type != .library(.automatic) else {
+            throw InternalError("Automatic type libraries should not be described.")
+        }
+
         self.product = product
         self.toolsVersion = toolsVersion
         self.buildParameters = buildParameters
@@ -1293,11 +1305,12 @@ public final class ProductBuildDescription {
             // a main symbol named that way to allow tests to link against it without
             // conflicts. If we're using a linker that doesn't support symbol renaming,
             // we will instead have generated a source file containing the redirect.
-            // Support for linking tests againsts executables is conditional on the tools
+            // Support for linking tests against executables is conditional on the tools
             // version of the package that defines the executable product.
-            if product.executableModule.underlyingTarget is SwiftTarget, toolsVersion >= .v5_5,
+            let executableTarget = try product.executableTarget()
+            if executableTarget.underlyingTarget is SwiftTarget, toolsVersion >= .v5_5,
                buildParameters.canRenameEntrypointFunctionName {
-                if let flags = buildParameters.linkerFlagsForRenamingMainFunction(of: product.executableModule) {
+                if let flags = buildParameters.linkerFlagsForRenamingMainFunction(of: executableTarget) {
                     args += flags
                 }
             }
@@ -1456,7 +1469,10 @@ public final class PluginDescription: Codable {
         testDiscoveryTarget: Bool = false,
         fileSystem: FileSystem
     ) throws {
-        assert(target.underlyingTarget is PluginTarget, "underlying target type mismatch \(target)")
+        guard target.underlyingTarget is PluginTarget else {
+            throw InternalError("underlying target type mismatch \(target)")
+        }
+
         self.package = package.identity
         self.targetName = target.name
         self.productNames = products.map{ $0.name }
@@ -1728,7 +1744,7 @@ public class BuildPlan {
             // Determine the appropriate tools version to use for the product.
             // This can affect what flags to pass and other semantics.
             let toolsVersion = graph.package(for: product)?.manifest.toolsVersion ?? .v5_5
-            productMap[product] = ProductBuildDescription(
+            productMap[product] = try ProductBuildDescription(
                 product: product,
                 toolsVersion: toolsVersion,
                 buildParameters: buildParameters,

Sources/Build/LLBuildManifestBuilder.swift

@@ -549,8 +549,8 @@ extension LLBuildManifestBuilder {
             // Depend on the binary for executable targets.
             if target.type == .executable {
                 // FIXME: Optimize.
-                let _product = plan.graph.allProducts.first {
-                    $0.type == .executable && $0.executableModule == target
+                let _product = try plan.graph.allProducts.first {
+                    try $0.type == .executable && $0.executableTarget() == target
                 }
                 if let product = _product {
                     guard let planProduct = plan.productMap[product] else {

Sources/PackageGraph/Pubgrub/Incompatibility.swift

@@ -45,8 +45,9 @@ public struct Incompatibility: Equatable, Hashable {
         }
 
         let normalizedTerms = try normalize(terms: terms.elements)
-        assert(normalizedTerms.count > 0,
-               "An incompatibility must contain at least one term after normalization.")
+        guard normalizedTerms.count > 0 else {
+            throw InternalError("An incompatibility must contain at least one term after normalization.")
+        }
         self.init(terms: OrderedSet(normalizedTerms), cause: cause)
     }
 }

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

@@ -227,7 +227,9 @@ public struct PubgrubDependencyResolver {
             let updatePackage = try container.underlying.loadPackageReference(at: boundVersion)
 
             if var existing = flattenedAssignments[updatePackage] {
-                assert(existing.binding == boundVersion, "Two products in one package resolved to different versions: \(existing.products)@\(existing.binding) vs \(products)@\(boundVersion)")
+                guard existing.binding == boundVersion else {
+                    throw InternalError("Two products in one package resolved to different versions: \(existing.products)@\(existing.binding) vs \(products)@\(boundVersion)")
+                }
                 existing.products.formUnion(products)
                 flattenedAssignments[updatePackage] = existing
             } else {
@@ -284,7 +286,9 @@ public struct PubgrubDependencyResolver {
 
             // Mark the package as overridden.
             if var existing = overriddenPackages[constraint.package] {
-                assert(existing.version == .unversioned, "Overridden package is not unversioned: \(constraint.package)@\(existing.version)")
+                guard existing.version == .unversioned else {
+                    throw InternalError("Overridden package is not unversioned: \(constraint.package)@\(existing.version)")
+                }
                 existing.products.formUnion(constraint.products)
                 overriddenPackages[constraint.package] = existing
             } else {

Sources/PackageGraph/ResolvedProduct.swift

@@ -8,6 +8,7 @@
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
 */
 
+import Basics
 import TSCBasic
 import PackageModel
 
@@ -34,8 +35,10 @@ public final class ResolvedProduct {
     /// The main executable target of product.
     ///
     /// Note: This property is only valid for executable products.
-    public var executableModule: ResolvedTarget {
-        precondition(type == .executable || type == .snippet, "This property should only be called for executable targets")
+    public func executableTarget() throws -> ResolvedTarget {
+        guard type == .executable || type == .snippet else {
+            throw InternalError("firstExecutableModule should only be called for executable targets")
+        }
         return targets.first(where: { $0.type == .executable || $0.type == .snippet })!
     }
 

Sources/PackageLoading/ManifestLoader.swift

@@ -278,7 +278,7 @@ public final class ManifestLoader: ManifestLoaderProtocol {
                 var targets = parsedManifest.targets
                 if products.isEmpty, targets.isEmpty,
                    fileSystem.isFile(path.parentDirectory.appending(component: moduleMapFilename)) {
-                    products.append(ProductDescription(
+                    try products.append(ProductDescription(
                         name: parsedManifest.name,
                         type: .library(.automatic),
                         targets: [parsedManifest.name])
@@ -524,7 +524,9 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         }
 
         // We should not have any fatal error at this point.
-        assert(result.errorOutput == nil)
+        guard result.errorOutput == nil else {
+            throw InternalError("unexpected error output: \(result.errorOutput!)")
+        }
 
         // We might have some non-fatal output (warnings/notes) from the compiler even when
         // we were able to parse the manifest successfully.
@@ -802,17 +804,18 @@ public final class ManifestLoader: ManifestLoaderProtocol {
         delegateQueue: DispatchQueue,
         completion: @escaping (Result<EvaluationResult, Error>) -> Void
     ) {
+        // The compiler has special meaning for files with extensions like .ll, .bc etc.
+        // Assert that we only try to load files with extension .swift to avoid unexpected loading behavior.
+        guard manifestPath.extension == "swift" else {
+            return completion(.failure(InternalError("Manifest files must contain .swift suffix in their name, given: \(manifestPath).")))
+        }
+
         var evaluationResult = EvaluationResult()
 
         delegateQueue.async {
             self.delegate?.willParse(manifest: manifestPath)
         }
 
-        // The compiler has special meaning for files with extensions like .ll, .bc etc.
-        // Assert that we only try to load files with extension .swift to avoid unexpected loading behavior.
-        assert(manifestPath.extension == "swift",
-               "Manifest files must contain .swift suffix in their name, given: \(manifestPath).")
-
         // For now, we load the manifest by having Swift interpret it directly.
         // Eventually, we should have two loading processes, one that loads only
         // the declarative package specification using the Swift compiler directly

Sources/PackageLoading/PackageBuilder.swift

@@ -889,7 +889,7 @@ public final class PackageBuilder {
                 moduleMapType = .none
             }
 
-            return ClangTarget(
+            return try ClangTarget(
                 name: potentialModule.name,
                 bundleName: bundleName,
                 defaultLocalization: manifest.defaultLocalization,
@@ -1127,7 +1127,9 @@ public final class PackageBuilder {
             }
 
             while true {
-                assert(searchPath.isDescendantOfOrEqual(to: packagePath), "search path \(searchPath) is outside the package \(packagePath)")
+                guard searchPath.isDescendantOfOrEqual(to: packagePath) else {
+                    throw InternalError("search path \(searchPath) is outside the package \(packagePath)")
+                }
                 // If we have already searched this path, skip.
                 if !pathsSearched.contains(searchPath) {
                     SwiftTarget.testManifestNames.forEach { name in
@@ -1181,7 +1183,7 @@ public final class PackageBuilder {
         // If enabled, create one test product for each test target.
         if self.shouldCreateMultipleTestProducts {
             for testTarget in testModules {
-                let product = Product(name: testTarget.name, type: .test, targets: [testTarget])
+                let product = try Product(name: testTarget.name, type: .test, targets: [testTarget])
                 append(product)
             }
         } else if !testModules.isEmpty {
@@ -1194,7 +1196,7 @@ public final class PackageBuilder {
             let productName = self.manifest.displayName + "PackageTests"
             let testManifest = try self.findTestManifest(in: testModules)
 
-            let product = Product(name: productName, type: .test, targets: testModules, testManifest: testManifest)
+            let product = try Product(name: productName, type: .test, targets: testModules, testManifest: testManifest)
             append(product)
         }
 
@@ -1247,7 +1249,7 @@ public final class PackageBuilder {
                 }
             }
 
-            append(Product(name: product.name, type: product.type, targets: targets))
+            try append(Product(name: product.name, type: product.type, targets: targets))
         }
 
         // Add implicit executables - for root packages and for dependency plugins.
@@ -1291,7 +1293,7 @@ public final class PackageBuilder {
             } else {
                 if self.manifest.packageKind.isRoot || implicitPlugInExecutables.contains(target.name) {
                     // Generate an implicit product for the executable target
-                    let product = Product(name: target.name, type: .executable, targets: [target])
+                    let product = try Product(name: target.name, type: .executable, targets: [target])
                     append(product)
                 }
             }
@@ -1304,7 +1306,7 @@ public final class PackageBuilder {
             if libraryTargets.isEmpty {
                 self.observabilityScope.emit(.noLibraryTargetsForREPL)
             } else {
-                let replProduct = Product(
+                let replProduct = try Product(
                     name: self.identity.description + Product.replProductSuffix,
                     type: .library(.dynamic),
                     targets: libraryTargets
@@ -1314,9 +1316,9 @@ public final class PackageBuilder {
         }
 
         // Create implicit snippet products
-        targets
+        try targets
             .filter { $0.type == .snippet }
-            .map { Product(name: $0.name, type: .snippet, targets: [$0]) }
+            .map { try Product(name: $0.name, type: .snippet, targets: [$0]) }
             .forEach(append)
 
         return products.map{ $0.item }
@@ -1357,7 +1359,6 @@ public final class PackageBuilder {
 
 /// We create this structure after scanning the filesystem for potential targets.
 private struct PotentialModule: Hashable {
-
     /// Name of the target.
     let name: String
 
@@ -1371,16 +1372,6 @@ private struct PotentialModule: Hashable {
 
     /// The target type.
     let type: TargetDescription.TargetType
-
-    /// The base prefix for the test target, used to associate with the target it tests.
-    public var basename: String {
-        guard isTest else {
-            fatalError("\(Swift.type(of: self)) should be a test target to access basename.")
-        }
-        precondition(name.hasSuffix(Target.testModuleNameSuffix))
-        let endIndex = name.index(name.endIndex, offsetBy: -Target.testModuleNameSuffix.count)
-        return String(name[name.startIndex..<endIndex])
-    }
 }
 
 private extension Manifest {

Sources/PackageLoading/PkgConfig.swift

@@ -80,7 +80,7 @@ public struct PkgConfig {
             )
         }
 
-        var parser = PkgConfigParser(pcFile: pcFile, fileSystem: fileSystem)
+        var parser = try PkgConfigParser(pcFile: pcFile, fileSystem: fileSystem)
         try parser.parse()
 
         func getFlags(from dependencies: [String]) throws -> (cFlags: [String], libs: [String]) {
@@ -152,8 +152,10 @@ internal struct PkgConfigParser {
     public private(set) var cFlags = [String]()
     public private(set) var libs = [String]()
 
-    public init(pcFile: AbsolutePath, fileSystem: FileSystem) {
-        precondition(fileSystem.isFile(pcFile))
+    public init(pcFile: AbsolutePath, fileSystem: FileSystem) throws {
+        guard fileSystem.isFile(pcFile) else {
+            throw StringError("invalid pcfile \(pcFile)")
+        }
         self.pcFile = pcFile
         self.fileSystem = fileSystem
     }
@@ -195,7 +197,9 @@ internal struct PkgConfigParser {
     }
 
     private mutating func parseKeyValue(line: String) throws {
-        precondition(line.contains(":"))
+        guard line.contains(":") else {
+            throw InternalError("invalid pcfile, expecting line to contain :")
+        }
         let (key, maybeValue) = line.spm_split(around: ":")
         let value = try resolveVariables(maybeValue?.spm_chuzzle() ?? "")
         switch key {

Sources/PackageModel/Manifest/ProductDescription.swift

@@ -8,6 +8,8 @@
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
 */
 
+import Basics
+
 /// The product description
 public struct ProductDescription: Equatable, Codable {
 
@@ -24,8 +26,10 @@ public struct ProductDescription: Equatable, Codable {
         name: String,
         type: ProductType,
         targets: [String]
-    ) {
-        precondition(type != .test, "Declaring test products isn't supported: \(name):\(targets)")
+    ) throws {
+        guard type != .test else {
+            throw InternalError("Declaring test products isn't supported: \(name):\(targets)")
+        }
         self.name = name
         self.type = type
         self.targets = targets

Sources/PackageModel/Product.swift

@@ -8,6 +8,7 @@
  See http://swift.org/CONTRIBUTORS.txt for Swift project authors
 */
 
+import Basics
 import TSCBasic
 
 import struct TSCUtility.PolymorphicCodableArray
@@ -32,14 +33,19 @@ public class Product: Codable {
     /// The suffix for REPL product name.
     public static let replProductSuffix: String = "__REPL"
 
-    public init(name: String, type: ProductType, targets: [Target], testManifest: AbsolutePath? = nil) {
-        precondition(!targets.isEmpty)
+    public init(name: String, type: ProductType, targets: [Target], testManifest: AbsolutePath? = nil) throws {
+        guard !targets.isEmpty else {
+            throw InternalError("Targets cannot be empty")
+        }
         if type == .executable {
-            assert(targets.filter({ $0.type == .executable }).count == 1,
-                   "Executable products should have exactly one executable target.")
+            guard targets.filter({ $0.type == .executable }).count == 1 else {
+                throw InternalError("Executable products should have exactly one executable target.")
+            }
         }
         if testManifest != nil {
-            assert(type == .test, "Test manifest should only be set on test products")
+            guard type == .test else {
+                throw InternalError("Test manifest should only be set on test products")
+            }
         }
         self.name = name
         self.type = type