Unstable book options parser#154070
Conversation
|
Some changes occurred in compiler/rustc_attr_parsing cc @jdonszelmann, @JonathanBrouwer Some changes occurred in compiler/rustc_passes/src/check_attr.rs |
|
rustbot has assigned @petrochenkov. Use Why was this reviewer chosen?The reviewer was selected based on:
|
This comment has been minimized.
This comment has been minimized.
6d964c6 to
96b9265
Compare
This comment has been minimized.
This comment has been minimized.
96b9265 to
d5f2c91
Compare
| next_idx: usize, | ||
| } | ||
|
|
||
| fn parse_compiler_flags(options_rs: &str, options_path: &Path) -> Features { |
There was a problem hiding this comment.
is there any way you can use syn here instead of open-coding it?
There was a problem hiding this comment.
alternatively, maybe we could move to the options to a separate file, use include! in the existing rust file, and then require them to be in a simpler format that's easier for syn to parse?
There was a problem hiding this comment.
Hi @jyn514 ! Currently I am parsing the whole file as a string, I will have a look at syn and report back!
There was a problem hiding this comment.
Parsing is much more straightforward with syn. Here’s my first attempt and I’d appreciate any feedback.
|
r? rustdoc I guess |
|
r? bootstrap |
b1a4466 to
deeb45a
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
|
It seems a bit unfortunate that we're essentially implementing this macro twice (once as a decl macro, once as a Syn parser) but it's probably worth the benefit. Can we update the |
| return Err(syn::Error::new( | ||
| name.span(), | ||
| format!( | ||
| "unexpected field count for option `{name}`: expected 4 or 5, found {}", |
There was a problem hiding this comment.
| "unexpected field count for option `{name}`: expected 4 or 5, found {}", | |
| "unexpected field count for option `{name}`: expected {REQUIRED_FIELDS} or {OPTIONAL_FIELDS}, found {}", |
|
These commits modify the If this was unintentional then you should revert the changes before this PR is merged. |
That's right. That's a concern to keep in mind. Should we consider adding a test that parses |
|
I believe |
Rollup of 10 pull requests Successful merges: - #154070 (Unstable book options parser) - #154371 (Use LocalDefId for more tcx method calls) - #154405 (Improve doc comment unicode guidance) - #154431 (Avoid ICE in explicit reference cast suggestion for unrelated leaf pr…) - #153528 (Fix LegacyKeyValueFormat report from docker build: mips) - #154246 (Add test for issue #101532: dead code warnings in const _) - #154421 (Rustdoc rejects html emits with json output) - #154428 (bootstrap: `-Zjson-target-spec` for synthetic targets) - #154437 (bootstrap.example.toml: Hint how to allow `build.warnings`) - #154454 (fix: [rustfmt] prevent panic when rewritng associated item delegations) Failed merges: - #154450 (Use the normal arg-parsing machinery for `-Zassert-incr-state`)
Rollup of 10 pull requests Successful merges: - #154070 (Unstable book options parser) - #154371 (Use LocalDefId for more tcx method calls) - #154405 (Improve doc comment unicode guidance) - #154431 (Avoid ICE in explicit reference cast suggestion for unrelated leaf pr…) - #153528 (Fix LegacyKeyValueFormat report from docker build: mips) - #154246 (Add test for issue #101532: dead code warnings in const _) - #154421 (Rustdoc rejects html emits with json output) - #154428 (bootstrap: `-Zjson-target-spec` for synthetic targets) - #154437 (bootstrap.example.toml: Hint how to allow `build.warnings`) - #154454 (fix: [rustfmt] prevent panic when rewritng associated item delegations) Failed merges: - #154450 (Use the normal arg-parsing machinery for `-Zassert-incr-state`)
Rollup merge of #154070 - mehdiakiki:unstable-book-options-parser, r=clubby789 Unstable book options parser Parses the `options!` macro in `compiler/rustc_session/src/options.rs` directly to extract the unstable (-Z) compiler flag names and descriptions to generate documentation for the unstable book. I took notice from the previous attempt which ran `rustc -Zhelp` and parsed the output and used this approach that reads the source directly. Used claude for the tedious char by char parsing parts but verified the code, I hope that's ok!
|
I'm unhappy to see that this was merged, as it seems unreasonably burdensome to maintenance of unstable options. |
Revert "Unstable book options parser" - Reverts rust-lang#154070 This reverts commit 6fd8466, reversing changes made to fda6d37. --- The changes in rust-lang#154070 are well-intentioned, but in their current form they place an unreasonable maintenance burden on the affected part of the compiler, out of proportion to the benefit gained. - Moving the unstable options to a separate `include!` file is marginally more convenient for the unstable-book-gen tool, but a big hassle for compiler contributors looking to read or modify the list of unstable options. It breaks normal file layout conventions, interferes with navigation, and interferes with IDE features, all without ever being truly necessary. - The extra `syn`-based parser in unstable-book-gen is quite complicated, almost entirely undocumented, and seemingly buggy. That makes it extremely difficult to understand or modify, which in turn makes it effectively impossible to ever change the `options!` macro on the compiler side. - The new tests in unstable-book-gen don't appear to be hooked up to run in CI. - The compiler changes don't appear to have been approved by a T-compiler reviewer. Before contemplating a revert, I tried to put together a fix-forward to resolve or mitigate some of these issues. But unstable-book-gen in its current state makes that impractical, so unfortunately I believe that the only way forward is to revert the original changes. --- I do think the reverted functionality is potentially useful, and I'm not fundamentally opposed to a revised version of it landing in the future, if the relevant concerns are addressed. But I feel strongly that the current version does not justify the real and troublesome maintenance burden that it imposes on compiler contributors.
|
Wouldn't a much easier alternative be to just add a new perma-unstable |
|
Then we have to build the compiler before building the unstable book. I guess that’s not the end of the world. |
Rollup merge of #154488 - Zalathar:unstable, r=jieyouxu Revert "Unstable book options parser" - Reverts #154070 This reverts commit 6fd8466, reversing changes made to fda6d37. --- The changes in #154070 are well-intentioned, but in their current form they place an unreasonable maintenance burden on the affected part of the compiler, out of proportion to the benefit gained. - Moving the unstable options to a separate `include!` file is marginally more convenient for the unstable-book-gen tool, but a big hassle for compiler contributors looking to read or modify the list of unstable options. It breaks normal file layout conventions, interferes with navigation, and interferes with IDE features, all without ever being truly necessary. - The extra `syn`-based parser in unstable-book-gen is quite complicated, almost entirely undocumented, and seemingly buggy. That makes it extremely difficult to understand or modify, which in turn makes it effectively impossible to ever change the `options!` macro on the compiler side. - The new tests in unstable-book-gen don't appear to be hooked up to run in CI. - The compiler changes don't appear to have been approved by a T-compiler reviewer. Before contemplating a revert, I tried to put together a fix-forward to resolve or mitigate some of these issues. But unstable-book-gen in its current state makes that impractical, so unfortunately I believe that the only way forward is to revert the original changes. --- I do think the reverted functionality is potentially useful, and I'm not fundamentally opposed to a revised version of it landing in the future, if the relevant concerns are addressed. But I feel strongly that the current version does not justify the real and troublesome maintenance burden that it imposes on compiler contributors.
Parses the
options!macro incompiler/rustc_session/src/options.rsdirectly to extract the unstable (-Z) compiler flag names and descriptions to generate documentation for the unstable book.I took notice from the previous attempt which ran
rustc -Zhelpand parsed the output and used this approach that reads the source directly.Used claude for the tedious char by char parsing parts but verified the code, I hope that's ok!