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

feat: Stabilize Edition 2024 #14828

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

Conversation

epage
Copy link
Contributor

@epage epage commented Nov 15, 2024

What does this PR try to resolve?

How should we test and review this PR?

Additional information

@epage epage marked this pull request as draft November 15, 2024 18:43
@rustbot
Copy link
Collaborator

rustbot commented Nov 15, 2024

r? @ehuss

rustbot has assigned @ehuss.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 15, 2024
@epage
Copy link
Contributor Author

epage commented Nov 15, 2024

Posting this now to not lose track of the documentation update mentioned in #14754.

@@ -507,7 +507,7 @@ resolver = "2"
- `"2"` ([`edition = "2021"`](manifest.md#the-edition-field) default): Introduces changes in [feature
unification](#features). See the [features chapter][features-2] for more
details.
- `"3"` (requires Rust 1.84+): Change the default for [`resolver.incompatible-rust-versions`] from `allow` to `fallback`
- `"3"` ([`edition = "2024"`](manifest.md#the-edition-field) default, requires Rust 1.84+): Change the default for [`resolver.incompatible-rust-versions`] from `allow` to `fallback`
Copy link
Contributor

@weiznich weiznich Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to point again to RFC-2052 especially to the hard constraints section there:

Warning-free code on edition N must compile on edition N+1 and have the same behavior.

Given that this changes the resolver behavior you can end up in situations that result in broken builds if you specify a minimal supported rust version and a too broad minimal dependency version. That can then result in a changed version resolution to an dependency version which does not contain a public element that is used by the actual crate.

No warning about the possible broken build is emitted with the 2021 edition!

Ed Page pointed out before that this only happens if there is no Cargo.lock file as otherwise dependency resolution is not rerun on edition updates. Also he pointed out that this code is already faulty.

I argue again that neither arguments are valid here as:

  • Projects without Cargo.lock file checked in git repositories where the default at the introduction of the 2021 edition, so cargo must expect that such projects still exists. After all these compile without warning on the latest cargo version
  • A broken minimal dependency version only caused issues with unstable features (-Z minimal-version) in the past, not with any updates (at least as long as the dependency follows semver, but that's assumed as default). So this is a new built-error mode. (Or worded differently the rust compiler itself does also introduce warning periods for similar "fixes")

EDIT: To be clear here, I'm not calling for not stabilizing this feature, I'm just asking for adding the warning required by the RFC.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discussed in #14754 and the Cargo team still checked their boxes and didn't raise a concern to block FCP.

Copy link
Contributor

@weiznich weiznich Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know the cargo team is responsible for managing cargo. The edition is a language feature, which means the cargo team is not the only responsible team to decide whether something violates the relevant RFC or not.

In addition to that above: Completing the FCP does not change fact that this violates that hard constraint as no such warning is emitted today. I would even argue that this might indicate that the cargo team knowingly decides to violate this RFC and therefore break the given stability guarantees (again).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moderator note: both of you have stated everything there is to it from your perspectives. Further discussion between you two is not fruitful in my opinion. So other cargo team members can chime in, but I would like it if you two refrained from further commentary on this topic unless asked to by cargo team members.

@ehuss
Copy link
Contributor

ehuss commented Nov 23, 2024

@epage Is this ready to go once nightly is updated tomorrow?

@epage
Copy link
Contributor Author

epage commented Nov 23, 2024

For what I know, yes

@epage
Copy link
Contributor Author

epage commented Nov 24, 2024

@ehuss I realized I forgot to update the functions on Edition. I then tried to update the tests as best as I could because of that. Likely missed stuff.

@ehuss
Copy link
Contributor

ehuss commented Nov 24, 2024

Thanks! Unfortunately it looks like the latest nightly may have failed to publish.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-documenting-cargo-itself Area: Cargo's documentation A-manifest Area: Cargo.toml issues A-unstable Area: nightly unstable support S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants