lint towards rejecting consts in patterns that do not implement PartialEq

[RalfJung]

I think we definitely don't want to allow such consts, so even while the general plan around structural matching is up in the air, we can start the process of getting non-PartialEq matches out of the ecosystem.

[rustbot]

r? @b-naber

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

[rustbot]

Some changes might have occurred in exhaustiveness checking

cc @Nadrieril

[RalfJung]

Crucially, tests/ui/rfcs/rfc-1445-restrict-constants-in-patterns/allow-use-behind-cousin-variant.rs is still accepted; this is the example where #62411 (indirect_structural_match) was initially over-eager.

[RalfJung]

@rust-lang/lang I found a surprising gap in our pattern-related future-incompat lints, which lead to us still accepting (without a warning) some consts as patterns that don't even implement PartialEq. I think we definitely don't want that (and @oli-obk said the intent of his lints was to catch all such cases), so this PR closes that gap. The new lint is immediately set up to show in cargo's reports. I expect this to be rare (one has to work pretty hard to get such a const to be accepted), so it doesn't seem worth the extra delay of having a phase where the lint only shows up for the local crates.

So the question for you is, are you okay with consts in patterns requiring at least a PartialEq bound? (There might be further restrictions, related to all this "structural equality" business. That doesn't have to be answered here. This is just asking whether we need at least PartialEq.)

If you are curious about what the affected code looks like, here are some examples:

• example 1
• example 2

On both cases, the reason the constant isn't PartialEq is an unnecessary trait bound introduced by derive(PartialEq). That's why the compiler is still able to generate code that compares the constant with the matched place. These unnecessary trait bounds aren't great, but I don't think that should affect our const-in-match-pattern rules.

[nikomatsakis]

Checking in from the @rust-lang/lang meeting and unnominating:

Our judgment is that this is a bug fix and only causes more warnings, so no FCP is required, and we are in favor of going forward.

However, we were pretty confused about what the story is around constants and matching -- requiring a PartialEq bound makes sense if we are invoking ==, but it's not clear if that is the case? Or sometimes the case? We would much appreciate to hear the latest thinking at some point (design meeting?).

@rustbot labels -I-lang-nominated

[RalfJung]

However, we were pretty confused about what the story is around constants and matching -- requiring a PartialEq bound makes sense if we are invoking ==, but it's not clear if that is the case? Or sometimes the case? We would much appreciate to hear the latest thinking at some point (design meeting?).

Yeah I think a design meeting is due, also see here. Maybe we shouldn't add further warnings until that happened, to avoid having to take back the warning again.

[RalfJung]

I started writing up a design meeting document and there good reasons for requiring PartialEq in either case. So as far as I am concerned, and based on t-lang feedback above, I think it makes sense to go ahead and land this, if the compiler team agrees.

@oli-obk @lcnr what do you think?

[est31]

Regarding the lint's name, maybe it is too general? I assume you do not want to forbid code like this:

enum Foo { Bar, Baz }
impl Foo {
    fn is_bar(&self) -> bool {
        matches!(self, Foo::Bar)
    }
}

Where Foo has no derive for PartialEq. I at least have been using this trick a few times when I thought that adding a derive was too burdensome.

Would the name constants_matching_without_partial_eq be better maybe? It's a bit longer but no idea what to remove. (also, generally, lints should be in plural form).

[RalfJung]

I renamed it to const_patterns_without_partial_eq.

[RalfJung]

There is the potential alternative of checking for Eq here instead of PartialEq. This would then subsume the "disallow floats in patterns" lint as well. However with t-lang in the past having been disinclined to disallow float patterns, it's not clear that we want to require Eq.

[oli-obk]

@bors r+

let's land this and figure out the details of consts in patterns in a design meeting with T-lang

[bors]

📌 Commit a1d6fc4 has been approved by oli-obk

It is now in the queue for this repository.

[bors]

⌛ Testing commit a1d6fc4 with merge 1f2bacf...

[bors]

☀️ Test successful - checks-actions
Approved by: oli-obk
Pushing 1f2bacf to master...

[rust-timer]

Finished benchmarking commit (1f2bacf): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.4%	[3.4%, 3.4%]	1
Improvements ✅ (primary)	-0.6%	[-0.6%, -0.6%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.6%	[-0.6%, -0.6%]	1

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 631.784s -> 630.892s (-0.14%)
Artifact size: 317.18 MiB -> 317.28 MiB (0.03%)