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

New lint ignored_unit_patterns #11242

Merged
merged 1 commit into from Aug 3, 2023
Merged

Conversation

samueltardieu
Copy link
Contributor

@samueltardieu samueltardieu commented Jul 27, 2023

This idea comes from #11238. I've put the lint in pedantic as it might trigger numerous positives (three in Clippy itself).

changelog: [ignored_unit_patterns]: new lint

@rustbot
Copy link
Collaborator

rustbot commented Jul 27, 2023

r? @giraffate

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

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Jul 27, 2023
@Jarcho
Copy link
Contributor

Jarcho commented Jul 27, 2023

I think it would be better for the name to mention that it's about pattern matching. Maybe ignored_unit_patterns.

clippy_lints/src/ignored_unit.rs Outdated Show resolved Hide resolved
clippy_lints/src/ignored_unit.rs Outdated Show resolved Hide resolved
clippy_lints/src/ignored_unit.rs Outdated Show resolved Hide resolved
clippy_lints/src/ignored_unit.rs Outdated Show resolved Hide resolved
@samueltardieu samueltardieu changed the title New lint ignored_unit New lint ignored_unit_patterns Jul 31, 2023
@samueltardieu
Copy link
Contributor Author

I think it would be better for the name to mention that it's about pattern matching. Maybe ignored_unit_patterns.

Name and PR updated

@samueltardieu
Copy link
Contributor Author

@Centri3 Thanks for your review, I think all points have been addressed.

@Centri3
Copy link
Member

Centri3 commented Aug 1, 2023

LGTM, I'll wait until @giraffate reviews as well though I think this should be good to go. Thanks!

@Centri3
Copy link
Member

Centri3 commented Aug 1, 2023

Oh yeah btw, I think in the future we can extend this to other patterns, like so: https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=55acf7c246efb174d34dfe0c4e177425, in a general pedantic lint for unnecessary wildcards. Though it's probably debatable on whether this is better in that case, it does prevent the type changing as well which may or may not be a good thing.

@samueltardieu
Copy link
Contributor Author

samueltardieu commented Aug 1, 2023 via email

@giraffate
Copy link
Contributor

Hmm, I'm wondering if restriction would be a good category. It looks like a strict coding style.

I would like hear @Centri3's opinion.

@Centri3
Copy link
Member

Centri3 commented Aug 3, 2023

pedantic is fine imo for the current lint, the only real strictness I think that may be undesirable in some cases is that the type cannot change, but it's opt in so it should be ok.

@giraffate
Copy link
Contributor

@bors r=Centri3,giraffate

Thanks!

@bors
Copy link
Collaborator

bors commented Aug 3, 2023

📌 Commit f9a6dfa has been approved by Centri3,giraffate

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Aug 3, 2023

⌛ Testing commit f9a6dfa with merge 1eb254e...

@bors
Copy link
Collaborator

bors commented Aug 3, 2023

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: Centri3,giraffate
Pushing 1eb254e to master...

@bors bors merged commit 1eb254e into rust-lang:master Aug 3, 2023
8 checks passed
@cloneable
Copy link

This check also catches _: () as function parameter. Is this intended? Based on the error help I'm not sure it is.

error: matching over `()` is more explicit
 --> src/lib.rs:8:12
  |
8 | pub fn moo(_: ()) {}
  |            ^ help: use `()` instead of `_`: `()`
  |
  = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#ignored_unit_patterns
  = note: `-D clippy::ignored-unit-patterns` implied by `-D clippy::pedantic`

@Centri3
Copy link
Member

Centri3 commented Aug 24, 2023

_: () is pat: ty, so yes, but I'm not sure if it's desirable

@cloneable
Copy link

Ok, I filed #11403 where this can be discussed, but feel free to close if all is good.

@samueltardieu samueltardieu deleted the issue-11238 branch November 29, 2023 22:41
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.

None yet

7 participants