api cleanup in preperation for identity improvments

motivation: infrastructure towards improve correctness by using identity more broadly throughout the code

changes:
* rename PackageReference::path to PackageReference::location
* make PackageReference kinf explicitly set by the callsite with no default
* rename PackageContainerConstraint::identifier to PackageContainerConstraint::package
* rename PackageContainer::identifier to PackageContainer::package
* adjust tests

Author: tomerd

Sources/Commands/SwiftPackageCollectionsTool.swift

@@ -221,7 +221,7 @@ public struct SwiftPackageCollectionsTool: ParsableCommand {
 
         mutating func run() throws {
             let identity = PackageIdentity(url: packageUrl)
-            let reference = PackageReference(identity: identity, path: packageUrl)
+            let reference = PackageReference.remote(identity: identity, location: packageUrl)
 
             do { // assume URL is for a package
                 let result = try with { collections in

Sources/Commands/SwiftTool.swift

@@ -138,7 +138,7 @@ private class ToolWorkspaceDelegate: WorkspaceDelegate {
             case .forced:
                 self.stdoutStream <<< "it was forced"
             case .newPackages(let packages):
-                let dependencies = packages.lazy.map({ "'\($0.path)'" }).joined(separator: ", ")
+                let dependencies = packages.lazy.map({ "'\($0.location)'" }).joined(separator: ", ")
                 self.stdoutStream <<< "the following dependencies were added: \(dependencies)"
             case .packageRequirementChange(let package, let state, let requirement):
                 self.stdoutStream <<< "dependency '\(package.name)' was "

Sources/PackageCollections/Providers/GitHubPackageMetadataProvider.swift

@@ -38,8 +38,8 @@ struct GitHubPackageMetadataProvider: PackageMetadataProvider {
         guard reference.kind == .remote else {
             return callback(.failure(Errors.invalidReferenceType(reference)))
         }
-        guard let baseURL = self.apiURL(reference.path) else {
-            return callback(.failure(Errors.invalidGitURL(reference.path)))
+        guard let baseURL = self.apiURL(reference.location) else {
+            return callback(.failure(Errors.invalidGitURL(reference.location)))
         }
 
         let metadataURL = baseURL

Sources/PackageCollections/Utility.swift

@@ -54,8 +54,8 @@ extension PackageReference {
     init(repository: RepositorySpecifier, kind: PackageReference.Kind = .remote) {
         self.init(
             identity: PackageIdentity(url: repository.url),
-            path: repository.url,
-            kind: kind
+            kind: kind,
+            location: repository.url
         )
     }
 }

Sources/PackageGraph/DependencyResolutionNode.swift

@@ -95,7 +95,7 @@ public enum DependencyResolutionNode {
         // Don’t create a version lock for anything but a product.
         guard specificProduct != nil else { return nil }
         return PackageContainerConstraint(
-            container: package,
+            package: package,
             versionRequirement: .exact(version),
             products: .specific([])
         )
@@ -108,7 +108,7 @@ public enum DependencyResolutionNode {
         // Don’t create a revision lock for anything but a product.
         guard specificProduct != nil else { return nil }
         return PackageContainerConstraint(
-            container: package,
+            package: package,
             requirement: .revision(revision),
             products: .specific([])
         )

Sources/PackageGraph/LocalPackageContainer.swift

@@ -24,35 +24,35 @@ import TSCUtility
 /// should be used as-is. Infact, they might not even have a git repository.
 /// Examples: Root packages, local dependencies, edited packages.
 public final class LocalPackageContainer: PackageContainer {
-    public let identifier: PackageReference
+    public let package: PackageReference
     private let mirrors: DependencyMirrors
     private let manifestLoader: ManifestLoaderProtocol
     private let toolsVersionLoader: ToolsVersionLoaderProtocol
     private let currentToolsVersion: ToolsVersion
 
     /// The file system that shoud be used to load this package.
-    private let fs: FileSystem
+    private let fileSystem: FileSystem
 
     /// cached version of the manifest
     private let manifest = ThreadSafeBox<Manifest>()
 
     private func loadManifest() throws -> Manifest {
         try manifest.memoize() {
             // Load the tools version.
-            let toolsVersion = try toolsVersionLoader.load(at: AbsolutePath(identifier.path), fileSystem: fs)
+            let toolsVersion = try toolsVersionLoader.load(at: AbsolutePath(package.location), fileSystem: fileSystem)
 
             // Validate the tools version.
-            try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: identifier.path)
+            try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: package.location)
 
             // Load the manifest.
             // FIXME: this should not block
             return try temp_await {
-                manifestLoader.load(package: AbsolutePath(identifier.path),
-                                    baseURL: identifier.path,
+                manifestLoader.load(package: AbsolutePath(package.location),
+                                    baseURL: package.location,
                                     version: nil,
                                     toolsVersion: toolsVersion,
-                                    packageKind: identifier.kind,
-                                    fileSystem: fs,
+                                    packageKind: package.kind,
+                                    fileSystem: fileSystem,
                                     on: .global(),
                                     completion: $0)
             }
@@ -66,24 +66,24 @@ public final class LocalPackageContainer: PackageContainer {
     public func getUpdatedIdentifier(at boundVersion: BoundVersion) throws -> PackageReference {
         assert(boundVersion == .unversioned, "Unexpected bound version \(boundVersion)")
         let manifest = try loadManifest()
-        return identifier.with(newName: manifest.name)
+        return package.with(newName: manifest.name)
     }
 
     public init(
-        _ identifier: PackageReference,
+        package: PackageReference,
         mirrors: DependencyMirrors,
         manifestLoader: ManifestLoaderProtocol,
         toolsVersionLoader: ToolsVersionLoaderProtocol,
         currentToolsVersion: ToolsVersion,
-        fs: FileSystem = localFileSystem
+        fileSystem: FileSystem = localFileSystem
     ) {
-        assert(URL.scheme(identifier.path) == nil, "unexpected scheme \(URL.scheme(identifier.path)!) in \(identifier.path)")
-        self.identifier = identifier
+        assert(URL.scheme(package.location) == nil, "unexpected scheme \(URL.scheme(package.location)!) in \(package.location)")
+        self.package = package
         self.mirrors = mirrors
         self.manifestLoader = manifestLoader
         self.toolsVersionLoader = toolsVersionLoader
         self.currentToolsVersion = currentToolsVersion
-        self.fs = fs
+        self.fileSystem = fileSystem
     }
 
     public func isToolsVersionCompatible(at version: Version) -> Bool {
@@ -113,6 +113,6 @@ public final class LocalPackageContainer: PackageContainer {
 
 extension LocalPackageContainer: CustomStringConvertible  {
     public var description: String {
-        return "LocalPackageContainer(\(identifier.path))"
+        return "LocalPackageContainer(\(package.location))"
     }
 }

Sources/PackageGraph/PackageContainer.swift

@@ -36,7 +36,7 @@ import struct TSCUtility.Version
 public protocol PackageContainer {
 
     /// The identifier for the package.
-    var identifier: PackageReference { get }
+    var package: PackageReference { get }
 
     /// Returns true if the tools version is compatible at the given version.
     func isToolsVersionCompatible(at version: Version) -> Bool
@@ -116,7 +116,7 @@ extension PackageContainer {
 public struct PackageContainerConstraint: Equatable, Hashable {
 
     /// The identifier for the container the constraint is on.
-    public let identifier: PackageReference
+    public let package: PackageReference
 
     /// The constraint requirement.
     public let requirement: PackageRequirement
@@ -126,22 +126,22 @@ public struct PackageContainerConstraint: Equatable, Hashable {
 
     /// Create a constraint requiring the given `container` satisfying the
     /// `requirement`.
-    public init(container identifier: PackageReference, requirement: PackageRequirement, products: ProductFilter) {
-        self.identifier = identifier
+    public init(package: PackageReference, requirement: PackageRequirement, products: ProductFilter) {
+        self.package = package
         self.requirement = requirement
         self.products = products
     }
 
     /// Create a constraint requiring the given `container` satisfying the
     /// `versionRequirement`.
-    public init(container identifier: PackageReference, versionRequirement: VersionSetSpecifier, products: ProductFilter) {
-        self.init(container: identifier, requirement: .versionSet(versionRequirement), products: products)
+    public init(package: PackageReference, versionRequirement: VersionSetSpecifier, products: ProductFilter) {
+        self.init(package: package, requirement: .versionSet(versionRequirement), products: products)
     }
 }
 
 extension PackageContainerConstraint: CustomStringConvertible {
     public var description: String {
-        return "Constraint(\(identifier), \(requirement), \(products)"
+        return "Constraint(\(self.package), \(requirement), \(products)"
     }
 }
 
@@ -151,7 +151,7 @@ extension PackageContainerConstraint: CustomStringConvertible {
 public protocol PackageContainerProvider {
     /// Get the container for a particular identifier asynchronously.
     func getContainer(
-        for identifier: PackageReference,
+        for package: PackageReference,
         skipUpdate: Bool,
         on queue: DispatchQueue,
         completion: @escaping (Result<PackageContainer, Swift.Error>) -> Void

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -201,7 +201,7 @@ private func createResolvedPackages(
         guard let package = manifestToPackage[node.manifest] else {
             return nil
         }
-        let isAllowedToVendUnsafeProducts = unsafeAllowedPackages.contains{ $0.path == package.manifest.url }
+        let isAllowedToVendUnsafeProducts = unsafeAllowedPackages.contains{ $0.location == package.manifest.url }
         return ResolvedPackageBuilder(
             package,
             productFilter: node.productFilter,

Sources/PackageGraph/PackageGraphRoot.swift

@@ -49,7 +49,7 @@ public struct PackageGraphRoot {
     public init(input: PackageGraphRootInput, manifests: [Manifest], explicitProduct: String? = nil) {
         self.packageRefs = zip(input.packages, manifests).map { (path, manifest) in
             let identity = PackageIdentity(url: manifest.url)
-            return PackageReference(identity: identity, path: path.pathString, kind: .root)
+            return .root(identity: identity, path: path)
         }
         self.manifests = manifests
 
@@ -73,11 +73,11 @@ public struct PackageGraphRoot {
     /// Returns the constraints imposed by root manifests + dependencies.
     public func constraints(mirrors: DependencyMirrors) -> [PackageContainerConstraint] {
         let constraints = packageRefs.map({
-            PackageContainerConstraint(container: $0, requirement: .unversioned, products: .everything)
+            PackageContainerConstraint(package: $0, requirement: .unversioned, products: .everything)
         })
         return constraints + dependencies.map({
             PackageContainerConstraint(
-                container: $0.createPackageRef(mirrors: mirrors),
+                package: $0.createPackageRef(mirrors: mirrors),
                 requirement: $0.requirement.toConstraintRequirement(),
                 products: $0.productFilter
             )

Sources/PackageGraph/PackageModel+Extensions.swift

@@ -27,8 +27,8 @@ extension PackageDependencyDescription {
 
         return PackageReference(
             identity: identity,
-            path: effectiveURL,
-            kind: requirement == .localPackage ? .local : .remote
+            kind: requirement == .localPackage ? .local : .remote,
+            location: effectiveURL
         )
     }
 }
@@ -38,7 +38,7 @@ extension Manifest {
     public func dependencyConstraints(productFilter: ProductFilter, mirrors: DependencyMirrors) -> [PackageContainerConstraint] {
         return dependenciesRequired(for: productFilter).map({
             return PackageContainerConstraint(
-                container: $0.createPackageRef(mirrors: mirrors),
+                package: $0.createPackageRef(mirrors: mirrors),
                 requirement: $0.requirement.toConstraintRequirement(),
                 products: $0.productFilter)
         })
@@ -49,17 +49,17 @@ extension PackageContainerConstraint {
     internal func nodes() -> [DependencyResolutionNode] {
         switch products {
         case .everything:
-            return [.root(package: identifier)]
+            return [.root(package: self.package)]
         case .specific:
             switch products {
             case .everything:
                 assertionFailure("Attempted to enumerate a root package’s product filter; root packages have no filter.")
                 return []
             case .specific(let set):
                 if set.isEmpty { // Pointing at the package without a particular product.
-                    return [.empty(package: identifier)]
+                    return [.empty(package: self.package)]
                 } else {
-                    return set.sorted().map { .product($0, package: identifier) }
+                    return set.sorted().map { .product($0, package: self.package) }
                 }
             }
         }
@@ -72,6 +72,6 @@ extension PackageReference {
     /// This should only be accessed when the reference is not local.
     public var repository: RepositorySpecifier {
         precondition(kind == .remote)
-        return RepositorySpecifier(url: path)
+        return RepositorySpecifier(url: self.location)
     }
 }

Sources/PackageGraph/PinsStore.swift

@@ -136,7 +136,7 @@ extension PinsStore.Pin: JSONMappable, JSONSerializable {
         let name: String? = json.get("package")
         let url: String = try json.get("repositoryURL")
         let identity = PackageIdentity(url: url)
-        let ref = PackageReference(identity: identity, path: url)
+        let ref = PackageReference.remote(identity: identity, location: url)
         self.packageRef = name.flatMap(ref.with(newName:)) ?? ref
         self.state = try json.get("state")
     }
@@ -145,7 +145,7 @@ extension PinsStore.Pin: JSONMappable, JSONSerializable {
     public func toJSON() -> JSON {
         return .init([
             "package": packageRef.name.toJSON(),
-            "repositoryURL": packageRef.path,
+            "repositoryURL": packageRef.location,
             "state": state
         ])
     }