fix an edge case where dependecy resolution can go into endless loop (#5535) (#5561)

Author: tomerd

Sources/PackageGraph/Pubgrub/PubgrubDependencyResolver.swift

@@ -526,6 +526,11 @@ public struct PubgrubDependencyResolver {
         var incompatibility = conflict
         var createdIncompatibility = false
 
+        // rdar://93335995
+        // hard protection from infinite loops
+        let maxIterations = 1000
+        var iterations: Int = 0
+
         while !isCompleteFailure(incompatibility, root: state.root) {
             var mostRecentTerm: Term?
             var mostRecentSatisfier: Assignment?
@@ -578,7 +583,11 @@ public struct PubgrubDependencyResolver {
             newTerms += priorCause.terms.filter { $0.node != _mostRecentSatisfier.term.node }
 
             if let _difference = difference {
-                newTerms.append(_difference.inverse)
+                // rdar://93335995
+                // do not add the exact inverse of a requirement as it can lead to endless loops
+                if _difference.inverse != mostRecentTerm {
+                    newTerms.append(_difference.inverse)
+                }
             }
 
             incompatibility = try Incompatibility(
@@ -595,6 +604,13 @@ public struct PubgrubDependencyResolver {
                     self.delegate?.satisfied(term: term, by: _mostRecentSatisfier, incompatibility: incompatibility)
                 }
             }
+
+            // rdar://93335995
+            // hard protection from infinite loops
+            iterations = iterations + 1
+            if iterations >= maxIterations {
+                break
+            }
         }
 
         self.delegate?.failedToResolve(incompatibility: incompatibility)

Sources/SPMTestSupport/Observability.swift

@@ -17,8 +17,8 @@ import func XCTest.XCTFail
 import func XCTest.XCTAssertEqual
 
 extension ObservabilitySystem {
-    public static func makeForTesting() -> TestingObservability {
-        let collector = TestingObservability.Collector()
+    public static func makeForTesting(verbose: Bool = true) -> TestingObservability {
+        let collector = TestingObservability.Collector(verbose: verbose)
         let observabilitySystem = ObservabilitySystem(collector)
         return TestingObservability(collector: collector, topScope: observabilitySystem.topScope)
     }
@@ -53,13 +53,18 @@ public struct TestingObservability {
         var diagnosticsHandler: DiagnosticsHandler { return self }
 
         let diagnostics: ThreadSafeArrayStore<Basics.Diagnostic>
+        private let verbose: Bool
 
-        init() {
+        init(verbose: Bool) {
+            self.verbose = verbose
             self.diagnostics = .init()
         }
 
         // TODO: do something useful with scope
         func handleDiagnostic(scope: ObservabilityScope, diagnostic: Basics.Diagnostic) {
+            if self.verbose {
+                print(diagnostic.description)
+            }
             self.diagnostics.append(diagnostic)
         }
 

Tests/PackageGraphTests/PubgrubTests.swift

@@ -61,7 +61,7 @@ let aRef = PackageReference.localSourceControl(identity: .plain("a"), path: .roo
 let bRef = PackageReference.localSourceControl(identity: .plain("b"), path: .root)
 let cRef = PackageReference.localSourceControl(identity: .plain("c"), path: .root)
 
-let rootRef = PackageReference.root(identity: PackageIdentity("root"), path: .root)
+let rootRef = PackageReference.root(identity: .plain("root"), path: .root)
 let rootNode = DependencyResolutionNode.root(package: rootRef)
 let rootCause = try! Incompatibility(Term(rootNode, .exact(v1)), root: rootNode)
 let _cause = try! Incompatibility("[email protected]", root: rootNode)
@@ -409,11 +409,139 @@ final class PubgrubTests: XCTestCase {
         solution.decide(.product("a", package: aRef), at: v2)
         solution.derive("b^1.0.0", cause: _cause)
 
-        XCTAssertEqual(try solution.satisfier(for: Term("b^1.0.0")) .term, "b^1.0.0")
+        XCTAssertEqual(try solution.satisfier(for: Term("b^1.0.0")).term, "b^1.0.0")
         XCTAssertEqual(try solution.satisfier(for: Term("¬a^1.0.0")).term, "[email protected]")
         XCTAssertEqual(try solution.satisfier(for: Term("a^2.0.0")).term, "[email protected]")
     }
 
+    // this test reconstruct the conditions described in radar/93335995
+    func testRadar93335995() throws  {
+        let observability = ObservabilitySystem.makeForTesting()
+        let delegate = ObservabilityDependencyResolverDelegate(observabilityScope: observability.topScope)
+        let solver = PubgrubDependencyResolver(provider: emptyProvider, observabilityScope: observability.topScope, delegate: delegate)
+        let state = PubgrubDependencyResolver.State(root: rootNode)
+
+        state.decide(.root(package: aRef), at: "1.1.0")
+
+        do {
+            let cause = Incompatibility(
+                terms: .init([
+                    Term(node: .root(package: aRef),
+                         requirement: .exact("1.1.0"),
+                         isPositive: true
+                    ),
+                    Term(node: .root(package: bRef),
+                         requirement: .range("1.1.0" ..< "2.0.0"),
+                         isPositive: true
+                    )
+                ]),
+                cause: .noAvailableVersion
+            )
+            state.derive(Term(node: .root(package: bRef), requirement: .range("1.1.0" ..< "2.0.0"), isPositive: true), cause: cause)
+        }
+
+        do {
+            let cause = Incompatibility(
+                terms: .init([
+                    Term(node: .root(package: aRef),
+                         requirement: .exact("1.1.0"),
+                         isPositive: true
+                    ),
+                    Term(node: .root(package: bRef),
+                         requirement: .range("1.3.1" ..< "2.0.0"),
+                         isPositive: true
+                    )
+                ]),
+                cause: .noAvailableVersion
+            )
+            state.derive(Term(node: .root(package: bRef), requirement: .range("1.3.1" ..< "2.0.0"), isPositive: false), cause: cause)
+            // order here matters to reproduce the issue
+            state.derive(Term(node: .root(package: bRef), requirement: .exact("1.3.0"), isPositive: false), cause: cause)
+            state.derive(Term(node: .root(package: bRef), requirement: .exact("1.3.1"), isPositive: false), cause: cause)
+            state.derive(Term(node: .root(package: bRef), requirement: .exact("1.2.0"), isPositive: false), cause: cause)
+            state.derive(Term(node: .root(package: bRef), requirement: .exact("1.1.0"), isPositive: false), cause: cause)
+        }
+
+        let conflict = Incompatibility(
+            terms: .init([
+                Term(node: .root(package: aRef),
+                     requirement: .exact("1.1.0"),
+                     isPositive: true
+                ),
+                Term(node: .root(package: bRef),
+                     requirement: .ranges(["1.1.1" ..< "1.2.0", "1.2.1" ..< "1.3.0"]),
+                     isPositive: true
+                )
+            ]),
+            cause: .noAvailableVersion
+        )
+
+        _ = try solver.resolve(state: state, conflict: conflict)
+        XCTAssertNoDiagnostics(observability.diagnostics)
+    }
+
+    func testNoInfiniteLoop() throws  {
+        let observability = ObservabilitySystem.makeForTesting()
+        let delegate = ObservabilityDependencyResolverDelegate(observabilityScope: observability.topScope)
+        let solver = PubgrubDependencyResolver(provider: emptyProvider, observabilityScope: observability.topScope, delegate: delegate)
+        let state = PubgrubDependencyResolver.State(root: rootNode)
+
+        do {
+            let cause = Incompatibility(
+                terms: .init([
+                    Term(node: .root(package: aRef),
+                         requirement: .exact("1.1.0"),
+                         isPositive: true
+                    ),
+                    Term(node: .root(package: bRef),
+                         requirement: .range("1.1.0" ..< "2.0.0"),
+                         isPositive: true
+                    )
+                ]),
+                cause: .noAvailableVersion
+            )
+            state.derive(Term(node: .root(package: aRef), requirement: .exact("1.1.0"), isPositive: true), cause: cause) // no decision available on this which will throw it into an infinite loop
+            state.derive(Term(node: .root(package: bRef), requirement: .range("1.1.0" ..< "2.0.0"), isPositive: true), cause: cause)
+        }
+
+        do {
+            let cause = Incompatibility(
+                terms: .init([
+                    Term(node: .root(package: aRef),
+                         requirement: .exact("1.1.0"),
+                         isPositive: true
+                    ),
+                    Term(node: .root(package: bRef),
+                         requirement: .range("1.2.0" ..< "2.0.0"),
+                         isPositive: true
+                    )
+                ]),
+                cause: .noAvailableVersion
+            )
+            state.derive(Term(node: .root(package: bRef), requirement: .range("1.2.0" ..< "2.0.0"), isPositive: false), cause: cause)
+            state.derive(Term(node: .root(package: bRef), requirement: .exact("1.2.0"), isPositive: false), cause: cause)
+            state.derive(Term(node: .root(package: bRef), requirement: .exact("1.1.0"), isPositive: false), cause: cause)
+        }
+
+        let conflict = Incompatibility(
+            terms: .init([
+                Term(node: .root(package: aRef),
+                     requirement: .exact("1.1.0"),
+                     isPositive: true
+                ),
+                Term(node: .root(package: bRef),
+                     requirement: .ranges(["1.1.1" ..< "1.2.0"]),
+                     isPositive: true
+                )
+            ]),
+            cause: .noAvailableVersion
+        )
+
+        XCTAssertThrowsError(try solver.resolve(state: state, conflict: conflict)) { error in
+            XCTAssertTrue(error is PubgrubDependencyResolver.PubgrubError)
+        }
+    }
+
     func testResolutionNoConflicts() {
         builder.serve("a", at: v1, with: ["a": ["b": (.versionSet(v1Range), .specific(["b"]))]])
         builder.serve("b", at: v1)
@@ -500,6 +628,58 @@ final class PubgrubTests: XCTestCase {
         ])
     }
 
+    func testUsecase1() throws {
+        builder.serve("a",
+                      at: "1.0.0",
+                      with: [
+                        "a": [
+                            "b": (.versionSet(.range("1.1.0" ..< "2.0.0")), .everything),
+                            "c": (.versionSet(.range("1.2.1" ..< "2.0.0")), .everything)
+                        ],
+                      ]
+        )
+
+        builder.serve("b",
+                      at: "1.0.0",
+                      with: [
+                        "b": [
+                            "c": (.versionSet(.range("1.3.1" ..< "2.0.0")), .everything),
+                        ],
+                      ]
+        )
+        builder.serve("b",
+                      at: "1.1.0",
+                      with: [
+                        "b": [
+                            "c": (.versionSet(.range("1.4.1" ..< "2.0.0")), .everything),
+                        ],
+                      ]
+        )
+
+        builder.serve("c",
+                      at: ["1.0.0", "1.1.0", "1.2.0", "1.2.1", "1.3.0", "1.3.1", "1.4.0", "1.4.1", "2.0.0", "2.1.0"],
+                      with: [
+                        "c": ["d": (.versionSet(v1Range), .everything)],
+                      ]
+        )
+
+        builder.serve("d", at: ["1.0.0", "1.1.0", "1.1.1", "1.1.2", "1.2.0", "1.2.1"])
+
+        let resolver = builder.create()
+        let dependencies = builder.create(dependencies: [
+            "a": (.versionSet(v1Range), .everything),
+            "c": (.versionSet(v1Range), .everything)
+        ])
+        let result = resolver.solve(constraints: dependencies)
+
+        AssertResult(result, [
+            ("a", .version(v1)),
+            ("b", .version("1.1.0")),
+            ("c", .version("1.4.1")),
+            ("d", .version("1.2.1"))
+        ])
+    }
+
     func testCycle1() {
         builder.serve("foo", at: v1_1, with: ["foo": ["foo": (.versionSet(v1Range), .specific(["foo"]))]])
 
@@ -2820,6 +3000,15 @@ class DependencyGraphBuilder {
         }
     }
 
+    func serve(
+        _ package: String,
+        at versions: [Version],
+        toolsVersion: ToolsVersion? = nil,
+        with dependencies: KeyValuePairs<String, OrderedCollections.OrderedDictionary<String, (PackageRequirement, ProductFilter)>> = [:]
+    ) {
+        self.serve(package, at: versions.map{ .version($0) }, toolsVersion: toolsVersion, with: dependencies)
+    }
+
     func serve(
         _ package: String,
         at version: Version,
@@ -2829,6 +3018,21 @@ class DependencyGraphBuilder {
         self.serve(package, at: .version(version), toolsVersion: toolsVersion, with: dependencies)
     }
 
+    func serve(
+        _ package: String,
+        at versions: [BoundVersion],
+        toolsVersion: ToolsVersion? = nil,
+        with dependencies: KeyValuePairs<String, OrderedCollections.OrderedDictionary<String, (PackageRequirement, ProductFilter)>> = [:]
+    ) {
+        let packageReference = reference(for: package)
+        self.serve(
+            packageReference,
+            at: versions,
+            toolsVersion: toolsVersion,
+            with: dependencies
+        )
+    }
+
     func serve(
         _ package: String,
         at version: BoundVersion,
@@ -2844,6 +3048,17 @@ class DependencyGraphBuilder {
         )
     }
 
+    func serve(
+        _ packageReference: PackageReference,
+        at versions: [BoundVersion],
+        toolsVersion: ToolsVersion? = nil,
+        with dependencies: KeyValuePairs<String, OrderedCollections.OrderedDictionary<String, (PackageRequirement, ProductFilter)>> = [:]
+    ) {
+        for version in versions {
+            serve(packageReference, at: version, toolsVersion: toolsVersion, with: dependencies)
+        }
+    }
+
     func serve(
         _ packageReference: PackageReference,
         at version: BoundVersion,