rustbuild: Cleanup global lint settings#62438
Conversation
rust-highfive
added
the
S-waiting-on-review
Mark-Simulacrum
left a comment
Mark-Simulacrum
left a comment
There was a problem hiding this comment.
r=me with nits resolved
Thanks!
src/bootstrap/bin/rustc.rs
Outdated
| env::var_os("RUSTC_EXTERNAL_TOOL").is_none() { | ||
| cmd.arg("-Dwarnings"); | ||
| cmd.arg("-Drust_2018_idioms"); | ||
| if stage != "0" && crate_name != Some("rustc_version") && // cfg(not(bootstrap)) |
There was a problem hiding this comment.
Can you move the cfg comment ontop of the if and expand it a bit, something like "Internal lints may change, so don't enable them on the bootstrap compiler. Also, only enable them for internal crates."
I think rustc_version is just excluded because it's actually external but happens to fall into the use_internal_crates check, could it be moved into that function and a comment added saying "this is actually an external crate"?
There was a problem hiding this comment.
Internal lints may change, so don't enable them on the bootstrap compiler
The reason is simpler - stage 0 compiler doesn't understand -Drustc::internal since it's too new for it and gives an error.
rustc_version for some reason reports stage == "1" when it's actually built by a stage 0 compiler that errors on -Drustc::internal (this happens during x.py doc, IIRC), it doesn't have issues with the lints themselves.
So it's also a part of the bootstrap problem and we'll be able to remove this exception during the next bootstrap compiler update, that's why it's not in use_internal_crates.
(There are other "false-positive" rustc_* crates which happen to pass the internal lint checks and therefore not listed as exceptions.)
I'll add a comment.
There was a problem hiding this comment.
I'd expect the stage 0 compiler to understand -Drustc::internal now, since we just bumped beta -- should we remove that check then?
There was a problem hiding this comment.
Hm, I think I tried it and it didn't work.
Does stage 0 include #61545 (merged yesterday)?
There was a problem hiding this comment.
Ah, no, that's ~one day too recent :)
| if env::var_os("RUSTC_DENY_WARNINGS").is_some() && | ||
| env::var_os("RUSTC_EXTERNAL_TOOL").is_none() { | ||
| cmd.arg("-Dwarnings"); | ||
| cmd.arg("-Drust_2018_idioms"); |
There was a problem hiding this comment.
It looks like we maybe lost bare_trait_objects here? Or is that included in rust_2018_idioms (it should be, I'd guess)?
There was a problem hiding this comment.
Yes, it's a part of the edition idiom group.
(And it's a part of -Dwarnings too now because it was turned into warn-by-default on both editions.)
|
@bors r=Mark-Simulacrum rollup |
|
📌 Commit 36a5aa8 has been approved by |
bors
added
S-waiting-on-bors
…lacrum rustbuild: Cleanup global lint settings Lint settings do not depend on `if let Some(target) = target` in any way, so they are moved out of that clause. Internal lints now respect `RUSTC_DENY_WARNINGS`. Crate name treatment is cleaned up a bit. cc rust-lang#61545 @flip1995 r? @Mark-Simulacrum
4 participants
Lint settings do not depend on
if let Some(target) = targetin any way, so they are moved out of that clause.Internal lints now respect
RUSTC_DENY_WARNINGS.Crate name treatment is cleaned up a bit.
cc #61545 @flip1995
r? @Mark-Simulacrum