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.

Sign up for GitHub

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

Jump to bottom

Extend maybe_misused_cfg lint over cfg(test) #11821

Merged
merged 3 commits into from
Nov 17, 2023
Merged

Extend maybe_misused_cfg lint over cfg(test) #11821

merged 3 commits into from
Nov 17, 2023

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Nov 16, 2023 •
edited by blyxyas
Loading

Fixes #11240.

One thought I had is that we could use the levenshtein distance (of 1) to ensure this is indeed test that was targeted. But maybe it's overkill, not sure.

changelog: [maybe_misused_cfg]: Extend lint over cfg(test)

r? @blyxyas

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Nov 16, 2023
@GuillaumeGomez GuillaumeGomez changed the title Extend maybe_misused_cfg lint over cfg(test) Extend maybe_misused_cfg lint over cfg(test) Nov 16, 2023
@GuillaumeGomez GuillaumeGomez force-pushed the misspelled-cfg branch 2 times, most recently from 1966d0d to b9eb3c5 Compare November 16, 2023 12:48
blyxyas
blyxyas requested changes Nov 16, 2023
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment

Choose a reason for hiding this comment

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

Pretty good first iteration (^･o･^)ﾉ”

clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

Thanks for the improvements, updated!

emilk
emilk approved these changes Nov 16, 2023
View reviewed changes
Copy link

@emilk emilk left a comment

Choose a reason for hiding this comment

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

Wonderful ❤️

clippy_lints/src/attrs.rs
Comment on lines +958 to +957
} else if let Some(ident) = meta.ident()
&& matches!(ident.name.as_str(), "tests" | "Test")
Copy link

Choose a reason for hiding this comment

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

Instead of deny-listing tests and Test, would it be possible to allow-list all configs passed in by --cfg plus the built-in ones (test, debug_assertions, …)?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll check for the full list then.

Copy link
Member Author

Choose a reason for hiding this comment

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

So back to this, I'm not sure it's a good approach because you might use a valid cfg that is not passed to the compiler (so it's cfged-out) and in this case it would be considered as an error since this cfg isn't known to clippy. So false-positive.

However on the other hand, it's already possible to have this issue currently, so not sure if we want to dive into this any further.

I think a next iteration on this would be to eventually check for all existing ones (the builtin ones and the ones provided to rustc) but we need to check pros and cons.

Does that sound acceptable?

Copy link

Choose a reason for hiding this comment

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

You are right; catching all cfg errors is definitely a different and much larger issue

Copy link
Member

@pietroalbini pietroalbini Nov 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Isn't that https://doc.rust-lang.org/nightly/unstable-book/compiler-flags/check-cfg.html?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it is. I discovered it afterwards and I linked to the tracking issue in my last comment of this PR.

clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
blyxyas
blyxyas approved these changes Nov 16, 2023
View reviewed changes
Copy link
Member

@blyxyas blyxyas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Just a nit and I think this is merge-ready! (With Emil's review)

clippy_lints/src/attrs.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Member Author

I applied suggestions except one which I think deserves more discussion and should happen in a follow-up PR. What do you think?

@blyxyas
Copy link
Member

blyxyas commented Nov 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Nov 17, 2023

📌 Commit f08037c has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 17, 2023

⌛ Testing commit f08037c with merge 8969c10...

bors added a commit that referenced this pull request Nov 17, 2023
@bors
Auto merge of #11821 - GuillaumeGomez:misspelled-cfg, r=blyxyas
8969c10 
Extend `maybe_misused_cfg` lint over `cfg(test)`

Fixes #11240.

One thought I had is that we could use the levenshtein distance (of 1) to ensure this is indeed `test` that was targeted. But maybe it's overkill, not sure.

r? `@blyxyas`
@bors
Copy link
Contributor

bors commented Nov 17, 2023

💔 Test failed - checks-action_test

@GuillaumeGomez
Copy link
Member Author

I added the missing changelog in my first comment. Sorry about that.

@blyxyas
Copy link
Member

blyxyas commented Nov 17, 2023

@bors retry

@blyxyas
Copy link
Member

blyxyas commented Nov 17, 2023

@bors r+

@bors
Copy link
Contributor

bors commented Nov 17, 2023

💡 This pull request was already approved, no need to approve it again.

  • There's another pull request that is currently being tested, blocking this pull request: Fix typo #11828

@bors
Copy link
Contributor

bors commented Nov 17, 2023

📌 Commit f08037c has been approved by blyxyas

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 17, 2023

⌛ Testing commit f08037c with merge 31e38fe...

@bors
Copy link
Contributor

bors commented Nov 17, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: blyxyas
Pushing 31e38fe to master...

@bors bors merged commit 31e38fe into rust-lang:master Nov 17, 2023
4 checks passed
@GuillaumeGomez GuillaumeGomez deleted the misspelled-cfg branch November 17, 2023 11:59
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Nov 17, 2023 •
edited
Loading

I was planning to open an issue to have a discussion about a wider check for all cfgs. However it would have brought issues in case what we thought was a typo was actually another cfg which was not activated.

But luckily for us, I just learned that the --check-cfg option in cargo is planning to be stabilized so this issue just got solved without needing us to do anything. \o/

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Nov 17, 2023 •
edited
Loading

Here's the issue: rust-lang/rust#82450

@GuillaumeGomez GuillaumeGomez mentioned this pull request Nov 18, 2023
Merged
bors added a commit that referenced this pull request Nov 19, 2023
@bors
Auto merge of #11840 - GuillaumeGomez:improve-maybe_misused_cfg, r=bl…
41140e3 
…yxyas

Improve maybe misused cfg

Follow-up of the improvements that were suggested to me in #11821:

 * I unified the output to use the same terms.
 * I updated the code to prevent creating a new symbol.

r? `@blyxyas`

changelog: [`maybe_misued_cfg`]: Output and code improvements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pietroalbini pietroalbini pietroalbini left review comments

@blyxyas blyxyas blyxyas approved these changes

@emilk emilk emilk approved these changes

Assignees

@blyxyas blyxyas

Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Extend maybe_misused_cfg to cover #[cfg(tests)]
6 participants
@GuillaumeGomez @blyxyas @bors @emilk @pietroalbini @rustbot