[PackageGraph] Use OrderedDictionary to store assignments

The assignments were stored in a regular dictionary which caused
non-deterministic crashes when a package graph contained dependencies
that were required via a revison and version in different subtrees.
Since the order in which the constrained appear matters, the assignment
also needs to be stored in order.

<rdar://problem/43107896>

Author: aciidgh

Sources/PackageGraph/DependencyResolver.swift

@@ -534,7 +534,7 @@ struct VersionAssignmentSet<C: PackageContainer>: Equatable, Sequence {
     //
     // FIXME: Does it really make sense to key on the identifier here. Should we
     // require referential equality of containers and use that to simplify?
-    fileprivate var assignments: [Identifier: (container: Container, binding: BoundVersion)]
+    fileprivate var assignments: OrderedDictionary<Identifier, (container: Container, binding: BoundVersion)>
 
     /// Create an empty assignment.
     init() {
@@ -681,7 +681,7 @@ struct VersionAssignmentSet<C: PackageContainer>: Equatable, Sequence {
     /// Check if the assignment is valid and complete.
     func checkIfValidAndComplete() -> Bool {
         // Validity should hold trivially, because it is an invariant of the collection.
-        for assignment in assignments.values {
+        for (_, assignment) in assignments {
             if !isValid(binding: assignment.binding, for: assignment.container) {
                 return false
             }
@@ -709,9 +709,9 @@ struct VersionAssignmentSet<C: PackageContainer>: Equatable, Sequence {
     typealias Iterator = AnyIterator<(Container, BoundVersion)>
 
     func makeIterator() -> Iterator {
-        var it = assignments.values.makeIterator()
+        var it = assignments.makeIterator()
         return AnyIterator {
-            if let next = it.next() {
+            if let (_, next) = it.next() {
                 return (next.container, next.binding)
             } else {
                 return nil

Tests/PackageGraphTests/DependencyResolverTests.swift

@@ -484,6 +484,45 @@ class DependencyResolverTests: XCTestCase {
         }
     }
 
+    func testRevisionConstraint2() throws {
+        // Test that requiring revision constraint for a dependency that is
+        // required via version transitively resolves correctly.
+
+        let develop = "develop"
+
+        let provider = MockPackagesProvider(containers: [
+            MockPackageContainer(name: "A", dependencies: [
+                develop: [
+                    (container: "B", requirement: .versionSet(v1Range)),
+                ],
+            ]),
+
+            MockPackageContainer(name: "B", dependencies: [
+                "1.1.0": [],
+                develop: [],
+            ]),
+
+            MockPackageContainer(name: "C", dependencies: [
+                develop: [
+                    (container: "A", requirement: .revision(develop)),
+                    (container: "B", requirement: .revision(develop)),
+                ],
+            ]),
+        ])
+
+        do {
+            let resolver = MockDependencyResolver(provider, MockResolverDelegate())
+            let result = try resolver.resolve(constraints: [
+                MockPackageConstraint(container: "C", requirement: .revision(develop)),
+            ])
+            XCTAssertEqual(result, [
+                "A": .revision(develop),
+                "B": .revision(develop),
+                "C": .revision(develop),
+            ])
+        }
+    }
+
     func testUnversionedConstraint() throws {
         let provider = MockPackagesProvider(containers: [
             MockPackageContainer(name: "A", dependenciesByVersion: [v1: [], v1_1: []]),

Tests/PackageGraphTests/XCTestManifests.swift

@@ -16,6 +16,7 @@ extension DependencyResolverTests {
         ("testPrereleaseResolve", testPrereleaseResolve),
         ("testResolve", testResolve),
         ("testResolveSubtree", testResolveSubtree),
+        ("testRevisionConstraint2", testRevisionConstraint2),
         ("testRevisionConstraint", testRevisionConstraint),
         ("testUnversionedConstraint", testUnversionedConstraint),
         ("testVersionAssignment", testVersionAssignment),