WIP: New lint: duplicate_match_guards

[ada4a]

Fixes #14971

WIP because:

• is there a way to remove the block braces from the suggestion?
• I guess the "Known problems" section is not really true, since eq_expr_value takes care of impure function calls?
• it would be good to support the recently stabilized if-let guards

changelog: new lint: [duplicate_match_guards]

[rustbot]

r? @y21

rustbot has assigned @y21.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[ada4a]

Since the lint is directed at copy-paste errors, maybe we should make this stricter, and make sure that the expr are "syntactically" equal, and don't just evaluate to the result.

[y21]

eq_expr_value already does that (it uses SpanlessEq, which checks for syntactic equality). Or how do you mean this?

[ada4a]

No yes, that's what I meant. If that's the case though, how is the following able to pass?

rust-clippy/tests/ui/duplicate_match_guards.rs

Lines 22 to 28 in 3e4ace9

	// not _identical_, but the meaning is the same
	0 if a > b => {
	if b < a {
	//~^ duplicate_match_guards
	return;
	}
	},

[y21]

eq_expr_value already does that (it uses SpanlessEq, which checks for syntactic equality). Or how do you mean this?

[y21]

I'm slightly confused by this section. This says "false negative", but then describes what sounds like would be a false positive ("the lint falsely warns if the condition has impure function calls which would change semantics"), but this case is actually handled by the lint anyway given that there's a test for it (so there's no issue at all)?

[ada4a]

You're absolutely right. I think that's a remnant of another lint I took inspiration from.

[y21]

I think suspicious would be fine for the category

[y21]

Is there a reason to not also lint if there's an else? It still seems just as (arguably even more) wrong if there's an else {} because the block will additionally just be unreachable, wouldn't it?

[ada4a]

I don't think there is, no. It's just that my motivating case in particular didn't have an else

[ada4a]

(arguably even more) wrong

Given this, maybe it would make sense to lint the else is Some case with a separate lint, placed into the correctness group?

[y21]

I see this is already making sure that there are no other statements, but we might also want to check that there's no #[cfg]'d out code in between. So e.g. I assume the following is currently a false positive:

match () {
  () if condition {
    #[cfg(...)]
    condition = true;
    if condition {
      
    }
  }
}

[y21]

Couldn't this lint in principle apply to regular if conditions too? E.g. this currently has no warning

let x = 1;
if x > 1 {
  if x > 1 {
    // always executes
  } else {
    // this block of code is simply unreachable
    todo!()
  }
} else { todo!() }

I'm not saying that this PR should implement that, but it makes me wonder if we should make the lint name a bit more general, like duplicate_nested_conditions or something so it could be extended in the future without needing a name change?

[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.