refactor ManagedDependencies

motivation: ManagedDependencies are based on location instead of on identity, refactor them to be based on identity as part of the transition to identity based logic

changes:
* transition ManagedDependencies underlying hashmap to be based on identity
* update the ManagedDependencies edit state to include the "basedOn" property which is only relevant to that state
* update calls ites to use package identity instead of location when reading/writing managed depedencies
* refactor related call-sites to handle edited state more safely and correctly
* add code comments to explain some of the more subtle business logic
* adjust tests where needed

Author: tomerd

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -22,7 +22,7 @@ extension PackageGraph {
         root: PackageGraphRoot,
         identityResolver: IdentityResolver,
         additionalFileRules: [FileRuleDescription] = [],
-        externalManifests: [Manifest],
+        externalManifests: OrderedDictionary<PackageIdentity, Manifest>,
         requiredDependencies: Set<PackageReference> = [],
         unsafeAllowedPackages: Set<PackageReference> = [],
         binaryArtifacts: [BinaryArtifact] = [],
@@ -36,10 +36,19 @@ extension PackageGraph {
 
         // Create a map of the manifests, keyed by their identity.
         let rootManifestsMap = root.manifests
-        let externalManifestsMap = externalManifests.map{ (identityResolver.resolveIdentity(for: $0.packageLocation), $0) }
-        let manifestMap = rootManifestsMap.merging(externalManifestsMap, uniquingKeysWith: { lhs, rhs in
-            return lhs
-        })
+        //let externalManifestsMap = externalManifests.map{ (identityResolver.resolveIdentity(for: $0.packageLocation), $0) }
+        //let manifestMap = rootManifestsMap.merging(externalManifests, uniquingKeysWith: { lhs, rhs in
+          //  return lhs
+        //})
+
+        // prefer roots
+        var manifestMap = rootManifestsMap
+        externalManifests.forEach {
+            if !manifestMap.keys.contains($0.key) {
+                manifestMap[$0.key] = $0.value
+            }
+        }
+
 
         let successors: (GraphLoadingNode) -> [GraphLoadingNode] = { node in
             node.requiredDependencies().compactMap{ dependency in

Sources/PackageModel/PackageReference.swift

@@ -71,19 +71,20 @@ public struct PackageReference: Codable {
 
 extension PackageReference: Equatable {
     public static func ==(lhs: PackageReference, rhs: PackageReference) -> Bool {
-        return lhs.identity == rhs.identity
+        return lhs.identity == rhs.identity //&& lhs.location == rhs.location
     }
 }
 
 extension PackageReference: Hashable {
     public func hash(into hasher: inout Hasher) {
-        hasher.combine(identity)
+        hasher.combine(self.identity)
+        //hasher.combine(self.location)
     }
 }
 
 extension PackageReference: CustomStringConvertible {
     public var description: String {
-        return "\(identity)\(self.location.isEmpty ? "" : "[\(self.location)]")"
+        return "\(self.identity)\(self.location.isEmpty ? "" : "[\(self.location)]")"
     }
 }
 

Sources/SPMTestSupport/MockWorkspace.swift

@@ -413,7 +413,7 @@ public final class MockWorkspace {
 
     public func set(
         pins: [PackageReference: CheckoutState] = [:],
-        managedDependencies: [ManagedDependency] = [],
+        managedDependencies: [Workspace.ManagedDependency] = [],
         managedArtifacts: [Workspace.ManagedArtifact] = []
     ) throws {
         let workspace = try self.getOrCreateWorkspace()
@@ -434,7 +434,7 @@ public final class MockWorkspace {
             workspace.state.artifacts.add(artifact)
         }
 
-        try workspace.state.saveState()
+        try workspace.state.save()
     }
 
     public func resetState() throws {
@@ -455,24 +455,32 @@ public final class MockWorkspace {
     }
 
     public struct ManagedDependencyResult {
-        public let managedDependencies: ManagedDependencies
+        public let managedDependencies: Workspace.ManagedDependencies
 
-        public init(_ managedDependencies: ManagedDependencies) {
+        public init(_ managedDependencies: Workspace.ManagedDependencies) {
             self.managedDependencies = managedDependencies
         }
 
         public func check(notPresent name: String, file: StaticString = #file, line: UInt = #line) {
-            let dependency = self.managedDependencies[forNameOrIdentity: name]
-            XCTAssert(dependency == nil, "Unexpectedly found \(name) in managed dependencies", file: file, line: line)
+            self.check(notPresent: .plain(name), file: file, line: line)
+        }
+
+        public func check(notPresent dependencyId: PackageIdentity, file: StaticString = #file, line: UInt = #line) {
+            let dependency = self.managedDependencies[dependencyId]
+            XCTAssertNil(dependency, "Unexpectedly found \(dependencyId) in managed dependencies", file: file, line: line)
         }
 
         public func checkEmpty(file: StaticString = #file, line: UInt = #line) {
             XCTAssertEqual(self.managedDependencies.count, 0, file: file, line: line)
         }
 
         public func check(dependency name: String, at state: State, file: StaticString = #file, line: UInt = #line) {
-            guard let dependency = managedDependencies[forNameOrIdentity: name] else {
-                XCTFail("\(name) does not exists", file: file, line: line)
+            self.check(dependency: .plain(name), at: state, file: file, line: line)
+        }
+
+        public func check(dependency dependencyId: PackageIdentity, at state: State, file: StaticString = #file, line: UInt = #line) {
+            guard let dependency = managedDependencies[dependencyId] else {
+                XCTFail("\(dependencyId) does not exists", file: file, line: line)
                 return
             }
             switch state {
@@ -486,8 +494,9 @@ public final class MockWorkspace {
                     XCTAssertEqual(dependency.checkoutState?.branch, branch, file: file, line: line)
                 }
             case .edited(let path):
-                if dependency.state != .edited(path) {
+                guard case .edited(_,  unmanagedPath: path) = dependency.state else {
                     XCTFail("Expected edited dependency; found '\(dependency.state)' instead", file: file, line: line)
+                    return
                 }
             case .local:
                 if dependency.state != .local {
@@ -764,3 +773,12 @@ extension CheckoutState {
         }
     }
 }
+
+extension Workspace.ManagedDependency {
+    public var checkoutState: CheckoutState? {
+        if case .checkout(let checkoutState) = state {
+            return checkoutState
+        }
+        return nil
+    }
+}

Sources/SPMTestSupport/misc.swift

@@ -229,7 +229,10 @@ public func loadPackageGraph(
     useXCBuildFileRules: Bool = false
 ) throws -> PackageGraph {
     let rootManifests = manifests.filter { $0.packageKind == .root }.spm_createDictionary{ ($0.path, $0) }
-    let externalManifests = manifests.filter { $0.packageKind != .root }
+    let externalManifests = manifests.filter { $0.packageKind != .root }.reduce(into: OrderedDictionary<PackageIdentity, Manifest>()) { partial, item in
+        partial[identityResolver.resolveIdentity(for: item.packageLocation)] = item
+    }
+
     let packages = Array(rootManifests.keys)
     let input = PackageGraphRootInput(packages: packages)
     let graphRoot = PackageGraphRoot(input: input, manifests: rootManifests, explicitProduct: explicitProduct)