Restore Target‐Based Resolution (#3006)

* Reverted 27f444f (leaving conflicts marked).

* Resolved conflicts.

* Added failing test.

* Fixed cache.

* Patched other potentential issue.

* Revert "Resolved conflicts."

This reverts commit bcf41e7.

* Revert "Reverted 27f444f (leaving conflicts marked)."

This reverts commit 4a0abc0.

Author: SDGGiesbrecht

Fixtures/DependencyResolution/Regressions/SRP/Package.swift

@@ -0,0 +1,19 @@
+// swift-tools-version:4.0
+
+import PackageDescription
+
+let package = Package(
+    name: "SRP",
+    products: [
+        .library(name: "SRP", targets: ["SRP"]),
+    ],
+    dependencies: [
+        .package(url: "https://github.com/IBM-Swift/BlueCryptor.git", from: "0.8.16"),
+        .package(url: "https://github.com/lorentey/BigInt.git", from: "3.0.0"),
+    ],
+    targets: [
+        .target(name: "SRP", dependencies: ["Cryptor", "BigInt"], path: "Sources"),
+        .testTarget(name: "SRPTests", dependencies: ["Cryptor", "SRP"]),
+    ],
+    swiftLanguageVersions: [4]
+)

Fixtures/DependencyResolution/Regressions/SRP/README.md

@@ -0,0 +1 @@
+Reduced from https://github.com/Bouke/SRP/tree/b166838d4cf9218933c03151d62e50772460f95e, which was tripping rdar://problem/65284674.

Fixtures/DependencyResolution/Regressions/SRP/Sources/SRP/SRP.swift

@@ -87,7 +87,7 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
     let _reversedVersions: [Version]
 
     /// The cached dependency information.
-    private var dependenciesCache: [String: (Manifest, [RepositoryPackageConstraint])] = [:]
+    private var dependenciesCache: [String: [ProductFilter: (Manifest, [RepositoryPackageConstraint])]] = [:]
     private var dependenciesCacheLock = Lock()
 
     init(
@@ -157,7 +157,7 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
 
     public override func getDependencies(at version: Version, productFilter: ProductFilter) throws -> [RepositoryPackageConstraint] {
         do {
-            return try cachedDependencies(forIdentifier: version.description) {
+            return try cachedDependencies(forIdentifier: version.description, productFilter: productFilter) {
                 let tag = knownVersions[version]!
                 let revision = try repository.resolveRevision(tag: tag)
                 return try getDependencies(at: revision, version: version, productFilter: productFilter)
@@ -170,7 +170,7 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
 
     public override func getDependencies(at revision: String, productFilter: ProductFilter) throws -> [RepositoryPackageConstraint] {
         do {
-            return try cachedDependencies(forIdentifier: revision) {
+            return try cachedDependencies(forIdentifier: revision, productFilter: productFilter) {
                 // resolve the revision identifier and return its dependencies.
                 let revision = try repository.resolveRevision(identifier: revision)
                 return try getDependencies(at: revision, productFilter: productFilter)
@@ -203,14 +203,15 @@ public class RepositoryPackageContainer: BasePackageContainer, CustomStringConve
 
     private func cachedDependencies(
         forIdentifier identifier: String,
+        productFilter: ProductFilter,
         getDependencies: () throws -> (Manifest, [RepositoryPackageConstraint])
     ) throws -> (Manifest, [RepositoryPackageConstraint]) {
         return try dependenciesCacheLock.withLock {
-            if let result = dependenciesCache[identifier] {
+            if let result = dependenciesCache[identifier, default: [:]][productFilter] {
                 return result
             }
             let result = try getDependencies()
-            dependenciesCache[identifier] = result
+            dependenciesCache[identifier, default: [:]][productFilter] = result
             return result
         }
     }

Fixtures/DependencyResolution/Regressions/SRP/Tests/SRPTests/SRPTests.swift

@@ -164,6 +164,20 @@ public func executeSwiftRun(
     return try SwiftPMProduct.SwiftRun.execute(args, packagePath: packagePath, env: env)
 }
 
+@discardableResult
+public func executeSwiftPackage(
+    _ packagePath: AbsolutePath,
+    configuration: Configuration = .Debug,
+    extraArgs: [String] = [],
+    Xcc: [String] = [],
+    Xld: [String] = [],
+    Xswiftc: [String] = [],
+    env: [String: String]? = nil
+) throws -> (stdout: String, stderr: String) {
+    let args = swiftArgs(configuration: configuration, extraArgs: extraArgs, Xcc: Xcc, Xld: Xld, Xswiftc: Xswiftc)
+    return try SwiftPMProduct.SwiftPackage.execute(args, packagePath: packagePath, env: env)
+}
+
 private func swiftArgs(
     configuration: Configuration,
     extraArgs: [String],

Sources/PackageGraph/RepositoryPackageContainer.swift

@@ -1240,14 +1240,28 @@ extension Workspace {
         var loadedManifests = [String: Manifest]()
 
         // Compute the transitive closure of available dependencies.
-        let allManifests = try! topologicalSort(inputManifests.map({ KeyedPair(($0, ProductFilter.everything), key: $0.name)})) { node in
+        struct NameAndFilter: Hashable { // Just because a raw tuple cannot be hashable.
+            let name: String
+            let filter: ProductFilter
+        }
+        let allManifestsWithPossibleDuplicates = try! topologicalSort(inputManifests.map({ KeyedPair(($0, ProductFilter.everything), key: NameAndFilter(name: $0.name, filter: .everything)) })) { node in
             return node.item.0.dependenciesRequired(for: node.item.1).compactMap({ dependency in
                 let url = config.mirrors.effectiveURL(forURL: dependency.url)
                 let manifest = loadedManifests[url] ?? loadManifest(forURL: url, diagnostics: diagnostics)
                 loadedManifests[url] = manifest
-                return manifest.flatMap({ KeyedPair(($0, dependency.productFilter), key: $0.name) })
+                return manifest.flatMap({ KeyedPair(($0, dependency.productFilter), key: NameAndFilter(name: $0.name, filter: dependency.productFilter)) })
             })
         }
+        var deduplication: Set<String> = []
+        var allManifests: [KeyedPair<(Manifest, ProductFilter), NameAndFilter>] = []
+        for node in allManifestsWithPossibleDuplicates {
+            if deduplication.contains(node.item.0.name) {
+                continue // A duplicate.
+            } else {
+                allManifests.append(node)
+                deduplication.insert(node.item.0.name)
+            }
+        }
 
         let allDependencyManifests = allManifests.map({ $0.item }).filter({ !root.manifests.contains($0.0) })
         let deps = allDependencyManifests.map({ ($0, state.dependencies[forURL: $0.url]!, $1) })

Sources/SPMTestSupport/misc.swift

@@ -65,4 +65,19 @@ class DependencyResolutionTests: XCTestCase {
             XCTAssertEqual(output, "♣︎K\n♣︎Q\n♣︎J\n♣︎10\n♣︎9\n♣︎8\n♣︎7\n♣︎6\n♣︎5\n♣︎4\n")
         }
     }
+
+    func testRepositoryCacheDoesNotDerailResolution() throws {
+        // From rdar://problem/65284674
+        // RepositoryPackageContainer used to erroneously cache dependencies based only on version,
+        // storing the result of the first product filter and then continually returning it for other filters too.
+        // This lead to corrupt graph states.
+        guard Resources.havePD4Runtime else {
+            throw XCTSkip("PackageDescription v4 is unavailable; this test requires the compiler script instead of a self‐hosted build.")
+        }
+
+        fixture(name: "DependencyResolution/Regressions/SRP") { prefix in
+            let (output, error) = try executeSwiftPackage(prefix, extraArgs: ["resolve"])
+            XCTAssert(error.isEmpty, output + error)
+        }
+    }
 }