improve correctness by using identity more broadly throughout the code (#3244)

motivation: improve correctness by using identity more broadly throughout the code, prepare to package registry

changes:
* refector as PackageDependencyDescription with "scm" and "local" cases, this reflects better the true meaning and associated data with the two options and prepares us to adding a third option for registry
* create new abstraction IdentityResolver, with default implementation that follows existing identity resolution logic
* move mirror logic into IdentityResolver, and simplify code that used mirrors
* move all logic to generate identity, apply mirrors to PackageDependencyDescription so it is handles as soon as possible in the call stack
* adjust call-sites
* adjust and add tests for mirror

Author: tomerd

Examples/package-info/Sources/package-info/main.swift

@@ -31,9 +31,10 @@ let packagePath = localFileSystem.currentWorkingDirectory!
 // There are several levels of information available.
 // Each takes longer to load than the level above it, but provides more detail.
 let diagnostics = DiagnosticsEngine()
-let manifest = try tsc_await { ManifestLoader.loadManifest(at: packagePath, kind: .local, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], on: .global(), completion: $0) }
-let loadedPackage = try tsc_await { PackageBuilder.loadPackage(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], diagnostics: diagnostics, on: .global(), completion: $0) }
-let graph = try Workspace.loadGraph(packagePath: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], diagnostics: diagnostics)
+let identityResolver = DefaultIdentityResolver()
+let manifest = try tsc_await { ManifestLoader.loadManifest(at: packagePath, kind: .local, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, on: .global(), completion: $0) }
+let loadedPackage = try tsc_await { PackageBuilder.loadPackage(at: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics, on: .global(), completion: $0) }
+let graph = try Workspace.loadGraph(packagePath: packagePath, swiftCompiler: swiftCompiler, swiftCompilerFlags: [], identityResolver: identityResolver, diagnostics: diagnostics)
 
 // EXAMPLES
 // ========

Fixtures/DependencyResolution/External/Mirror/App/Package.swift

@@ -0,0 +1,17 @@
+// swift-tools-version:5.3
+
+import PackageDescription
+
+let package = Package(
+    name: "App",
+    dependencies: [
+        .package(name: "Foo", url: "../Foo", .branch("main")),
+        .package(name: "Bar", url: "../Bar", .branch("main")),
+    ],
+    targets: [
+        .target(name: "App", dependencies: [
+            .product(name: "Foo", package: "Foo"),
+            .product(name: "Bar", package: "Bar"),
+        ], path: "./")
+    ]
+)

Fixtures/DependencyResolution/External/Mirror/App/main.swift

@@ -0,0 +1,5 @@
+import Foo
+import Bar
+
+public func main() {
+}

Fixtures/DependencyResolution/External/Mirror/Bar/Bar.swift

@@ -0,0 +1,2 @@
+public func hello() {
+}

Fixtures/DependencyResolution/External/Mirror/Bar/Package.swift

@@ -0,0 +1,12 @@
+// swift-tools-version:4.2
+import PackageDescription
+
+let package = Package(
+    name: "Bar",
+    products: [
+        .library(name: "Bar", targets: ["Bar"]),
+    ],
+    targets: [
+        .target(name: "Bar", path: "./"),
+    ]
+)

Fixtures/DependencyResolution/External/Mirror/BarMirror/Bar.swift

@@ -0,0 +1,2 @@
+public func hello() {
+}

Fixtures/DependencyResolution/External/Mirror/BarMirror/Package.swift

@@ -0,0 +1,12 @@
+// swift-tools-version:4.2
+import PackageDescription
+
+let package = Package(
+    name: "Bar",
+    products: [
+        .library(name: "Bar", targets: ["Bar"]),
+    ],
+    targets: [
+        .target(name: "Bar", path: "./"),
+    ]
+)

Fixtures/DependencyResolution/External/Mirror/Foo/Foo.swift

@@ -0,0 +1,2 @@
+public func hello() {
+}

Fixtures/DependencyResolution/External/Mirror/Foo/Package.swift

@@ -0,0 +1,12 @@
+// swift-tools-version:4.2
+import PackageDescription
+
+let package = Package(
+    name: "Foo",
+    products: [
+        .library(name: "Foo", targets: ["Foo"]),
+    ],
+    targets: [
+        .target(name: "Foo", path: "./"),
+    ]
+)

Sources/Commands/Describe.swift

@@ -100,14 +100,22 @@ fileprivate struct DescribedPackage: Encodable {
 
     /// Represents a package dependency for the sole purpose of generating a description.
     struct DescribedPackageDependency: Encodable {
+        let identity: PackageIdentity
         let name: String?
         let url: String?
         let requirement: PackageDependencyDescription.Requirement?
 
         init(from dependency: PackageDependencyDescription) {
+            self.identity = dependency.identity
             self.name = dependency.explicitNameForTargetDependencyResolutionOnly
-            self.url = dependency.location
-            self.requirement = dependency.requirement
+            switch dependency {
+            case .local(let data):
+                self.url = data.path.pathString
+                self.requirement = nil
+            case .scm(let data):
+                self.url = data.location
+                self.requirement = data.requirement
+            }
         }
     }
 

Sources/Commands/SwiftTool.swift

@@ -784,6 +784,7 @@ public class SwiftTool {
             case (false, .shared):
                 cachePath = try self.getCachePath().map{ $0.appending(component: "manifests") }
             }
+
             return try ManifestLoader(
                 // Always use the host toolchain's resources for parsing manifest.
                 manifestResources: self._hostToolchain.get().manifestResources,

Sources/PackageGraph/LocalPackageContainer.swift

@@ -25,7 +25,7 @@ import TSCUtility
 /// Examples: Root packages, local dependencies, edited packages.
 public final class LocalPackageContainer: PackageContainer {
     public let package: PackageReference
-    private let mirrors: DependencyMirrors
+    private let identityResolver: IdentityResolver
     private let manifestLoader: ManifestLoaderProtocol
     private let toolsVersionLoader: ToolsVersionLoaderProtocol
     private let currentToolsVersion: ToolsVersion
@@ -53,6 +53,7 @@ public final class LocalPackageContainer: PackageContainer {
                                     version: nil,
                                     revision: nil,
                                     toolsVersion: toolsVersion,
+                                    identityResolver: identityResolver,
                                     fileSystem: fileSystem,
                                     diagnostics: nil,
                                     on: .global(),
@@ -62,7 +63,7 @@ public final class LocalPackageContainer: PackageContainer {
     }
 
     public func getUnversionedDependencies(productFilter: ProductFilter) throws -> [PackageContainerConstraint] {
-        return try loadManifest().dependencyConstraints(productFilter: productFilter, mirrors: mirrors)
+        return try loadManifest().dependencyConstraints(productFilter: productFilter)
     }
 
     public func getUpdatedIdentifier(at boundVersion: BoundVersion) throws -> PackageReference {
@@ -73,15 +74,15 @@ public final class LocalPackageContainer: PackageContainer {
 
     public init(
         package: PackageReference,
-        mirrors: DependencyMirrors,
+        identityResolver: IdentityResolver,
         manifestLoader: ManifestLoaderProtocol,
         toolsVersionLoader: ToolsVersionLoaderProtocol,
         currentToolsVersion: ToolsVersion,
         fileSystem: FileSystem = localFileSystem
     ) {
         assert(URL.scheme(package.location) == nil, "unexpected scheme \(URL.scheme(package.location)!) in \(package.location)")
         self.package = package
-        self.mirrors = mirrors
+        self.identityResolver = identityResolver
         self.manifestLoader = manifestLoader
         self.toolsVersionLoader = toolsVersionLoader
         self.currentToolsVersion = currentToolsVersion

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -20,7 +20,7 @@ extension PackageGraph {
     /// Load the package graph for the given package path.
     public static func load(
         root: PackageGraphRoot,
-        mirrors: DependencyMirrors = [:],
+        identityResolver: IdentityResolver,
         additionalFileRules: [FileRuleDescription] = [],
         externalManifests: [Manifest],
         requiredDependencies: Set<PackageReference> = [],
@@ -39,25 +39,24 @@ extension PackageGraph {
         // the URL but that shouldn't be needed after <rdar://problem/33693433>
         // Ensure that identity and package name are the same once we have an
         // API to specify identity in the manifest file
-        let manifestMapSequence = (root.manifests + externalManifests).map({ (PackageIdentity(url: $0.packageLocation), $0) })
+        let manifestMapSequence = (root.manifests + externalManifests).map({ (identityResolver.resolveIdentity(for: $0.packageLocation), $0) })
         let manifestMap = Dictionary(uniqueKeysWithValues: manifestMapSequence)
         let successors: (GraphLoadingNode) -> [GraphLoadingNode] = { node in
-            node.requiredDependencies().compactMap({ dependency in
-                let url = mirrors.effectiveURL(forURL: dependency.location)
-                return manifestMap[PackageIdentity(url: url)].map { manifest in
+            node.requiredDependencies().compactMap{ dependency in
+                return manifestMap[dependency.identity].map { manifest in
                     GraphLoadingNode(manifest: manifest, productFilter: dependency.productFilter)
                 }
-            })
+            }
         }
 
         // Construct the root manifest and root dependencies set.
         let rootManifestSet = Set(root.manifests)
-        let rootDependencies = Set(root.dependencies.compactMap({
-            manifestMap[PackageIdentity(url: $0.location)]
-        }))
+        let rootDependencies = Set(root.dependencies.compactMap{
+            manifestMap[$0.identity]
+        })
         let rootManifestNodes = root.manifests.map { GraphLoadingNode(manifest: $0, productFilter: .everything) }
         let rootDependencyNodes = root.dependencies.lazy.compactMap { (dependency: PackageDependencyDescription) -> GraphLoadingNode? in
-            guard let manifest = manifestMap[PackageIdentity(url: dependency.location)] else { return nil }
+            guard let manifest = manifestMap[dependency.identity] else { return nil }
             return GraphLoadingNode(manifest: manifest, productFilter: dependency.productFilter)
         }
         let inputManifests = rootManifestNodes + rootDependencyNodes
@@ -130,7 +129,7 @@ extension PackageGraph {
         // Resolve dependencies and create resolved packages.
         let resolvedPackages = try createResolvedPackages(
             allManifests: allManifests,
-            mirrors: mirrors,
+            identityResolver: identityResolver,
             manifestToPackage: manifestToPackage,
             rootManifestSet: rootManifestSet,
             unsafeAllowedPackages: unsafeAllowedPackages,
@@ -188,7 +187,7 @@ private func checkAllDependenciesAreUsed(_ rootPackages: [ResolvedPackage], _ di
 /// Create resolved packages from the loaded packages.
 private func createResolvedPackages(
     allManifests: [GraphLoadingNode],
-    mirrors: DependencyMirrors,
+    identityResolver: IdentityResolver,
     manifestToPackage: [Manifest: Package],
     // FIXME: This shouldn't be needed once <rdar://problem/33693433> is fixed.
     rootManifestSet: Set<Manifest>,
@@ -211,7 +210,7 @@ private func createResolvedPackages(
 
     // Create a map of package builders keyed by the package identity.
     let packageMapByIdentity: [PackageIdentity: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{
-        let identity = PackageIdentity(url: $0.package.manifest.packageLocation)
+        let identity = identityResolver.resolveIdentity(for: $0.package.manifest.packageLocation)
         return (identity, $0)
     }
     let packageMapByName: [String: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{ ($0.package.name, $0) }
@@ -223,8 +222,15 @@ private func createResolvedPackages(
         var dependencies = [ResolvedPackageBuilder]()
         // Establish the manifest-declared package dependencies.
         package.manifest.dependenciesRequired(for: packageBuilder.productFilter).forEach { dependency in
-            let dependencyURL = mirrors.effectiveURL(forURL: dependency.location)
-            let dependencyIdentity = PackageIdentity(url: dependencyURL)
+            let dependencyIdentity = dependency.identity
+            // FIXME: change this validation logic to use identity instead of location
+            let dependencyLocation: String
+            switch dependency {
+            case .local(let data):
+                dependencyLocation = data.path.pathString
+            case .scm(let data):
+                dependencyLocation = data.location
+            }
 
             // Use the package name to lookup the dependency. The package name will be present in packages with tools version >= 5.2.
             if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly, let resolvedPackage = packageMapByName[explicitDependencyName] {
@@ -234,7 +240,7 @@ private func createResolvedPackages(
                     // FIXME: this works but the way we find out about this is based on a side effect, need to improve it when working on identity
                     let error = PackageGraphError.dependencyAlreadySatisfiedByName(
                         dependencyPackageName: package.name,
-                        dependencyURL: dependencyURL,
+                        dependencyLocation: dependencyLocation,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         name: explicitDependencyName)
                     let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
@@ -251,7 +257,7 @@ private func createResolvedPackages(
                 guard !dependencies.contains(resolvedPackage) else {
                     let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         dependencyPackageName: package.name,
-                        dependencyURL: dependencyURL,
+                        dependencyLocation: dependencyLocation,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         identity: dependencyIdentity)
                     let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
@@ -262,10 +268,10 @@ private func createResolvedPackages(
                     // check if this resolvedPackage url is the same as the dependency one
                     // if not, this means that the dependencies share the same identity
                     // FIXME: this works but the way we find out about this is based on a side effect, need to improve it when working on identity
-                    if resolvedPackage.package.manifest.packageLocation != dependencyURL {
+                    if resolvedPackage.package.manifest.packageLocation != dependencyLocation {
                         let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                             dependencyPackageName: package.name,
-                            dependencyURL: dependencyURL,
+                            dependencyLocation: dependencyLocation,
                             otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                             identity: dependencyIdentity)
                         let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
@@ -274,7 +280,7 @@ private func createResolvedPackages(
                         let error = PackageGraphError.incorrectPackageDependencyName(
                             dependencyPackageName: package.name,
                             dependencyName: explicitDependencyName,
-                            dependencyURL: dependencyURL,
+                            dependencyLocation: dependencyLocation,
                             resolvedPackageName: resolvedPackage.package.name,
                             resolvedPackageURL: resolvedPackage.package.manifest.packageLocation)
                         let diagnosticLocation = PackageLocation.Local(name: package.name, packagePath: package.path)
@@ -414,14 +420,11 @@ private func createResolvedPackages(
                     // explicitly reference the package containing the product, or for the product, package and
                     // dependency to share the same name. We don't check this in manifest loading for root-packages so
                     // we can provide a more detailed diagnostic here.
-                    let referencedPackageURL = mirrors.effectiveURL(forURL: product.packageBuilder.package.manifest.packageLocation)
-                    let referencedPackageIdentity = PackageIdentity(url: referencedPackageURL)
+                    let referencedPackageIdentity = identityResolver.resolveIdentity(for: product.packageBuilder.package.manifest.packageLocation)
                     guard let packageDependency = (packageBuilder.package.manifest.dependencies.first { package in
-                        let packageURL = mirrors.effectiveURL(forURL: package.location)
-                        let packageIdentity = PackageIdentity(url: packageURL)
-                        return packageIdentity == referencedPackageIdentity
+                        return package.identity == referencedPackageIdentity
                     }) else {
-                        throw InternalError("dependency reference for \(referencedPackageURL) not found")
+                        throw InternalError("dependency reference for \(product.packageBuilder.package.manifest.packageLocation) not found")
                     }
 
                     let packageName = product.packageBuilder.package.name

Sources/PackageGraph/PackageGraph.swift

@@ -25,13 +25,13 @@ enum PackageGraphError: Swift.Error {
     case productDependencyIncorrectPackage(name: String, package: String)
 
     /// The package dependency name does not match the package name.
-    case incorrectPackageDependencyName(dependencyPackageName: String, dependencyName: String, dependencyURL: String, resolvedPackageName: String, resolvedPackageURL: String)
+    case incorrectPackageDependencyName(dependencyPackageName: String, dependencyName: String, dependencyLocation: String, resolvedPackageName: String, resolvedPackageURL: String)
 
     /// The package dependency already satisfied by a different dependency package
-    case dependencyAlreadySatisfiedByIdentifier(dependencyPackageName: String, dependencyURL: String, otherDependencyURL: String, identity: PackageIdentity)
+    case dependencyAlreadySatisfiedByIdentifier(dependencyPackageName: String, dependencyLocation: String, otherDependencyURL: String, identity: PackageIdentity)
 
     /// The package dependency already satisfied by a different dependency package
-    case dependencyAlreadySatisfiedByName(dependencyPackageName: String, dependencyURL: String, otherDependencyURL: String, name: String)
+    case dependencyAlreadySatisfiedByName(dependencyPackageName: String, dependencyLocation: String, otherDependencyURL: String, name: String)
 
     /// The product dependency was found but the package name was not referenced correctly (tools version > 5.2).
     case productDependencyMissingPackage(
@@ -233,12 +233,12 @@ fileprivate extension PackageDependencyDescription {
             parameters.append("name: \"\(name)\"")
         }
 
-        if requirement == .localPackage {
-            parameters.append("path: \"\(self.location)\"")
-        } else {
-            parameters.append("url: \"\(self.location)\"")
-
-            switch requirement {
+        switch self {
+        case .local(let data):
+            parameters.append("path: \"\(data.path)\"")
+        case .scm(let data):
+            parameters.append("url: \"\(data.location)\"")
+            switch data.requirement {
             case .branch(let branch):
                 parameters.append(".branch(\"\(branch)\")")
             case .exact(let version):
@@ -253,8 +253,6 @@ fileprivate extension PackageDependencyDescription {
                 } else {
                     parameters.append(".upToNextMinor(\"\(range.lowerBound)\"..<\"\(range.upperBound)\")")
                 }
-            case .localPackage:
-                fatalError("handled above")
             }
         }