improve and relax the validation of explicit dependency names (#3687)

motivation: follow up work to relaxing the requirement to set an explicit name on dependency, this relaxes the requirement that such name (if/when set) must match the manifest's name attribute, which we want to deprecate over time

changes:
* change the validation flow to ignore the name set in the manifest, instead manage an in-memory list of names used for target based package lookup which suffices
* remove a few tests that are no longer needed given the relaxed requirment
* fix a couple of tests that were always wrong but previous validation did not catch

Author: tomerd

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -205,10 +205,12 @@ private func createResolvedPackages(
             return nil
         }
         let isAllowedToVendUnsafeProducts = unsafeAllowedPackages.contains{ $0.location == package.manifest.packageLocation }
+        let allowedToOverride = rootManifestSet.contains(node.manifest)
         return ResolvedPackageBuilder(
             package,
             productFilter: node.productFilter,
-            isAllowedToVendUnsafeProducts: isAllowedToVendUnsafeProducts
+            isAllowedToVendUnsafeProducts: isAllowedToVendUnsafeProducts,
+            allowedToOverride: allowedToOverride
         )
     }
 
@@ -218,21 +220,15 @@ private func createResolvedPackages(
         return ($0.package.identity, $0)
     }
 
-    // in case packages have same manifest name this map can miss packages which will lead to missing product errors
-    // our plan is to deprecate the use of manifest + dependency explicit name in target dependency lookup and instead lean 100% on identity
-    // which means this map would go away too
-    let packageMapByNameForTargetDependencyResolutionOnly = packageBuilders.reduce(into: [String: ResolvedPackageBuilder](), { partial, item in
-        partial[item.package.manifestName] = item
-    })
-
     // Scan and validate the dependencies
     for packageBuilder in packageBuilders {
         let package = packageBuilder.package
 
         var dependencies = [ResolvedPackageBuilder]()
+        var dependenciesByNameForTargetDependencyResolution = [String: ResolvedPackageBuilder]()
+
         // Establish the manifest-declared package dependencies.
         package.manifest.dependenciesRequired(for: packageBuilder.productFilter).forEach { dependency in
-            let dependencyIdentity = dependency.identity
             // FIXME: change this validation logic to use identity instead of location
             let dependencyLocation: String
             switch dependency {
@@ -245,57 +241,57 @@ private func createResolvedPackages(
                 fatalError("registry based dependencies not implemented yet")
             }
 
-            // 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 = packageMapByNameForTargetDependencyResolutionOnly[explicitDependencyName] {
+            // Otherwise, look it up by its identity.
+            if let resolvedPackage = packageMapByIdentity[dependency.identity] {
+                // check if this resolved package already listed in the dependencies
+                // 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
                 guard !dependencies.contains(resolvedPackage) else {
-                    // check if this resolvedPackage already listed in the dependencies
-                    // this means that the dependencies share the same name
-                    // 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(
+                    let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         package: package.identity.description,
                         dependencyLocation: dependencyLocation,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
-                        name: explicitDependencyName)
+                        identity: dependency.identity)
                     return diagnostics.emit(error, location: package.diagnosticLocation)
                 }
-                return dependencies.append(resolvedPackage)
-            }
 
-            // Otherwise, look it up by its identity.
-            if let resolvedPackage = packageMapByIdentity[dependencyIdentity] {
-                // check if this resolvedPackage already listed in the dependencies
-                // 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
-                guard !dependencies.contains(resolvedPackage) else {
+                // check if the resolved package 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
+                if resolvedPackage.package.manifest.packageLocation != dependencyLocation && !resolvedPackage.allowedToOverride {
                     let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         package: package.identity.description,
                         dependencyLocation: dependencyLocation,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
-                        identity: dependencyIdentity)
+                        identity: dependency.identity)
                     return diagnostics.emit(error, location: package.diagnosticLocation)
                 }
-                // check that the explicit package dependency name matches the package name.
-                if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly, resolvedPackage.package.manifestName != explicitDependencyName {
-                    // 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 != dependencyLocation {
-                        let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
-                            package: package.identity.description,
-                            dependencyLocation: dependencyLocation,
-                            otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
-                            identity: dependencyIdentity)
-                        return diagnostics.emit(error, location: package.diagnosticLocation)
-                    } else  {
-                        let error = PackageGraphError.incorrectPackageDependencyName(
+
+                // checks if two dependencies have the same explicit name which can cause target based dependency package lookup issue
+                if let explicitDependencyName = dependency.explicitNameForTargetDependencyResolutionOnly {
+                    if let previouslyResolvedPackage = dependenciesByNameForTargetDependencyResolution[explicitDependencyName] {
+                        let error = PackageGraphError.dependencyAlreadySatisfiedByName(
                             package: package.identity.description,
-                            dependencyName: explicitDependencyName,
                             dependencyLocation: dependencyLocation,
-                            resolvedPackageManifestName: resolvedPackage.package.manifestName,
-                            resolvedPackageURL: resolvedPackage.package.manifest.packageLocation)
+                            otherDependencyURL: previouslyResolvedPackage.package.manifest.packageLocation,
+                            name: explicitDependencyName)
                         return diagnostics.emit(error, location: package.diagnosticLocation)
                     }
                 }
+
+                // checks if two dependencies have the same implicit (identity based) name which can cause target based dependency package lookup issue
+                if let previouslyResolvedPackage = dependenciesByNameForTargetDependencyResolution[dependency.identity.description] {
+                    let error = PackageGraphError.dependencyAlreadySatisfiedByName(
+                        package: package.identity.description,
+                        dependencyLocation: dependencyLocation,
+                        otherDependencyURL: previouslyResolvedPackage.package.manifest.packageLocation,
+                        name: dependency.identity.description)
+                    return diagnostics.emit(error, location: package.diagnosticLocation)
+                }
+
+                let nameForTargetDependencyResolution = dependency.explicitNameForTargetDependencyResolutionOnly ?? dependency.identity.description
+                dependenciesByNameForTargetDependencyResolution[nameForTargetDependencyResolution] = resolvedPackage
+                
                 dependencies.append(resolvedPackage)
             }
         }
@@ -588,10 +584,13 @@ private final class ResolvedPackageBuilder: ResolvedBuilder<ResolvedPackage> {
 
     let isAllowedToVendUnsafeProducts: Bool
 
-    init(_ package: Package, productFilter: ProductFilter, isAllowedToVendUnsafeProducts: Bool) {
+    let allowedToOverride: Bool
+
+    init(_ package: Package, productFilter: ProductFilter, isAllowedToVendUnsafeProducts: Bool, allowedToOverride: Bool) {
         self.package = package
         self.productFilter = productFilter
         self.isAllowedToVendUnsafeProducts = isAllowedToVendUnsafeProducts
+        self.allowedToOverride = allowedToOverride
     }
 
     override func constructImpl() throws -> ResolvedPackage {

Sources/PackageGraph/PackageGraph.swift

@@ -21,9 +21,6 @@ enum PackageGraphError: Swift.Error {
     /// The product dependency not found.
     case productDependencyNotFound(package: String, targetName: String, dependencyProductName: String, dependencyPackageName: String?, dependencyProductInDecl: Bool)
 
-    /// The package dependency name does not match the package name.
-    case incorrectPackageDependencyName(package: String, dependencyName: String, dependencyLocation: String, resolvedPackageManifestName: String, resolvedPackageURL: String)
-
     /// The package dependency already satisfied by a different dependency package
     case dependencyAlreadySatisfiedByIdentifier(package: String, dependencyLocation: String, otherDependencyURL: String, identity: PackageIdentity)
 
@@ -58,7 +55,7 @@ public struct PackageGraph {
 
     /// Returns all the targets in the graph, regardless if they are reachable from the root targets or not.
     public let allTargets: Set<ResolvedTarget>
- 
+
     /// Returns all the products in the graph, regardless if they are reachable from the root targets or not.
     public let allProducts: Set<ResolvedProduct>
 
@@ -79,17 +76,18 @@ public struct PackageGraph {
         return rootPackages.contains(package)
     }
 
+    private let targetsToPackages: [ResolvedTarget: ResolvedPackage]
     /// Returns the package that contains the target, or nil if the target isn't in the graph.
     public func package(for target: ResolvedTarget) -> ResolvedPackage? {
         return self.targetsToPackages[target]
     }
-    private let targetsToPackages: [ResolvedTarget: ResolvedPackage]
 
-    /// Returns the package that contains the product, or nil if the product isn't in the graph.
-     public func package(for product: ResolvedProduct) -> ResolvedPackage? {
-         return self.productsToPackages[product]
-     }
+
     private let productsToPackages: [ResolvedProduct: ResolvedPackage]
+    /// Returns the package that contains the product, or nil if the product isn't in the graph.
+    public func package(for product: ResolvedProduct) -> ResolvedPackage? {
+        return self.productsToPackages[product]
+    }
 
     /// All root and root dependency packages provided as input to the graph.
     public let inputPackages: [ResolvedPackage]
@@ -189,32 +187,26 @@ extension PackageGraphError: CustomStringConvertible {
 
         case .cycleDetected(let cycle):
             return "cyclic dependency declaration found: " +
-                (cycle.path + cycle.cycle).map({ $0.name }).joined(separator: " -> ") +
-                " -> " + cycle.cycle[0].name
+            (cycle.path + cycle.cycle).map({ $0.name }).joined(separator: " -> ") +
+            " -> " + cycle.cycle[0].name
 
         case .productDependencyNotFound(let package, let targetName, let dependencyProductName, let dependencyPackageName, let dependencyProductInDecl):
             if dependencyProductInDecl {
                 return "product '\(dependencyProductName)' is declared in the same package '\(package)' and can't be used as a dependency for target '\(targetName)'."
             } else {
                 return "product '\(dependencyProductName)' required by package '\(package)' target '\(targetName)' \(dependencyPackageName.map{ "not found in package '\($0)'" } ?? "not found")."
             }
-        case .incorrectPackageDependencyName(let package, let dependencyName, let dependencyURL, let resolvedPackageManifestName, let resolvedPackageURL):
-            return """
-                '\(package)' dependency on '\(dependencyURL)' has an explicit name '\(dependencyName)' which does not match the \
-                name '\(resolvedPackageManifestName)' set for '\(resolvedPackageURL)'
-                """
-
         case .dependencyAlreadySatisfiedByIdentifier(let package, let dependencyURL, let otherDependencyURL, let identity):
             return "'\(package)' dependency on '\(dependencyURL)' conflicts with dependency on '\(otherDependencyURL)' which has the same identity '\(identity)'"
 
         case .dependencyAlreadySatisfiedByName(let package, let dependencyURL, let otherDependencyURL, let name):
             return "'\(package)' dependency on '\(dependencyURL)' conflicts with dependency on '\(otherDependencyURL)' which has the same explicit name '\(name)'"
 
         case .productDependencyMissingPackage(
-                let productName,
-                let targetName,
-                let packageIdentifier
-            ):
+            let productName,
+            let targetName,
+            let packageIdentifier
+        ):
 
             let solution = """
             reference the package in the target dependency with '.product(name: "\(productName)", package: \

Sources/PackageGraph/ResolvedPackage.swift

@@ -35,6 +35,7 @@ public final class ResolvedPackage: ObjectIdentifierProtocol {
 
     /// The name of the package as entered in the manifest.
     /// This should rarely be used beyond presentation purposes
+    //@available(*, deprecated)
     public var manifestName: String {
         return self.underlyingPackage.manifestName
     }

Sources/PackageModel/Package.swift

@@ -60,6 +60,7 @@ public final class Package: ObjectIdentifierProtocol, Encodable {
 
     /// The name of the package as entered in the manifest.
     /// This should rarely be used beyond presentation purposes
+    //@available(*, deprecated)
     public var manifestName: String {
         return manifest.name
     }

Tests/BuildTests/BuildPlanTests.swift

@@ -1020,7 +1020,7 @@ final class BuildPlanTests: XCTestCase {
                     packageKind: .root,
                     packageLocation: "/Pkg",
                     dependencies: [
-                        .scm(location: "Clibgit", requirement: .upToNextMajor(from: "1.0.0"))
+                        .scm(location: "/Clibgit", requirement: .upToNextMajor(from: "1.0.0"))
                     ],
                     targets: [
                         TargetDescription(name: "exe", dependencies: []),

Tests/PackageGraphTests/PackageGraphTests.swift

@@ -1108,51 +1108,6 @@ class PackageGraphTests: XCTestCase {
         }
     }
 
-    func testInvalidExplicitPackageDependencyName() throws {
-        let fs = InMemoryFileSystem(emptyFiles:
-            "/Foo/Sources/Foo/foo.swift",
-            "/Bar/Sources/Baar/bar.swift"
-        )
-
-        let diagnostics = DiagnosticsEngine()
-        _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics,
-            manifests: [
-                Manifest.createV4Manifest(
-                    name: "Foo",
-                    path: "/Foo",
-                    packageKind: .root,
-                    packageLocation: "/Foo",
-                    dependencies: [
-                        .scm(deprecatedName: "Baar", location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")),
-                    ],
-                    targets: [
-                        TargetDescription(name: "Foo", dependencies: ["Baar"]),
-                    ]),
-                Manifest.createV4Manifest(
-                    name: "Bar",
-                    path: "/Bar",
-                    packageKind: .local,
-                    packageLocation: "/Bar",
-                    products: [
-                        ProductDescription(name: "Baar", type: .library(.automatic), targets: ["Baar"])
-                    ],
-                    targets: [
-                        TargetDescription(name: "Baar"),
-                    ]),
-            ]
-        )
-
-        DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in
-            result.check(
-                diagnostic: """
-                    'foo' dependency on '/Bar' has an explicit name 'Baar' which does not match the name 'Bar' set for '/Bar'
-                    """,
-                behavior: .error,
-                location: "'Foo' /Foo"
-            )
-        }
-    }
-
     func testConditionalTargetDependency() throws {
         let fs = InMemoryFileSystem(emptyFiles:
             "/Foo/Sources/Foo/source.swift",
@@ -1885,71 +1840,6 @@ class PackageGraphTests: XCTestCase {
             XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)")
         }
     }
-    
-    func testTargetDependencies_Post52_ManifestNameNotMatchedWithURL() throws {
-        let fs = InMemoryFileSystem(emptyFiles:
-            "/Foo/Sources/Foo/foo.swift",
-            "/Bar/Sources/Bar/bar.swift"
-        )
-
-        let manifests = try [
-            Manifest.createManifest(
-                name: "Foo",
-                path: "/Foo",
-                packageKind: .root,
-                packageLocation: "/Foo",
-                v: .v5_2,
-                dependencies: [
-                    .scm(deprecatedName: "Bar", location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")),
-                ],
-                targets: [
-                    TargetDescription(name: "Foo", dependencies: ["ProductBar"]),
-                ]),
-            Manifest.createManifest(
-                name: "Some-Bar",
-                path: "/Bar",
-                packageKind: .local,
-                packageLocation: "/Bar",
-                v: .v5_2,
-                products: [
-                    ProductDescription(name: "ProductBar", type: .library(.automatic), targets: ["Bar"])
-                ],
-                targets: [
-                    TargetDescription(name: "Bar"),
-                ]),
-        ]
-
-        do {
-            let diagnostics = DiagnosticsEngine()
-            _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: manifests)
-            DiagnosticsEngineTester(diagnostics, ignoreNotes: true) { result in
-                result.check(
-                    diagnostic: """
-                        'foo' dependency on '/Bar' has an explicit name 'Bar' which does not match the name 'Some-Bar' set for '/Bar'
-                        """,
-                    behavior: .error,
-                    location: "'Foo' /Foo"
-                )
-            }
-        }
-
-        // fix it
-
-        do {
-            let fixedManifests = [
-                try manifests[0].withDependencies([
-                    .scm(deprecatedName: "Some-Bar", location: "/Bar", requirement: .upToNextMajor(from: "1.0.0")),
-                ]).withTargets([
-                    TargetDescription(name: "Foo", dependencies: [.product(name: "ProductBar", package: "Some-Bar")]),
-                ]),
-                manifests[1] // same
-            ]
-
-            let diagnostics = DiagnosticsEngine()
-            _ = try loadPackageGraph(fs: fs, diagnostics: diagnostics, manifests: fixedManifests)
-            XCTAssert(diagnostics.diagnostics.isEmpty, "\(diagnostics.diagnostics)")
-        }
-    }
 }