[NFC] Replace PackageConditionProtocol with PackageCondition (#7117)

MaxDesiatov · web-flow · commit e9fa37305950 · 2023-12-05T16:59:35.000Z
Unblocks `PackageGraph` value types refactoring in #7160 and macros cross-compilation fix in #7118. Multiple places in our codebase pass around and store `[any PackageConditionProtocol]` arrays of existentials with `FIXME` notes that those should be sets and not arrays. We also can't convert types containing these arrays to value types with auto-generated `Hashable` conformance, since neither elements of these arrays nor arrays themselves are `Hashable`. Adding `Hashable` requirement to `PackageConditionProtocol` doesn't fix the issue, since existentials of this protocol still wouldn't conform to `Hashable`. A simple alternative is to create `enum PackageCondition` with cases for each condition type. We only have two of those types and they're always declared in the same module. There's no use case for externally defined types for this protocol. That allows us to convert uses of `[any PackageConditionProtocol]` to `[PackageCondition]`. Existing protocol is kept around until clients of SwiftPM can migrate to the new `PackageCondition` enum. This also allows us to remove a custom conformance to `Hashable` on `ResolvedTarget`, unblocking a conversion of `ResolvedTarget` to a value type and making it `Sendable` in the future.
diff --git a/Sources/PackageGraph/PackageGraph+Loading.swift b/Sources/PackageGraph/PackageGraph+Loading.swift
@@ -869,10 +869,10 @@ private final class ResolvedTargetBuilder: ResolvedBuilder<ResolvedTarget> {
     enum Dependency {
 
         /// Dependency to another target, with conditions.
-        case target(_ target: ResolvedTargetBuilder, conditions: [PackageConditionProtocol])
+        case target(_ target: ResolvedTargetBuilder, conditions: [PackageCondition])
 
         /// Dependency to a product, with conditions.
-        case product(_ product: ResolvedProductBuilder, conditions: [PackageConditionProtocol])
+        case product(_ product: ResolvedProductBuilder, conditions: [PackageCondition])
     }
 
     /// The target reference.
@@ -918,7 +918,10 @@ private final class ResolvedTargetBuilder: ResolvedBuilder<ResolvedTarget> {
                 try self.target.validateDependency(target: targetBuilder.target)
                 return .target(try targetBuilder.construct(), conditions: conditions)
             case .product(let productBuilder, let conditions):
-                try self.target.validateDependency(product: productBuilder.product, productPackage: productBuilder.packageBuilder.package.identity)
+                try self.target.validateDependency(
+                    product: productBuilder.product,
+                    productPackage: productBuilder.packageBuilder.package.identity
+                )
                 let product = try productBuilder.construct()
                 if !productBuilder.packageBuilder.isAllowedToVendUnsafeProducts {
                     try self.diagnoseInvalidUseOfUnsafeFlags(product)
diff --git a/Sources/PackageGraph/Resolution/ResolvedTarget.swift b/Sources/PackageGraph/Resolution/ResolvedTarget.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2020 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -19,10 +19,10 @@ public final class ResolvedTarget {
     /// Represents dependency of a resolved target.
     public enum Dependency {
         /// Direct dependency of the target. This target is in the same package and should be statically linked.
-        case target(_ target: ResolvedTarget, conditions: [PackageConditionProtocol])
+        case target(_ target: ResolvedTarget, conditions: [PackageCondition])
 
         /// The target depends on this product.
-        case product(_ product: ResolvedProduct, conditions: [PackageConditionProtocol])
+        case product(_ product: ResolvedProduct, conditions: [PackageCondition])
 
         public var target: ResolvedTarget? {
             switch self {
@@ -38,7 +38,7 @@ public final class ResolvedTarget {
             }
         }
 
-        public var conditions: [PackageConditionProtocol] {
+        public var conditions: [PackageCondition] {
             switch self {
             case .target(_, let conditions): return conditions
             case .product(_, let conditions): return conditions
@@ -141,7 +141,7 @@ public final class ResolvedTarget {
     /// The list of platforms that are supported by this target.
     public let platforms: SupportedPlatforms
 
-    /// Create a target instance.
+    /// Create a resolved target instance.
     public init(
         target: Target,
         dependencies: [Dependency],
diff --git a/Sources/PackageLoading/PackageBuilder.swift b/Sources/PackageLoading/PackageBuilder.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2021 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -697,7 +697,7 @@ public final class PackageBuilder {
 
         // The created targets mapped to their name.
         var targets = [String: Target]()
-        // If a direcotry is empty, we don't create a target object for them.
+        // If a directory is empty, we don't create a target object for them.
         var emptyModules = Set<String>()
 
         // Start iterating the potential targets.
@@ -710,7 +710,7 @@ public final class PackageBuilder {
 
             // Get the dependencies of this target.
             let dependencies: [Target.Dependency] = try manifestTarget.map {
-                try $0.dependencies.compactMap { dependency in
+                try $0.dependencies.compactMap { dependency -> Target.Dependency? in
                     switch dependency {
                     case .target(let name, let condition):
                         // We don't create an object for targets which have no sources.
@@ -1145,12 +1145,12 @@ public final class PackageBuilder {
         return table
     }
 
-    func buildConditions(from condition: PackageConditionDescription?) -> [PackageConditionProtocol] {
-        var conditions: [PackageConditionProtocol] = []
+    func buildConditions(from condition: PackageConditionDescription?) -> [PackageCondition] {
+        var conditions = [PackageCondition]()
 
         if let config = condition?.config.flatMap({ BuildConfiguration(rawValue: $0) }) {
             let condition = ConfigurationCondition(configuration: config)
-            conditions.append(condition)
+            conditions.append(.configuration(condition))
         }
 
         if let platforms = condition?.platformNames.map({
@@ -1163,7 +1163,7 @@ public final class PackageBuilder {
            !platforms.isEmpty
         {
             let condition = PlatformsCondition(platforms: platforms)
-            conditions.append(condition)
+            conditions.append(.platforms(condition))
         }
 
         return conditions
diff --git a/Sources/PackageModel/BuildSettings.swift b/Sources/PackageModel/BuildSettings.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2018 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -43,14 +43,14 @@ public enum BuildSettings {
         /// The assignment value.
         public var values: [String]
 
-        // FIXME: This should be a set but we need Equatable existential (or AnyEquatable) for that.
+        // FIXME: This should use `Set` but we need to investigate potential build failures on Linux caused by using it.
         /// The condition associated with this assignment.
-        public var conditions: [PackageConditionProtocol] {
+        public var conditions: [PackageCondition] {
             get {
-                return _conditions.map{ $0.condition }
+                return _conditions.map { $0.underlying }
             }
             set {
-                _conditions = newValue.map{ PackageConditionWrapper($0) }
+                _conditions = newValue.map { PackageConditionWrapper($0) }
             }
         }
 
diff --git a/Sources/PackageModel/Manifest/PackageConditionDescription.swift b/Sources/PackageModel/Manifest/PackageConditionDescription.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2020 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -27,11 +27,12 @@ public protocol PackageConditionProtocol: Codable {
     func satisfies(_ environment: BuildEnvironment) -> Bool
 }
 
-/// Wrapper for package condition so it can be conformed to Codable.
+/// Wrapper for package condition so it can conform to Codable.
 struct PackageConditionWrapper: Codable, Equatable, Hashable {
     var platform: PlatformsCondition?
     var config: ConfigurationCondition?
 
+    @available(*, deprecated, renamed: "underlying")
     var condition: PackageConditionProtocol {
         if let platform {
             return platform
@@ -42,6 +43,17 @@ struct PackageConditionWrapper: Codable, Equatable, Hashable {
         }
     }
 
+    var underlying: PackageCondition {
+        if let platform {
+            return .platforms(platform)
+        } else if let config {
+            return .configuration(config)
+        } else {
+            fatalError("unreachable")
+        }
+    }
+
+    @available(*, deprecated, message: "Use .init(_ condition: PackageCondition) instead")
     init(_ condition: PackageConditionProtocol) {
         switch condition {
         case let platform as PlatformsCondition:
@@ -52,6 +64,55 @@ struct PackageConditionWrapper: Codable, Equatable, Hashable {
             fatalError("unknown condition \(condition)")
         }
     }
+
+    init(_ condition: PackageCondition) {
+        switch condition {
+        case let .platforms(platforms):
+            self.platform = platforms
+        case let .configuration(configuration):
+            self.config = configuration
+        }
+    }
+}
+
+/// One of possible conditions used in package manifests to restrict targets from being built for certain platforms or
+/// build configurations.
+public enum PackageCondition: Hashable {
+    case platforms(PlatformsCondition)
+    case configuration(ConfigurationCondition)
+
+    public func satisfies(_ environment: BuildEnvironment) -> Bool {
+        switch self {
+        case .configuration(let configuration):
+            return configuration.satisfies(environment)
+        case .platforms(let platforms):
+            return platforms.satisfies(environment)
+        }
+    }
+
+    public var platformsCondition: PlatformsCondition? {
+        guard case let .platforms(platformsCondition) = self else {
+            return nil
+        }
+
+        return platformsCondition
+    }
+
+    public var configurationCondition: ConfigurationCondition? {
+        guard case let .configuration(configurationCondition) = self else {
+            return nil
+        }
+
+        return configurationCondition
+    }
+
+    public init(platforms: [Platform]) {
+        self = .platforms(.init(platforms: platforms))
+    }
+
+    public init(configuration: BuildConfiguration) {
+        self = .configuration(.init(configuration: configuration))
+    }
 }
 
 /// Platforms condition implies that an assignment is valid on these platforms.
diff --git a/Sources/PackageModel/Target/Target.swift b/Sources/PackageModel/Target/Target.swift
@@ -79,10 +79,10 @@ public class Target: PolymorphicCodableProtocol {
     /// A target dependency to a target or product.
     public enum Dependency {
         /// A dependency referencing another target, with conditions.
-        case target(_ target: Target, conditions: [PackageConditionProtocol])
+        case target(_ target: Target, conditions: [PackageCondition])
 
         /// A dependency referencing a product, with conditions.
-        case product(_ product: ProductReference, conditions: [PackageConditionProtocol])
+        case product(_ product: ProductReference, conditions: [PackageCondition])
 
         /// The target if the dependency is a target dependency.
         public var target: Target? {
@@ -103,7 +103,7 @@ public class Target: PolymorphicCodableProtocol {
         }
 
         /// The dependency conditions.
-        public var conditions: [PackageConditionProtocol] {
+        public var conditions: [PackageCondition] {
             switch self {
             case .target(_, let conditions):
                 return conditions
diff --git a/Sources/SPMBuildCore/Plugins/PluginInvocation.swift b/Sources/SPMBuildCore/Plugins/PluginInvocation.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2021-2022 Apple Inc. and the Swift project authors
+// Copyright (c) 2021-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -584,7 +584,7 @@ public extension PluginTarget {
 }
 
 fileprivate extension Target.Dependency {
-    var conditions: [PackageConditionProtocol] {
+    var conditions: [PackageCondition] {
         switch self {
         case .target(_, let conditions): return conditions
         case .product(_, let conditions): return conditions
diff --git a/Sources/XCBuildSupport/PIF.swift b/Sources/XCBuildSupport/PIF.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2020 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -1002,7 +1002,7 @@ public enum PIF {
             }
 
             public var conditions: [String] {
-                let filters = [PlatformsCondition(platforms: [packageModelPlatform])].toPlatformFilters().map { (filter: PIF.PlatformFilter) -> String in
+                let filters = [PackageCondition(platforms: [packageModelPlatform])].toPlatformFilters().map { filter in
                     if filter.environment.isEmpty {
                         return filter.platform
                     } else {
diff --git a/Sources/XCBuildSupport/PIFBuilder.swift b/Sources/XCBuildSupport/PIFBuilder.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2021 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -781,7 +781,7 @@ final class PackagePIFProjectBuilder: PIFProjectBuilder {
     private func addDependency(
         to target: ResolvedTarget,
         in pifTarget: PIFTargetBuilder,
-        conditions: [PackageConditionProtocol],
+        conditions: [PackageCondition],
         linkProduct: Bool
     ) {
         // Only add the binary target as a library when we want to link against the product.
@@ -803,7 +803,7 @@ final class PackagePIFProjectBuilder: PIFProjectBuilder {
     private func addDependency(
         to product: ResolvedProduct,
         in pifTarget: PIFTargetBuilder,
-        conditions: [PackageConditionProtocol],
+        conditions: [PackageCondition],
         linkProduct: Bool
     ) {
         pifTarget.addDependency(
@@ -1498,15 +1498,15 @@ private extension BuildSettings.AssignmentTable {
 
 private extension BuildSettings.Assignment {
     var configurations: [BuildConfiguration] {
-        if let configurationCondition = conditions.lazy.compactMap({ $0 as? ConfigurationCondition }).first {
+        if let configurationCondition = conditions.lazy.compactMap(\.configurationCondition).first {
             return [configurationCondition.configuration]
         } else {
             return BuildConfiguration.allCases
         }
     }
 
     var pifPlatforms: [PIF.BuildSettings.Platform]? {
-        if let platformsCondition = conditions.lazy.compactMap({ $0 as? PlatformsCondition }).first {
+        if let platformsCondition = conditions.lazy.compactMap(\.platformsCondition).first {
             return platformsCondition.platforms.compactMap { PIF.BuildSettings.Platform(rawValue: $0.name) }
         } else {
             return nil
@@ -1537,10 +1537,10 @@ public struct DelayedImmutable<Value> {
     }
 }
 
-extension Array where Element == PackageConditionProtocol {
+extension [PackageCondition] {
     func toPlatformFilters() -> [PIF.PlatformFilter] {
         var result: [PIF.PlatformFilter] = []
-        let platformConditions = self.compactMap{ $0 as? PlatformsCondition }.flatMap{ $0.platforms }
+        let platformConditions = self.compactMap(\.platformsCondition).flatMap { $0.platforms }
 
         for condition in platformConditions {
             switch condition {
diff --git a/Tests/PackageLoadingTests/PackageBuilderTests.swift b/Tests/PackageLoadingTests/PackageBuilderTests.swift
@@ -2,7 +2,7 @@
 //
 // This source file is part of the Swift open source project
 //
-// Copyright (c) 2014-2021 Apple Inc. and the Swift project authors
+// Copyright (c) 2014-2023 Apple Inc. and the Swift project authors
 // Licensed under Apache License v2.0 with Runtime Library Exception
 //
 // See http://swift.org/LICENSE.txt for license information
@@ -19,8 +19,7 @@ import XCTest
 import class TSCBasic.InMemoryFileSystem
 
 /// Tests for the handling of source layout conventions.
-class PackageBuilderTests: XCTestCase {
-
+final class PackageBuilderTests: XCTestCase {
     func testDotFilesAreIgnored() throws {
         let fs = InMemoryFileSystem(emptyFiles:
             "/Sources/foo/.Bar.swift",
@@ -2990,7 +2989,7 @@ class PackageBuilderTests: XCTestCase {
 
         var assignment = BuildSettings.Assignment()
         assignment.values = ["YOLO"]
-        assignment.conditions = [PlatformsCondition(platforms: [PackageModel.Platform.custom(name: "bestOS", oldestSupportedVersion: .unknown)])]
+        assignment.conditions = [PackageCondition(platforms: [.custom(name: "bestOS", oldestSupportedVersion: .unknown)])]
 
         var settings = BuildSettings.AssignmentTable()
         settings.add(assignment, for: .SWIFT_ACTIVE_COMPILATION_CONDITIONS)
