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

detect allow_attributes on internal attributes #13643

Closed

Conversation

jdonszelmann
Copy link
Contributor

I was working on attributes, part of which is that I might remove AttrStyle here. Don't worry about that, but it meant I was refactoring this and I saw no reason we don't lint on internal attributes. Is there a good reason? Then feel free to close :)

changelog:

changelog: [allow_attributes]: lint internal attributes too

@rustbot
Copy link
Collaborator

rustbot commented Nov 1, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @xFrednet (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 (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 1, 2024
@xFrednet
Copy link
Member

xFrednet commented Nov 1, 2024

Hey king, welcome to Clippy or at least the Clippy repo ^^

This is intentional according to the lint description:

This lint only warns outer attributes (#[allow]), as inner attributes (#![allow]) are usually used to enable or disable lints on a global scale.

This was just a wild guess during the implementation, but it seems to mostly hold true. Especially on the root module I'd say it's likely that people just add #![allows] according to their style opposed to what lints actually get triggered.

The nicest solution would be to add a configuration for it. If you're interested here are the docs: https://doc.rust-lang.org/clippy/development/adding_lints.html#adding-configuration-to-a-lint

@jdonszelmann
Copy link
Contributor Author

hi Fred! well, that makes sense I should've read. As we discussed in private, maybe make this an early pass lint, though it seems hard because cfg() hasn't been processed at that point yet? Consider this PR to be retracted I guess, I was to eager to delete something

@xFrednet
Copy link
Member

xFrednet commented Nov 2, 2024

There are two places that EarlyLintPasses can hook into: LintStore::register_pre_expansion_pass() and LintStore::register_early_pass(). Clippy almost exclusively uses register_early_pass which happens after macros and cfg's have been expanded. So you shouldn't encounter or need to parse any cfg attributes at this stage. If you do, you can assume that they're true and just ignore them :D

@bors
Copy link
Contributor

bors commented Nov 7, 2024

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

@xFrednet
Copy link
Member

xFrednet commented Nov 7, 2024

I think we can close this one, since the main motivation was the usage of the attribute style and with it now being/becoming an early lint we can keep this check :D

@jdonszelmann
Copy link
Contributor Author

Oops, thought it already was closed. Yes, this pr is wrong and closing is correct

@xFrednet xFrednet closed this Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants