Overhaul RUSTFLAGS handling for `compiler/` crates #138035

nnethercote · 2025-03-05T05:35:40Z

Currently some flags are put into RUSTFLAGS for all compiler/ crates, but RUSTFLAGS is ignored for proc macro crates.

Also, most compiler crates have #![warn(unreachable_pub)] in them, but a few are missing, and it's easy to overlook if a new crate is added.

This PR introduces a neat way to apply a flag to all compiler/ crates, and uses it to replace all the #![warn(unreachable_pub)] attributes with a single -Wunreachable-pub mention.

r? @jieyouxu

Currently they are passed to the ones satisfying `Mode::Rustc` via RUSTFLAGS. But RUSTFLAGS is ignored for proc macro crates. This commit makes those flags get added directly to the command for proc macro crates.

And fix the new errors in the handful of crates that didn't have a `#![warn(unreachable_pub)]`.

It's no longer necessary now that `-Wunreachable_pub` is being passed.

rustbot · 2025-03-05T05:35:47Z

Some changes occurred to MIR optimizations

cc @rust-lang/wg-mir-opt

Some changes occurred in exhaustiveness checking

cc @Nadrieril

rustc_error_messages was changed

cc @davidtwco, @compiler-errors, @TaKO8Ki

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

Some changes occurred in compiler/rustc_attr_data_structures

cc @jdonszelmann

Some changes occurred to the CTFE machinery

cc @RalfJung, @oli-obk, @lcnr

Some changes occurred in compiler/rustc_attr_parsing

cc @jdonszelmann

Some changes occurred in compiler/rustc_sanitizers

cc @rust-lang/project-exploit-mitigations, @rcvalle

nnethercote · 2025-03-05T05:35:50Z

Best reviewed one commit at a time.

nnethercote · 2025-03-05T05:39:25Z

#137930 is another PR where this new functionality will immediately be useful.

rust-log-analyzer · 2025-03-05T05:51:51Z

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)

    Checking termcolor v1.4.1
    Checking home v0.5.9
    Checking build_helper v0.1.0 (/checkout/src/build_helper)
    Checking xz2 v0.1.7
error: constants have by default a `'static` lifetime
    |
    |
118 | pub const FLAGS_FOR_RUSTC: &[&'static str] = &[
    |                              -^^^^^^^---- help: consider removing `'static`: `&str`
    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#redundant_static_lifetimes
    = note: `-D clippy::redundant-static-lifetimes` implied by `-D warnings`
    = help: to override `-D warnings` add `#[allow(clippy::redundant_static_lifetimes)]`

jieyouxu · 2025-03-05T06:19:39Z

r? @onur-ozkan (for the rustc wrapper bit, I'd appreciate a second pair of eyes, or reroll

RalfJung · 2025-03-05T06:51:59Z

src/bootstrap/src/bin/rustc.rs

@@ -145,6 +145,35 @@ fn main() {
        cmd.arg("-Z").arg("on-broken-pipe=kill");
    }

+    // In `cargo.rs` we add the `FLAGS_FOR_RUSTC` flags for all `compiler/`
+    // crates to RUSTFLAGS. However, RUSTFLAGS is ignored for `compiler/` proc
+    // macro crates, because RUSTFLAGS is ignored when `--target` is given. So,


RUSTFLAGS is ignored when --target is given

That's not quite true, it is not ignored. But it only applies to target crates, not to host crates.

RalfJung · 2025-03-05T06:52:50Z

src/bootstrap/src/bin/rustc.rs

+    let crate_type = parse_value_from_args(&orig_args, "--crate-type");
+    if crate_type == Some("proc-macro") {
+        if let Ok(rustflags) = env::var("RUSTFLAGS") {
+            // Count how many of flags from `FLAGS_FOR_RUSTC` are in RUSTFLAGS.


Why do we have to use a fragile hacky heuristic like that? That shouldn't be needed. Bootstrap should clearly tell the wrapper what to add where, we should never do guesswork like this.

I know very little about bootstrap and this was something I managed to get working. I'm happy to hear other ideas and implement them but I'll need a more detailed explanation than "bootstrap should clearly tell the wrapper" - how and where would this be done? :)

I was imagining something like this, which already exists:

rust/src/bootstrap/src/bin/rustc.rs

Lines 138 to 140 in c85d3f6

if let Ok(lint_flags) = env::var("RUSTC_LINT_FLAGS") {

cmd.args(lint_flags.split_whitespace());

}

From some of the other discussion it sounds like what you dislike about this approach is that it will pass the flags to all invocations, not just the ones for workspace crates. However, non-workspace crates run with --cap-lints so that should be entirely non-consequential?

The discussion about this seems to be spread across multiple Zulip topics, issues, and PRs, so I have no idea which options have been considered. Has RUSTC_LINT_FLAGS been discarded for some reason?

This thread has most of the discussion.

I didn't even know RUSTC_LINT_FLAGS was a thing until just now. Like I said above (and in the thread), I know very little about bootstrap and I've tried several different approaches based on suggestions people have made at different times.

That's fair -- I just thought bootstrap people were involved in the discussion / review so I'm a bit surprised it took so long for someone to suggest this. Let's make sure to include bootstrap people in the next PR.

I didn't know this env var existed either, but adding something like it was literally the first thing that came to my mind ("bootstrap should tell the wrapper to add some flags"), and then going over the rustc.rs file it took no time to see that flag. I guess I am underestimating how fragmented bootstrap knowledge is.

@jieyouxu, @onur-ozkan, and @Kobzol have all been involved in the various discussions and PRs. That's half the bootstrap reviewers listed in triagebot.toml. @jieyouxu also started a thread last week on t-infra/bootstrap to ask for additional ideas.

I guess I am indeed underestimating how fragmented bootstrap knowledge is.

I also had no idea about the flag :) bootstrap's build logic is distributed amongst several places, it's full of hacks and edge cases, and hidden invariants that can break when you modify completely unrelated code.

As I said, I didn't know about the flag either. I was about to suggest to add something like this flag (it seemed a lot cleaner than what this PR does), went looking to the sources for inspiration on how to call it, and then saw that it already existed.

jieyouxu · 2025-03-05T06:54:07Z

The rustc wrapper approach seems okay to me, but would appreicate a double-check since last time I thought it was okay to do some RUSTFLAGS adjustments in the wrapper I broke things :D

src/bootstrap/src/utils/shared_helpers.rs

petrochenkov · 2025-03-05T12:29:41Z

This PR introduces a neat way to apply a flag to all compiler/ crates,

IIRC, this was done several times already (I personally did it at least once), but it always gets broken after some time by bootstrap changes - for proc macros, for build scripts, for rtstartup, for bootstrap itself.
I wish there was a way to test this somehow, to ensure that it doesn't regress.

onur-ozkan · 2025-03-06T05:57:01Z

This PR introduces a neat way to apply a flag to all compiler/ crates,

IIRC, this was done several times already (I personally did it at least once), but it always gets broken after some time by bootstrap changes - for proc macros, for build scripts, for rtstartup, for bootstrap itself. I wish there was a way to test this somehow, to ensure that it doesn't regress.

One way to do this is by unconditionally passing an arbitrary flag and asserting its presence in each compiler crate. Ugly, but it could work I think.

jieyouxu · 2025-03-06T16:39:12Z

Bootstrap thread: https://rust-lang.zulipchat.com/#narrow/channel/326414-t-infra.2Fbootstrap/topic/Consistent.20handling.20of.20compiler.2F*.20RUSTFLAGs.20and.20lints

bors · 2025-03-09T16:27:58Z

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

nnethercote · 2025-03-11T02:16:41Z

After the failure of the workspace lints approach (merged in #138084, reverted in #138304 for breaking miri and other external tools) I have implemented a RUSTC_LINT_FLAGS-based approach in #138331 which has the light footprint of this PR (i.e. doesn't touch many lines of code) while being much less hacky.

nnethercote added 3 commits March 5, 2025 16:03

Pass certain flags when building all compiler/ crates.

5a2e69c

Currently they are passed to the ones satisfying `Mode::Rustc` via RUSTFLAGS. But RUSTFLAGS is ignored for proc macro crates. This commit makes those flags get added directly to the command for proc macro crates.

Add -Wunreachable_pub to FLAGS_FOR_RUSTC.

016e9d7

And fix the new errors in the handful of crates that didn't have a `#![warn(unreachable_pub)]`.

Remove #![warn(unreachable_pub)] from all compiler/ crates.

e14bcdc

It's no longer necessary now that `-Wunreachable_pub` is being passed.

rustbot assigned jieyouxu Mar 5, 2025

rustbot assigned onur-ozkan and unassigned jieyouxu Mar 5, 2025

jieyouxu self-assigned this Mar 5, 2025

RalfJung reviewed Mar 5, 2025

View reviewed changes

cuviper reviewed Mar 5, 2025

View reviewed changes

src/bootstrap/src/utils/shared_helpers.rs Show resolved Hide resolved

bjorn3 mentioned this pull request Mar 5, 2025

Bootstrap does not set cfg(bootstrap) for build scripts #138032

Open

This was referenced Mar 6, 2025

Investigate and implement a scheme to consistently apply RUSTFLAGS and lints for all compiler crates #138106

Open

Use workspace lints for crates in compiler/ #138084

Merged

jieyouxu added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 6, 2025

nnethercote closed this Mar 11, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Overhaul RUSTFLAGS handling for `compiler/` crates #138035

Overhaul RUSTFLAGS handling for `compiler/` crates #138035

nnethercote commented Mar 5, 2025

rustbot commented Mar 5, 2025

nnethercote commented Mar 5, 2025

nnethercote commented Mar 5, 2025

rust-log-analyzer commented Mar 5, 2025

jieyouxu commented Mar 5, 2025 •

edited

Loading

RalfJung Mar 5, 2025

RalfJung Mar 5, 2025

nnethercote Mar 5, 2025

RalfJung Mar 10, 2025

nnethercote Mar 10, 2025

RalfJung Mar 11, 2025 •

edited

Loading

nnethercote Mar 11, 2025

RalfJung Mar 11, 2025

Kobzol Mar 11, 2025

RalfJung Mar 11, 2025 •

edited

Loading

jieyouxu Mar 11, 2025

jieyouxu commented Mar 5, 2025

petrochenkov commented Mar 5, 2025

onur-ozkan commented Mar 6, 2025

jieyouxu commented Mar 6, 2025

bors commented Mar 9, 2025

nnethercote commented Mar 11, 2025

	if let Ok(lint_flags) = env::var("RUSTC_LINT_FLAGS") {
	cmd.args(lint_flags.split_whitespace());
	}

Overhaul RUSTFLAGS handling for compiler/ crates #138035

Overhaul RUSTFLAGS handling for compiler/ crates #138035

Conversation

nnethercote commented Mar 5, 2025

rustbot commented Mar 5, 2025

nnethercote commented Mar 5, 2025

nnethercote commented Mar 5, 2025

rust-log-analyzer commented Mar 5, 2025

jieyouxu commented Mar 5, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung Mar 11, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jieyouxu commented Mar 5, 2025

petrochenkov commented Mar 5, 2025

onur-ozkan commented Mar 6, 2025

jieyouxu commented Mar 6, 2025

bors commented Mar 9, 2025

nnethercote commented Mar 11, 2025

Overhaul RUSTFLAGS handling for `compiler/` crates #138035

Overhaul RUSTFLAGS handling for `compiler/` crates #138035

jieyouxu commented Mar 5, 2025 •

edited

Loading

RalfJung Mar 11, 2025 •

edited

Loading

RalfJung Mar 11, 2025 •

edited

Loading