Skip to content
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

Lint to warn on use of #[deny(lint)] in code #13963

Open
epage opened this issue Jan 8, 2025 · 4 comments
Open

Lint to warn on use of #[deny(lint)] in code #13963

epage opened this issue Jan 8, 2025 · 4 comments
Labels
A-lint Area: New lints

Comments

@epage
Copy link

epage commented Jan 8, 2025

What it does

Warn on use of #[deny(lint)] / #![deny(lint)]

Advantage

  • Avoid builds breaking due to a new Rust toolchain changing the behavior of a lint
  • Give developers flexibility to be sloppy during development, resolving out the lints before merging (generally being enforced in CI)

Drawbacks

  • It becomes awkward to allow this warning. A parent scope can do it. Unsure if you could in the same scope with !. Cargo.toml [lints] could also

Example

#![deny(elided-lifetimes-in-paths)]

pub struct Foo<'a> {}

fn foo(f: Foo) {}

Could be written as:

#![warn(elided-lifetimes-in-paths)]

pub struct Foo<'a> {}

fn foo(f: Foo) {}

with the suggestion to turn warnings into errors where relevant (e.g. CI)

@epage epage added the A-lint Area: New lints label Jan 8, 2025
@y21
Copy link
Member

y21 commented Jan 8, 2025

IMO there are a lot of perfectly valid and good use cases for local denys, e.g. #[deny(clippy::unwrap_used)] on a GlobalAlloc impl where unwinding would be undefined behavior, or #[deny(clippy::missing_trait_methods)] on a trait impl for a wrapper type that must delegate all method calls to the wrapped type. In those cases it seems right to outright deny builds and not allow that at all even during development because it leads to wrong behavior.
I don't think those use cases should have to allow a lint.

@epage
Copy link
Author

epage commented Jan 8, 2025

How is that different than warn and then denying?

The only reason I could see for putting those in is to instead forbid

@y21
Copy link
Member

y21 commented Jan 8, 2025

Can you explain what you mean by "and then denying"? Like, #[warn(clippy::unwrap_used)] and then -Dclippy::unwrap_used on the command line? That globally denies the restriction lint for the whole crate when it's only relevant for one specific impl. Or -Dwarnings? That will make all warnings into errors, not that one particular warning.

I have my editor set up to run clippy on save, so #[deny] would be used in that situation to have clippy treat that specific instance more like a compiler error and so that it stands out from e.g. dead_code which can be mostly ignored during prototyping/quick edit-run cycles.

Also, forbid isn't exactly right here either; I might have a catch_unwind and then unwrapping should be fine, so I may want to still #[expect] the lint which wouldn't work with forbid.

@epage
Copy link
Author

epage commented Jan 8, 2025

So if I'm understanding correctly, these cases are important enough that you want immediate, blocking feedback on, rather than seeing it in the list of warnings and deciding to act on it?

Maybe I'm biased by my own workflows or the types of projects I work on but I've never needed to do that and I suspect it would be limited if I did step into areas you mentioned, like GlobalAlloc, that this doesn't take away from the value of this proposed lint but either suggests that people may need to occasionally allow . It could be allow by default but I think that would remove most of the benefit as this would be helping people discover a common, happy path for development rather than having to re-discover that deny shouldn't generally be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-lint Area: New lints
Projects
None yet
Development

No branches or pull requests

2 participants