`unusable_matches_binding` lint implementation #11661

p-fuchs · 2023-10-12T18:39:42Z

Hey,
This PR contains new lint implementation along with UI tests.
Fixes #7351.

changelog: [unusable_matches_binding]: Lint implementation and UI test

rustbot · 2023-10-12T18:39:48Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @dswij (or someone else) soon.

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

dswij

Thanks for the PR! Overall, the implementation looks good, but I have some comments.

clippy_lints/src/matches/mod.rs

clippy_lints/src/matches/unusable_matches_binding.rs

dswij · 2023-10-15T19:10:38Z

tests/ui/unusable_matches_binding.stderr

@@ -0,0 +1,42 @@
+error: using identificator (`unusable_binding`) as matches! pattern argument without guard matches all cases


Let's be brief for error messages. I think something like

error: binding `unusable_binding` is not used

Then we can explain more in detail in note or help

The current message is a bit verbose, I agree, but in my opinion that suggested message isn't really much better because rustc already emits a warning here with an almost identical message (unused variable: unusable_binding), so I don't think that the suggested message would provide much more value than what rustc already says about this.

The problem imo is not so much that the binding isn't used, but rather that the user didn't mean to create a binding in the first place, and someone that doesn't know how matches! works likely won't understand why a binding is created either.
The user more likely meant to compare two values (I've seen this confusion happen a few times in forums, thinking that matches!(1, x) behaves like 1 == x), so I think it'd be better if this message focused on the fact that it won't do that and that this matches! is useless/always evaluates to true.

How about something like

error: identifier pattern in `matches!` macro always evaluates to true note: the identifier pattern matches any value and creates an unusable binding in the process help: if you meant to compare two values, use `x == y` or `discriminant(x) == discriminant(y)`

and someone that doesn't know how matches! works likely won't understand why a binding is created either.

Fair point

rustc already emits a warning here with an almost identical message (unused variable: unusable_binding)

Regarding this, I wonder if we should have this lint in Clippy at all. Users would stumble into rustc's lint first before running clippy. Having this as rustc_lint might be more beneficial

I've changed the error message here.
Isn't this lint too "specific" to be included in the rustc?

I wouldn't say it's too specific for rustc lints. But the main problem is not the "specific"-ness itself, but rather that users will stumble into rustc lints first before running clippy. Then, the lint itself will be useless because it's "too late".

I agree that it's a bit "unfortunate" that rustc's unused_variables lint triggers here and prevents us from emitting better diagnostics. It could do a much better job at explaining what the issue is and how to fix it. Might make sense to specialize the behavior of the unused_variables lint, if it sees that the parent node of the identifier pattern is a matches! expansion?

OTOH, the lint is in the correctness category (deny by default), so I think that will stand out more than rustc's lint? I'm not sure.
On that note: I do agree that matches!(val, num) should be deny-by-default: that is totally useless code and will always evaluate to true no matter what, but smth. like matches!(val, num if num % 2 == 0) (or any guard really) isn't useless in the same way, as it still checks something.

I guess it's better to have something than nothing. Let's see how this lint fares, maybe if it's good enough we can have it uplifted to rustc lint

dswij · 2023-10-15T19:23:07Z

tests/ui/unusable_matches_binding.stderr

+   = help: to override `-D warnings` add `#[allow(clippy::unusable_matches_binding)]`
+   = note: this error originates in the macro `matches` (in Nightly builds, run with -Z macro-backtrace for more info)
+
+error: using identificator (`used_binding`) as matches! pattern argument matches all cases


The error message is pretty confusing for this one. The argument here won't match all cases.

The only special case is probably some mutating expression in matches! macro. But, I don't think this is a common case, and shouldn't be written that way anyway. We should only care about this when we are writing clippy suggestions.

Simplifying the logic to just check unused binding should be good enough for this lint.

After the PR change we need also to check the case when the guard is present because in that situation different error message should be displayed (compared to the case when the guard is missing).

bors · 2023-10-19T12:11:58Z

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

Closes rust-lang#7351

rustbot · 2023-10-19T14:48:11Z

Error: The feature review_requested is not enabled in this repository.
To enable it add its section in the triagebot.toml in the root of the repository.

Please file an issue on GitHub at triagebot if there's a problem with this bot, or reach out on #t-infra on Zulip.

dswij · 2023-10-26T18:51:39Z

@y21 Would you mind helping to review the code and run lintcheck for this? :)

y21

Some initial thoughts :)

y21 · 2023-10-26T19:41:45Z

clippy_lints/src/matches/mod.rs

+    /// ### Why is this bad?
+    /// It could be hard to determine why the compiler complains about an unused variable,
+    /// which was introduced by the `matches!` macro expansion, also, it could introduce
+    /// behavior that was not intended by the programmer.


I find this section a little bit ambiguous/unclear.
I think it would help to explain more clearly where the variable is actually coming from. How about something like this:

Contrary to the == operator, which compares two values, the matches! macro compares a value to a pattern.
An invocation such as matches!(5, x) expands to the following code:

match 5 { x => true, _ => false }

The identifier pattern x matches any value and creates a binding that stores its value.

This makes identifier patterns in matches! without any use in guards useless: the macro always evaluates to true as no equality checking actually takes place and the binding that is created is unusable outside of the macro.

y21 · 2023-10-26T19:42:35Z

clippy_lints/src/matches/mod.rs

+    /// let z = unrelated_data_source > 6;
+    /// ```
+    #[clippy::version = "1.75.0"]
+    pub UNUSABLE_MATCHES_BINDING,


Suggested change

pub UNUSABLE_MATCHES_BINDING,

pub UNUSABLE_MATCHES_BINDINGS,

Lint names should be in plural, so that it makes sense when read together with the #[allow] attribute: "allow unusable matches bindings"

y21 · 2023-10-26T19:43:01Z

clippy_lints/src/matches/mod.rs

+    #[clippy::version = "1.75.0"]
+    pub UNUSABLE_MATCHES_BINDING,
+    correctness,
+    "checks for unused bindings in `matches!` macro"


Suggested change

"checks for unused bindings in `matches!` macro"

"checks for unusable bindings in `matches!` macro"

I assume this was meant? More consistent with the lint name

y21 · 2023-10-26T19:43:42Z

clippy_lints/src/matches/unusable_matches_binding.rs

+pub(crate) fn check_matches<'tcx>(cx: &LateContext<'tcx>, arms: &'tcx [Arm<'tcx>]) {
+    for arm in arms {
+        if let PatKind::Binding(_, id, _, None) = arm.pat.kind {
+            if !is_local_used(cx, arm.body, id) {


I don't think this check here does anything. The body of the arm should always be the literal expression true and never references the binding, so this is always false (or well, true when inverted).

y21 · 2023-10-26T19:44:29Z

tests/ui/unusable_matches_binding.stderr

+error: identifier pattern in `matches!` macro always evaluates to the value of the guard
+  --> $DIR/unusable_matches_binding.rs:19:60
+   |
+LL |     let _ = matches!(matching_data_source, used_binding if used_binding.is_valid());


I think correctness would be a good category for matches!(42, x) with no guard, as that is totally useless, but when there is a guard, like here, I feel like this is a bit too strong...

Something like matches!(value, num if num % 2 == 0) is not useless in the same way matches!(value, num) is, because the guard actually ends up checking its value.

I think it's just as bad and should not be used so I agree with the current implementation.

y21 · 2023-10-26T23:26:00Z

@y21 Would you mind helping to review the code and run lintcheck for this? :)

Re. lintcheck, it looks like there aren't any matches (no pun intended), at least not for the 100 most downloaded crates. I'm not surprised though, considering the unused_variables rustc lint should've sort of warned them

bors · 2023-12-16T18:26:27Z

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

GuillaumeGomez · 2024-03-17T12:51:11Z

This is actually already triggered by rustc's unused_variable so can be closed.

dswij · 2024-03-17T13:04:05Z

Thank you for the PR @p-fuchs , it's really appreciated. But since a rustc lint already checks for this, we shouldn't add another redundant one.

smoelius · 2024-03-17T15:58:43Z

Should #7351 be closed as well?

GuillaumeGomez · 2024-03-17T17:27:25Z

Yes it should.

rustbot assigned dswij Oct 12, 2023

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

dswij reviewed Oct 15, 2023

View reviewed changes

p-fuchs added 3 commits October 19, 2023 15:46

unusable_matches_binding lint implementation

7cda04a

Closes rust-lang#7351

Improved documentation of unusable_matches_binding lint

40a05e0

Lint (and generated messages) simplified

7223b92

p-fuchs force-pushed the unusable-matches-binding branch from 02f3280 to 7223b92 Compare October 19, 2023 14:37

p-fuchs requested review from y21 and dswij October 19, 2023 14:48

y21 reviewed Oct 26, 2023

View reviewed changes

GuillaumeGomez mentioned this pull request Mar 13, 2024

Add new unusable_matches_bindings lint #12480

Closed

dswij closed this Mar 17, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`unusable_matches_binding` lint implementation #11661

`unusable_matches_binding` lint implementation #11661

p-fuchs commented Oct 12, 2023

rustbot commented Oct 12, 2023

dswij left a comment

dswij Oct 15, 2023

y21 Oct 15, 2023

dswij Oct 16, 2023

p-fuchs Oct 19, 2023

dswij Oct 19, 2023

y21 Oct 19, 2023 •

edited

Loading

dswij Oct 26, 2023

dswij Oct 15, 2023

p-fuchs Oct 19, 2023

bors commented Oct 19, 2023

rustbot commented Oct 19, 2023

dswij commented Oct 26, 2023

y21 left a comment

y21 Oct 26, 2023

y21 Oct 26, 2023

y21 Oct 26, 2023

y21 Oct 26, 2023

y21 Oct 26, 2023

GuillaumeGomez Mar 13, 2024

y21 commented Oct 26, 2023

bors commented Dec 16, 2023

GuillaumeGomez commented Mar 17, 2024

dswij commented Mar 17, 2024

smoelius commented Mar 17, 2024

GuillaumeGomez commented Mar 17, 2024

		@@ -0,0 +1,42 @@
		error: using identificator (`unusable_binding`) as matches! pattern argument without guard matches all cases

	"checks for unused bindings in `matches!` macro"
	"checks for unusable bindings in `matches!` macro"

unusable_matches_binding lint implementation #11661

unusable_matches_binding lint implementation #11661

Conversation

p-fuchs commented Oct 12, 2023

rustbot commented Oct 12, 2023

dswij left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 Oct 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors commented Oct 19, 2023

rustbot commented Oct 19, 2023

dswij commented Oct 26, 2023

y21 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

y21 commented Oct 26, 2023

bors commented Dec 16, 2023

GuillaumeGomez commented Mar 17, 2024

dswij commented Mar 17, 2024

smoelius commented Mar 17, 2024

GuillaumeGomez commented Mar 17, 2024

`unusable_matches_binding` lint implementation #11661

`unusable_matches_binding` lint implementation #11661

y21 Oct 19, 2023 •

edited

Loading