Fix PubGrub Container (#3860)

SDGGiesbrecht · web-flow · commit 06351c5328eb · 2021-11-18T16:41:36.000-08:00
Spun out of #3838. This contains only the pieces related to PubGrubPackageContainer and the bug in its caching. The relevant code branches are traversed and tested under both resolution modes. (But I think the context that flushes out the problems never occurs naturally under legacy resolution.)
diff --git a/Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift b/Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift
@@ -1028,7 +1028,7 @@ private struct DiagnosticReportBuilder {
 
 /// A container for an individual package. This enhances PackageContainer to add PubGrub specific
 /// logic which is mostly related to computing incompatibilities at a particular version.
-private final class PubGrubPackageContainer {
+internal final class PubGrubPackageContainer {
     /// The underlying package container.
     let underlying: PackageContainer
 
@@ -1037,10 +1037,10 @@ private final class PubGrubPackageContainer {
 
     /// The map of dependencies to version set that indicates the versions that have had their
     /// incompatibilities emitted.
-    private var emittedIncompatibilities = ThreadSafeKeyValueStore<PackageReference, VersionSetSpecifier>()
+    private var emittedIncompatibilities = ThreadSafeKeyValueStore<DependencyResolutionNode, VersionSetSpecifier>()
 
     /// Whether we've emitted the incompatibilities for the pinned versions.
-    private var emittedPinnedVersionIncompatibilities = ThreadSafeBox(false)
+    private var emittedPinnedVersionIncompatibilities = ThreadSafeKeyValueStore<DependencyResolutionNode, Bool>()
 
     init(underlying: PackageContainer, pinsMap: PinsStore.PinsMap) {
         self.underlying = underlying
@@ -1191,20 +1191,28 @@ private final class PubGrubPackageContainer {
 
             // Skip if we already emitted incompatibilities for this dependency such that the selected
             // falls within the previously computed bounds.
-            if emittedIncompatibilities[dep.package]?.contains(version) != true {
-                constraints.append(dep)
+            if emittedIncompatibilities[node]?.contains(version) != true {
+                for node in dep.nodes() {
+                    constraints.append(
+                        PackageContainerConstraint(
+                            package: node.package,
+                            requirement: dep.requirement,
+                            products: node.productFilter
+                        )
+                    )
+                }
             }
         }
 
         // Emit the dependencies at the pinned version if we haven't emitted anything else yet.
         if version == pinnedVersion, emittedIncompatibilities.isEmpty {
             // We don't need to emit anything if we already emitted the incompatibilities at the
             // pinned version.
-            if self.emittedPinnedVersionIncompatibilities.get() ?? false {
+            if self.emittedPinnedVersionIncompatibilities[node] ?? false {
                 return []
             }
 
-            self.emittedPinnedVersionIncompatibilities.put(true)
+            self.emittedPinnedVersionIncompatibilities[node] = true
 
             // Since the pinned version is most likely to succeed, we don't compute bounds for its
             // incompatibilities.
@@ -1223,30 +1231,29 @@ private final class PubGrubPackageContainer {
 
         let (lowerBounds, upperBounds) = try self.computeBounds(for: node,
                                                                 constraints: constraints,
-                                                                startingWith: version,
-                                                                products: node.productFilter)
-
-        return try constraints.map { constraint in
-            var terms: OrderedSet<Term> = []
-            let lowerBound = lowerBounds[constraint.package] ?? "0.0.0"
-            let upperBound = upperBounds[constraint.package] ?? Version(version.major + 1, 0, 0)
-            assert(lowerBound < upperBound)
-
-            // We only have version-based requirements at this point.
-            guard case .versionSet(let vs) = constraint.requirement else {
-                throw InternalError("Unexpected unversioned requirement: \(constraint)")
-            }
+                                                                startingWith: version)
+
+        return try constraints.flatMap { constraint in
+            return try constraint.nodes().map { constraintNode in
+                var terms: OrderedSet<Term> = []
+                let lowerBound = lowerBounds[constraintNode] ?? "0.0.0"
+                let upperBound = upperBounds[constraintNode] ?? Version(version.major + 1, 0, 0)
+                assert(lowerBound < upperBound)
+
+                // We only have version-based requirements at this point.
+                guard case .versionSet(let vs) = constraint.requirement else {
+                    throw InternalError("Unexpected unversioned requirement: \(constraint)")
+                }
 
-            for constraintNode in constraint.nodes() {
                 let requirement: VersionSetSpecifier = .range(lowerBound ..< upperBound)
                 terms.append(Term(node, requirement))
                 terms.append(Term(not: constraintNode, vs))
 
                 // Make a record for this dependency so we don't have to recompute the bounds when the selected version falls within the bounds.
-                self.emittedIncompatibilities[constraint.package] = requirement.union(emittedIncompatibilities[constraint.package] ?? .empty)
-            }
+                self.emittedIncompatibilities[constraintNode] = requirement.union(emittedIncompatibilities[constraintNode] ?? .empty)
 
-            return try Incompatibility(terms, root: root, cause: .dependency(node: node))
+                return try Incompatibility(terms, root: root, cause: .dependency(node: node))
+            }
         }
     }
 
@@ -1259,9 +1266,8 @@ private final class PubGrubPackageContainer {
     private func computeBounds(
         for node: DependencyResolutionNode,
         constraints: [PackageContainerConstraint],
-        startingWith firstVersion: Version,
-        products: ProductFilter
-    ) throws -> (lowerBounds: [PackageReference: Version], upperBounds: [PackageReference: Version]) {
+        startingWith firstVersion: Version
+    ) throws -> (lowerBounds: [DependencyResolutionNode: Version], upperBounds: [DependencyResolutionNode: Version]) {
         let preloadCount = 3
 
         // nothing to do
@@ -1274,15 +1280,15 @@ private final class PubGrubPackageContainer {
             for version in versions {
                 DispatchQueue.sharedConcurrent.async(group: sync) {
                     if self.underlying.isToolsVersionCompatible(at: version) {
-                        _ = try? self.underlying.getDependencies(at: version, productFilter: products)
+                        _ = try? self.underlying.getDependencies(at: version, productFilter: node.productFilter)
                     }
                 }
             }
             sync.wait()
         }
 
-        func compute(_ versions: [Version], upperBound: Bool) -> [PackageReference: Version] {
-            var result: [PackageReference: Version] = [:]
+        func compute(_ versions: [Version], upperBound: Bool) -> [DependencyResolutionNode: Version] {
+            var result: [DependencyResolutionNode: Version] = [:]
             var previousVersion = firstVersion
 
             for (index, version) in versions.enumerated() {
@@ -1299,17 +1305,19 @@ private final class PubGrubPackageContainer {
                 let bound = upperBound ? version : previousVersion
 
                 let isToolsVersionCompatible = self.underlying.isToolsVersionCompatible(at: version)
-                for constraint in constraints where !result.keys.contains(constraint.package) {
-                    // If we hit a version which doesn't have a compatible tools version then that's the boundary.
-                    // Record the bound if the tools version isn't compatible at the current version.
-                    if !isToolsVersionCompatible {
-                        result[constraint.package] = bound
-                    } else {
-                        // Get the dependencies at this version.
-                        if let currentDependencies = try? self.underlying.getDependencies(at: version, productFilter: products) {
-                            // Record this version as the bound for our list of dependencies, if appropriate.
-                            if currentDependencies.first(where: { $0.package == constraint.package }) != constraint {
-                                result[constraint.package] = bound
+                for constraint in constraints {
+                    for constraintNode in constraint.nodes() where !result.keys.contains(constraintNode) {
+                        // If we hit a version which doesn't have a compatible tools version then that's the boundary.
+                        // Record the bound if the tools version isn't compatible at the current version.
+                        if !isToolsVersionCompatible {
+                            result[constraintNode] = bound
+                        } else {
+                            // Get the dependencies at this version.
+                            if let currentDependencies = try? self.underlying.getDependencies(at: version, productFilter: constraintNode.productFilter) {
+                                // Record this version as the bound for our list of dependencies, if appropriate.
+                                if currentDependencies.first(where: { $0.package == constraint.package }) != constraint {
+                                    result[constraintNode] = bound
+                                }
                             }
                         }
                     }
@@ -1334,12 +1342,12 @@ private final class PubGrubPackageContainer {
 
         let sync = DispatchGroup()
 
-        var upperBounds = [PackageReference: Version]()
+        var upperBounds = [DependencyResolutionNode: Version]()
         DispatchQueue.sharedConcurrent.async(group: sync) {
             upperBounds = compute(Array(versions.dropFirst(idx + 1)), upperBound: true)
         }
 
-        var lowerBounds = [PackageReference: Version]()
+        var lowerBounds = [DependencyResolutionNode: Version]()
         DispatchQueue.sharedConcurrent.async(group: sync) {
             lowerBounds = compute(Array(versions.dropLast(versions.count - idx).reversed()), upperBound: false)
         }
diff --git a/Sources/SPMTestSupport/MockPackageContainer.swift b/Sources/SPMTestSupport/MockPackageContainer.swift
@@ -26,6 +26,8 @@ public class MockPackageContainer: PackageContainer {
     public let package: PackageReference
 
     let dependencies: [String: [Dependency]]
+    let filteredMode: Bool
+    let filteredDependencies: [ProductFilter: [Dependency]]
 
     public var unversionedDeps: [MockPackageContainer.Constraint] = []
 
@@ -47,7 +49,13 @@ public class MockPackageContainer: PackageContainer {
     }
 
     public func getDependencies(at revision: String, productFilter: ProductFilter) -> [MockPackageContainer.Constraint] {
-        return dependencies[revision]!.map { value in
+        let dependencies: [Dependency]
+        if filteredMode {
+            dependencies = filteredDependencies[productFilter]!
+        } else {
+            dependencies = self.dependencies[revision]!
+        }
+        return dependencies.map { value in
             let (package, requirement) = value
             return MockPackageContainer.Constraint(package: package, requirement: requirement, products: productFilter)
         }
@@ -97,6 +105,29 @@ public class MockPackageContainer: PackageContainer {
         self.package = package
         self._versions = dependencies.keys.compactMap(Version.init(_:)).sorted()
         self.dependencies = dependencies
+        self.filteredMode = false
+        self.filteredDependencies = [:]
+    }
+
+    public init(
+        name: String,
+        dependenciesByProductFilter: [ProductFilter: [(container: String, versionRequirement: VersionSetSpecifier)]]
+    ) {
+        var dependencies: [ProductFilter: [Dependency]] = [:]
+        for (filter, deps) in dependenciesByProductFilter {
+            dependencies[filter] = deps.map {
+                let path = AbsolutePath("/\($0.container)")
+                let ref = PackageReference.localSourceControl(identity: .init(path: path), path: path)
+                return (ref, .versionSet($0.versionRequirement))
+            }
+        }
+        let path = AbsolutePath("/\(name)")
+        let ref = PackageReference.localSourceControl(identity: .init(path: path), path: path)
+        self.package = ref
+        self._versions = [Version(1, 0, 0)]
+        self.dependencies = [:]
+        self.filteredMode = true
+        self.filteredDependencies = dependencies
     }
 }
 
diff --git a/Tests/PackageGraphTests/PubgrubTests.swift b/Tests/PackageGraphTests/PubgrubTests.swift
@@ -1160,6 +1160,110 @@ final class PubgrubTests: XCTestCase {
         XCTAssertMatch(delegate.events, ["didResolve 'bar' at '2.0.1'"])
         XCTAssertMatch(delegate.events, ["solved: 'bar' at '2.0.1', 'foo' at '1.1.0'"])
     }
+
+    func testPubGrubPackageContainerCacheParameterization() {
+        let container = PubGrubPackageContainer(
+            underlying: MockPackageContainer(
+                name: "Package",
+                dependenciesByProductFilter: [
+                    .specific(["FilterA"]): [(
+                        container: "DependencyA",
+                        versionRequirement: .exact(Version(1, 0, 0))
+                    )],
+                    .specific(["FilterB"]): [(
+                        container: "DependencyB",
+                        versionRequirement: .exact(Version(1, 0, 0))
+                    )]
+                ]),
+            pinsMap: PinsStore.PinsMap()
+        )
+        let rootLocation = AbsolutePath("/Root")
+        let otherLocation = AbsolutePath("/Other")
+        let dependencyALocation = AbsolutePath("/DependencyA")
+        let dependencyBLocation = AbsolutePath("/DependencyB")
+        let root = PackageReference(identity: PackageIdentity(path: rootLocation), kind: .fileSystem(rootLocation))
+        let other = PackageReference.localSourceControl(identity: .init(path: otherLocation), path: otherLocation)
+        let dependencyA = PackageReference.localSourceControl(identity: .init(path: dependencyALocation), path: dependencyALocation)
+        let dependencyB = PackageReference.localSourceControl(identity: .init(path: dependencyBLocation), path: dependencyBLocation)
+        XCTAssertEqual(
+            try container.incompatibilites(
+                at: Version(1, 0, 0),
+                node: .product(
+                    "FilterA",
+                    package: other
+                ),
+                overriddenPackages: [:],
+                root: .root(package: root)
+            ),
+            [
+                Incompatibility(
+                    terms: [
+                        Term(
+                            node: .product("FilterA", package: other), requirement: .range(Version(0, 0, 0)..<Version(2, 0, 0)),
+                            isPositive: true
+                        ),
+                        Term(
+                            node: .product("FilterA", package: dependencyA), requirement: .exact(Version(1, 0, 0)),
+                            isPositive: false
+                        )
+                    ],
+                    cause: .dependency(node: .product("FilterA", package: other))
+                ),
+                Incompatibility(
+                    terms: [
+                        Term(
+                            node: .product("FilterA", package: other), requirement: .range(Version(0, 0, 0)..<Version(2, 0, 0)),
+                            isPositive: true
+                        ),
+                        Term(
+                            node: .empty(package: other), requirement: .exact(Version(1, 0, 0)),
+                            isPositive: false
+                        ),
+                    ],
+                    cause: .dependency(node: .product("FilterA", package: other))
+                )
+            ]
+        )
+        XCTAssertEqual(
+            try container.incompatibilites(
+                at: Version(1, 0, 0),
+                node: .product(
+                    "FilterB",
+                    package: other
+                ),
+                overriddenPackages: [:],
+                root: .root(package: root)
+            ),
+            [
+                Incompatibility(
+                    terms: [
+                        Term(
+                            node: .product("FilterB", package: other), requirement: .range(Version(0, 0, 0)..<Version(2, 0, 0)),
+                            isPositive: true
+                        ),
+                        Term(
+                            node: .product("FilterB", package: dependencyB), requirement: .exact(Version(1, 0, 0)),
+                            isPositive: false
+                        )
+                    ],
+                    cause: .dependency(node: .product("FilterB", package: other))
+                ),
+                Incompatibility(
+                    terms: [
+                        Term(
+                            node: .product("FilterB", package: other), requirement: .range(Version(0, 0, 0)..<Version(2, 0, 0)),
+                            isPositive: true
+                        ),
+                        Term(
+                            node: .empty(package: other), requirement: .exact(Version(1, 0, 0)),
+                            isPositive: false
+                        ),
+                    ],
+                    cause: .dependency(node: .product("FilterB", package: other))
+                )
+            ]
+        )
+    }
 }
 
 final class PubGrubTestsBasicGraphs: XCTestCase {
