Make SpanlessEq more consistent

[flip1995]

Description

The SpanlessEq::eq_expr implementation has a wildcard match arm that just returns false.

rust-clippy/clippy_utils/src/hir_utils.rs

Line 305 in d227f18

_ => false,

This is inconsistent with SpanlessHash, which implements hashing for each expressions type.

rust-clippy/clippy_utils/src/hir_utils.rs

Lines 785 to 789 in d227f18

	ExprKind::Unary(lop, le) => {
	std::mem::discriminant(&lop).hash(&mut self.s);
	self.hash_expr(le);
	},
	}

PartialEq and Hash implementations have to be consistent. The same should apply to SpanlessEq and SpanlessHash

Version

rustc 1.69.0-nightly (d7948c843 2023-01-26)
binary: rustc
commit-hash: d7948c843de94245c794e8c63dd4301a78bb5ba3
commit-date: 2023-01-26
host: x86_64-unknown-linux-gnu
release: 1.69.0-nightly
LLVM version: 15.0.7

Additional Labels

No response

[llogiq]

Note that SpanlessEq is used in eq_op, so changing the default to true would introduce false positives there whenever new expr variants are added.

Also the consistency between Hash and Eq goes only one way: Two equal values must hash to the same hash, but two unequal values may hash to the same hash.

So I'd suggest seeking further consistency here is misguided. Perhaps we should add a comment stating that fact.

[flip1995]

so changing the default to true would introduce false positives there whenever new expr variants are added.

I don't suggest changing the default. We should remove the wildcard completely. My main motivation for this is, that then, when new ExprKinds are added in rustc, contributors are forced to also deal with this in eq_expr. Currently only hash_expr is updated.

I'm fine with as a first step to do a NFC and replace the wildcard with all missing expr kinds and keep => false for them.

---

uh, removing the wildcard completely might not even be possible, because we have to deal with different ExprKinds that are never equal.

[Jarcho]

Probably worth having the final pattern be (ExprKind::A | ExprKind::B | ..., _). At least that way adding a new variant will cause an error.

[Yoric]

I'd be happy to pick this issue as a way to familiarize myself with clippy's code.

But

uh, removing the wildcard completely might not even be possible, because we have to deal with different ExprKinds that are never equal.

Does this mean that this issue is actionable or not?

[flip1995]

Does this mean that this issue is actionable or not?

What should be done is to check if every ExprKind is handled in the eq_expr implementation. And if not it should be handled.

To get rid of the wildcard, I think we can try Jarchos suggestion.

[Yoric]

I'll take a look later today.

[gernot-ohner]

@rustbot claim

[gernot-ohner]

I believe that this issue can now be closed since this PR was merged #11736.
I had just removed the tag to auto-close it because I believed that there was additional work to be done (& it later turned out that there wasn't).

[flip1995]

Thanks! Yes this is all done now.