Repository navigation
Conversation
|
Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @llogiq (or someone else) some time within the next two weeks. Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (
Why was this reviewer chosen?The reviewer was selected based on:
|
|
Lintcheck changes for f928210
This comment will be updated if you push new changes |
c96e11b to
1540858
Compare
|
Hello @mehbark, mind if we use your PR for some teaching? |
This comment has been minimized.
This comment has been minimized.
changelog: [`blanket_allow_warnings`]: new lint
Co-authored-by: Daniel Scherzer <daniel.e.scherzer@gmail.com>
|
Some changes occurred in clippy_lints/src/doc cc @notriddle |
|
This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ca284d2 to
f928210
Compare
There was a problem hiding this comment.
Some questions about this lint that I don't know the answers to:
Is this even something we want?
What's the right name for this?
What category is this?
Should this be extended to check for #[allow(clippy)]?
Would a customizable allow/except denylist be better?
How do we issue an "unsilenceable" warning instead of an error?
Also the error messages, documentation, and testing could be better.
I kinda like the lint idea. I think it being an optional lint would help environments, where there needs to be stricter control over the amount of allowed lints to go through - especially in CI scenarios.
I think the name is fine? Unless there are other opinions :D
The category I would actually suggest maybe the clippy::suspicious one, it really does feel like something is off if someone has to allow an entire category blanked file.
Customizable allow and deny list would be very nice, however I do see value (from my work experience) of a global ban on allow warnings, especially for CI/CD tests.
I'm not quite sure how do we issue an unsilenceable warning yet, I think @blyxyas would know much better than me in this case 😶
I also ran this across the lintcheck groups and there is indeed just one occurence of this lint being applied - in the regex crate in the builders.rs file on line 1:1 where someone actually used this. I think the impact on regular crates is therefore minimal and could work well for those who opt-in as an additional safeguard.
I tried the lint with some macros and edge cases, from what I've tried it seemed to work nice.
I think so far just removing the TODO and maybe some additional comments would be nice, after that's done I will approve, but of course the final approval is on @blyxyas.
Thanks!
| pub(super) fn check<'cx>(cx: &EarlyContext<'cx>, name: Symbol, items: &[MetaItemInner], attr: &'cx Attribute) { | ||
| for lint in items { | ||
| // Check if the attribute is in an external macro and therefore out of the developer's control | ||
| // TODO?: (taken from `allow_attributes_without_reason`; should this be an `attrs` util? ) |
|
☔ The latest upstream changes (possibly #17678) made this pull request unmergeable. Please resolve the merge conflicts. |
Based on #1648, this lint checks for
alloworexpectattributes with thewarningsgroup; for example,#![allow(warnings)]instead of#![allow(non_snake_case)].This is generally suspicious (though perhaps not the literal category), especially if you're using clippy. The use of "blanket" comes from
blanket_clippy_restriction_lints.Some questions about this lint that I don't know the answers to:
#[allow(clippy)]?allow/exceptdenylist be better?Also the error messages, documentation, and testing could be better.
changelog: [
blanket_allow_warnings]: new lint