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

Updates to edition handling. #9184

Merged
merged 11 commits into from
Feb 23, 2021
Merged

Updates to edition handling. #9184

merged 11 commits into from
Feb 23, 2021

Conversation

ehuss
Copy link
Contributor

@ehuss ehuss commented Feb 18, 2021

This introduces some updates for edition handling (split into commits for review). In short:

  • cargo-features = ["edition2021"] can be used to opt-in to 2021 support without needing to pass around -Z unstable-options. I tried to emphasize in the docs that this is only for testing and experimentation.
  • Make "2" the default resolver for 2021 edition.
  • Make cargo fix --edition mean the "next" edition from the present one, and support 2021. Also, if already at the latest edition, generate a warning instead an error.
  • I decided to allow cargo fix --edition from 2018 to 2021 on the nightly channel without an explicit opt-in. It's tricky to implement with an opt-in (see comment in diff).

Partial for #9048.
Fixes #9047.

There shouldn't be any functional changes here.

* Some doc comments.
* Construct `FixArgs` at once so it doesn't need to bother with unnecessary Options.
* Remove IdiomEditionMismatch, it is not used.
* Use a general deduping mechanism for fix messages.
What was previously "Fixing" was a message for after the fixes had
been applied. I think it would be clearer if it said "Fixed",
to indicate that the fixes had actually finished.

The new "Fixing" is posted just before it starts. This is verbose-only
since it is a little noisy.
This helps indicate which edition you are moving from and to.
This is intended to help make it easier to test the 2021 edition.
* `--edition` always means "next" edition.
* `--edition` when on the most recent edition is not an error, just a warning.
* Support fix to 2021 edition.
# Conflicts:
#	src/cargo/ops/fix.rs
#	src/doc/src/reference/unstable.md
@rust-highfive
Copy link

r? @Eh2406

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 18, 2021
@ehuss
Copy link
Contributor Author

ehuss commented Feb 18, 2021

Opening as a draft to get feedback.

@ehuss
Copy link
Contributor Author

ehuss commented Feb 18, 2021

Also, I'm uncertain if I should add edition to the [workspace] (#5784). When considering RFC 2906 I think it might get confusing, but I'm not sure. I think we probably should though, maybe?

@ehuss
Copy link
Contributor Author

ehuss commented Feb 19, 2021

Oh, I'd also like to remove --prepare-for, to simplify the code. Would that be OK?

Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Looking good to me! All the changes here look great and I like the new tests. For the edition-in-workspace option I've got a follow-up question below. For removing --prepare-for I think that's fine to go ahead and do as part of this

src/cargo/ops/fix.rs Outdated Show resolved Hide resolved
} else {
None
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I sort of forget the implications of this, can you remind me? What I'm worried about is a set of workspace crates that have mixed editions, but you could do that today as well with a set of crates in a workspace using different resolver versions too. How does Cargo handle that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The resolver is always global for a workspace, so if there is some workspace member that specifies resolver="2", that is ignored (only the top-level matters).

However, thanks for pointing this out. There was a bug where if you set 2021 in a member, it would warn about the resolver being set incorrectly. I pushed a change where the default resolver behavior is now set in one place.

This attempts to centralize all the edition stuff in one place (the
`Edition` enum) so that adding a new edition or stabilizing one should
be relatively little work (and more importantly, we don't miss things).

Importantly, this changes `cargo new` to default to the latest stable.
It also changes the `cargo fix --edition-idiom` behavior to only apply
idioms for the *current* edition.
This was deprecated, never officially part of the stable release.
Copy link
Member

@alexcrichton alexcrichton left a comment

Choose a reason for hiding this comment

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

Ah ok thanks for clearing that up. I think though we still want to add edition to [workspace] for virtual manifests at the root of workspaces. Otherwise, even if everything is in the 2021 edition, you'd still need to explicitly set resolver = "2" I think?

src/cargo/core/workspace.rs Outdated Show resolved Hide resolved
This fixes an issue where warnings would be printed for packages
migrating to 2021 in a workspace (that the "resolver" field is ignored,
which is wrong).

This also places the default resolver logic in one place, and should
make it easier to update later.
@ehuss
Copy link
Contributor Author

ehuss commented Feb 19, 2021

Ah ok thanks for clearing that up. I think though we still want to add edition to [workspace] for virtual manifests at the root of workspaces. Otherwise, even if everything is in the 2021 edition, you'd still need to explicitly set resolver = "2" I think?

Yea, that was my question above.

My only concern is that regarding RFC 2906, until that is implemented, then the only thing the edition field in [workspace] will do is change the resolver. I'm worried people might be confused by that (like it doesn't set the default edition, or have any other effect).

I'm fine with adding it, I just wanted to check what you think about that.

@alexcrichton
Copy link
Member

Oh I was under the assumption that an implementation of edition in [workspace] would cause all workspace members to inherit that if not otherwise specified. I never really considered the interactions with rust-lang/rfcs#2906...

In any case I agree that if the only purpose is to update the resolver then it doesn't make much sense to have edition in [workspace] just yet.

@ehuss ehuss marked this pull request as ready for review February 23, 2021 16:38
@ehuss
Copy link
Contributor Author

ehuss commented Feb 23, 2021

OK, I think this should be ready to go.

@alexcrichton
Copy link
Member

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 23, 2021

📌 Commit a82a23c has been approved by alexcrichton

@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 Feb 23, 2021
@bors
Copy link
Contributor

bors commented Feb 23, 2021

⌛ Testing commit a82a23c with merge 8eb0e9a...

@bors
Copy link
Contributor

bors commented Feb 23, 2021

☀️ Test successful - checks-actions
Approved by: alexcrichton
Pushing 8eb0e9a to master...

@bors bors merged commit 8eb0e9a into rust-lang:master Feb 23, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
Update cargo

11 commits in bf5a5d5e5d3ae842a63bfce6d070dfd438cf6070..572e201536dc2e4920346e28037b63c0f4d88b3c
2021-02-18 15:49:14 +0000 to 2021-02-24 16:51:20 +0000
- Pass the error message format to rustdoc (rust-lang/cargo#9128)
- Fix test target_in_environment_contains_lower_case (rust-lang/cargo#9203)
- Fix hang on broken stderr. (rust-lang/cargo#9201)
- Make it more clear which module is being tested when running cargo test (rust-lang/cargo#9195)
- Updates to edition handling. (rust-lang/cargo#9184)
- Add --cfg and --rustc-cfg flags to output compiler configuration (rust-lang/cargo#9002)
- Run rustdoc doctests relative to the workspace (rust-lang/cargo#9105)
- Add support for [env] section in .cargo/config.toml (rust-lang/cargo#9175)
- Add schema field and `features2` to the index. (rust-lang/cargo#9161)
- Document the default location where cargo install emitting build artifacts (rust-lang/cargo#9189)
- Do not exit prematurely if anything failed installing. (rust-lang/cargo#9185)
bors added a commit that referenced this pull request Jul 15, 2021
`cargo fix --edition`: extend warning when on latest edition

This extends the warning issued when `cargo fix --edition` is run when the user is already on the latest edition.  Before #9184, there were instructions on what to do, but those probably should not have been completely removed.  It seems likely that some users may get the steps out of order, so this hopefully tries to explain them clearly.
@ehuss ehuss added this to the 1.52.0 milestone Feb 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

cargo fix --edition needs fixing for Rust 2021
5 participants