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

Make weird name lints trigger behind cfg_attr #97266

Merged
merged 2 commits into from
May 25, 2022

Conversation

est31
Copy link
Member

@est31 est31 commented May 22, 2022

The weird name lints (unknown_lints, renamed_and_removed_lints), the lints that lint the linting, were previously not firing for lint level declarations behind cfg_attr, as they were only running before expansion.

Now, this will give a unknown_lints warning:

#[cfg_attr(all(), allow(this_lint_does_not_exist))]
fn foo() {}

Lint level declarations behind a cfg_attr whose condition is not applying are still ignored. So this still won't give a warning:

#[cfg_attr(any(), allow(this_lint_does_not_exist))]
fn foo() {}

Furthermore, this PR also makes the weird name lints respect level delcarations for them that were hidden by cfg_attr, making them consistent to other lints. So this will now not issue a warning:

#[cfg_attr(all(), allow(unknown_lints))]
mod foo {
    #[allow(does_not_exist)]
    fn foo() {
    }
}

Fixes #97094

@rustbot rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label May 22, 2022
@rust-highfive
Copy link
Collaborator

r? @lcnr

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 22, 2022
@rust-log-analyzer

This comment has been minimized.

@lcnr
Copy link
Contributor

lcnr commented May 23, 2022

Do we want that, considering that in the future one might write #[cfg_attr(version = "1.70", allow(some_new_lint))]? Or am I misunderstanding something here

@est31
Copy link
Member Author

est31 commented May 23, 2022

@lcnr it will only lint if the condition inside the cfg evaluates to true. If the condition is false, it won't lint. See the last entry in the added test. I've also edited the PR description to make this clearer. Making it peek inside all cfg_attr regardless of whether they apply or not is a possibility, but I agree that it's not a good idea, which is why I didn't chose it for this PR.

@rust-log-analyzer

This comment has been minimized.

@est31 est31 force-pushed the unknown_lints_cfg_attr branch 2 times, most recently from 606c093 to e9f26c0 Compare May 23, 2022 16:21
@est31
Copy link
Member Author

est31 commented May 23, 2022

I'm now using i == 0 because it still works, and have changed the test to ensure that it works with that debug flag enabled. I've also reordered the bodies as requested. re-r? @lcnr

Copy link
Contributor

@lcnr lcnr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one nit, then r=me

compiler/rustc_lint/src/early.rs Show resolved Hide resolved
@est31
Copy link
Member Author

est31 commented May 23, 2022

@lcnr your understanding is correct, yes. To make sure, I've removed the == 0 check locally and tested it, and indeed it emitted the warnings multiple times, so the == 0 is needed. I've added your suggestion to the PR.

@est31
Copy link
Member Author

est31 commented May 24, 2022

re-r? @lcnr

@lcnr
Copy link
Contributor

lcnr commented May 24, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit 7a36d24d93d00569c7907228f96d259f38a6f571 has been approved by lcnr

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 24, 2022
Previously, we were emitting weird name lints (for renamed or unknown lints)
before expansion, most importantly before cfg expansion.
This meant that the weird name lints would not fire
for lint attributes hidden inside cfg_attr. The same applied
for lint level specifications of those lints.

By moving the lints for the lint names to the post-expansion
phase, these issues are resolved.
@est31
Copy link
Member Author

est31 commented May 24, 2022

Sorry for pinging you again, @lcnr I've improved the last two tests a little to not have a cfg_attr inside the module but only a cfg. Now they read #[allow(nonex_lint_fn)]. I think this improves test readability because the thing being tested in those two last tests is the cfg_attr of the module outside. re-r? @lcnr

@lcnr
Copy link
Contributor

lcnr commented May 24, 2022

@bors r+

@bors
Copy link
Contributor

bors commented May 24, 2022

📌 Commit 2a8b60f has been approved by lcnr

bors added a commit to rust-lang-ci/rust that referenced this pull request May 24, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93966 (document expectations for Waker::wake)
 - rust-lang#97266 (Make weird name lints trigger behind cfg_attr)
 - rust-lang#97355 (Remove unused brush image)
 - rust-lang#97358 (Update minifier-rs version to 0.1.0)
 - rust-lang#97363 (Fix a small mistake in `SliceIndex`'s documentation)
 - rust-lang#97364 (Fix weird indentation in continue_keyword docs)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 896a59d into rust-lang:master May 25, 2022
@rustbot rustbot added this to the 1.63.0 milestone May 25, 2022
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Aug 15, 2023
…compiler-errors

Add test for unknown_lints from another file.

This adds a test for rust-lang#84936 which was incidentally fixed via rust-lang#97266. It is a strange issue where `#![allow(unknown_lints)]` at the crate root was not applying to unknown lints that fired in a non-inline-module. I did not dig further into how rust-lang#97266 fixed it, but I did verify it. I couldn't find any existing tests which did anything similar.

Closes rust-lang#84936
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

unknown_lints doesn't trigger in all cfg_attr
6 participants