Filter out duplicate package manifest conditions

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. 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 package. There's no use case for externally defined types for this protocol. That allows us to convert uses of `[any PackageConditionProtocol]` to `Set<PackageCondition>`. Existing protocol kept around until clients of SwiftPM can migrate to the new `PackageCondition` enum.

Author: MaxDesiatov

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: Set<PackageCondition>)
 
         /// Dependency to a product, with conditions.
-        case product(_ product: ResolvedProductBuilder, conditions: [PackageConditionProtocol])
+        case product(_ product: ResolvedProductBuilder, conditions: Set<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)

Sources/PackageGraph/Resolution/ResolvedTarget.swift

@@ -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: Set<PackageCondition>)
 
         /// The target depends on this product.
-        case product(_ product: ResolvedProduct, conditions: [PackageConditionProtocol])
+        case product(_ product: ResolvedProduct, conditions: Set<PackageCondition>)
 
         public var target: ResolvedTarget? {
             switch self {
@@ -38,7 +38,7 @@ public final class ResolvedTarget {
             }
         }
 
-        public var conditions: [PackageConditionProtocol] {
+        public var conditions: Set<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],

Sources/PackageLoading/PackageBuilder.swift

@@ -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.
@@ -1136,7 +1136,7 @@ public final class PackageBuilder {
             // Create an assignment for this setting.
             var assignment = BuildSettings.Assignment()
             assignment.values = values
-            assignment.conditions = self.buildConditions(from: setting.condition)
+            assignment.uniqueConditions = self.buildConditions(from: setting.condition)
 
             // Finally, add the assignment to the assignment table.
             table.add(assignment, for: decl)
@@ -1145,12 +1145,12 @@ public final class PackageBuilder {
         return table
     }
 
-    func buildConditions(from condition: PackageConditionDescription?) -> [PackageConditionProtocol] {
-        var conditions: [PackageConditionProtocol] = []
+    func buildConditions(from condition: PackageConditionDescription?) -> Set<PackageCondition> {
+        var conditions: Set<PackageCondition> = []
 
         if let config = condition?.config.flatMap({ BuildConfiguration(rawValue: $0) }) {
             let condition = ConfigurationCondition(configuration: config)
-            conditions.append(condition)
+            conditions.insert(.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.insert(.platforms(condition))
         }
 
         return conditions

Sources/PackageModel/BuildSettings.swift

@@ -43,15 +43,25 @@ 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.
         /// The condition associated with this assignment.
+        @available(*, deprecated, renamed: "uniqueConditions")
         public var conditions: [PackageConditionProtocol] {
             get {
                 return _conditions.map{ $0.condition }
             }
             set {
                 _conditions = newValue.map{ PackageConditionWrapper($0) }
             }
+        }        
+
+        /// A set of unique conditions associated with this assignment.
+        public var uniqueConditions: Set<PackageCondition> {
+            get {
+                return Set(_conditions.map(\.underlying))
+            }
+            set {
+                _conditions = newValue.map { PackageConditionWrapper($0) }
+            }
         }
 
         private var _conditions: [PackageConditionWrapper]
@@ -64,16 +74,15 @@ public enum BuildSettings {
 
     /// Build setting assignment table which maps a build setting to a list of assignments.
     public struct AssignmentTable: Codable {
-        public private(set) var assignments: [Declaration: [Assignment]]
+        public private(set) var assignments: [Declaration: Set<Assignment>]
 
         public init() {
             assignments = [:]
         }
 
         /// Add the given assignment to the table.
         mutating public func add(_ assignment: Assignment, for decl: Declaration) {
-            // FIXME: We should check for duplicate assignments.
-            assignments[decl, default: []].append(assignment)
+            assignments[decl, default: []].insert(assignment)
         }
     }
 
@@ -102,7 +111,7 @@ public enum BuildSettings {
             // Add values from each assignment if it satisfies the build environment.
             let values = assignments
                 .lazy
-                .filter { $0.conditions.allSatisfy { $0.satisfies(self.environment) } }
+                .filter { $0.uniqueConditions.allSatisfy { $0.satisfies(self.environment) } }
                 .flatMap { $0.values }
 
             return Array(values)

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,53 @@ 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
+        }
+    }
+}
+
+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.

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: Set<PackageCondition>)
 
         /// A dependency referencing a product, with conditions.
-        case product(_ product: ProductReference, conditions: [PackageConditionProtocol])
+        case product(_ product: ProductReference, conditions: Set<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: Set<PackageCondition> {
             switch self {
             case .target(_, let conditions):
                 return conditions

Sources/SPMBuildCore/Plugins/PluginInvocation.swift

@@ -584,7 +584,7 @@ public extension PluginTarget {
 }
 
 fileprivate extension Target.Dependency {
-    var conditions: [PackageConditionProtocol] {
+    var conditions: Set<PackageCondition> {
         switch self {
         case .target(_, let conditions): return conditions
         case .product(_, let conditions): return conditions

Sources/XCBuildSupport/PIF.swift

@@ -1002,7 +1002,8 @@ public enum PIF {
             }
 
             public var conditions: [String] {
-                let filters = [PlatformsCondition(platforms: [packageModelPlatform])].toPlatformFilters().map { (filter: PIF.PlatformFilter) -> String in
+                let filters = Set([.init(platforms: [packageModelPlatform])])
+                    .toPlatformFilters().map { (filter: PIF.PlatformFilter) -> String in
                     if filter.environment.isEmpty {
                         return filter.platform
                     } else {

Sources/XCBuildSupport/PIFBuilder.swift

@@ -781,7 +781,7 @@ final class PackagePIFProjectBuilder: PIFProjectBuilder {
     private func addDependency(
         to target: ResolvedTarget,
         in pifTarget: PIFTargetBuilder,
-        conditions: [PackageConditionProtocol],
+        conditions: Set<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: Set<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 = uniqueConditions.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 = uniqueConditions.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 Set<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 {

Tests/PackageLoadingTests/PackageBuilderTests.swift

@@ -2990,7 +2990,7 @@ class PackageBuilderTests: XCTestCase {
 
         var assignment = BuildSettings.Assignment()
         assignment.values = ["YOLO"]
-        assignment.conditions = [PlatformsCondition(platforms: [PackageModel.Platform.custom(name: "bestOS", oldestSupportedVersion: .unknown)])]
+        assignment.uniqueConditions = Set([.init(platforms: [PackageModel.Platform.custom(name: "bestOS", oldestSupportedVersion: .unknown)])])
 
         var settings = BuildSettings.AssignmentTable()
         settings.add(assignment, for: .SWIFT_ACTIVE_COMPILATION_CONDITIONS)