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

Switch rustdoc to Rust 2021 #89075

Closed
wants to merge 3 commits into from
Closed

Conversation

camelid
Copy link
Member

@camelid camelid commented Sep 18, 2021

Closes #89074. 🎉

@camelid camelid added A-edition-2021 Area: The 2021 edition T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Sep 18, 2021
@rust-highfive
Copy link
Collaborator

rustdoc-json-types is a public (although nightly-only) API. Consider changing src/librustdoc/json/conversions.rs instead; otherwise, make sure you update format_version.

cc @CraftSpider,@aDotInTheVoid

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(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 Sep 18, 2021
@camelid
Copy link
Member Author

camelid commented Sep 18, 2021

cc @rust-lang/rustdoc

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member Author

camelid commented Sep 18, 2021

My process for making this change was as follows:

  1. Run RUSTFLAGS='-D rust-2021-compatibility' x build with download-rustc enabled, so it only built rustdoc.
  2. Apply the sole suggestion it gave (ecc041d).
  3. Change edition = "2018" to edition = "2021".
  4. Run x build again. The build completed successfully.

(And now CI is running tests.)

I didn't use cargo fix --edition because I don't know if there's a way to run it in rust-lang/rust given that we use rustbuild.

@camelid
Copy link
Member Author

camelid commented Sep 18, 2021

I don't think we need to wait on the compiler being switched to Rust 2021 because editions are supposed to allow you to use crates with other editions :)

@rust-log-analyzer

This comment has been minimized.

src/librustdoc/doctest.rs Outdated Show resolved Hide resolved
src/tools/rustdoc-themes/Cargo.toml Show resolved Hide resolved
@camelid
Copy link
Member Author

camelid commented Sep 18, 2021

Looks like I'll need to update tidy 😆

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 18, 2021
Once all crates have been switched to Rust 2021, I think this code
should be changed to be similar to the old version but with 2021 instead
of 2018. For now, allow both editions during the transition.
@camelid
Copy link
Member Author

camelid commented Sep 19, 2021

I made the change to tidy in this PR since it's very small and is a blocker, but let me know if you'd rather that I open a separate PR for it.

@camelid camelid added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 19, 2021
@camelid
Copy link
Member Author

camelid commented Sep 19, 2021

Only remaining decision point is #89075 (comment), and then I think this should be good to merge!

@Mark-Simulacrum
Copy link
Member

I think I would prefer to hold this (it doesn't seem that pressing) until I finish up the migration work with bootstrap. I'll likely cherry pick some of your work at least, but I want to make sure we don't mark crates with 2021 if we haven't verified them as OK with the compatibility lint.

I'd also like to follow-up with the rfc 2229 group about the insignificant dtor - #89075 (comment) - because I don't think we should be linting at all in that case. Of course, it's not necessary to block landing migrations on those fixes, but it also seems like we do not gain anything significant from pushing this forward quickly? And we'd get some additional testing if we hold off a little.

@camelid
Copy link
Member Author

camelid commented Sep 19, 2021

Sure, it's not that pressing, but I figured it'd be another way to test the edition. I'm happy to drop the change to rustdoc-themes; all of the other changes I tested with RUSTFLAGS='-D rust-2021-compatibility' before updating the edition field. But I'm also happy to wait for your bootstrap changes if you'd rather do that first.

I'd also like to follow-up with the rfc 2229 group about the insignificant dtor - #89075 (comment) - because I don't think we should be linting at all in that case.

The root variable (config) is of type LangString, which doesn't seem to implement Drop. However, one of the captured fields is a Vec, so maybe that's why it's triggering?

@camelid camelid added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 19, 2021
@camelid
Copy link
Member Author

camelid commented Sep 19, 2021

I'd also like to follow-up with the rfc 2229 group about the insignificant dtor - #89075 (comment) - because I don't think we should be linting at all in that case. Of course, it's not necessary to block landing migrations on those fixes, but it also seems like we do not gain anything significant from pushing this forward quickly? And we'd get some additional testing if we hold off a little.

This is the full error in case it's useful to the WG (in this case, I ran rustc with edition set to 2021, but the error looks the same as when I used edition = "2018"):

error: changes to closure capture in Rust 2021 will affect drop order
    --> src/librustdoc/doctest.rs:966:46
     |
966  |             testfn: test::DynTestFn(Box::new(move || {
     |                                              ^^^^^^^
...
975  |                     config.should_panic,
     |                     ------------------- in Rust 2018, this closure captures all of `config`, but in Rust 2021, it will only capture `config.should_panic`
976  |                     no_run,
977  |                     config.test_harness,
     |                     ------------------- in Rust 2018, this closure captures all of `config`, but in Rust 2021, it will only capture `config.test_harness`
...
981  |                     config.compile_fail,
     |                     ------------------- in Rust 2018, this closure captures all of `config`, but in Rust 2021, it will only capture `config.compile_fail`
982  |                     config.error_codes,
     |                     ------------------ in Rust 2018, this closure captures all of `config`, but in Rust 2021, it will only capture `config.error_codes`
...
1050 |     }
     |     -
     |     |
     |     in Rust 2018, `config` is dropped here, but in Rust 2021, only `config.should_panic` will be dropped here as part of the closure
     |     in Rust 2018, `config` is dropped here, but in Rust 2021, only `config.test_harness` will be dropped here as part of the closure
     |     in Rust 2018, `config` is dropped here, but in Rust 2021, only `config.compile_fail` will be dropped here as part of the closure
     |     in Rust 2018, `config` is dropped here, but in Rust 2021, only `config.error_codes` will be dropped here as part of the closure
     |
     = note: `-D rust-2021-incompatible-closure-captures` implied by `-D rust-2021-compatibility`
     = note: for more information, see <https://doc.rust-lang.org/nightly/edition-guide/rust-2021/disjoint-capture-in-closures.html>
help: add a dummy let to cause `config` to be fully captured
     |
966  ~             testfn: test::DynTestFn(Box::new(move || {
967  +                 let _ = &config;
     |

@Mark-Simulacrum
Copy link
Member

I've filed #89080 to track the specific case we're seeing here.

Sure, it's not that pressing, but I figured it'd be another way to test the edition

For sure! I expect we'll get this merged by the end of the week at the latest, so I don't think it loses us much - I think migrating the crates deps first will also help us test more than the leaves first approach, fwiw. (Since we get far more testing of leaves in e.g. crater runs, but partial migrations of dependencies are less visible).

@camelid
Copy link
Member Author

camelid commented Sep 19, 2021

Sounds good! :)

@Mark-Simulacrum
Copy link
Member

OK, I talked to @arora-aman and it looks like we never finalized the insignificant destructor work by actually applying the attributes to std -- a PR doing that should be forthcoming shortly.

I think it probably makes sense to not try to one off migrate individual crates in the tree -- it'll generate more PRs and ultimately feels like it'll be more hassle than just rolling everything into one PR (which migrates everything except the library/ crates, as there's currently a bug with the panic macro edition handling -- we can't migrate libcore at least yet).

I've posted #89103 to replace this PR -- that rolls in an update to 2021 across almost all in-tree crates.

bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 21, 2021
Migrate in-tree crates to 2021

This replaces rust-lang#89075 (cherry picking some of the commits from there), and closes rust-lang#88637 and fixes rust-lang#89074.

It excludes a migration of the library crates for now (see tidy diff) because we have some pending bugs around macro spans to fix there.

I instrumented bootstrap during the migration to make sure all crates moved from 2018 to 2021 had the compatibility warnings applied first.

Originally, the intent was to support cargo fix --edition within bootstrap, but this proved fairly difficult to pull off. We'd need to architect the check functionality to support running cargo check and cargo fix within the same x.py invocation, and only resetting sysroots on check. Further, it was found that cargo fix doesn't behave too well with "not quite workspaces", such as Clippy which has several crates. Bootstrap runs with --manifest-path ... for all the tools, and this makes cargo fix only attempt migration for that crate. We can't use e.g. --workspace due to needing to maintain sysroots for different phases of compilation appropriately.

It is recommended to skip the mass migration of Cargo.toml's to 2021 for review purposes; you can also use `git diff d6cd2c6 -I'^edition = .20...$'` to ignore the edition = 2018/21 lines in the diff.
@bors
Copy link
Contributor

bors commented Sep 21, 2021

☔ The latest upstream changes (presumably #89103) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid
Copy link
Member Author

camelid commented Sep 21, 2021

Closing now that #89103 has been merged.

@camelid camelid closed this Sep 21, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Sep 28, 2021
Migrate in-tree crates to 2021

This replaces rust-lang#89075 (cherry picking some of the commits from there), and closes rust-lang#88637 and fixes rust-lang#89074.

It excludes a migration of the library crates for now (see tidy diff) because we have some pending bugs around macro spans to fix there.

I instrumented bootstrap during the migration to make sure all crates moved from 2018 to 2021 had the compatibility warnings applied first.

Originally, the intent was to support cargo fix --edition within bootstrap, but this proved fairly difficult to pull off. We'd need to architect the check functionality to support running cargo check and cargo fix within the same x.py invocation, and only resetting sysroots on check. Further, it was found that cargo fix doesn't behave too well with "not quite workspaces", such as Clippy which has several crates. Bootstrap runs with --manifest-path ... for all the tools, and this makes cargo fix only attempt migration for that crate. We can't use e.g. --workspace due to needing to maintain sysroots for different phases of compilation appropriately.

It is recommended to skip the mass migration of Cargo.toml's to 2021 for review purposes; you can also use `git diff d6cd2c6 -I'^edition = .20...$'` to ignore the edition = 2018/21 lines in the diff.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-edition-2021 Area: The 2021 edition S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Switch rustdoc to Rust 2021
7 participants