identity improvements

motivation: improve correctness by using identity more broadly throughout the codebase

changes:
* expose identity on Package, ResolvedPackage and GraphLoadingNode
* reduce calls to identityResolver.resolveIdentity since we now have identity more broadly available (also improves performance)
* ManifestLoader now take PackageIdentity to reduce calls to identityResolver.resolveIdentity when the identity is already known
* introduce PackageIdentity.root as a utility for root packages, since their identity is straight-forward
* adjust call-sites
* adjust tests

Author: tomerd

Sources/Build/BuildOperationBuildSystemDelegateHandler.swift

@@ -215,13 +215,8 @@ public struct BuildDescription: Codable {
         self.testDiscoveryCommands = testDiscoveryCommands
         self.copyCommands = copyCommands
 
-        self.builtTestProducts = try plan.buildProducts.filter{ $0.product.type == .test }.map { desc in
-            // FIXME(perf): Provide faster lookups.
-            guard let package = (plan.graph.packages.first{ $0.products.contains(desc.product) }) else {
-                throw InternalError("package with product \(desc.product) not found")
-            }
+        self.builtTestProducts = plan.buildProducts.filter{ $0.product.type == .test }.map { desc in
             return BuiltTestProduct(
-                packageName: package.name,
                 productName: desc.product.name,
                 binaryPath: desc.binary
             )

Sources/Commands/Describe.swift

@@ -55,7 +55,7 @@ fileprivate struct DescribedPackage: Encodable {
     let swiftLanguagesVersions: [String]?
 
     init(from package: Package) {
-        self.name = package.name
+        self.name = package.manifestName // TODO: rename property to manifestName?
         self.path = package.path.pathString
         self.toolsVersion = "\(package.manifest.toolsVersion.major).\(package.manifest.toolsVersion.minor)"
             + (package.manifest.toolsVersion.patch == 0 ? "" : ".\(package.manifest.toolsVersion.patch)")

Sources/Commands/SwiftPackageTool.swift

@@ -174,8 +174,11 @@ extension SwiftPackageTool {
                 workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
             }
             guard let manifest = manifests.first else { return }
+            // FIXME: use PackageIdentity.root?
+            let packageIdentity = workspace.identityResolver.resolveIdentity(for: manifest.packageLocation)
 
             let builder = PackageBuilder(
+                identity: packageIdentity,
                 manifest: manifest,
                 productFilter: .everything,
                 path: try swiftTool.getPackageRoot(),
@@ -240,8 +243,11 @@ extension SwiftPackageTool {
             let manifest = try temp_await {
                 workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
             }[0]
+            // FIXME: use PackageIdentity.root?
+            let packageIdentity = workspace.identityResolver.resolveIdentity(for: manifest.packageLocation)
 
             let builder = PackageBuilder(
+                identity: packageIdentity,
                 manifest: manifest,
                 productFilter: .everything,
                 path: try swiftTool.getPackageRoot(),
@@ -582,7 +588,7 @@ extension SwiftPackageTool {
                 destination = output
             } else {
                 let graph = try swiftTool.loadPackageGraph()
-                let packageName = graph.rootPackages[0].name
+                let packageName = graph.rootPackages[0].manifestName // TODO: use identity instead?
                 destination = packageRoot.appending(component: "\(packageName).zip")
             }
 
@@ -657,10 +663,10 @@ extension SwiftPackageTool {
                 dstdir = outpath.parentDirectory
             case let outpath?:
                 dstdir = outpath
-                projectName = graph.rootPackages[0].name
+                projectName = graph.rootPackages[0].manifestName // TODO: use identity instead?
             case _:
                 dstdir = try swiftTool.getPackageRoot()
-                projectName = graph.rootPackages[0].name
+                projectName = graph.rootPackages[0].manifestName // TODO: use identity instead?
             }
             let xcodeprojPath = Xcodeproj.buildXcodeprojPath(outputDir: dstdir, projectName: projectName)
 

Sources/Commands/SwiftTestTool.swift

@@ -352,6 +352,12 @@ public struct SwiftTestTool: SwiftCommand {
 
     /// Processes the code coverage data and emits a json.
     private func processCodeCoverage(_ testProducts: [BuiltTestProduct], swiftTool: SwiftTool) throws {
+        let workspace = try swiftTool.getActiveWorkspace()
+        let root = try swiftTool.getWorkspaceRoot()
+        let rootManifest = try temp_await {
+            workspace.loadRootManifests(packages: root.packages, diagnostics: swiftTool.diagnostics, completion: $0)
+        }[0]
+
         // Merge all the profraw files to produce a single profdata file.
         try mergeCodeCovRawDataFiles(swiftTool: swiftTool)
 
@@ -360,7 +366,7 @@ public struct SwiftTestTool: SwiftCommand {
             // Export the codecov data as JSON.
             let jsonPath = codeCovAsJSONPath(
                 buildParameters: buildParameters,
-                packageName: product.packageName)
+                packageName: rootManifest.name)
             try exportCodeCovAsJSON(to: jsonPath, testBinary: product.binaryPath, swiftTool: swiftTool)
         }
     }

Sources/Commands/show-dependencies.swift

@@ -44,7 +44,7 @@ private final class PlainTextDumper: DependenciesDumper {
 
                 let pkgVersion = package.manifest.version?.description ?? "unspecified"
 
-                stream <<< "\(hanger)\(package.name)<\(package.manifest.packageLocation)@\(pkgVersion)>\n"
+                stream <<< "\(hanger)\(package.identity.description)<\(package.manifest.packageLocation)@\(pkgVersion)>\n"
 
                 if !package.dependencies.isEmpty {
                     let replacement = (index == packages.count - 1) ?  "    " : "│   "
@@ -69,7 +69,7 @@ private final class FlatListDumper: DependenciesDumper {
     func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) {
         func recursiveWalk(packages: [ResolvedPackage]) {
             for package in packages {
-                stream <<< package.name <<< "\n"
+                stream <<< package.identity.description <<< "\n"
                 if !package.dependencies.isEmpty {
                     recursiveWalk(packages: package.dependencies)
                 }
@@ -88,7 +88,8 @@ private final class DotDumper: DependenciesDumper {
             let url = package.manifest.packageLocation
             if nodesAlreadyPrinted.contains(url) { return }
             let pkgVersion = package.manifest.version?.description ?? "unspecified"
-            stream <<< #""\#(url)" [label="\#(package.name)\n\#(url)\n\#(pkgVersion)"]"# <<< "\n"
+            // 👀 is this correct? or manifest name is better?
+            stream <<< #""\#(url)" [label="\#(package.identity.description)\n\#(url)\n\#(pkgVersion)"]"# <<< "\n"
             nodesAlreadyPrinted.insert(url)
         }
 
@@ -130,7 +131,7 @@ private final class JSONDumper: DependenciesDumper {
     func dump(dependenciesOf rootpkg: ResolvedPackage, on stream: OutputByteStream) {
         func convert(_ package: ResolvedPackage) -> JSON {
             return .orderedDictionary([
-                "name": .string(package.name),
+                "name": .string(package.manifestName), // TODO: remove? add identity?
                 "url": .string(package.manifest.packageLocation),
                 "version": .string(package.manifest.version?.description ?? "unspecified"),
                 "path": .string(package.path.pathString),

Sources/PackageGraph/GraphLoadingNode.swift

@@ -20,13 +20,17 @@ import TSCUtility
 /// - SeeAlso: DependencyResolutionNode
 public struct GraphLoadingNode: Equatable, Hashable {
 
+    /// The package identity.
+    public let identity: PackageIdentity
+
     /// The package manifest.
     public let manifest: Manifest
 
     /// The product filter applied to the package.
     public let productFilter: ProductFilter
 
-    public init(manifest: Manifest, productFilter: ProductFilter) {
+    public init(identity: PackageIdentity, manifest: Manifest, productFilter: ProductFilter) {
+        self.identity = identity
         self.manifest = manifest
         self.productFilter = productFilter
     }
@@ -41,9 +45,9 @@ extension GraphLoadingNode: CustomStringConvertible {
     public var description: String {
         switch productFilter {
         case .everything:
-            return manifest.name
+            return self.manifest.name
         case .specific(let set):
-            return "\(manifest.name)[\(set.sorted().joined(separator: ", "))]"
+            return "\(self.manifest.name)[\(set.sorted().joined(separator: ", "))]"
         }
     }
 }

Sources/PackageGraph/LocalPackageContainer.swift

@@ -39,22 +39,23 @@ public final class LocalPackageContainer: PackageContainer {
     private func loadManifest() throws -> Manifest {
         try manifest.memoize() {
             // Load the tools version.
-            let toolsVersion = try toolsVersionLoader.load(at: AbsolutePath(package.location), fileSystem: fileSystem)
+            let toolsVersion = try self.toolsVersionLoader.load(at: AbsolutePath(self.package.location), fileSystem: self.fileSystem)
 
             // Validate the tools version.
-            try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: package.location)
+            try toolsVersion.validateToolsVersion(self.currentToolsVersion, packagePath: self.package.location)
 
             // Load the manifest.
             // FIXME: this should not block
             return try temp_await {
-                manifestLoader.load(at: AbsolutePath(package.location),
-                                    packageKind: package.kind,
-                                    packageLocation: package.location,
+                manifestLoader.load(at: AbsolutePath(self.package.location),
+                                    packageIdentity: self.package.identity,
+                                    packageKind: self.package.kind,
+                                    packageLocation: self.package.location,
                                     version: nil,
                                     revision: nil,
                                     toolsVersion: toolsVersion,
-                                    identityResolver: identityResolver,
-                                    fileSystem: fileSystem,
+                                    identityResolver: self.identityResolver,
+                                    fileSystem: self.fileSystem,
                                     diagnostics: nil,
                                     on: .global(),
                                     completion: $0)