Add new matches_instead_of_eq lint

[GuillaumeGomez]

View all comments

Fixes #14688.

r? @samueltardieu

changelog: Add new pedantic matches_instead_of_eq lint

[github-actions]

Lintcheck changes for 528c1a0

Lint	Added	Removed	Changed
clippy::matches_instead_of_eq	36	0	0

This comment will be updated if you push new changes

[GuillaumeGomez]

With dogfood errors, I was able to strenghten quite a lot the lint and handle suggestions with dereferencing.

One of the nice changes that came with this lint is:

-        if cx
-            .tcx
-            .crate_types()
-            .iter()
-            .any(|t: &CrateType| matches!(t, CrateType::ProcMacro))
-        {
+        if cx.tcx.crate_types().contains(&CrateType::ProcMacro) {

[lukaslueg]

Maybe a nit: If the type being compared is PartialEq but manually implements that, there may be a subtle difference in behavior, as matches! only does the pattern-matching, while the custom PartialEq-impl may go beyond that; even if one might consider such a PartialEq-impl hazardous for exactly this reason, we may want to not lint if PartialEq is custom-made, as the lint might cause a change in behavior.

[GuillaumeGomez]

Very good point. I'll handle this case as well, thanks!

[GuillaumeGomez]

Added suggested change.

[samueltardieu]

Suggested change

	/// Checks for the use of `matches!` with types that implement `PartialEq` (not manually).
	/// Checks for the use of `matches!` with types that automatically derive `PartialEq`.

View changes since the review

[samueltardieu]

I think this won't catch types whose components have a non-automatically-derived PartialEq, for example here:

enum E { … }

impl PartialEq for E {
    …
}

#[derive(PartialEq)]
struct S(E);

fn f(s: &S) {
    if matches!(s, S(E::…)) { … }
}

In this case, we have multiple choices:

• not lint
• lint with MaybeIncorrect and a note saying that E's PartialEq may not behave as pattern matching

While in other cases, I think we can lint with MachineApplicable.

View changes since the review

[GuillaumeGomez]

I think we shouldn't lint in this case to follow the "do not lint if manual PartialEq impl" rule.

[samueltardieu]

The check will have to be carefully placed as to not analyze all types recursively every time there is a `matches!` macro call. You might also want to cache the analysis results.

[rustbot]

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[GuillaumeGomez]

Finally took the time to implement the suggestion for transitive non-automatic PartialEq implementation.

[Jarcho]

This lint is highly reliant on the optimizer and the optimizer is highly unreliable. The more complex the body containing the match is, the more likely the optimizer is to fail. See https://godbolt.org/z/9zosK9WMj for a simple example.

For a pattern like Some(0) this will always work fine since everything is unconditionally inlined, but even Ok(0) can fail depending on the error type. Debug builds will also suffer since they always end up with function calls.

[GuillaumeGomez]

@Jarcho: Just to confirm: did you comment on the right PR? This is a pedantic lint, not a perf one. Or did I misunderstand your point?

[GuillaumeGomez]

Fixed dogfood. :)

[Jarcho]

The lint is suggesting something with notable performance regression. The category of the lint isn't relevant.

[GuillaumeGomez]

Interesting: it doesn't inline the PartialEq implementation like it does for matches!.

[GuillaumeGomez]

Just thought: seems like a bug in rustc/LLM rather than coming from this lint. So from here, should we wait for a solution upstream or is it ok to move forward with this lint?

[Jarcho]

This isn't really a bug. == is a function call, but matches is a direct comparison on the relevant parts of the value. What the complier sees when you use == is essentially:

{
  let tmp = Some(0);
  PartialEq::eq(&value, &tmp)
}

matches! looks like:

discriminant(&value) == CONSTANT
  && read_enum_field!(value, CONSTANT, 0) == 0

There compiler can't assume that PartialEq::eq does the same thing as pattern matching so the only way to go from the former to the later is via generic optimization passes. They might work, but that's impossible to guarantee in general.

[GuillaumeGomez]

I thought the PartialEq implementation was inlined most of the time. TIL. It's a shame as it allows things like this. So what do you want to do with this lint? We close it? We never automatically apply it and add a big warning in the description?

[Jarcho]

It's a normal function with normal inlining rules (The derive adds #[inline], but that just makes it a little more likely). For something like a fieldless enum this is fine since eq is just a primitive comparison.

[rustbot]

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

[nylvim]

There is an unstable marker trait, StructuralPartialEq, that ensures a similar precondition, but only for named constants, not struct-like patterns.