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 callsites to use package identity instead of location when reading/writing managed depedencies
* refactor related call-sites to handle edited state more safely and correctly
* adjust tests where needed

Author: tomerd

Sources/SPMTestSupport/MockWorkspace.swift

@@ -378,8 +378,8 @@ public final class MockWorkspace {
 
     public func set(
         pins: [PackageReference: CheckoutState] = [:],
-        managedDependencies: [ManagedDependency] = [],
-        managedArtifacts: [ManagedArtifact] = []
+        managedDependencies: [Workspace.ManagedDependency] = [],
+        managedArtifacts: [Workspace.ManagedArtifact] = []
     ) throws {
         let workspace = try self.getOrCreateWorkspace()
         let pinsStore = try workspace.pinsStore.load()
@@ -420,24 +420,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 {
@@ -451,8 +459,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 {
@@ -463,9 +472,9 @@ public final class MockWorkspace {
     }
 
     public struct ManagedArtifactResult {
-        public let managedArtifacts: ManagedArtifacts
+        public let managedArtifacts: Workspace.ManagedArtifacts
 
-        public init(_ managedArtifacts: ManagedArtifacts) {
+        public init(_ managedArtifacts: Workspace.ManagedArtifacts) {
             self.managedArtifacts = managedArtifacts
         }
 
@@ -486,7 +495,7 @@ public final class MockWorkspace {
         public func check(
             packageName: String,
             targetName: String,
-            source: ManagedArtifact.Source,
+            source: Workspace.ManagedArtifact.Source,
             path: AbsolutePath,
             file: StaticString = #file,
             line: UInt = #line
@@ -718,3 +727,12 @@ extension CheckoutState {
         }
     }
 }
+
+extension Workspace.ManagedDependency {
+    public var checkoutState: CheckoutState? {
+        if case .checkout(let checkoutState) = state {
+            return checkoutState
+        }
+        return nil
+    }
+}

Sources/Workspace/ManagedArtifact.swift

@@ -14,83 +14,83 @@ import SourceControl
 import TSCBasic
 import TSCUtility
 
-/// A downloaded artifact managed by the workspace.
-public struct ManagedArtifact {
-    /// The package reference.
-    public let packageRef: PackageReference
-
-    /// The name of the binary target the artifact corresponds to.
-    public let targetName: String
-
-    /// The source of the artifact (local or remote).
-    public let source: Source
-
-    /// The path of the artifact on disk
-    public let path: AbsolutePath
-
-    public init(
-        packageRef: PackageReference,
-        targetName: String,
-        source: Source,
-        path: AbsolutePath
-    ) {
-        self.packageRef = packageRef
-        self.targetName = targetName
-        self.source = source
-        self.path = path
-    }
+extension Workspace {
+    /// A downloaded artifact managed by the workspace.
+    public struct ManagedArtifact {
+        /// The package reference.
+        public let packageRef: PackageReference
+
+        /// The name of the binary target the artifact corresponds to.
+        public let targetName: String
+
+        /// The source of the artifact (local or remote).
+        public let source: Source
+
+        /// The path of the artifact on disk
+        public let path: AbsolutePath
+
+        public init(
+            packageRef: PackageReference,
+            targetName: String,
+            source: Source,
+            path: AbsolutePath
+        ) {
+            self.packageRef = packageRef
+            self.targetName = targetName
+            self.source = source
+            self.path = path
+        }
 
-    /// Create an artifact downloaded from a remote url.
-    public static func remote(
-        packageRef: PackageReference,
-        targetName: String,
-        url: String,
-        checksum: String,
-        path: AbsolutePath
-    ) -> ManagedArtifact {
-        return ManagedArtifact(
-            packageRef: packageRef,
-            targetName: targetName,
-            source: .remote(url: url, checksum: checksum),
-            path: path
-        )
-    }
+        /// Create an artifact downloaded from a remote url.
+        public static func remote(
+            packageRef: PackageReference,
+            targetName: String,
+            url: String,
+            checksum: String,
+            path: AbsolutePath
+        ) -> ManagedArtifact {
+            return ManagedArtifact(
+                packageRef: packageRef,
+                targetName: targetName,
+                source: .remote(url: url, checksum: checksum),
+                path: path
+            )
+        }
 
-    /// Create an artifact present locally on the filesystem.
-    public static func local(
-        packageRef: PackageReference,
-        targetName: String,
-        path: AbsolutePath
-    ) -> ManagedArtifact {
-        return ManagedArtifact(
-            packageRef: packageRef,
-            targetName: targetName,
-            source: .local,
-            path: path
-        )
-    }
+        /// Create an artifact present locally on the filesystem.
+        public static func local(
+            packageRef: PackageReference,
+            targetName: String,
+            path: AbsolutePath
+        ) -> ManagedArtifact {
+            return ManagedArtifact(
+                packageRef: packageRef,
+                targetName: targetName,
+                source: .local,
+                path: path
+            )
+        }
 
-    /// Represents the source of the artifact.
-    public enum Source: Equatable {
+        /// Represents the source of the artifact.
+        public enum Source: Equatable {
 
-        /// Represents a remote artifact, with the url it was downloaded from, its checksum, and its path relative to
-        /// the workspace artifacts path.
-        case remote(url: String, checksum: String)
+            /// Represents a remote artifact, with the url it was downloaded from, its checksum, and its path relative to
+            /// the workspace artifacts path.
+            case remote(url: String, checksum: String)
 
-        /// Represents a locally available artifact, with its path relative to its package.
-        case local
+            /// Represents a locally available artifact, with its path relative to its package.
+            case local
+        }
     }
 }
 
-// MARK: - CustomStringConvertible
-
-extension ManagedArtifact: CustomStringConvertible {
+extension Workspace.ManagedArtifact: CustomStringConvertible {
     public var description: String {
         return "<ManagedArtifact: \(self.packageRef.name).\(self.targetName) \(self.source) \(self.path)>"
     }
 }
 
-extension ManagedArtifact.Source: CustomStringConvertible {
+extension Workspace.ManagedArtifact.Source: CustomStringConvertible {
     public var description: String {
         switch self {
         case .local:
@@ -101,42 +101,42 @@ extension ManagedArtifact.Source: CustomStringConvertible {
     }
 }
 
-// MARK: -
+// MARK: - ManagedArtifacts
 
-/// A collection of managed artifacts which have been downloaded.
-public final class ManagedArtifacts {
+extension Workspace {
+    /// A collection of managed artifacts which have been downloaded.
+    public final class ManagedArtifacts {
 
-    /// A mapping from package url, to target name, to ManagedArtifact.
-    private var artifactMap: [String: [String: ManagedArtifact]]
+        /// A mapping from package url, to target name, to ManagedArtifact.
+        private var artifactMap: [String: [String: ManagedArtifact]]
 
-    internal var artifacts: AnyCollection<ManagedArtifact> {
-        AnyCollection(artifactMap.values.lazy.flatMap({ $0.values }))
-    }
+        internal var artifacts: AnyCollection<ManagedArtifact> {
+            AnyCollection(artifactMap.values.lazy.flatMap({ $0.values }))
+        }
 
-    init(artifactMap: [String: [String: ManagedArtifact]] = [:]) {
-        self.artifactMap = artifactMap
-    }
+        init(artifactMap: [String: [String: ManagedArtifact]] = [:]) {
+            self.artifactMap = artifactMap
+        }
 
-    public subscript(packageURL packageURL: String, targetName targetName: String) -> ManagedArtifact? {
-        artifactMap[packageURL]?[targetName]
-    }
+        public subscript(packageURL packageURL: String, targetName targetName: String) -> ManagedArtifact? {
+            artifactMap[packageURL]?[targetName]
+        }
 
-    public subscript(packageName packageName: String, targetName targetName: String) -> ManagedArtifact? {
-        artifacts.first(where: { $0.packageRef.name == packageName && $0.targetName == targetName })
-    }
+        public subscript(packageName packageName: String, targetName targetName: String) -> ManagedArtifact? {
+            artifacts.first(where: { $0.packageRef.name == packageName && $0.targetName == targetName })
+        }
 
-    public func add(_ artifact: ManagedArtifact) {
-        artifactMap[artifact.packageRef.location, default: [:]][artifact.targetName] = artifact
-    }
+        public func add(_ artifact: ManagedArtifact) {
+            artifactMap[artifact.packageRef.location, default: [:]][artifact.targetName] = artifact
+        }
 
-    public func remove(packageURL: String, targetName: String) {
-        artifactMap[packageURL]?[targetName] = nil
+        public func remove(packageURL: String, targetName: String) {
+            artifactMap[packageURL]?[targetName] = nil
+        }
     }
 }
 
-// MARK: - Collection
-
-extension ManagedArtifacts: Collection {
+extension Workspace.ManagedArtifacts: Collection {
     public var startIndex: AnyIndex {
         artifacts.startIndex
     }
@@ -145,7 +145,7 @@ extension ManagedArtifacts: Collection {
         artifacts.endIndex
     }
 
-    public subscript(index: AnyIndex) -> ManagedArtifact {
+    public subscript(index: AnyIndex) -> Workspace.ManagedArtifact {
         artifacts[index]
     }
 
@@ -154,9 +154,7 @@ extension ManagedArtifacts: Collection {
     }
 }
 
-// MARK: - CustomStringConvertible
-
-extension ManagedArtifacts: CustomStringConvertible {
+extension Workspace.ManagedArtifacts: CustomStringConvertible {
     public var description: String {
         "<ManagedArtifacts: \(Array(artifacts))>"
     }