[PubGrub] Relax isValidDecision check to handle prerelease versions correctly

The intersection between release and pre-release ranges is
allowed to produce a pre-release range which means that the
solver is allowed to make a pre-release version decision
even when some of the versions didn't allow pre-releases.

Author: xedin

Sources/PackageGraph/Resolution/PubGrub/Term.swift

@@ -104,9 +104,38 @@ public struct Term: Equatable, Hashable {
     /// - There has to be no decision for it.
     /// - The package version has to match all assignments.
     public func isValidDecision(for solution: PartialSolution) -> Bool {
+        // The intersection between release and pre-release ranges is
+        // allowed to produce a pre-release range which means that the
+        // solver is allowed to make a pre-release version decision
+        // even when some of the versions didn't allow pre-releases.
+        //
+        // This means that we should ignore pre-release differences
+        // while checking derivations and assert only if the term is
+        // pre-release but the last assignment wasn't.
+        if self.supportsPrereleases {
+            if let assignment = solution.assignments.last(where: { $0.term.node == self.node }) {
+                assert(assignment.term.supportsPrereleases)
+            }
+        }
+
         for assignment in solution.assignments where assignment.term.node == self.node {
             assert(!assignment.isDecision, "Expected assignment to be a derivation.")
-            guard self.satisfies(assignment.term) else { return false }
+
+            // This is not great but dragging `ignorePrereleases` through all the APIs seems
+            // worse. This is valid because we can have a derivation chain which is something
+            // like - "0.0.1"..<"1.0.0" -> "0.0.4-latest"..<"0.0.6" and make a decision
+            // `0.0.4-alpha5` based on that if there is no `0.0.4` release. In vacuum this is
+            // (currently) incorrect because `0.0.4-alpha5` doesn't satisfy the initial
+            // range that doesn't support pre-release versions but since the solver is
+            // allowed to derive a pre-release range we consider the original range to
+            // be pre-release range implicitly.
+            let term = if self.supportsPrereleases && !assignment.term.supportsPrereleases {
+                Term(self.node, self.requirement.withoutPrereleases)
+            } else {
+                self
+            }
+
+            guard term.satisfies(assignment.term) else { return false }
         }
         return true
     }

Sources/PackageGraph/VersionSetSpecifier.swift

@@ -479,6 +479,23 @@ extension VersionSetSpecifier {
             ranges.contains(where: \.supportsPrereleases)
         }
     }
+
+    package var withoutPrereleases: VersionSetSpecifier {
+        if !supportsPrereleases {
+            return self
+        }
+
+        return switch self {
+        case .empty, .any:
+            self
+        case .range(let range):
+            .range(range.withoutPrerelease)
+        case .ranges(let ranges):
+            .ranges(ranges.map { $0.withoutPrerelease })
+        case .exact(let version):
+            .exact(version.withoutPrerelease)
+        }
+    }
 }
 
 extension VersionSetSpecifier: CustomStringConvertible {
@@ -524,10 +541,31 @@ fileprivate extension Range where Bound == Version {
     var supportsPrereleases: Bool {
         self.lowerBound.supportsPrerelease || self.upperBound.supportsPrerelease
     }
+
+    var withoutPrerelease: Range<Version> {
+        if !supportsPrereleases {
+            return self
+        }
+
+        return Range(uncheckedBounds: (
+            lower: self.lowerBound.withoutPrerelease,
+            upper: self.upperBound.withoutPrerelease
+        ))
+    }
 }
 
 fileprivate extension Version {
     var supportsPrerelease: Bool {
         !self.prereleaseIdentifiers.isEmpty
     }
+
+    var withoutPrerelease: Version {
+        Version(
+            self.major,
+            self.minor,
+            self.patch,
+            prereleaseIdentifiers: [],
+            buildMetadataIdentifiers: self.buildMetadataIdentifiers
+        )
+    }
 }

Tests/PackageGraphTests/PubgrubTests.swift

@@ -3282,6 +3282,158 @@ final class PubGrubBacktrackTests: XCTestCase {
                 .contains(where: { $0.message == "[DependencyResolver] resolved 'd' @ '2.3.0'" })
         )
     }
+
+    func testPrereleaseVersionSelection() throws {
+        try builder.serve("a", at: "1.0.0", with: [
+            "a": [
+                "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"]))
+            ]
+        ])
+
+        try builder.serve("b", at: "1.0.0-prerelease-20240710")
+
+        try builder.serve("c", at: "1.0.0", with: [
+            "c": [
+                "d": (.versionSet(.range("1.0.5"..<"2.0.0")), .specific(["d"]))
+            ]
+        ])
+        try builder.serve("c", at: "1.0.1")
+
+        try builder.serve("d", at: "1.0.0-prerelease-20240710")
+        try builder.serve("d", at: "1.0.6-prerelease-1")
+        try builder.serve("d", at: "1.0.6")
+
+        let resolver = builder.create()
+        // The order matters here because solver used to assign `b` before `a`.
+        let dependencies1 = try builder.create(dependencies: [
+            "a": (.versionSet(.exact("1.0.0")), .specific(["a"])),
+            "b": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["b"]))
+        ])
+
+        AssertResult(resolver.solve(constraints: dependencies1), [
+            ("a", .version("1.0.0")),
+            ("b", .version("1.0.0-prerelease-20240710"))
+        ])
+
+        let dependencies2 = try builder.create(dependencies: [
+            "c": (.versionSet(.exact("1.0.0")), .specific(["c"])),
+            "d": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["d"]))
+        ])
+
+        AssertResult(resolver.solve(constraints: dependencies2), [
+            ("c", .version("1.0.0")),
+            ("d", .version("1.0.6"))
+        ])
+    }
+
+    func testPrereleaseExactRequirement() throws {
+        try builder.serve("c", at: "1.0.0", with: [
+            "c": [
+                "d": (.versionSet(.range("1.0.4"..<"2.0.0")), .specific(["d"]))
+            ]
+        ])
+        try builder.serve("c", at: "1.0.1")
+
+        try builder.serve("d", at: "1.0.0-prerelease-20240710")
+        try builder.serve("d", at: "1.0.4")
+        try builder.serve("d", at: "1.0.6-prerelease-1")
+
+        let resolver = builder.create()
+
+        let exactDependencies = try builder.create(dependencies: [
+            "c": (.versionSet(.exact("1.0.0")), .specific(["c"])),
+            "d": (.versionSet(.exact("1.0.6-prerelease-1")), .specific(["d"]))
+        ])
+
+        // FIXME: This should produce a valid solution but cannot at the
+        // moment because "1.0.4"..<"2.0.0" doesn't support pre-release versions
+        // and there is no way to infer it.
+        let resultWithExact = resolver.solve(constraints: exactDependencies)
+        XCTAssertMatch(
+            resultWithExact.errorMsg,
+            .contains("Dependencies could not be resolved because root depends on 'd' 1.0.6-prerelease-1")
+        )
+
+        let rangeDependency = try builder.create(dependencies: [
+            "c": (.versionSet(.exact("1.0.0")), .specific(["c"])),
+            "d": (.versionSet(.range("1.0.5"..<"1.0.6-prerelease-2")), .specific(["d"]))
+        ])
+
+        let resultWithRange = resolver.solve(constraints: rangeDependency)
+        AssertResult(resultWithRange, [
+            ("c", .version("1.0.0")),
+            ("d", .version("1.0.6-prerelease-1"))
+        ])
+    }
+
+    func testReleaseOverPrerelease() throws {
+        try builder.serve("a", at: "1.0.0", with: [
+            "a": [
+                "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"]))
+            ]
+        ])
+
+        try builder.serve("b", at: "1.0.0-prerelease-20240616")
+        try builder.serve("b", at: "1.0.0")
+
+        let resolver = builder.create()
+        // The order matters here because solver used to assign `b` before `a`.
+        let dependencies1 = try builder.create(dependencies: [
+            "a": (.versionSet(.exact("1.0.0")), .specific(["a"])),
+            "b": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["b"]))
+        ])
+
+        AssertResult(resolver.solve(constraints: dependencies1), [
+            ("a", .version("1.0.0")),
+            ("b", .version("1.0.0"))
+        ])
+
+        let dependencies2 = try builder.create(dependencies: [
+            "b": (.versionSet(.range("1.0.0-latest"..<"2.0.0")), .specific(["b"])),
+            "a": (.versionSet(.exact("1.0.0")), .specific(["a"]))
+        ])
+
+        AssertResult(resolver.solve(constraints: dependencies2), [
+            ("a", .version("1.0.0")),
+            ("b", .version("1.0.0"))
+        ])
+    }
+
+    func testPrereleaseInferenceThroughDependencies() throws {
+        try builder.serve("a", at: "1.0.0", with: [
+            "a": [
+                "b": (.versionSet(.range("1.0.0"..<"2.0.0-latest")), .specific(["b"]))
+            ]
+        ])
+
+        try builder.serve("b", at: "0.0.8-prerelease-20230310")
+        try builder.serve("b", at: "0.0.8")
+        try builder.serve("b", at: "1.0.0-prerelease-20230616")
+        try builder.serve("b", at: "1.0.0")
+        try builder.serve("b", at: "1.9.9-prerelease-20240702")
+
+        let resolver = builder.create()
+        // The order matters here because solver used to assign `b` before `a`.
+        let dependencies1 = try builder.create(dependencies: [
+            "a": (.versionSet(.exact("1.0.0")), .specific(["a"])),
+            "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"]))
+        ])
+
+        AssertResult(resolver.solve(constraints: dependencies1), [
+            ("a", .version("1.0.0")),
+            ("b", .version("1.9.9-prerelease-20240702"))
+        ])
+
+        let dependencies2 = try builder.create(dependencies: [
+            "b": (.versionSet(.range("0.0.8"..<"2.0.0")), .specific(["b"])),
+            "a": (.versionSet(.exact("1.0.0")), .specific(["a"]))
+        ])
+
+        AssertResult(resolver.solve(constraints: dependencies2), [
+            ("a", .version("1.0.0")),
+            ("b", .version("1.9.9-prerelease-20240702"))
+        ])
+    }
 }
 
 extension PinsStore.PinState {