relax strickter validation to warning to allow transition (#3713)

motivation: recent change to SwiftPM introduced strickter validation of dependencies uniquness to address undefined behavior. however, we want to allow users some times to adjust before making a breaking change

changes: update the validation to emit a warning instead of error under the specific condition the previous behavior incorrectly ignored!

Author: tomerd

Sources/PackageGraph/PackageGraph+Loading.swift

@@ -216,7 +216,7 @@ private func createResolvedPackages(
 
     // Create a map of package builders keyed by the package identity.
     // This is guaranteed to be unique so we can use spm_createDictionary
-    let packageMapByIdentity: [PackageIdentity: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{
+    let packagesByIdentity: [PackageIdentity: ResolvedPackageBuilder] = packageBuilders.spm_createDictionary{
         return ($0.package.identity, $0)
     }
 
@@ -242,7 +242,7 @@ private func createResolvedPackages(
             }
 
             // Otherwise, look it up by its identity.
-            if let resolvedPackage = packageMapByIdentity[dependency.identity] {
+            if let resolvedPackage = packagesByIdentity[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
@@ -255,16 +255,24 @@ private func createResolvedPackages(
                     return diagnostics.emit(error, location: package.diagnosticLocation)
                 }
 
-                // check if the resolved package url is the same as the dependency one
+                // check if the resolved package location 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
+                // which only allowed when overriding
                 if resolvedPackage.package.manifest.packageLocation != dependencyLocation && !resolvedPackage.allowedToOverride {
                     let error = PackageGraphError.dependencyAlreadySatisfiedByIdentifier(
                         package: package.identity.description,
                         dependencyLocation: dependencyLocation,
                         otherDependencyURL: resolvedPackage.package.manifest.packageLocation,
                         identity: dependency.identity)
-                    return diagnostics.emit(error, location: package.diagnosticLocation)
+                    // 9/2021 this is currently emitting a warning only to support
+                    // backwards compatibility with older versions of SwiftPM that had too weak of a validation
+                    // we will upgrade this to an error in a few versions to tighten up the validation
+                    if dependency.explicitNameForTargetDependencyResolutionOnly == .none ||
+                        resolvedPackage.package.manifestName == dependency.explicitNameForTargetDependencyResolutionOnly {
+                        diagnostics.emit(.warning(error.description + ". this will be upgraded to an error in future versions of SwiftPM."), location: package.diagnosticLocation)
+                    } else {
+                        return diagnostics.emit(error, location: package.diagnosticLocation)
+                    }
                 }
 
                 // checks if two dependencies have the same explicit name which can cause target based dependency package lookup issue

Sources/PackageModel/Manifest.swift

@@ -168,7 +168,7 @@ public final class Manifest: ObjectIdentifierProtocol {
     /// Returns the package dependencies required for a particular products filter.
     public func dependenciesRequired(for productFilter: ProductFilter) -> [PackageDependency] {
         #if ENABLE_TARGET_BASED_DEPENDENCY_RESOLUTION
-        // If we have already calcualted it, returned the cached value.
+        // If we have already calculated it, returned the cached value.
         if let dependencies = self._requiredDependencies[productFilter] {
             return self.dependencies
         } else {

Tests/WorkspaceTests/WorkspaceTests.swift

@@ -5658,7 +5658,7 @@ final class WorkspaceTests: XCTestCase {
         ])
     }
 
-    func testDuplicateDependencyIdentityAtRoot() throws {
+    func testDuplicateDependencyIdentityWithNameAtRoot() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()
 
@@ -5720,6 +5720,68 @@ final class WorkspaceTests: XCTestCase {
         }
     }
 
+    func testDuplicateDependencyIdentityWithoutNameAtRoot() throws {
+        let sandbox = AbsolutePath("/tmp/ws/")
+        let fs = InMemoryFileSystem()
+
+        let workspace = try MockWorkspace(
+            sandbox: sandbox,
+            fs: fs,
+            roots: [
+                MockPackage(
+                    name: "Root",
+                    targets: [
+                        MockTarget(name: "RootTarget", dependencies: [
+                            .product(name: "FooProduct", package: "FooUtilityPackage"),
+                            .product(name: "BarProduct", package: "BarUtilityPackage")
+                        ]),
+                    ],
+                    products: [],
+                    dependencies: [
+                        .scm(path: "foo/utility", requirement: .upToNextMajor(from: "1.0.0")),
+                        .scm(path: "bar/utility", requirement: .upToNextMajor(from: "1.0.0")),
+                    ],
+                    toolsVersion: .v5
+                ),
+            ],
+            packages: [
+                MockPackage(
+                    name: "FooUtilityPackage",
+                    path: "foo/utility",
+                    targets: [
+                        MockTarget(name: "FooTarget"),
+                    ],
+                    products: [
+                        MockProduct(name: "FooProduct", targets: ["FooTarget"]),
+                    ],
+                    versions: ["1.0.0", "2.0.0"]
+                ),
+                // this package never gets loaded since the dependency declaration identity is the same as "FooPackage"
+                MockPackage(
+                    name: "BarUtilityPackage",
+                    path: "bar/utility",
+                    targets: [
+                        MockTarget(name: "BarTarget"),
+                    ],
+                    products: [
+                        MockProduct(name: "BarProduct", targets: ["BarTarget"]),
+                    ],
+                    versions: ["1.0.0", "2.0.0"]
+                ),
+            ]
+        )
+
+        workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
+            DiagnosticsEngineTester(diagnostics) { result in
+                result.check(
+                    diagnostic: "'root' dependency on '/tmp/ws/pkgs/bar/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'",
+                    behavior: .error,
+                    location: "'Root' /tmp/ws/roots/Root"
+                )
+            }
+        }
+    }
+
     func testDuplicateExplicitDependencyName_AtRoot() throws {
         let sandbox = AbsolutePath("/tmp/ws/")
         let fs = InMemoryFileSystem()
@@ -6257,8 +6319,6 @@ final class WorkspaceTests: XCTestCase {
             ]
         )
 
-        // FIXME: rdar://72940946
-        // we need to improve this situation or diagnostics when working on identity
         workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
             DiagnosticsEngineTester(diagnostics) { result in
                 result.check(
@@ -6282,8 +6342,8 @@ final class WorkspaceTests: XCTestCase {
                     name: "Root",
                     targets: [
                         MockTarget(name: "RootTarget", dependencies: [
-                            .product(name: "FooUtilityProduct", package: "FooUtilityPackage"),
-                            .product(name: "BarProduct", package: "BarPackage")
+                            .product(name: "FooUtilityProduct", package: "utility"),
+                            .product(name: "BarProduct", package: "bar")
                         ]),
                     ],
                     products: [],
@@ -6311,7 +6371,7 @@ final class WorkspaceTests: XCTestCase {
                     path: "bar",
                     targets: [
                         MockTarget(name: "BarTarget", dependencies: [
-                            .product(name: "OtherUtilityProduct", package: "OtherUtilityPackage"),
+                            .product(name: "OtherUtilityProduct", package: "utility"),
                         ]),
                     ],
                     products: [
@@ -6337,12 +6397,19 @@ final class WorkspaceTests: XCTestCase {
             ]
         )
 
-        // FIXME: rdar://72940946
-        // we need to improve this situation or diagnostics when working on identity
+        // 9/2021 this is currently emitting a warning only to support backwards compatibility
+        // we will upgrade this to an error in a few versions to tighten up the validation
         workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
             DiagnosticsEngineTester(diagnostics) { result in
                 result.check(
-                    diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'OtherUtilityPackage'.",
+                    diagnostic: "'bar' dependency on '/tmp/ws/pkgs/other-foo/utility' conflicts with dependency on '/tmp/ws/pkgs/foo/utility' which has the same identity 'utility'. this will be upgraded to an error in future versions of SwiftPM.",
+                    behavior: .warning,
+                    location: "'BarPackage' /tmp/ws/pkgs/bar"
+                )
+                // FIXME: rdar://72940946
+                // we need to improve this situation or diagnostics when working on identity
+                result.check(
+                    diagnostic: "product 'OtherUtilityProduct' required by package 'bar' target 'BarTarget' not found in package 'utility'.",
                     behavior: .error,
                     location: "'BarPackage' /tmp/ws/pkgs/bar"
                 )
@@ -6420,10 +6487,10 @@ final class WorkspaceTests: XCTestCase {
             ]
         )
 
-        // FIXME: rdar://72940946
-        // we need to improve this situation or diagnostics when working on identity
         workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
             DiagnosticsEngineTester(diagnostics) { result in
+                // FIXME: rdar://72940946
+                // we need to improve this situation or diagnostics when working on identity
                 result.check(
                     diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage",
                     behavior: .error,
@@ -6505,10 +6572,10 @@ final class WorkspaceTests: XCTestCase {
             ]
         )
 
-        // FIXME: rdar://72940946
-        // we need to improve this situation or diagnostics when working on identity
         workspace.checkPackageGraph(roots: ["Root"]) { graph, diagnostics in
             DiagnosticsEngineTester(diagnostics) { result in
+                // FIXME: rdar://72940946
+                // we need to improve this situation or diagnostics when working on identity
                 result.check(
                     diagnostic: "cyclic dependency declaration found: Root -> FooUtilityPackage -> BarPackage -> FooUtilityPackage",
                     behavior: .error,
@@ -6573,10 +6640,10 @@ final class WorkspaceTests: XCTestCase {
             ]
         )
 
-        // FIXME: rdar://72940946
-        // we need to improve this situation or diagnostics when working on identity
         workspace.checkPackageGraph(roots: ["foo"]) { graph, diagnostics in
             DiagnosticsEngineTester(diagnostics) { result in
+                // FIXME: rdar://72940946
+                // we need to improve this situation or diagnostics when working on identity
                 result.check(
                     diagnostic: "cyclic dependency declaration found: Root -> BarPackage -> Root",
                     behavior: .error,