use identity instead of location where possible (#3744)

motivation: adoption of SE-0292 package registry

changes:
* remove assumption and runtime check about RepositoprySpecifie available for all PacakgeRefrences
* adjust call-sites and test

Author: tomerd

Sources/PackageGraph/LocalPackageContainer.swift

@@ -42,7 +42,7 @@ public final class LocalPackageContainer: PackageContainer {
             let toolsVersion = try self.toolsVersionLoader.load(at: AbsolutePath(self.package.location), fileSystem: self.fileSystem)
 
             // Validate the tools version.
-            try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: self.package.location)
+            try toolsVersion.validateToolsVersion(self.currentToolsVersion, packageIdentity: self.package.identity)
 
             // Load the manifest.
             // FIXME: this should not block

Sources/PackageGraph/PackageModel+Extensions.swift

@@ -70,13 +70,3 @@ extension PackageContainerConstraint {
         }
     }
 }
-
-extension PackageReference {
-    /// The repository of the package.
-    ///
-    /// This should only be accessed when the reference is not local.
-    public var repository: RepositorySpecifier {
-        precondition(kind == .remote)
-        return RepositorySpecifier(url: self.location)
-    }
-}

Sources/PackageGraph/RepositoryPackageContainer.swift

@@ -29,8 +29,8 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
     // about the container to identify it for diagnostics.
     public struct GetDependenciesError: Error, CustomStringConvertible, DiagnosticLocationProviding {
 
-        /// The container (repository) that encountered the error.
-        public let containerIdentifier: String
+        /// The repository url that encountered the error.
+        public let url: String
 
         /// The source control reference (version, branch, revision, etc) that was involved.
         public let reference: String
@@ -42,12 +42,12 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
         public let suggestion: String?
 
         public var diagnosticLocation: DiagnosticLocation? {
-            return PackageLocation.Remote(url: containerIdentifier, reference: reference)
+            return PackageLocation.Remote(url: self.url, reference: self.reference)
         }
 
         /// Description shown for errors of this kind.
         public var description: String {
-            var desc = "\(underlyingError) in \(containerIdentifier)"
+            var desc = "\(underlyingError) in \(self.url)"
             if let suggestion = suggestion {
                 desc += " (\(suggestion))"
             }
@@ -175,7 +175,7 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
             }.1
         } catch {
             throw GetDependenciesError(
-                containerIdentifier: package.repository.url, reference: version.description, underlyingError: error, suggestion: nil)
+                url: package.location, reference: version.description, underlyingError: error, suggestion: nil)
         }
     }
 
@@ -193,7 +193,11 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
                 if let rev = try? repository.resolveRevision(identifier: revision), repository.exists(revision: rev) {
                     // Revision does exist, so something else must be wrong.
                     throw GetDependenciesError(
-                        containerIdentifier: package.repository.url, reference: revision, underlyingError: error, suggestion: nil)
+                        url: package.location,
+                        reference: revision,
+                        underlyingError: error,
+                        suggestion: .none
+                    )
                 }
                 else {
                     // Revision does not exist, so we customize the error.
@@ -203,12 +207,20 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
                     let mainBranchExists = (try? repository.resolveRevision(identifier: "main")) != nil
                     let suggestion = (revision == "master" && mainBranchExists) ? "did you mean ‘main’?" : nil
                     throw GetDependenciesError(
-                        containerIdentifier: package.repository.url, reference: revision,
-                        underlyingError: StringError(errorMessage), suggestion: suggestion)
+                        url: package.location,
+                        reference: revision,
+                        underlyingError: StringError(errorMessage),
+                        suggestion: suggestion
+                    )
                 }
             }
             // If we get this far without having thrown an error, we wrap and throw the underlying error.
-            throw GetDependenciesError(containerIdentifier: package.repository.url, reference: revision, underlyingError: error, suggestion: nil)
+            throw GetDependenciesError(
+                url: package.location,
+                reference: revision,
+                underlyingError: error,
+                suggestion: .none
+            )
         }
     }
 
@@ -277,7 +289,7 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
     /// version of the package manager.
     private func isValidToolsVersion(_ toolsVersion: ToolsVersion) -> Bool {
         do {
-            try toolsVersion.validateToolsVersion(currentToolsVersion, packagePath: "")
+            try toolsVersion.validateToolsVersion(currentToolsVersion, packageIdentity: .plain("unknown"))
             return true
         } catch {
             return false
@@ -303,22 +315,20 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
     }
 
     private func loadManifest(fileSystem: FileSystem, version: Version?, packageVersion: String) throws -> Manifest {
-        let packageLocation = self.package.repository.url
-
         // Load the tools version.
         let toolsVersion = try self.toolsVersionLoader.load(at: .root, fileSystem: fileSystem)
 
         // Validate the tools version.
         try toolsVersion.validateToolsVersion(
-            self.currentToolsVersion, packagePath: packageLocation, packageVersion: packageVersion)
+            self.currentToolsVersion, packageIdentity: self.package.identity, packageVersion: packageVersion)
 
         // Load the manifest.
         // FIXME: this should not block
         return try temp_await {
             self.manifestLoader.load(at: AbsolutePath.root,
                                      packageIdentity: self.package.identity,
                                      packageKind: self.package.kind,
-                                     packageLocation: packageLocation,
+                                     packageLocation: self.package.location,
                                      version: version,
                                      revision: nil,
                                      toolsVersion: toolsVersion,
@@ -335,7 +345,7 @@ public class RepositoryPackageContainer: PackageContainer, CustomStringConvertib
     }
 
     public var description: String {
-        return "RepositoryPackageContainer(\(package.repository.url.debugDescription))"
+        return "RepositoryPackageContainer(\(self.package.location))"
     }
 }
 
@@ -400,7 +410,7 @@ public class RepositoryPackageContainerProvider: PackageContainerProvider {
         }
 
         // Resolve the container using the repository manager.
-        repositoryManager.lookup(repository: package.repository, skipUpdate: skipUpdate, on: queue) { result in
+        repositoryManager.lookup(repository: .init(url: package.location), skipUpdate: skipUpdate, on: queue) { result in
             queue.async {
                 // Create the container wrapper.
                 let result = result.tryMap { handle -> PackageContainer in

Sources/PackageModel/Diagnostics.swift

@@ -15,8 +15,8 @@ import TSCUtility
 
 /// The diagnostic triggered when the package has a newer tools version than the installed tools.
 public struct RequireNewerTools: DiagnosticData, Swift.Error {
-    /// The path of the package.
-    public let packagePath: String
+    /// The identity of the package.
+    public let packageIdentity: PackageIdentity
 
     /// The version of the package.
     public let packageVersion: String?
@@ -28,19 +28,19 @@ public struct RequireNewerTools: DiagnosticData, Swift.Error {
     public let packageToolsVersion: ToolsVersion
 
     public init(
-        packagePath: String,
+        packageIdentity: PackageIdentity,
         packageVersion: String? = nil,
         installedToolsVersion: ToolsVersion,
         packageToolsVersion: ToolsVersion
     ) {
-        self.packagePath = packagePath
+        self.packageIdentity = packageIdentity
         self.packageVersion = packageVersion
         self.installedToolsVersion = installedToolsVersion
         self.packageToolsVersion = packageToolsVersion
     }
 
     public var description: String {
-        var text = "package at '\(packagePath)'"
+        var text = "package '\(self.packageIdentity)'"
         if let version = self.packageVersion {
             text += " @ \(version)"
         }
@@ -51,8 +51,8 @@ public struct RequireNewerTools: DiagnosticData, Swift.Error {
 
 /// The diagnostic triggered when the package has an unsupported tools version.
 public struct UnsupportedToolsVersion: DiagnosticData, Swift.Error {
-    /// The path of the package.
-    public let packagePath: String
+    /// The identity of the package.
+    public let packageIdentity: PackageIdentity
 
     /// The version of the package.
     public let packageVersion: String?
@@ -68,19 +68,19 @@ public struct UnsupportedToolsVersion: DiagnosticData, Swift.Error {
     }
 
     public init(
-        packagePath: String,
+        packageIdentity: PackageIdentity,
         packageVersion: String? = nil,
         currentToolsVersion: ToolsVersion,
         packageToolsVersion: ToolsVersion
     ) {
-        self.packagePath = packagePath
+        self.packageIdentity = packageIdentity
         self.packageVersion = packageVersion
         self.currentToolsVersion = currentToolsVersion
         self.packageToolsVersion = packageToolsVersion
     }
 
     public var description: String {
-        var text = "package at '\(self.packagePath)'"
+        var text = "package '\(self.packageIdentity)'"
         if let version = self.packageVersion {
             text += " @ \(version)"
         }

Sources/PackageModel/ToolsVersion.swift

@@ -104,8 +104,8 @@ public struct ToolsVersion: Equatable, Hashable, Codable {
     /// version of the package manager.
     public func validateToolsVersion(
         _ currentToolsVersion: ToolsVersion,
-        packagePath: String,
-        packageVersion: String? = nil
+        packageIdentity: PackageIdentity,
+        packageVersion: String? = .none
     ) throws {
         // We don't want to throw any error when using the special vNext version.
         if SwiftVersion.currentVersion.isDevelopment && self == .vNext {
@@ -115,7 +115,7 @@ public struct ToolsVersion: Equatable, Hashable, Codable {
         // Make sure the package has the right minimum tools version.
         guard self >= .minimumRequired else {
             throw UnsupportedToolsVersion(
-                packagePath: packagePath,
+                packageIdentity: packageIdentity,
                 packageVersion: packageVersion,
                 currentToolsVersion: currentToolsVersion,
                 packageToolsVersion: self
@@ -125,7 +125,7 @@ public struct ToolsVersion: Equatable, Hashable, Codable {
         // Make sure the package isn't newer than the current tools version.
         guard currentToolsVersion >= self else {
             throw RequireNewerTools(
-                packagePath: packagePath,
+                packageIdentity: packageIdentity,
                 packageVersion: packageVersion,
                 installedToolsVersion: currentToolsVersion,
                 packageToolsVersion: self

Sources/Workspace/ResolverPrecomputationProvider.swift

@@ -105,7 +105,7 @@ private struct LocalPackageContainer: PackageContainer {
 
     func isToolsVersionCompatible(at version: Version) -> Bool {
         do {
-            try manifest.toolsVersion.validateToolsVersion(currentToolsVersion, packagePath: "")
+            try manifest.toolsVersion.validateToolsVersion(currentToolsVersion, packageIdentity: .plain("unknown"))
             return true
         } catch {
             return false

Sources/Workspace/Workspace.swift

@@ -1040,7 +1040,7 @@ extension Workspace {
                 self.loadManifest(packageIdentity: dependency.packageRef.identity,
                                   packageKind: .local,
                                   packagePath: destination,
-                                  packageLocation: dependency.packageRef.repository.url,
+                                  packageLocation: dependency.packageRef.location,
                                   diagnostics: diagnostics,
                                   completion: $0)
             }
@@ -1066,7 +1066,7 @@ extension Workspace {
             // Get handle to the repository.
             // TODO: replace with async/await when available
             let handle = try temp_await {
-                repositoryManager.lookup(repository: dependency.packageRef.repository, skipUpdate: true, on: .sharedConcurrent, completion: $0)
+                repositoryManager.lookup(repository: .init(url: dependency.packageRef.location), skipUpdate: true, on: .sharedConcurrent, completion: $0)
             }
             let repo = try handle.open()
 
@@ -1634,7 +1634,7 @@ extension Workspace {
                 let toolsVersion = try toolsVersionLoader.load(at: packagePath, fileSystem: fileSystem)
 
                 // Validate the tools version.
-                try toolsVersion.validateToolsVersion(currentToolsVersion, packagePath: packagePath.pathString)
+                try toolsVersion.validateToolsVersion(currentToolsVersion, packageIdentity: packageIdentity)
 
                 // Load the manifest.
                 // The delegate callback is only passed any diagnostics emitted during the parsing of the manifest, but they are also forwarded up to the caller.
@@ -2687,14 +2687,19 @@ extension Workspace {
             }
         }
 
+        guard case .remote = package.kind else {
+            throw InternalError("invalid package kind \(package.kind)")
+        }
+        let repository = RepositorySpecifier(url: package.location)
+
         // If not, we need to get the repository from the checkouts.
         // FIXME: this should not block
         let handle = try temp_await {
-            repositoryManager.lookup(repository: package.repository, skipUpdate: true, on: .sharedConcurrent, completion: $0)
+            repositoryManager.lookup(repository: repository, skipUpdate: true, on: .sharedConcurrent, completion: $0)
         }
 
         // Clone the repository into the checkouts.
-        let path = self.location.repositoriesCheckoutsDirectory.appending(component: package.repository.basename)
+        let path = self.location.repositoriesCheckoutsDirectory.appending(component: repository.basename)
 
         try fileSystem.chmod(.userWritable, path: path, options: [.recursive, .onlyFiles])
         try fileSystem.removeFileTree(path)
@@ -2721,14 +2726,18 @@ extension Workspace {
         package: PackageReference,
         at checkoutState: CheckoutState
     ) throws -> AbsolutePath {
+        guard case .remote = package.kind else {
+            throw InternalError("invalid package kind \(package.kind)")
+        }
+
         // Get the repository.
         let path = try fetch(package: package)
 
         // Check out the given revision.
         let workingCopy = try repositoryManager.openWorkingCopy(at: path)
 
         // Inform the delegate.
-        delegate?.willCheckOut(repository: package.repository.url, revision: checkoutState.description, at: path)
+        delegate?.willCheckOut(repository: package.location, revision: checkoutState.description, at: path)
 
         // Do mutable-immutable dance because checkout operation modifies the disk state.
         try fileSystem.chmod(.userWritable, path: path, options: [.recursive, .onlyFiles])
@@ -2742,7 +2751,7 @@ extension Workspace {
             checkoutState: checkoutState))
         try state.saveState()
 
-        delegate?.didCheckOut(repository: package.repository.url, revision: checkoutState.description, at: path, error: nil)
+        delegate?.didCheckOut(repository: package.location, revision: checkoutState.description, at: path, error: nil)
 
         return path
     }
@@ -2789,7 +2798,6 @@ extension Workspace {
 
     /// Removes the clone and checkout of the provided specifier.
     fileprivate func remove(package: PackageReference) throws {
-
         guard let dependency = state.dependencies[forURL: package.location] else {
             throw InternalError("trying to remove \(package.name) which isn't in workspace")
         }
@@ -2808,7 +2816,7 @@ extension Workspace {
         }
 
         // Inform the delegate.
-        delegate?.removing(repository: dependency.packageRef.repository.url)
+        delegate?.removing(repository: dependency.packageRef.location)
 
         // Compute the dependency which we need to remove.
         let dependencyToRemove: ManagedDependency
@@ -2823,6 +2831,10 @@ extension Workspace {
             state.dependencies.remove(forURL: dependencyToRemove.packageRef.location)
         }
 
+        guard case .remote = dependencyToRemove.packageRef.kind else {
+            throw InternalError("invalid package kind \(dependencyToRemove.packageRef.kind)")
+        }
+
         // Remove the checkout.
         let dependencyPath = self.location.repositoriesCheckoutsDirectory.appending(dependencyToRemove.subpath)
         let workingCopy = try repositoryManager.openWorkingCopy(at: dependencyPath)
@@ -2834,7 +2846,7 @@ extension Workspace {
         try fileSystem.removeFileTree(dependencyPath)
 
         // Remove the clone.
-        try repositoryManager.remove(repository: dependencyToRemove.packageRef.repository)
+        try repositoryManager.remove(repository: .init(url: dependencyToRemove.packageRef.location))
 
         // Save the state.
         try state.saveState()

Tests/CommandsTests/PackageToolTests.swift

@@ -768,7 +768,7 @@ final class PackageToolTests: XCTestCase {
                     let path = try SwiftPMProduct.packagePath(for: pkg, packageRoot: fooPath)
                     let pin = pinsStore.pinsMap[PackageIdentity(path: path)]!
                     XCTAssertEqual(pin.packageRef.identity, PackageIdentity(path: path))
-                    XCTAssert(pin.packageRef.repository.url.hasSuffix(pkg))
+                    XCTAssert(pin.packageRef.location.hasSuffix(pkg))
                     XCTAssertEqual(pin.state.version, "1.2.3")
                 }
             }

Tests/PackageGraphTests/RepositoryPackageContainerProviderTests.swift

@@ -286,7 +286,7 @@ class RepositoryPackageContainerProviderTests: XCTestCase {
                 _ = try container.getDependencies(at: revision.identifier, productFilter: .nothing)
             } catch let error as RepositoryPackageContainer.GetDependenciesError {
                 let error = error.underlyingError as! UnsupportedToolsVersion
-                XCTAssertMatch(error.description, .and(.prefix("package at '/' @"), .suffix("is using Swift tools version 3.1.0 which is no longer supported; consider using '// swift-tools-version:4.0' to specify the current tools version")))
+                XCTAssertMatch(error.description, .and(.prefix("package '\(PackageIdentity(url: specifier.url))' @"), .suffix("is using Swift tools version 3.1.0 which is no longer supported; consider using '// swift-tools-version:4.0' to specify the current tools version")))
             }
         }
     }