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

Deny warnings in libcore and libstd #57184

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@varkor
Copy link
Member

varkor commented Dec 28, 2018

This probably fixes #57178 (though there may still be some crates that need warnings denied). At least after this change, rustc currently produces no warnings during compilation.

r? @oli-obk

@ollie27

This comment has been minimized.

Copy link
Contributor

ollie27 commented Dec 28, 2018

rustbuild handles denying warnings. If you want to deny warnings when building std at stage 0 then this exception needs to be removed:

rust/src/bootstrap/builder.rs

Lines 999 to 1002 in 79d8a0f

// in std, we want to avoid denying warnings for stage 0 as that makes cfg's painful.
if self.config.deny_warnings && !(mode == Mode::Std && stage == 0) {
cargo.env("RUSTC_DENY_WARNINGS", "1");
}

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 29, 2018

I think we shouldn't remove the exception. I could be convinced that if the max-stage to be built is not 0 then we should do something like -Awarnings but I don't personally care that much.

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 29, 2018

Ah, I didn't realise this was a deliberate decision. What sort of situations does it make painful? In my experience warnings are usually fixed quite quickly: at worst, it's a #[cfg_attr(stage0, ...)] away, which seems better than printing a load of warning messages.

@oli-obk

This comment has been minimized.

Copy link
Contributor

oli-obk commented Dec 29, 2018

There's very little point to these kind of warnings (they'll go away when the next beta comes around). While any other kind of warning will be caught at stage 1, it makes for a bad user experience when modifying libstd and only seeing warnings at stage 1 (Since running tests requires stage 1). So I think we should neither silence the warnings nor deny them and just keep the status quo.

What sort of situations does it make painful? In my experience warnings are usually fixed quite quickly: at worst, it's a #[cfg_attr(stage0, ...)] away, which seems better than printing a load of warning messages.

It's annoying when you change the compiler so that you need to change libstd, too, and run everything with --keep-stage 0, because everything else would take forever. Now when everything works and you go back to building without keep-stage, suddenly everything breaks and you need to add cfgs. These changes are just noise though, since they'll go away with the next boostrap update.

You can also run into error/warning pingpong between stage 0 and stage 1, because the changes you do to get stage 0 to work end up bricking stage 1

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 29, 2018

Those problems could be addressed by adding a config.toml option or compiler flag to disable #[deny(warnings)] on stage 0, which is not used on the testing infrastructure. This would mean that developers would potentially have one extra step at the end of a change — recompiling with stage 0 and conditionally cfging any warnings that did appear, but it would mean the compiler is always warning-free.

The philosophy of not (implicitly) ignoring warnings seems like a sensible one, even if warnings per se shouldn't be as worrying as errors.

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Dec 29, 2018

Adding another option seems fine. I'm pretty neutral on this overall, just don't want to go back to unconditionally denying warnings inside core/std directly (it should be a rustbuild thing).

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Dec 30, 2018

Okay, I'll update the pull request soon.

@varkor varkor force-pushed the varkor:deny-warnings-lib branch from d404af6 to 0e54bdf Jan 3, 2019

@varkor

This comment has been minimized.

Copy link
Member

varkor commented Jan 3, 2019

I've added a allow-stage0-warnings = false config.toml option. This should allow for the existing methodology of ignoring warnings until after development, when the stage 0 warnings can be cfg'd out, but will also mean we don't accidentally permit bad behaviour in stage 0.

r? @Mark-Simulacrum

@varkor varkor assigned Mark-Simulacrum and unassigned oli-obk Jan 3, 2019

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 17, 2019

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment