Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(resolver): Treat unset MSRV as compatible #13791

Merged
merged 1 commit into from
May 1, 2024

Conversation

epage
Copy link
Contributor

@epage epage commented Apr 23, 2024

What does this PR try to resolve?

Have the resolver treat no-MSRV as rust-version = "*", like cargo add does for version-requirement selection

How should we test and review this PR?

We last tweaked this logic in #13066.
However, we noticed this was inconsistent with cargo add in automatically selecting version requirements.

It looks like this is a revert of #13066, taking us back to the behavior in #12950.
In #12950 there was a concern about the proliferation of no-MSRV and whether we should de-prioritize those to make the chance of success more likely.

There are no right answes here, only which wrong answer is ok enough.

  • Do we treat lack of rust version as rust-version = "*" as some people expect or do we try to be smart?
  • If a user adds or removes rust-version, how should that affect the priority?

One piece of new information is that the RFC for this has us trying to fill the no-MSRV gap with
rust-version = some-value-representing-the-current-toolchain>.

See also #9930 (comment)

r? @Eh2406

Additional information

We last tweaked this logic in rust-lang#13066.
However, we noticed this was inconsistent with `cargo add` in
automatically selecting version requirements.

It looks like this is a revert of rust-lang#13066, taking us back to the behavior
in rust-lang#12950.
In rust-lang#12950 there was a concern about the proliferation of no-MSRV and
whether we should de-prioritize those to make the chance of success more
likely.

There are no right answes here, only which wrong answer is ok enough.
- Do we treat lack of rust version as `rust-version = "*"` as some
  people expect or do we try to be smart?
- If a user adds or removes `rust-version`, how should that affect the
  priority?

One piece of new information is that the RFC for this has us trying to
fill the no-MSRV gap with
`rust-version = some-value-representing-the-current-toolchain>`.
@rustbot rustbot added A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 23, 2024
@Eh2406
Copy link
Contributor

Eh2406 commented Apr 27, 2024

I wish we had taken better notes about what problems #13066 fixed. I remember feeling strongly about it at the time.

Let me try and reconstructed through brute force. There are three categories of compatibility Incompatible (I), None (N) and, Compatible (C). Which leads to six possible release orders.

  • If C is the most recent then we should obviously pick that one. The highest version release has a compatible MSRV, there is no reason to look at an older one. (Oldest to newest INC and NIC).
  • If I is the most recent then we should look an the next older. The most recent release has told us it won't work, so we should try something else. This is also a normal situation, the package has an MSRV policy which has updated past our needs. (Oldest to newest NCI and CNI).
  • If N is the most recent then the library has removed its MSRV policy. Do we try it because the library has made no statement, or do we go back to an older version where the library promised to work? If we grab an old version, then removing a MSRV policy is a decision that cargo (the tool) will actively fight you on. (Oldest to newest ICN and CIN).

The think about it differently:

Older Newer main PR
N C C C
I C C C
C I C C
N I N N
I N N N
C N C N

Generated with https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=5c0b44e43e2cac5b4f598c24e6eff132

So clearly I've done something wrong because they both always agree. Feel free to correct my table (and my testing examples) when you spot the problem. I found the bug. I was inputting from oldest to newest, and then expecting the best choice to be in position 0. A simple summaries.reverse() before the sort better matches the fallback behavior in the real code.

@epage
Copy link
Contributor Author

epage commented Apr 27, 2024

I wish we had taken better notes about what problems #13066 fixed. I remember feeling strongly about it at the time.

One change from when we had that conversation is that we had a lot less clear of a picture of how to deal with not enough data to make good choices. Since then, the RFC evolved to include rust-version = "<current-toolchain>" which helped close some of that gap (on top of any second order effects from making it easier to deal with MSRVs).

So clearly I've done something wrong because they both always agree. Feel free to correct my table (and my testing examples) when you spot the problem.

I'll have to come back later to dig into this. I think the core of the problem is when there are at least 3 versions involved, rather than just 2.

@Eh2406
Copy link
Contributor

Eh2406 commented Apr 27, 2024

After dinner the problem became obvious, and a better design for the script also seems worth doing. The table has been updated.

@epage
Copy link
Contributor Author

epage commented Apr 29, 2024

To summarize, when upgrading from an MSRV compatible to an unset MSRV, we are changing the behavior from "prefer MSRV compatible" to "prefer unset". The nuance comes in when dealing with what dependents can rely on working for version with an unset MSRV.

Initially

  • if unset MSRV has an effective MSRV that matches the old versions, then users may prefer that
  • if unset MSRV has an effective MSRV that is still below the user's, then user may prefer that
  • if the unset MSRV has an effective MSRV that is above the user's, then the user would prefer the compatible version

However, over time, the user will upgrade their rust version far enough that they won't want a years-old version but one of those mysterious versions. It could be latest, who knows.

  • Picking the latest might work
  • Picking the latest will work but won't make the user happy
    • However, we may not want to optimize for the case of "moving away from an MSRV"
    • However, this could send a signal to the user that there is a mismatch in expectations. The question is how much we want to be policing this. I lean towards "we shouldn't".

Another angle to this is if the dependency adds back in an MSRV.

  • If the new MSRV is like the old MSRV and we pick "unset" then you were likely never supported in the first place and I don't care as much
  • If the new MSRV higher than the old MSRV, this is somewhat like when the MSRV was removed. It depends on circumstances but will eventually not matter.

From this analysis, I'm finding that I don't care enough about the differences between the two cases which leads me to preferring consistency with cargo add (and i prefer that behavior because it would be a lot messier in that circumstance to support the resolver's current behavior)

@Eh2406
Copy link
Contributor

Eh2406 commented May 1, 2024

Seams convincing.
@bors r+

@bors
Copy link
Contributor

bors commented May 1, 2024

📌 Commit 89ead6f has been approved by Eh2406

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 1, 2024
@bors
Copy link
Contributor

bors commented May 1, 2024

⌛ Testing commit 89ead6f with merge 82dca28...

@bors
Copy link
Contributor

bors commented May 1, 2024

☀️ Test successful - checks-actions
Approved by: Eh2406
Pushing 82dca28 to master...

@bors bors merged commit 82dca28 into rust-lang:master May 1, 2024
21 checks passed
@epage epage deleted the msrv-unset branch May 2, 2024 00:07
@fintelia
Copy link
Contributor

fintelia commented May 2, 2024

I'm glad for the conclusion, but if you view this from the perspective of the crate author I think this wouldn't have been very hard to figure out:

If a crate author publishes a new semver compatible version with an unset MSRV, do they want their users to switch to it? Or do they want people to keep using the older release forever?

bors added a commit to rust-lang-ci/rust that referenced this pull request May 3, 2024
Update cargo

18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f
2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000
- chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857)
- Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571)
- fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852)
- refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851)
- perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849)
- Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850)
- chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848)
- Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845)
- Clean package perf improvements (rust-lang/cargo#13818)
- fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841)
- fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839)
- chore(ci): Ignore openssl deps (rust-lang/cargo#13840)
- fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837)
- fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791)
- fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836)
- chore(deps): update compatible (rust-lang/cargo#13834)
- Error when unstable lints are specified but not enabled (rust-lang/cargo#13805)
- fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833)

r? ghost

Note: this includes the fix that was beta backported in rust-lang#124647
bors added a commit to rust-lang-ci/rust that referenced this pull request May 4, 2024
Update cargo

18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f
2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000
- chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857)
- Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571)
- fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852)
- refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851)
- perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849)
- Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850)
- chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848)
- Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845)
- Clean package perf improvements (rust-lang/cargo#13818)
- fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841)
- fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839)
- chore(ci): Ignore openssl deps (rust-lang/cargo#13840)
- fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837)
- fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791)
- fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836)
- chore(deps): update compatible (rust-lang/cargo#13834)
- Error when unstable lints are specified but not enabled (rust-lang/cargo#13805)
- fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833)

r? ghost

Note: this includes the fix that was beta backported in rust-lang#124647
@ehuss ehuss added this to the 1.80.0 milestone May 8, 2024
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request May 24, 2024
Update cargo

18 commits in 6087566b3fa73bfda29702632493e938b12d19e5..05364cb2f61a2c2b091e061c1f42b207dfb5f81f
2024-04-30 20:45:20 +0000 to 2024-05-03 16:48:59 +0000
- chore(deps): update msrv (3 versions) to v1.76 (rust-lang/cargo#13857)
- Stabilize `-Zcheck-cfg` as always enabled (rust-lang/cargo#13571)
- fix(lints): Prevent inheritance from bring exposed for published packages (rust-lang/cargo#13852)
- refactor: remove unnecessary branch for link binary on macOS (rust-lang/cargo#13851)
- perf(toml): Avoid inferring when targets are known (rust-lang/cargo#13849)
- Update continuous-integration.md: Include CircleCI reference (rust-lang/cargo#13850)
- chore(deps): update msrv (1 version) to v1.78 (rust-lang/cargo#13848)
- Workaround copying file returning EAGAIN on ZFS on mac OS (rust-lang/cargo#13845)
- Clean package perf improvements (rust-lang/cargo#13818)
- fix(toml): Validate crates_types/proc-macro for bin like others (rust-lang/cargo#13841)
- fix(toml): On 2024 Edition, disallow ignored `default-features` when inheriting (rust-lang/cargo#13839)
- chore(ci): Ignore openssl deps (rust-lang/cargo#13840)
- fix(lints): Remove ability to specify `-` in lint name (rust-lang/cargo#13837)
- fix(resolver): Treat unset MSRV as compatible (rust-lang/cargo#13791)
- fix(toml): Don't lose 'public' when inheriting a dep (rust-lang/cargo#13836)
- chore(deps): update compatible (rust-lang/cargo#13834)
- Error when unstable lints are specified but not enabled (rust-lang/cargo#13805)
- fix(lint): Warn not Error on unsupported lint tool (rust-lang/cargo#13833)

r? ghost

Note: this includes the fix that was beta backported in #124647
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-dependency-resolution Area: dependency resolution and the resolver S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants