Add matches_prerelease to allow update to prerelease version if it meets Semver compatible

[linyihai]

What 's this PR want to solve?

The main purpose is to solve the upper bound semantic approved by rfc#precise-pre-release-cargo-update

it extends the matching mechanism of the pre-release version,

So before,

1.2.3  -> ^1.2.3 -> >=1.2.3, <2.0.0 (with implicit holes excluding pre-release versions)
would become

1.2.3  -> ^1.2.3 -> >=1.2.3, <2.0.0-0
Note that the old syntax implicitly excluded 2.0.0-<prerelease> which we have have to explicitly exclude by referencing the smallest possible pre-release version of -0.

If this PR could be merged, it maybe fixed the cargo update --precise <pre-release> upper bound semantic

[linyihai]

Sorry, need to address the node test firstly

[linyihai]

Oh, node test is a call to the node program to verify version compatibility, which is used for comparison testing.

In this case, I didn't know if there was a corresponding pre-release compatibility match test for node, so I bypassed the node test

[Eh2406]

Experimented with this branch, definitely found cases I was not expecting. For example the requirement >=0.24, <=0.25 does not matches_prerelease with version 0.24.1-PRE. Or ~0.34 not matching 0.34.7-p1.

[linyihai]

Experimented with this branch, definitely found cases I was not expecting. For example the requirement >=0.24, <=0.25 does not matches_prerelease with version 0.24.1-PRE. Or ~0.34 not matching 0.34.7-p1.

Oops，the paritial-version doesn't take into consideration. Thank you for your feedback.

[Eh2406]

Thanks for the update! I tried again and found more examples that didn't match my expectations:

• =4.1 with 4.1.1-rev.0
• ^4.5 with 4.5.0-alpha.1
• ~0 with 0.1.0-0
• =4.2 with 4.2.1 (this one doesn't even involve prerelease)

These are of course just "my expectations" and the "correct behavior" may not match what's in my head at the moment.
For context I am generating these with https://github.com/pubgrub-rs/semver-pubgrub/tree/new-pre-release
and reviewing the cases where the two algorithms disagree to figure out which one looks right. There are definitely cases where that code is also getting the wrong answer. For example I have no idea what correct behavior is for ^4.4.11, <4.5 with 4.5.0-alpha.1.

[linyihai]

My apologies. My Last commit have some regression and I am eager to correct it.

I test you example and use it to explain the match logic

    let ref r = req("=4.1");
    // Match >= 4.1.0, < 4.2.0-0
    assert_prerelease_match_all(r, &["4.1.1-rev.0"]);
    assert_prerelease_match_none(r, &["1.2.4", "4.1.0-rev.0", "4.2.0-0", "4.2.0"]);

    let ref r = req("^4.5");
    // Match >= 4.5.0, < 5.0.0-0
    assert_prerelease_match_all(r, &["4.5.0", "4.6.0-0", "4.6.0"]);
    assert_prerelease_match_none(r, &["1.2.4", "4.5.0-rev.0"]);

    let ref r = req("~0");
    // Match >= 0.0.0, < 1.0.0-0
    assert_prerelease_match_all(r, &["0.1.0-0"]);
    assert_prerelease_match_none(
        r,
        &["0.0.0-pre", "1.2.4", "4.1.0-rev.0", "4.2.0-0", "4.2.0"],
    );

    let ref r = req("=4.2");
    // Match >= 4.2.0, < 4.3.0-0
    assert_prerelease_match_all(r, &["4.2.0", "4.2.1"]);
    assert_prerelease_match_none(r, &["1.2.4", "4.1.0-rev.0", "4.2.0-0", "4.3.0-0", "4.3.0"]);

    let ref r = req("^4.4.11, <4.5");
    // Match >= 4.4.11, < 4.5.0
    assert_prerelease_match_all(r, &["4.5.0-alpha.1"]);
    assert_prerelease_match_none(r, &["1.2.4", "4.1.0-rev.0", "4.2.0-0", "4.5.0"]);

[Eh2406]

Thanks for the update!

req(">= 4.1") == req(">= 4.1.0") not match "4.1.0-rev.0" feels really odd to me. Intuitively it looks like =4.1 should match something that starts 4.1. So perhaps we should pad one more zero so that it the sugars to 4.1.0-0. But if we do that, there is no way to specify "I'm broken on the prereleases and require a fix from the stable release" because =4.1.0 would be sugar to 4.1.0-0. What we think the "correct" behavior should be?

I tried again and found more examples:

• <0.10 with 0.10.0-BP-beta.1 (this is the flipside of the one we just discussed)
• =3.0.0-alpha.24 with 3.0.0-alpha.25 (this I think is just wrong)

[linyihai]

I am very appreciate your feedback.

=3.0.0-alpha.24 with 3.0.0-alpha.25

This had been corrected.

Then for comparison, I collated the versions that currently have pre-release comparison logic

# Op::Exact
- =I.J.K equivalent to >=I.J.K, <I.J.(K+1)-0
- =I.J equivalent to >=I.J.0, <I.(J+1).0-0
- =I equivalent to >=I.0.0, <(I+1).0.0-0

# Op::Greater
- >I.J.K
- >I.J equivalent to >=I.(J+1).0-0
- >I equivalent to >=(I+1).0.0-0

# Op::GreaterEq
- >=I.J.K
- >=I.J equivalent to >=I.J.0
- >=I equivalent to >=I.0.0

# Op::Less
- <I.J.K
- <I.J equivalent to <I.J.0
- <I equivalent to <I.0.0

# Op::LessEq
- <=I.J.K
- <=I.J equivalent to <I.(J+1).0-0
- <=I equivalent to <(I+1).0.0-0

# Op::Tilde
- ~I.J.K — equivalent to >=I.J.K, <I.(J+1).0-0
- ~I.J — equivalent to >=I.J.0, <I.(J+1).0-0
- ~I — >=I.0.0, <(I+1).0.0-0

# Op::Caret
- ^I.J.K (for I>0) — equivalent to >=I.J.K, <(I+1).0.0-0
- ^0.J.K (for J>0) — equivalent to >=0.J.K, <0.(J+1).0-0
- ^0.0.K — equivalent to >=0.0.K, <0.0.(K+1)-0
- ^I.J (for I>0 or J>0) — equivalent to >=I.J.0, <(I+1).0.0-0)
- ^0.0 — equivalent to >=0.0.0, <0.1.0-0
- ^I — equivalent to >=I.0.0, <(I+1).0.0-0

At present, I think this version is much better than the first commit, but I feel that there are still some corners that have not been sorted out, which may be discussed separately next time

[Eh2406]

Thank you for the prompt fixes. I only had a brief chance to look at this today, and it requires more thought than I had available. I will try and look at it again next week.

[epage]

Should we be trialing this in Cargo under a nightly feature before trying to merge this into a stable API?

[weihanglo]

nit: extra newline

Suggested change

	// If VersionReq missing Minor, Patch, then filling them with 0
	// If VersionReq missing Minor, Patch, then filling them with 0

[weihanglo]

If I got everything right, these are the current-proposed semantic
(bold texts are semantic missing in #321 (comment) and guessed by me).

In both matches and matches_prerelease columns the equivalent matching semantic is using the old VersionReq::matches minus this prelease specific behavior.

req	matches	matches_prerelease
Op::Exact
=I.J.K	=I.J.K	>=I.J.K, <I.J.(K+1)-0
=I.J	>=I.J.0, <I.(J+1).0	>=I.J.0, <I.(J+1).0-0
=I	>=I.0.0, <(I+1).0.0	>=I.0.0, <(I+1).0.0-0
Op::Greater
>I.J.K	>I.J.K	>I.J.K
>I.J	>=I.(J+1).0	>=I.(J+1).0-0
>I	>=(I+1).0.0	>=(I+1).0.0-0
Op::GreaterEq
>=I.J.K	>=I.J.K	>=I.J.K
>=I.J	>=I.J.0	>=I.J.0
>=I	>=I.0.0	>=I.0.0
Op::Less
<I.J.K	<I.J.K	<I.J.K
<I.J	<I.J.0	<I.J.0
<I	<I.0.0	<I.0.0
Op::LessEq
<=I.J.K	<=I.J.K	<=I.J.K
<=I.J	<I.(J+1).0	<I.(J+1).0-0
<=I	<(I+1).0.0	<(I+1).0.0-0
Op::Tilde
~I.J.K	>=I.J.K, <I.(J+1).0	>=I.J.K, <I.(J+1).0-0
~I.J	=I.J	>=I.J.0, <I.(J+1).0-0
~I	=I	>=I.0.0, <(I+1).0.0-0
Op::Caret
^I.J.K (for I>0)	>=I.J.K, <(I+1).0.0	>=I.J.K, <(I+1).0.0-0
^0.J.K (for J>0)	>=0.J.K, <0.(J+1).0	>=0.J.K, <0.(J+1).0-0
^0.0.K	=0.0.K	>=0.0.K, <0.0.(K+1)-0
^I.J	^I.J.0	>=I.J.0, <(I+1).0.0-0
^0.0	=0.0	>=0.0.0, <0.1.0-0
^I	=I	>=I.0.0, <(I+1).0.0-0
Op::Wildcard
I.J.*	=I.J	=I.J
I.* or I.*.*	=I	=I

table markdown source

| req              | matches             | matches_prerelease     |
|------------------|---------------------|------------------------|
| `Op::Exact`      |                     |                        |
| =I.J.K           | =I.J.K              | >=I.J.K, <I.J.(K+1)-0  |
| =I.J             | >=I.J.0, <I.(J+1).0 | >=I.J.0, <I.(J+1).0-0  |
| =I               | >=I.0.0, <(I+1).0.0 | >=I.0.0, <(I+1).0.0-0  |
| `Op::Greater`    |                     |                        |
| >I.J.K           | >I.J.K              | **>I.J.K**             |
| >I.J             | >=I.(J+1).0         | >=I.(J+1).0-0          |
| >I               | >=(I+1).0.0         | >=(I+1).0.0-0          |
| `Op::GreaterEq`  |                     |                        |
| >=I.J.K          | >=I.J.K             | **>=I.J.K**            |
| >=I.J            | >=I.J.0             | >=I.J.0                |
| >=I              | >=I.0.0             | >=I.0.0                |
| `Op::Less`       |                     |                        |
| <I.J.K           | <I.J.K              | **<I.J.K**             |
| <I.J             | <I.J.0              | <I.J.0                 |
| <I               | <I.0.0              | <I.0.0                 |
| `Op::LessEq`     |                     |                        |
| <=I.J.K          | <=I.J.K             | **<=I.J.K**            |
| <=I.J            | <I.(J+1).0          | <I.(J+1).0-0           |
| <=I              | <(I+1).0.0          | <(I+1).0.0-0           |
| `Op::Tilde`      |                     |                        |
| ~I.J.K           | >=I.J.K, <I.(J+1).0 | >=I.J.K, <I.(J+1).0-0  |
| ~I.J             | =I.J                | >=I.J.0, <I.(J+1).0-0  |
| ~I               | =I                  | >=I.0.0, <(I+1).0.0-0  |
| `Op::Caret`      |                     |                        |
| ^I.J.K (for I>0) | >=I.J.K, <(I+1).0.0 | >=I.J.K, <(I+1).0.0-0  |
| ^0.J.K (for J>0) | >=0.J.K, <0.(J+1).0 | >=0.J.K,  <0.(J+1).0-0 |
| ^0.0.K           | =0.0.K              | >=0.0.K,  <0.0.(K+1)-0 |
| ^I.J             | ^I.J.0              | >=I.J.0,  <(I+1).0.0-0 |
| ^0.0             | =0.0                | >=0.0.0, <0.1.0-0      |
| ^I               | =I                  | >=I.0.0, <(I+1).0.0-0  |
| `Op::Wildcard`   |                     |                        |
| `I.J.*`          | =I.J                | =I.J                   |
| `I.*` or `I.*.*` | =I                  | =I                     |

[weihanglo]

@linyihai Please help correct if anything wrong.

[linyihai]

In both matches and matches_prerelease columns the equivalent matching semantic is using the old VersionReq::matches.

The two are different, even if they are same literally

In the matches_prerelease proposed semantic, it doesn't follow the limit

semver/src/eval.rs

Lines 14 to 21 in 69efd3c

	// If a version has a prerelease tag (for example, 1.2.3-alpha.3) then it
	// will only be allowed to satisfy req if at least one comparator with the
	// same major.minor.patch also has a prerelease tag.
	for cmp in &req.comparators {
	if pre_is_compatible(cmp, ver) {
	return true;
	}
	}

For example, see playground example

[weihanglo]

Yean I forgot that part. It's the old matches minus prerelease speicial treatment, right?

diff --git a/src/eval.rs b/src/eval.rs
index e6e3894..6cdf99d 100644
--- a/src/eval.rs
+++ b/src/eval.rs
@@ -7,20 +7,7 @@ pub(crate) fn matches_req(req: &VersionReq, ver: &Version) -> bool {
         }
     }
 
-    if ver.pre.is_empty() {
-        return true;
-    }
-
-    // If a version has a prerelease tag (for example, 1.2.3-alpha.3) then it
-    // will only be allowed to satisfy req if at least one comparator with the
-    // same major.minor.patch also has a prerelease tag.
-    for cmp in &req.comparators {
-        if pre_is_compatible(cmp, ver) {
-            return true;
-        }
-    }
-
-    false
+    true
 }
 
 pub(crate) fn matches_comparator(cmp: &Comparator, ver: &Version) -> bool {

[linyihai]

It equivalents what you said, but I add a flag to sidestep that

    if ver.pre.is_empty() || prerelease_matches {
        return true;
    }

[linyihai]

Should we be trialing this in Cargo under a nightly feature before trying to merge this into a stable API?

rust-lang/rfcs#3493 is the only customer of this features now. And the matches_prerelease proposed semantic differs from the original matches in some ways. So this proposed semantic may be implemented in Cargo.

BTW, rust-lang/rfcs#3493 doesn't give all scenes need to be considered, it only give a caret op example

So before,

1.2.3  -> ^1.2.3 -> >=1.2.3, <2.0.0 (with implicit holes excluding pre-release versions)
would become

1.2.3  -> ^1.2.3 -> >=1.2.3, <2.0.0-0
Note that the old syntax implicitly excluded 2.0.0-<prerelease> which we have have to explicitly exclude by referencing the smallest possible pre-release version of -0.

And I found I haven't read fully The semantic versioner for npm, who knows what inspiration this might bring

In short, it is better to carry this implementation into Cargo, and can be better combined with cargo design

[linyihai]

By this commit, these stuffs have been added:

• Add node semver compatibility (with includePrerelease=true) test.
• According to the node semver compatibility test, a mirror-version implemention comes out, which behind the feature mirror_node_matches_prerelease
• rewrite test_matches_prerelease.rs to better reflect current implementions.

I found the diffenence between node semver compatibility implemtation and the current-proposed semantic

• if the VersionReq is partial, the former will padding the Prerelease Tag with 0 except 'Tilde Op'.
• And for Caret Op, it will also padding the Prerelease Tag with "0" if the Major is 0.

If my assumption is correct, then the node semver compatibilit implemention also suitable for the rust-lang/rfcs#3493, but these node semantic is non-default, you need to set includePrerelease=true.

[Eh2406]

Thank you for all the work you put into this! The details here are tricky and overwhelming and I really appreciate you sticking with it.

Compatibility with npm is nice but not required. Starting with their semantics was a very good idea for a place to start, someone else thought this was the right choice. But, we have every right to decide we want different semantics if that's what we decide we need.

I was able to reproduce the semantics as of 3464fd1 in my project. The diff is mostly boilerplate. The only two exceptions were the lower bound for ^x|^x.y|^x.y.z and the upper bound for <x|<x.y|<x.y.z. I had to change from expanding to x.y.z-0 -> x.y.z. This rarely makes a difference, because requirements that don't mention prerelease are not allowed to match versions with prereleases, so it's extremely rare that which prereleases it matches to matters. In fact, there are no examples on crates.io where the difference matters. And the fuzzing tools tend to minimize two examples that seem irrelevant. <0, ^0.0.0-0 and ^0, ^0.0.0-0 matched on 0.0.0-0. There all 0 because of the minimizer, the pattern can be generalized to other numbers.

I've assumed that if matches then also matches_prerelease. If that is one of the axioms of our design then we need to fix these obscure cases. Either by changing the behavior of matches_prerelease to be less intuitive. Or by changing the behavior of matches to match. Which would technically be a breaking change. But on such obscure inputs that perhaps it's not worth making a fuss about.

[gabrik]

Is there any plan to merge this?

[Eh2406]

I doubt there is in the short term. The Cargo Team is still intermittently trying to figure out what semantics we want. I doubt this will get merged in this repo until the semantic are pin down precisely.

[linyihai]

The proposed semantics isn't determinate, welcome any ideas and feadbacks.

BTW, this proposal had merged into Cargo, if you like to test it, please refer to https://doc.rust-lang.org/cargo/reference/unstable.html?highlight=unstable#precise-pre-release.