[DependencyResolver] Use merging for constraint container update.

 - This fixes some subtlety in the previous code about which merge operations
   could perform partial update on the container, some call sites relied
   critically on one container not doing that.

 - Instead, switch both containers to only expose a `merging` method which
   provides a new result. This is somewhat less efficient with the current
   implementations, but should go out in the wash once we switch to more
   efficient implementations backed by persistent data structures.

Author: ddunbar

Sources/PackageGraph/DependencyResolver.swift

@@ -208,6 +208,10 @@ func ==(_ lhs: BoundVersion, _ rhs: BoundVersion) -> Bool {
 }
 
 /// A container for constraints for a set of packages.
+///
+/// This data structure is only designed to represent satisfiable constraint
+/// sets, it cannot represent sets including containers which have an empty
+/// constraint.
 //
 // FIXME: Maybe each package should just return this, instead of a list of
 // `PackageContainerConstraint`s. That won't work if we decide this should
@@ -229,7 +233,10 @@ struct PackageContainerConstraintSet<C: PackageContainer>: Collection {
     }
 
     /// Create an constraint set from known values.
+    ///
+    /// The initial constraints should never be unsatisfiable.
     init(_ constraints: [Identifier: VersionSetSpecifier]) {
+        assert(constraints.values.filter({ $0 == .empty }).isEmpty)
         self.constraints = constraints
     }
 
@@ -243,41 +250,45 @@ struct PackageContainerConstraintSet<C: PackageContainer>: Collection {
         return constraints[identifier] ?? .any
     }
 
-    /// Merge the given version requirement for the container `identifier`.
+    /// Create a constraint set by merging the `versionRequirement` for container `identifier`.
     ///
-    /// - Returns: False if the merger has made the set unsatisfiable; i.e. true
-    /// when the resulting set is satisfiable, if it was already so.
-    private mutating func merge(versionRequirement: VersionSetSpecifier, for identifier: Identifier) -> Bool {
-        let intersection: VersionSetSpecifier
-        if let existing = constraints[identifier] {
-            intersection = existing.intersection(versionRequirement)
-        } else {
-            intersection = versionRequirement
+    /// - Returns: The new set, or nil the resulting set is unsatisfiable.
+    private func merging(
+        versionRequirement: VersionSetSpecifier, for identifier: Identifier
+    ) -> PackageContainerConstraintSet<C>?
+    {
+        let intersection = self[identifier].intersection(versionRequirement)
+        if intersection == .empty {
+            return nil
         }
-        constraints[identifier] = intersection
-        return intersection != .empty
+        var result = self
+        result.constraints[identifier] = intersection
+        return result
     }
 
-    /// Merge the given `constraint`.
+    /// Create a constraint set by merging `constraint`.
     ///
-    /// - Returns: False if the merger has made the set unsatisfiable; i.e. true
-    /// when the resulting set is satisfiable, if it was already so.
-    mutating func merge(_ constraint: PackageContainerConstraint<Identifier>) -> Bool {
-        return merge(versionRequirement: constraint.versionRequirement, for: constraint.identifier)
+    /// - Returns: The new set, or nil the resulting set is unsatisfiable.
+    func merging(_ constraint: PackageContainerConstraint<Identifier>) -> PackageContainerConstraintSet<C>? {
+        return merging(versionRequirement: constraint.versionRequirement, for: constraint.identifier)
     }
 
-    /// Merge the given constraint set.
+    /// Create a new constraint set by merging the given constraint set.
     ///
     /// - Returns: False if the merger has made the set unsatisfiable; i.e. true
     /// when the resulting set is satisfiable, if it was already so.
-    mutating func merge(_ constraints: PackageContainerConstraintSet<Container>) -> Bool {
-        var satisfiable = true
+    func merging(
+        _ constraints: PackageContainerConstraintSet<Container>
+    ) -> PackageContainerConstraintSet<C>?
+    {
+        var result = self
         for (key, versionRequirement) in constraints {
-            if !merge(versionRequirement: versionRequirement, for: key) {
-                satisfiable = false
+            guard let merged = result.merging(versionRequirement: versionRequirement, for: key) else {
+                return nil
             }
+            result = merged
         }
-        return satisfiable
+        return result
     }
 
     // MARK: Collection Conformance
@@ -343,41 +354,36 @@ struct VersionAssignmentSet<C: PackageContainer>: Sequence {
         }
     }
 
-    /// Merge in the bindings from the given `assignment`.
+    /// Create a new assignment set by merging in the bindings from `assignment`.
     ///
-    /// - Returns: False if the merge cannot be made (the assignments contain
-    /// incompatible versions).
-    mutating func merge(_ assignment: VersionAssignmentSet<Container>) -> Bool {
+    /// - Returns: The new assignment, or nil if the merge cannot be made (the
+    /// assignments contain incompatible versions).
+    func merging(_ assignment: VersionAssignmentSet<Container>) -> VersionAssignmentSet<Container>? {
         // In order to protect the assignment set, we first have to test whether
         // the merged constraint sets are satisfiable.
         //
-        // FIXME: Move to non-mutating methods with results, in order to have a
-        // nice consistent API with `PackageContainerConstraintSet.merge`.
-        //
         // FIXME: This is very inefficient; we should decide whether it is right
         // to handle it here or force the main resolver loop to handle the
         // discovery of this property.
-        var mergedConstraints = constraints
-        guard mergedConstraints.merge(assignment.constraints) else {
-            return false
+        guard let _ = constraints.merging(assignment.constraints) else {
+            return nil
         }
 
         // The induced constraints are satisfiable, so we *can* union the
         // assignments without breaking our internal invariant on
         // satisfiability.
+        var result = self
         for (container, binding) in assignment {
-            if let existing = self[container] {
+            if let existing = result[container] {
                 if existing != binding {
-                    // NOTE: We are returning here with the data structure
-                    // partially updated, which feels wrong. See FIXME above.
-                    return false
+                    return nil
                 }
             } else {
-                self[container] = binding
+                result[container] = binding
             }
         }
 
-        return true
+        return result
     }
 
     /// The combined version constraints induced by the assignment.
@@ -407,8 +413,10 @@ struct VersionAssignmentSet<C: PackageContainer>: Sequence {
                 // FIXME: Error handling, except that we probably shouldn't have
                 // needed to refetch the dependencies at this point.
                 for constraint in try! container.getDependencies(at: version) {
-                    let satisfiable = result.merge(constraint)
-                    assert(satisfiable)
+                    guard let merged = result.merging(constraint) else {
+                        preconditionFailure("unsatisfiable constraint set")
+                    }
+                    result = merged
                 }
             }
         }
@@ -661,9 +669,10 @@ public class DependencyResolver<
         // FIXME: We should have a test for this, probably by adding some kind
         // of statistics on the number of backtracks.
         for constraint in constraints {
-            if !allConstraints.merge(constraint) {
+            guard let merged = allConstraints.merging(constraint) else {
                 return nil
             }
+            allConstraints = merged
         }
 
         for constraint in constraints {
@@ -685,12 +694,9 @@ public class DependencyResolver<
                 throw DependencyResolverError.unimplemented
             }
 
-            // We found a valid assignment, attempt to merge it with the current solution.
-            //
-            // FIXME: It is rather important, subtle, and confusing that this
-            // `merge` doesn't mutate the assignment but the one on the
-            // constraint set does. We should probably make them consistent.
-            guard assignment.merge(subtreeAssignment) else {
+            // We found a valid subtree assignment, attempt to merge it with the
+            // current solution.
+            guard let newAssignment = assignment.merging(subtreeAssignment) else {
                 // The assignment couldn't be merged with the current
                 // assignment, or the constraint sets couldn't be merged.
                 //
@@ -701,13 +707,16 @@ public class DependencyResolver<
                 throw DependencyResolverError.unimplemented
             }
 
-            // Merge the working constraint set.
+            // Update the working assignment and constraint set.
             //
             // This should always be feasible, because all prior constraints
             // were part of the input constraint request (see comment around
             // initial `merge` outside the loop).
-            let mergable = allConstraints.merge(subtreeAssignment.constraints)
-            precondition(mergable)
+            assignment = newAssignment
+            guard let merged = allConstraints.merging(subtreeAssignment.constraints) else {
+                preconditionFailure("unsatisfiable constraints while merging subtree")
+            }
+            allConstraints = merged
         }
 
         return assignment

Tests/PackageGraphTests/DependencyResolverTests.swift

@@ -141,27 +141,25 @@ class DependencyResolverTests: XCTestCase {
         XCTAssertEqual(set.containerIdentifiers.map{ $0 }, [])
 
         // Check basics.
-        XCTAssertTrue(set.merge(MockPackageConstraint(container: "A", versionRequirement: v1Range)))
+        set = set.merging(MockPackageConstraint(container: "A", versionRequirement: v1Range))!
         XCTAssertEqual(set.containerIdentifiers.map{ $0 }, ["A"])
         XCTAssertEqual(set["A"], v1Range)
-        XCTAssertTrue(set.merge(MockPackageConstraint(container: "B", versionRequirement: v2Range)))
+        set = set.merging(MockPackageConstraint(container: "B", versionRequirement: v2Range))!
         XCTAssertEqual(set.containerIdentifiers.sorted(), ["A", "B"])
 
         // Check merging a constraint which makes the set unsatisfiable.
-        XCTAssertFalse(set.merge(MockPackageConstraint(container: "A", versionRequirement: v2Range)))
-        XCTAssertEqual(set["A"], VersionSetSpecifier.empty)
+        XCTAssert(set.merging(MockPackageConstraint(container: "A", versionRequirement: v2Range)) == nil)
 
         // Check merging other sets.
         var set2 = ConstraintSet()
-        _ = set2.merge(MockPackageConstraint(container: "C", versionRequirement: v1Range))
-        XCTAssertTrue(set.merge(set2))
+        set2 = set2.merging(MockPackageConstraint(container: "C", versionRequirement: v1Range))!
+        set = set.merging(set2)!
         XCTAssertEqual(set.containerIdentifiers.map{ $0 }.sorted(), ["A", "B", "C"])
         var set3 = ConstraintSet()
-        _ = set3.merge(MockPackageConstraint(container: "C", versionRequirement: v2Range))
-        _ = set3.merge(MockPackageConstraint(container: "D", versionRequirement: v1Range))
-        _ = set3.merge(MockPackageConstraint(container: "E", versionRequirement: v1Range))
-        XCTAssertFalse(set.merge(set3)) // "C" requirement is unsatisfiable
-        XCTAssertEqual(set.containerIdentifiers.map{ $0 }.sorted(), ["A", "B", "C", "D", "E"])
+        set3 = set3.merging(MockPackageConstraint(container: "C", versionRequirement: v2Range))!
+        set3 = set3.merging(MockPackageConstraint(container: "D", versionRequirement: v1Range))!
+        set3 = set3.merging(MockPackageConstraint(container: "E", versionRequirement: v1Range))!
+        XCTAssert(set.merging(set3) == nil) // "C" requirement is unsatisfiable
     }
 
     func testVersionAssignment() {
@@ -214,21 +212,24 @@ class DependencyResolverTests: XCTestCase {
                 v2: []])
         var assignment2 = VersionAssignmentSet<MockPackageContainer>()
         assignment2[d] = .version(v1)
-        XCTAssertTrue(assignment.merge(assignment2))
+        if let mergedAssignment = assignment.merging(assignment2) {
+            assignment = mergedAssignment
+        } else {
+            return XCTFail("unexpected failure merging assignment")
+        }
         XCTAssertEqual(assignment.constraints, ["C": v1_0Range, "E": v1Range])
 
         // Check merger of an assignment with incompatible constraints.
         let d2 = MockPackageContainer(name: "D2", dependenciesByVersion: [
                 v1: [(container: "E", versionRequirement: v2Range)]])
         var assignment3 = VersionAssignmentSet<MockPackageContainer>()
         assignment3[d2] = .version(v1)
-        XCTAssertFalse(assignment.merge(assignment3))
-        XCTAssertEqual(assignment.constraints, ["C": v1_0Range, "E": v1Range])
+        XCTAssertEqual(assignment.merging(assignment3), nil)
 
         // Check merger of an incompatible assignment.
         var assignment4 = VersionAssignmentSet<MockPackageContainer>()
         assignment4[d] = .version(v2)
-        XCTAssertFalse(assignment.merge(assignment4))
+        XCTAssertEqual(assignment.merging(assignment4), nil)
     }
 
     /// Check the basic situations for resolving a subtree.