Skip to content

Lint on deny(warnings), deny(deprecated), and similar #5196

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

Closed
cuviper opened this issue Feb 19, 2020 · 5 comments
Closed

Lint on deny(warnings), deny(deprecated), and similar #5196

cuviper opened this issue Feb 19, 2020 · 5 comments

Comments

@cuviper
Copy link
Member

cuviper commented Feb 19, 2020

Denying all warnings, or even just deprecation warnings, is a forward compatibility hazard, and can also have unintended effects like masking Crater failures. I posted about this on /r/rust, and /u/Luroalive asked me to file an issue to add a clippy lint. It may actually be worth a first-class rustc lint in the future-incompatible group, but we can consider it here first.

GitHub suggested the related issue #287 that was closed, but perhaps it's worth another look with this new context. cc @llogiq @Manishearth

@Manishearth
Copy link
Member

I don't think clippy should handle this, especially since clippy is run on CI in the cases you do want to have all warnings on deny (and clippy can't catch cfgs).

I think this is a crater problem, crater should be capping lints (cc @pietroalbini)

This certainly doesn't qualify as a future incompatibility lint in rustc. Rustc is already permitted to make changes to existing lints that cause breakages, the stability policy for lints is basically "do not radically change their meaning". You have this hazard even without the blanket deny, as long as you have any deny.

@pietroalbini
Copy link
Member

I think this is a crater problem, crater should be capping lints

Crater can indeed do that, by passing cap-lints=warn to the experiment. The release team does that for beta runs.

@cuviper
Copy link
Member Author

cuviper commented Feb 19, 2020

FWIW, @llogiq also shared a rust-unofficial/patterns page about this:
https://github.com/rust-unofficial/patterns/blob/master/anti_patterns/deny-warnings.md

This certainly doesn't qualify as a future incompatibility lint in rustc. Rustc is already permitted to make changes to existing lints that cause breakages, the stability policy for lints is basically "do not radically change their meaning". You have this hazard even without the blanket deny, as long as you have any deny.

True, but deprecated and blanket warnings are even less stable than most, as they will inherently increase in scope over time.

Crater can indeed do that, by passing cap-lints=warn to the experiment. The release team does that for beta runs.

Aha, and the test failure in reem/rust-mac#15 did show up in rust-lang/rust#66244 (comment), but AFAICS nobody triaged that one.

Still, as noted on reddit, it's not always desirable for Crater to cap lints, because some problems may depend on the behavior of default-denied lints.

@llogiq
Copy link
Contributor

llogiq commented Feb 19, 2020

As I wrote on the antipattern page, to deny a warning (group) is to opt out of stability around that warning (group) altogether, so one takes responsibility to check if the build still works with each new version. Depending on the target audience, that may be every nightly.

I don't think we need a clippy lint here. People who do it will feel the self-inflicted pain sooner or later anyway.

@Manishearth
Copy link
Member

Still, as noted on reddit, it's not always desirable for Crater to cap lints, because some problems may depend on the behavior of default-denied lints.

I don't think we have a stability policy for default denied lints either.

I think it used to be an antipattern, I'm not sure if it still is given that cap-lints is the norm.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants