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] = [],
@@ -35,11 +35,11 @@ extension PackageGraph {
         let observabilityScope = ObservabilitySystem.topScope.makeChildScope(description: "Loading Package Graph")
 
         // Create a map of the manifests, keyed by their identity.
-        let rootManifestsMap = root.manifests
-        let externalManifestsMap = try externalManifests.map{ (try identityResolver.resolveIdentity(for: $0.packageKind), $0) }
-        let manifestMap = rootManifestsMap.merging(externalManifestsMap, uniquingKeysWith: { lhs, rhs in
-            return lhs
-        })
+        var manifestMap = externalManifests
+        // prefer roots
+        root.manifests.forEach {
+            manifestMap[$0.key] = $0.value
+        }
 
         let successors: (GraphLoadingNode) -> [GraphLoadingNode] = { node in
             node.requiredDependencies().compactMap{ dependency in

Sources/PackageModel/PackageIdentity.swift

@@ -46,7 +46,7 @@ public struct PackageIdentity: CustomStringConvertible {
     }
 
     /// Creates a package identity from a URL.
-    /// - Parameter url: The package's URL.
+    /// - Parameter urlString: The package's URL.
     // FIXME: deprecate this
     public init(urlString: String) {
         self.description = Self.provider.init(urlString).description

Sources/PackageModel/PackageReference.swift

@@ -115,14 +115,16 @@ public struct PackageReference: Encodable {
 }
 
 extension PackageReference: Equatable {
+    // TODO: consider location as well?
     public static func ==(lhs: PackageReference, rhs: PackageReference) -> Bool {
         return lhs.identity == rhs.identity
     }
 }
 
 extension PackageReference: Hashable {
+    // TODO: consider location as well?
     public func hash(into hasher: inout Hasher) {
-        hasher.combine(identity)
+        hasher.combine(self.identity)
     }
 }
 

Sources/SPMTestSupport/MockWorkspace.swift

@@ -405,7 +405,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()
@@ -426,7 +426,7 @@ public final class MockWorkspace {
             workspace.state.artifacts.add(artifact)
         }
 
-        try workspace.state.saveState()
+        try workspace.state.save()
     }
 
     public func resetState() throws {
@@ -447,24 +447,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 {
@@ -478,8 +486,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 {
@@ -757,8 +766,8 @@ extension CheckoutState {
     }
 }
 
-fileprivate extension PackageReference.Kind {
-    var displayName: String {
+extension PackageReference.Kind {
+    fileprivate var displayName: String {
         switch self {
         case .root:
             return "root"
@@ -771,3 +780,12 @@ fileprivate extension PackageReference.Kind {
         }
     }
 }
+
+extension Workspace.ManagedDependency {
+    fileprivate var checkoutState: CheckoutState? {
+        if case .checkout(let checkoutState) = state {
+            return checkoutState
+        }
+        return .none
+    }
+}

Sources/SPMTestSupport/misc.swift

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