Simplify the internal API for declaring command-line options #132754

Zalathar · 2024-11-08T01:57:07Z

The internal APIs for declaring command-line options are old, and intimidatingly complex. This PR replaces them with a single function that takes explicit stability and kind arguments, making it easier to see how each option is handled, and whether it is treated as stable or unstable.

We also don't appear to have any tests for the output of rustc --help and similar, so I've added a run-make test to verify that this PR doesn't change any output. (There is already a similar run-make test for rustdoc's help output.)

The librustdoc changes are simply adjusting to updated compiler APIs; no functional change intended.

A side-effect of these changes is that rustfmt can once again format the entirety of these option declaration lists, which it was not doing before.

rustbot · 2024-11-08T01:57:15Z

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
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 · 2024-11-08T01:57:17Z

The run-make-support library was changed

cc @jieyouxu

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

Zalathar · 2024-11-08T04:06:48Z

compiler/rustc_session/src/config.rs

+            "Link the generated crate(s) to the specified native\n\
+                library NAME. The optional KIND can be one of\n\
+                static, framework, or dylib (the default).\n\
+                Optional comma separated MODIFIERS\n\
+                (bundle|verbatim|whole-archive|as-needed)\n\
+                may be specified each with a prefix of either '+' to\n\


IIRC this is one of the entries that used to confuse rustfmt, causing the rest of the list to quietly not be auto-formatted at all.

Replacing the hard line-breaks with \n\ seems to work around that, without changing the output seen by the new run-make test.

tests/run-make/rustc-help/help-v.diff

GuillaumeGomez

Changes look good to me, thanks! I'll let someone from the compiler team confirm it's good on their side as well.

Zalathar · 2024-11-08T11:54:45Z

Hmm, it occurs to me that the help output is slightly different on beta/stable, but only if RUSTC_BOOTSTRAP is not set.

Looking at some of the other run-make tests, it seems that they already pass -Z flags to the compiler without any special nightly-only directives, so hopefully that variable is set and things will be fine? Even if it somehow isn't fine, I guess the worst-case outcome is that someone is slightly inconvenienced when doing the next beta branch.

jieyouxu · 2024-11-09T14:23:21Z

Hmm, it occurs to me that the help output is slightly different on beta/stable, but only if RUSTC_BOOTSTRAP is not set.

Looking at some of the other run-make tests, it seems that they already pass -Z flags to the compiler without any special nightly-only directives, so hopefully that variable is set and things will be fine? Even if it somehow isn't fine, I guess the worst-case outcome is that someone is slightly inconvenienced when doing the next beta branch.

compiletest is AFAIK always run with RUSTC_BOOTSTRAP=1

rust/src/bootstrap/src/core/build_steps/test.rs

Line 2091 in 4b198d6

cmd.env("RUSTC_BOOTSTRAP", "1");

so it shouldn't cause problems beta bumping.

jieyouxu

Thank you, this looks like a significant readibility cleanup for the options handling! One minor question, but otherwise looks good.

jieyouxu · 2024-11-09T15:08:29Z

compiler/rustc_session/src/config.rs

+        opt(
+            Stable,
+            Multi,
+            "",
+            "cfg",
+            "Configure the compilation environment.\n\
+                SPEC supports the syntax `NAME[=\"VALUE\"]`.",
+            "SPEC",
+        ),


Question: what does rustc_short_optgroups actually mean?

Returns the "short" subset of the rustc command line options

Like the options that have a shorthand...? I was trying to figure out what the distinction of rustc_short_optgroups vs rustc_optgroups was lol.

Anyway, this doesn't have to be in this PR (probably for the better).

I believe it means “the subset of options that we print when running rustc --help without -v”.

It’s not a great name!

jieyouxu · 2024-11-09T15:13:40Z

@bors r=GuillaumeGomez,jieyouxu

bors · 2024-11-09T15:13:43Z

📌 Commit b8377e5 has been approved by GuillaumeGomez,jieyouxu

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#132341 (Reject raw lifetime followed by `'`, like regular lifetimes do) - rust-lang#132363 (Enforce that raw lifetimes must be valid raw identifiers) - rust-lang#132744 (add regression test for rust-lang#90781) - rust-lang#132754 (Simplify the internal API for declaring command-line options) - rust-lang#132772 (use `download-rustc="if-unchanged"` as a global default) - rust-lang#132774 (Use lld with non-LLVM backends) - rust-lang#132799 (Make `Ty::primitive_symbol` recognize `str`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#132754 - Zalathar:opts, r=GuillaumeGomez,jieyouxu Simplify the internal API for declaring command-line options The internal APIs for declaring command-line options are old, and intimidatingly complex. This PR replaces them with a single function that takes explicit `stability` and `kind` arguments, making it easier to see how each option is handled, and whether it is treated as stable or unstable. We also don't appear to have any tests for the output of `rustc --help` and similar, so I've added a run-make test to verify that this PR doesn't change any output. (There is already a similar run-make test for rustdoc's help output.) --- The librustdoc changes are simply adjusting to updated compiler APIs; no functional change intended. --- A side-effect of these changes is that rustfmt can once again format the entirety of these option declaration lists, which it was not doing before.

Remove `rustc_session::config::rustc_short_optgroups` Follow-up to rust-lang#132754 (comment). The name `rustc_short_optgroups` has always been confusing, because it is unrelated to the distinction between short and long options (i.e. `-s` vs `--long`), and instead means something like “the subset of command-line options that are printed by `rustc --help` without `-v`”. So let's merge that function into the main `rustc_optgroups`, and store the relevant bit of information in a boolean field in `RustcOptGroup` instead. --- This PR also modifies `RustcOptGroup` to store its various strings directly, instead of inside a boxed `apply` closure. That turned out to not be necessary for the main change, but is a worthwhile cleanup in its own right.

Rollup merge of rust-lang#132891 - Zalathar:short-opt-groups, r=jieyouxu Remove `rustc_session::config::rustc_short_optgroups` Follow-up to rust-lang#132754 (comment). The name `rustc_short_optgroups` has always been confusing, because it is unrelated to the distinction between short and long options (i.e. `-s` vs `--long`), and instead means something like “the subset of command-line options that are printed by `rustc --help` without `-v`”. So let's merge that function into the main `rustc_optgroups`, and store the relevant bit of information in a boolean field in `RustcOptGroup` instead. --- This PR also modifies `RustcOptGroup` to store its various strings directly, instead of inside a boxed `apply` closure. That turned out to not be necessary for the main change, but is a worthwhile cleanup in its own right.

…eyouxu Simplify the internal API for declaring command-line options The internal APIs for declaring command-line options are old, and intimidatingly complex. This PR replaces them with a single function that takes explicit `stability` and `kind` arguments, making it easier to see how each option is handled, and whether it is treated as stable or unstable. We also don't appear to have any tests for the output of `rustc --help` and similar, so I've added a run-make test to verify that this PR doesn't change any output. (There is already a similar run-make test for rustdoc's help output.) --- The librustdoc changes are simply adjusting to updated compiler APIs; no functional change intended. --- A side-effect of these changes is that rustfmt can once again format the entirety of these option declaration lists, which it was not doing before.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#132341 (Reject raw lifetime followed by `'`, like regular lifetimes do) - rust-lang#132363 (Enforce that raw lifetimes must be valid raw identifiers) - rust-lang#132744 (add regression test for rust-lang#90781) - rust-lang#132754 (Simplify the internal API for declaring command-line options) - rust-lang#132772 (use `download-rustc="if-unchanged"` as a global default) - rust-lang#132774 (Use lld with non-LLVM backends) - rust-lang#132799 (Make `Ty::primitive_symbol` recognize `str`) r? `@ghost` `@rustbot` modify labels: rollup

Remove `rustc_session::config::rustc_short_optgroups` Follow-up to rust-lang#132754 (comment). The name `rustc_short_optgroups` has always been confusing, because it is unrelated to the distinction between short and long options (i.e. `-s` vs `--long`), and instead means something like “the subset of command-line options that are printed by `rustc --help` without `-v`”. So let's merge that function into the main `rustc_optgroups`, and store the relevant bit of information in a boolean field in `RustcOptGroup` instead. --- This PR also modifies `RustcOptGroup` to store its various strings directly, instead of inside a boxed `apply` closure. That turned out to not be necessary for the main change, but is a worthwhile cleanup in its own right.

Zalathar added 4 commits November 8, 2024 12:46

Add a run-make test for rustc --help and similar

8f7f9b9

Use a method to apply RustcOptGroup to getopts::Options

584c820

Simplify command-line-option declarations in the compiler

001013c

Simplify command-line-argument declarations in librustdoc

b8377e5

rustbot assigned GuillaumeGomez Nov 8, 2024

Zalathar commented Nov 8, 2024

View reviewed changes

jieyouxu self-assigned this Nov 8, 2024

GuillaumeGomez reviewed Nov 8, 2024

View reviewed changes

tests/run-make/rustc-help/help-v.diff Show resolved Hide resolved

GuillaumeGomez approved these changes Nov 8, 2024

View reviewed changes

jieyouxu approved these changes Nov 9, 2024

View reviewed changes

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 Nov 9, 2024

matthiaskrgr mentioned this pull request Nov 9, 2024

Rollup of 7 pull requests #132815

Merged

bors merged commit 88acd49 into rust-lang:master Nov 9, 2024
6 checks passed

rustbot added this to the 1.84.0 milestone Nov 9, 2024

Zalathar deleted the opts branch November 9, 2024 22:59

GuillaumeGomez mentioned this pull request Nov 10, 2024

rustdoc book: acknowledge --document-hidden-items #132876

Open

Zalathar mentioned this pull request Nov 11, 2024

Remove rustc_session::config::rustc_short_optgroups #132891

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Simplify the internal API for declaring command-line options #132754

Simplify the internal API for declaring command-line options #132754

Zalathar commented Nov 8, 2024 •

edited

Loading

rustbot commented Nov 8, 2024

rustbot commented Nov 8, 2024

Zalathar Nov 8, 2024 •

edited

Loading

GuillaumeGomez left a comment

Zalathar commented Nov 8, 2024

jieyouxu commented Nov 9, 2024 •

edited

Loading

jieyouxu left a comment

jieyouxu Nov 9, 2024 •

edited

Loading

jieyouxu Nov 9, 2024

Zalathar Nov 9, 2024

jieyouxu commented Nov 9, 2024

bors commented Nov 9, 2024

Simplify the internal API for declaring command-line options #132754

Simplify the internal API for declaring command-line options #132754

Conversation

Zalathar commented Nov 8, 2024 • edited Loading

rustbot commented Nov 8, 2024

rustbot commented Nov 8, 2024

Zalathar Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

GuillaumeGomez left a comment

Choose a reason for hiding this comment

Zalathar commented Nov 8, 2024

jieyouxu commented Nov 9, 2024 • edited Loading

jieyouxu left a comment

Choose a reason for hiding this comment

jieyouxu Nov 9, 2024 • edited Loading

Choose a reason for hiding this comment

jieyouxu Nov 9, 2024

Choose a reason for hiding this comment

Zalathar Nov 9, 2024

Choose a reason for hiding this comment

jieyouxu commented Nov 9, 2024

bors commented Nov 9, 2024

Zalathar commented Nov 8, 2024 •

edited

Loading

Zalathar Nov 8, 2024 •

edited

Loading

jieyouxu commented Nov 9, 2024 •

edited

Loading

jieyouxu Nov 9, 2024 •

edited

Loading