semver icon indicating copy to clipboard operation
semver copied to clipboard

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

Open linyihai opened this issue 1 year ago • 13 comments

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 avatar Jul 09 '24 06:07 linyihai

Sorry, need to address the node test firstly

linyihai avatar Jul 09 '24 07:07 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

linyihai avatar Jul 09 '24 08:07 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.

Eh2406 avatar Jul 15 '24 19:07 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.

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

linyihai avatar Jul 16 '24 02:07 linyihai

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.

Eh2406 avatar Jul 17 '24 16:07 Eh2406

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"]);

linyihai avatar Jul 18 '24 12:07 linyihai

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)

Eh2406 avatar Jul 18 '24 17:07 Eh2406

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

linyihai avatar Jul 19 '24 14:07 linyihai

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.

Eh2406 avatar Jul 19 '24 21:07 Eh2406

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

epage avatar Jul 23 '24 14:07 epage

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

https://github.com/rust-lang/rfcs/pull/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, https://github.com/rust-lang/rfcs/pull/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 avatar Jul 24 '24 03:07 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 https://github.com/rust-lang/rfcs/pull/3493, but these node semantic is non-default, you need to set includePrerelease=true.

linyihai avatar Jul 25 '24 10:07 linyihai

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 https://github.com/dtolnay/semver/commit/3464fd136a180fd64c6b0a394ccb0479ccb8d967 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.

Eh2406 avatar Jul 25 '24 17:07 Eh2406

Is there any plan to merge this?

gabrik avatar Oct 23 '24 13:10 gabrik

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.

Eh2406 avatar Oct 23 '24 13:10 Eh2406

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.

linyihai avatar Oct 26 '24 02:10 linyihai