feat: add cfg_not_test lint #11293

mrnossiom · 2023-08-04T00:22:31Z

changelog: new lint: [cfg_not_test]

I don't know whether to lint only the attr or also the item associated to it. I guess this would mean putting the check in another place than check_attribute but I can't find a way to get the associated item to the attribute.

Also, I'm not sure how to document this lint, I feel like my explications are bad.

rustbot · 2023-08-04T00:22:35Z

r? @Jarcho

(rustbot has picked a reviewer for you, use r? to override)

clippy_lints/src/cfg_not_test.rs

Centri3 · 2023-08-04T01:31:15Z

clippy_lints/src/cfg_not_test.rs

+    fn check_attribute(&mut self, cx: &EarlyContext<'_>, attr: &rustc_ast::Attribute) {
+        if let Some(ident) = attr.ident()
+            && ident.name == rustc_span::sym::cfg
+            && contains_not_test(attr.meta_item_list().as_deref(), false)


This won't detect it if --test is passed.

This could be a pre-expansion pass lint instead so it's ran before cfg is expanded, but these are very buggy and soft-deprecated. See #6610 for an example of one of these issues. If it's made into a pre-expansion pass, this should probably be moved to attrs.rs and combined with maybe_misused_cfg, that way we can minimize its usage to only a few lints.

Whether or not it not detecting it if --test is passed is an issue is up to you, it's probably not as big of a deal as other lints.

I didn't know about cfg expansion, does this only concern cfg_attr? or does normal cfgs get expanded too ? If cfg_attr can be expanded, should I change my lint to be a late pass ? I've also read in other issues that many attribute lints had issues in early pass.

Concerning attrs.rs, I saw the file but choose to follow the Clippy dev book. I don't know if grouping lints has an advantage (btw, why isn't there more folders for lints?). Also, this deserves his own lint, and IMHO has not much to do with maybe_misused_cfg.

Last, I didn't exactly understand the issue about the --test flag.

I didn't know about cfg expansion, does this only concern cfg_attr?

No, it concerns cfg as well. In the HIR, and even AST, cfgs are expanded to either nothing or unchanged depending on whether it should "be in that compilation run". So, if not(test) is successful, it'll be entirely gone before linting occurs. Thus, if the user passes --test, there will be no warnings.

For something like not(test), not a big deal, but something like cfg(all(windows, not(test)) is not good.

I'd recommend messing around a bit on godbolt if you're still having trouble understanding: https://godbolt.org/z/WxPWeM416, you can see the AST before macro-expansion, after, and the HIR by passing different things to unpretty.

(btw, why isn't there more folders for lints?)

Only because nobody's taken the time to :D I'd be 100% in favor of merging a PR to clean that up a bit.

I don't know if grouping lints has an advantage

It's cleaner, but more importantly, it puts them in the same lint pass which has a huge performance boost. For on-by-default (warn/deny) lints, that you almost never want to globally disallow, this is great. In the case of this lint, it's allow-by-default, but I'd still say it's ok in the same lint pass as it does the exact same thing as other lints.

Also, this deserves his own lint, and IMHO has not much to do with maybe_misused_cfg.

I actually agree now; that one is warn-by-default and this one is a restriction lint. So, no complaints from me.

I'm going to see for the expansion

merging a PR to clean that up a bit.

Would definitely be willing to do that 😉

In the case of this lint, it's allow-by-default

I thought restriction was warn-by-default (it's not). In the original issue, @blyxyas states "I feel like the drawback is too heavy for an allow-by-default lint" which I understand as "the lint should be warn-by-default"

I think she meant enabled-by-default, restriction is allow-by-default and generally, lints in that group aren't applicable in every situation or go against idiomatic code. There are valid reasons for using not(test).

For an example of when it's not quite as applicable, maybe when getting coverage they'd have a cfg set using RUSTFLAGS and use any(not(test), coverage) to include it in coverage but not testing, perhaps to speed up compilation.

clippy_lints/src/cfg_not_test.rs

tests/ui/cfg_not_test.rs

clippy_lints/src/cfg_not_test.rs

tests/ui/cfg_not_test.rs

clippy_lints/src/cfg_not_test.rs

blyxyas · 2023-08-04T18:15:28Z

I thought restriction was warn-by-default (it's not). In the original issue, @blyxyas states "I feel like the drawback is too heavy for an allow-by-default lint" which I understand as "the lint should be warn-by-default"

Totally a typo, it should not be warn by default imo

bors · 2023-08-23T12:28:04Z

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

Jarcho · 2023-08-23T14:02:28Z

LGTM, but since you were already reviewing this. r? @Centri3

rustbot · 2023-08-24T12:37:26Z

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git rebase -i master
$ # delete any merge commits in the editor that appears
$ git push --force-with-lease

The following commits are merge commits:

e80f48b

bors · 2023-08-24T18:31:23Z

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

xFrednet · 2024-03-29T12:15:45Z

@Centri3 is currently inactive. @Jarcho would you mind taking this review back? If you don't have the time rn, you can reassign it using r? clippy or to me :)

r? @Jarcho

mrnossiom · 2024-04-01T12:53:58Z

Hi @Jarcho,

It's been a little time since I've made this PR. I would love to finish it. I remember being blocked by not understanding exactly what was blocking.

If I am correct, early passes are kind of unstable and buggy, but the information I need is not available after cfg expansion ? Or are cfg's expanded before early passes ?

Jarcho · 2024-04-01T16:21:48Z

Early passes are fine and have access to the ast before any cfg based removal has happened. The passes we don't want to add are pre-expansion passes, but that's not needed for this.

mrnossiom · 2024-04-01T18:11:51Z

So this PR is good as is, then ?

clippy_lints/src/cfg_not_test.rs

Jarcho · 2024-06-12T06:03:56Z

If you haven't already you'll have to rebase and rebless the tests. There was a ui_test update that changed the format a bit.

bors · 2024-06-15T23:44:19Z

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

bors · 2024-06-16T20:24:31Z

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

xFrednet · 2024-07-04T12:37:19Z

Hey, this is a ping from triage. @Jarcho can you give this PR a review? It's totally fine if you don't have the time right now, you can reassign the PR to a random team member using r? clippy.

@mrnossiom Would you mind rebasing the PR again?

@rustbot ready

bors · 2024-07-04T16:23:06Z

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

Jarcho · 2024-07-05T02:07:38Z

Marked this as approved. @rust-lang/clippy can someone merge this if they see it before I do since this is now multiple times I've missed it.

Manishearth · 2024-07-05T03:21:27Z

@bors r=Jarcho

bors · 2024-07-05T03:21:30Z

📌 Commit 0b1f696 has been approved by Jarcho

It is now in the queue for this repository.

bors · 2024-07-05T03:22:35Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout 11234 (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self 11234 --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging clippy_lints/src/lib.rs
CONFLICT (content): Merge conflict in clippy_lints/src/lib.rs
Auto-merging clippy_lints/src/declared_lints.rs
Auto-merging CHANGELOG.md
Automatic merge failed; fix conflicts and then commit the result.

Alexendoo · 2024-07-05T11:34:02Z

@bors r=Jarcho

bors · 2024-07-05T11:34:04Z

📌 Commit dd37441 has been approved by Jarcho

It is now in the queue for this repository.

bors · 2024-07-05T11:42:50Z

⌛ Testing commit dd37441 with merge 94a000b...

bors · 2024-07-05T11:51:29Z

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Jarcho
Pushing 94a000b to master...

rustbot assigned Jarcho Aug 4, 2023

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Aug 4, 2023

Centri3 reviewed Aug 4, 2023

View reviewed changes

clippy_lints/src/cfg_not_test.rs Show resolved Hide resolved

tests/ui/cfg_not_test.rs Show resolved Hide resolved

clippy_lints/src/cfg_not_test.rs Outdated Show resolved Hide resolved

rustbot assigned Centri3 and unassigned Jarcho Aug 23, 2023

rustbot assigned Jarcho and unassigned Centri3 Mar 29, 2024

Jarcho requested changes Jun 11, 2024

View reviewed changes

clippy_lints/src/cfg_not_test.rs Outdated Show resolved Hide resolved

clippy_lints/src/cfg_not_test.rs Outdated Show resolved Hide resolved

Jarcho approved these changes Jul 5, 2024

View reviewed changes

feat: add cfg_not_test lint

2c09ac3

test: add cfg_not_test tests

dd37441

bors merged commit 94a000b into rust-lang:master Jul 5, 2024
11 checks passed

mrnossiom deleted the 11234 branch July 5, 2024 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add cfg_not_test lint #11293

feat: add cfg_not_test lint #11293

mrnossiom commented Aug 4, 2023

rustbot commented Aug 4, 2023

Centri3 Aug 4, 2023 •

edited

Loading

mrnossiom Aug 4, 2023

Centri3 Aug 4, 2023

mrnossiom Aug 4, 2023 •

edited

Loading

Centri3 Aug 4, 2023 •

edited

Loading

blyxyas commented Aug 4, 2023

bors commented Aug 23, 2023

Jarcho commented Aug 23, 2023

rustbot commented Aug 24, 2023

bors commented Aug 24, 2023

xFrednet commented Mar 29, 2024

mrnossiom commented Apr 1, 2024

Jarcho commented Apr 1, 2024

mrnossiom commented Apr 1, 2024

Jarcho commented Jun 12, 2024

bors commented Jun 15, 2024

bors commented Jun 16, 2024

xFrednet commented Jul 4, 2024

bors commented Jul 4, 2024

Jarcho commented Jul 5, 2024

Manishearth commented Jul 5, 2024

bors commented Jul 5, 2024

bors commented Jul 5, 2024

Alexendoo commented Jul 5, 2024

bors commented Jul 5, 2024

bors commented Jul 5, 2024

bors commented Jul 5, 2024

feat: add cfg_not_test lint #11293

feat: add cfg_not_test lint #11293

Conversation

mrnossiom commented Aug 4, 2023

rustbot commented Aug 4, 2023

Centri3 Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

mrnossiom Aug 4, 2023

Choose a reason for hiding this comment

Centri3 Aug 4, 2023

Choose a reason for hiding this comment

mrnossiom Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

Centri3 Aug 4, 2023 • edited Loading

Choose a reason for hiding this comment

blyxyas commented Aug 4, 2023

bors commented Aug 23, 2023

Jarcho commented Aug 23, 2023

rustbot commented Aug 24, 2023

bors commented Aug 24, 2023

xFrednet commented Mar 29, 2024

mrnossiom commented Apr 1, 2024

Jarcho commented Apr 1, 2024

mrnossiom commented Apr 1, 2024

Jarcho commented Jun 12, 2024

bors commented Jun 15, 2024

bors commented Jun 16, 2024

xFrednet commented Jul 4, 2024

bors commented Jul 4, 2024

Jarcho commented Jul 5, 2024

Manishearth commented Jul 5, 2024

bors commented Jul 5, 2024

bors commented Jul 5, 2024

Alexendoo commented Jul 5, 2024

bors commented Jul 5, 2024

bors commented Jul 5, 2024

bors commented Jul 5, 2024

Centri3 Aug 4, 2023 •

edited

Loading

mrnossiom Aug 4, 2023 •

edited

Loading

Centri3 Aug 4, 2023 •

edited

Loading