[PubGrub] Narrow down cases when pre-release decisions are marked invalid (swiftlang#7799)

Pre-release decisions are valid if the derived range they were inferred
from supports pre-release versions.

### Motivation:

Currently, after deriving a pre-release assignment, the solver would
assert
in some circumstances depending on order in which the dependencies are
specified in the manifest.

This change makes it possible to infer pre-release verison of a package 
even when intermediate derivations did not support pre-release versions.

For example one package could declare `swift-syntax` dependency as
`508.0.1` ..< `601.0.0` and another could narrow it to `600.0.0-latest`
..< `601.0.0`.

Since the most constrained range in such case supports pre-releases a
decision
to use the lastest pre-release version of `600.0.0` should be valid if
there
is no `600.0.0` release yet.

It's important to note that on each path a decision for a package is
always
made based on the last undecided term's requirements which means -
based on the current semantics - if such derivation doesn't support
pre-release,
even though intermediate ones did, the solver would select a released
version.
This PR is not aim to change that.

There is a caveat which we cannot narrowly fix - if package depends on
an exact
pre-release version the inference is going to fail when there is another
dependency
to the same package that doesn't support pre-releases. For example:

Package `A` depends on packages `B` and `C` as follows:

`A` depends on `B` = `0.0.1-alpha` and `C` = `1.0.0`
`B` version `0.0.1-alpha` has no dependencies
`C` version `1.0.0` depends on `B` - `0.0.1` ..< `0.0.2`

In this situation the solver would fail because `0.0.1-alpha` does not
satisfy range - `0.0.1` ..< `0.0.2` inferred from `C`.

### Modifications:

- Delays version decisions for packages with pre-release ranges until
there are no more packages without pre-release versions. This also means
that if a previous decision narrows down the range to releases-only the
package would be made a contender on the next step of the solver.

- Relaxes the `isValidDecision` check for pre-release decisions but
makes sure that the last derivation supports pre-release versions.

### Result:

In situations like
swiftlang#7658 and
swiftlang#7643 the
solver would no longer assert and would produce a solution with a
pre-release version for `swift-syntax`.

Author: xedin

Sources/PackageGraph/Resolution/PubGrub/PubGrubDependencyResolver.swift

@@ -764,7 +764,17 @@ public struct PubGrubDependencyResolver {
                 let start = DispatchTime.now()
                 let counts = try result.get()
                 // forced unwraps safe since we are testing for count and errors above
-                let pkgTerm = undecided.min { counts[$0]! < counts[$1]! }!
+                let pkgTerm = undecided.min {
+                    // Prefer packages that don't allow pre-release versions
+                    // to allow propagation logic to find dependencies that
+                    // limit the range before making any decisions. This means
+                    // that we'd always prefer release versions.
+                    if $0.supportsPrereleases != $1.supportsPrereleases {
+                        return !$0.supportsPrereleases
+                    }
+
+                    return counts[$0]! < counts[$1]!
+                }!
                 self.delegate?.willResolve(term: pkgTerm)
                 // at this point the container is cached
                 let container = try self.provider.getCachedContainer(for: pkgTerm.node.package)

Sources/PackageGraph/Resolution/PubGrub/Term.swift

@@ -40,6 +40,10 @@ public struct Term: Equatable, Hashable {
         )
     }
 
+    package var supportsPrereleases: Bool {
+        self.requirement.supportsPrereleases
+    }
+
     /// Check if this term satisfies another term, e.g. if `self` is true,
     /// `other` must also be true.
     public func satisfies(_ other: Term) -> Bool {
@@ -100,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. This 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. 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

@@ -466,6 +466,38 @@ extension VersionSetSpecifier {
     }
 }
 
+extension VersionSetSpecifier {
+    package var supportsPrereleases: Bool {
+        switch self {
+        case .empty, .any:
+            false
+        case .exact(let version):
+            version.supportsPrerelease
+        case .range(let range):
+            range.supportsPrereleases
+        case .ranges(let ranges):
+            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 {
     public var description: String {
         switch self {
@@ -505,4 +537,35 @@ fileprivate extension Range where Bound == Version {
     func isHigherThan(_ other: Range<Bound>) -> Bool {
         return other.isLowerThan(self)
     }
+
+    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 {

Tests/PackageGraphTests/VersionSetSpecifierTests.swift

@@ -151,4 +151,29 @@ final class VersionSetSpecifierTests: XCTestCase {
         XCTAssertTrue(VersionSetSpecifier.range("2.0.1"..<"2.0.2") == VersionSetSpecifier.ranges(["2.0.1"..<"2.0.2"]))
         XCTAssertTrue(VersionSetSpecifier.ranges(["2.0.1"..<"2.0.2"]) == VersionSetSpecifier.range("2.0.1"..<"2.0.2"))
     }
+
+    func testPrereleases() {
+        XCTAssertFalse(VersionSetSpecifier.any.supportsPrereleases)
+        XCTAssertFalse(VersionSetSpecifier.empty.supportsPrereleases)
+        XCTAssertFalse(VersionSetSpecifier.exact("0.0.1").supportsPrereleases)
+
+        XCTAssertTrue(VersionSetSpecifier.exact("0.0.1-latest").supportsPrereleases)
+        XCTAssertTrue(VersionSetSpecifier.range("0.0.1-latest" ..< "2.0.0").supportsPrereleases)
+        XCTAssertTrue(VersionSetSpecifier.range("0.0.1" ..< "2.0.0-latest").supportsPrereleases)
+
+        XCTAssertTrue(VersionSetSpecifier.ranges([
+            "0.0.1" ..< "0.0.2",
+            "0.0.1" ..< "2.0.0-latest",
+        ]).supportsPrereleases)
+
+        XCTAssertTrue(VersionSetSpecifier.ranges([
+            "0.0.1-latest" ..< "0.0.2",
+            "0.0.1" ..< "2.0.0",
+        ]).supportsPrereleases)
+
+        XCTAssertFalse(VersionSetSpecifier.ranges([
+            "0.0.1" ..< "0.0.2",
+            "0.0.1" ..< "2.0.0",
+        ]).supportsPrereleases)
+    }
 }