Enable use of product ID to distinguish duplicate product names (#5990)

Resolves rdar://102138022

Author: elsh

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -522,18 +522,20 @@ private class DuplicateProductsChecker {
     init(packageBuilders: [ResolvedPackageBuilder]) {
         for packageBuilder in packageBuilders {
             let pkgID = packageBuilder.package.identity.description.lowercased()
-            packageIDToBuilder[pkgID] = packageBuilder
+            self.packageIDToBuilder[pkgID] = packageBuilder
         }
     }
 
     func run(lookupByProductIDs: Bool = false, observabilityScope: ObservabilityScope) throws {
         var productToPkgMap = [String: [String]]()
-        for (_, pkgBuilder) in packageIDToBuilder {
+        for (pkgID, pkgBuilder) in packageIDToBuilder {
             let useProductIDs = pkgBuilder.package.manifest.disambiguateByProductIDs || lookupByProductIDs
             let depProductRefs = pkgBuilder.package.targets.map{$0.dependencies}.flatMap{$0}.compactMap{$0.product}
             for depRef in depProductRefs {
                 if let depPkg = depRef.package?.lowercased() {
-                    checkedPkgIDs.append(depPkg)
+                    if !checkedPkgIDs.contains(depPkg) {
+                        checkedPkgIDs.append(depPkg)
+                    }
                     let depProductIDs = packageIDToBuilder[depPkg]?.package.products.filter { $0.identity == depRef.identity }.map { useProductIDs && $0.isDefaultLibrary ? $0.identity : $0.name } ?? []
                     for depID in depProductIDs {
                         productToPkgMap[depID, default: []].append(depPkg)
@@ -543,18 +545,30 @@ private class DuplicateProductsChecker {
                     productToPkgMap[depRef.name, default: []].append(contentsOf: depPkgs)
                     checkedPkgIDs.append(contentsOf: depPkgs)
                 }
+                if !checkedPkgIDs.contains(pkgID.lowercased()) {
+                    checkedPkgIDs.append(pkgID.lowercased())
+                }
             }
             for (depIDOrName, depPkgs) in productToPkgMap.filter({Set($0.value).count > 1}) {
                 let name = depIDOrName.components(separatedBy: "_").dropFirst().joined(separator: "_")
                 throw PackageGraphError.duplicateProduct(product: name.isEmpty ? depIDOrName : name, packages: depPkgs.sorted())
             }
         }
 
-        let uncheckedPkgs = packageIDToBuilder.filter{!checkedPkgIDs.contains($0.key)}
-        for (pkgID, pkgBuilder) in uncheckedPkgs {
-            let productIDOrNames = pkgBuilder.products.map { pkgBuilder.package.manifest.disambiguateByProductIDs && $0.product.isDefaultLibrary ? $0.product.identity : $0.product.name }
-            for productIDOrName in productIDOrNames {
-                productToPkgMap[productIDOrName, default: []].append(pkgID)
+        // Check packages that exist but are not in a dependency graph
+        let untrackedPkgs = packageIDToBuilder.filter{!checkedPkgIDs.contains($0.key.lowercased())}
+        for (pkgID, pkgBuilder) in untrackedPkgs {
+            for product in pkgBuilder.products {
+                // Check if checking product ID only is safe
+                let useIDOnly = lookupByProductIDs && product.product.isDefaultLibrary
+                if !useIDOnly {
+                    // This untracked pkg could have a product name conflicting with a
+                    // product name from another package, but since it's not depended on
+                    // by other packages, keep track of both this product's name and ID
+                    // just in case other packages are < .v5_8
+                    productToPkgMap[product.product.name, default: []].append(pkgID)
+                }
+                productToPkgMap[product.product.identity, default: []].append(pkgID)
             }
         }
 

Sources/PackageModel/PackageModel.swift

@@ -124,7 +124,7 @@ extension Package.Error: CustomStringConvertible {
 
 extension Manifest {
     public var disambiguateByProductIDs: Bool {
-        return self.toolsVersion >= .vNext
+        return self.toolsVersion >= .v5_8
     }
 }
 

Sources/PackageModel/Target.swift

@@ -54,9 +54,12 @@ public class Target: PolymorphicCodableProtocol {
         public var identity: String {
             if let pkg = package {
                 return pkg.lowercased() + "_" + name
+            } else {
+                // this is hit only if this product is referenced `.byName(name)`
+                // which assumes the name of this product, its package, and its target
+                // all have the same name
+                return name.lowercased() + "_" + name
             }
-            // package ID won't be included for products referenced only by name
-            return name
         }
 
         /// Creates a product reference instance.

Tests/BuildTests/BuildPlanTests.swift

@@ -122,7 +122,7 @@ final class BuildPlanTests: XCTestCase {
                 Manifest.createRootManifest(
                     name: "thisPkg",
                     path: .init(path: "/thisPkg"),
-                    toolsVersion: .vNext,
+                    toolsVersion: .v5_8,
                     dependencies: [
                         .localSourceControl(path: .init(path: "/fooPkg"), requirement: .upToNextMajor(from: "1.0.0")),
                         .localSourceControl(path: .init(path: "/barPkg"), requirement: .upToNextMajor(from: "2.0.0")),
@@ -188,7 +188,7 @@ final class BuildPlanTests: XCTestCase {
                 Manifest.createFileSystemManifest(
                     name: "fooPkg",
                     path: .init(path: "/fooPkg"),
-                    toolsVersion: .vNext,
+                    toolsVersion: .v5_8,
                     dependencies: [
                         .localSourceControl(path: .init(path: "/barPkg"), requirement: .upToNextMajor(from: "1.0.0")),
                         .localSourceControl(path: .init(path: "/bazPkg"), requirement: .upToNextMajor(from: "1.0.0")),
@@ -227,7 +227,7 @@ final class BuildPlanTests: XCTestCase {
                 Manifest.createRootManifest(
                     name: "thisPkg",
                     path: .init(path: "/thisPkg"),
-                    toolsVersion: .vNext,
+                    toolsVersion: .v5_8,
                     dependencies: [
                         .localSourceControl(path: .init(path: "/xPkg"), requirement: .upToNextMajor(from: "1.0.0")),
                         .localSourceControl(path: .init(path: "/yPkg"), requirement: .upToNextMajor(from: "1.0.0")),
@@ -298,7 +298,7 @@ final class BuildPlanTests: XCTestCase {
                 Manifest.createFileSystemManifest(
                     name: "fooPkg",
                     path: .init(path: "/fooPkg"),
-                    toolsVersion: .vNext,
+                    toolsVersion: .v5_8,
                     dependencies: [
                         .localSourceControl(path: .init(path: "/barPkg"), requirement: .upToNextMajor(from: "1.0.0")),
                         .localSourceControl(path: .init(path: "/bazPkg"), requirement: .upToNextMajor(from: "1.0.0")),
@@ -360,7 +360,7 @@ final class BuildPlanTests: XCTestCase {
                 Manifest.createFileSystemManifest(
                     name: "fooPkg",
                     path: .init(path: "/fooPkg"),
-                    toolsVersion: .vNext,
+                    toolsVersion: .v5_8,
                     dependencies: [
                         .localSourceControl(path: .init(path: "/barPkg"), requirement: .upToNextMajor(from: "1.0.0")),
                     ],
@@ -503,7 +503,7 @@ final class BuildPlanTests: XCTestCase {
                 Manifest.createRootManifest(
                     name: "thisPkg",
                     path: .init(path: "/thisPkg"),
-                    toolsVersion: .vNext,
+                    toolsVersion: .v5_8,
                     dependencies: [
                         .localSourceControl(path: .init(path: "/fooPkg"), requirement: .upToNextMajor(from: "1.0.0")),
                         .localSourceControl(path: .init(path: "/barPkg"), requirement: .upToNextMajor(from: "2.0.0")),