Add T: PartialEq bounds to derived StructuralPartialEq impls.

[kpreid]

View all comments

Fixes #147714.

This is a breaking change to fix a bug, so it will need a crater run if it is to be done at all.

The changes in const_to_pat.rs are entirely to avoid regressing diagnostic quality, and should not make any difference to what code is accepted. The changes in compute_applicable_impls_for_diagnostics and its callers are entirely to be able to reuse that algorithm for this purpose.

@rustbot label +T-lang

[kpreid]

This PR is probably less correct than the just-posted #156809, because the documentation of StructuralPartialEq says that it ensures that “interpreting the value of the constant as a pattern is equivalent to calling PartialEq”, and so the trait bounds should require transitive StructuralPartialEq, not just that the applicable PartialEq impl is the derived one. Still, I'll leave this open until things settle down more.

Currently waiting on an answer to my question about a route to to fixing the diagnostics.

[theemathas]

Adding T: StructuralPartialEq bounds like in #156809 is incorrect. Consider the following code, which compiles in current stable rust, despite Box not implementing StructuralPartialEq:

const X: Option<Box<()>> = None;

fn main() {
    if let X = X {}
}

[theemathas]

Relevant documentation: https://doc.rust-lang.org/nightly/reference/patterns.html#r-patterns.const

The StructuralPartialEq trait does not correspond to the concept of "structural equality" in the documentation. In fact, this concept of "structural equality" cannot be represented as a trait at all, due to the "active variant" clause for enums, which makes structural equality depend on the const's value, not just the type.

Instead, the StructuralPartialEq trait corresponds to some notion of "shallow" structural equality, which is not written down in the reference.

[kpreid]

I am working on improving this PR to prevent a diagnostic regression. I believe it is possible to do so cheaply by being a little more clever about which possible error to emit (among all the errors possible to produce from different elements of the valtree being converted to a pattern).

[kpreid]

Actually, I misunderstood the UI tests that showed the regression. The worse diagnostic is emitted in UI tests where there is a generic argument that doesn't derive(PartialEq) but which is not involved in the specific value of the constant, e.g. if a locally-defined enum shaped like Option<T> is instantiated as Option::<NonStructural>::None.

In this case, we used to hit the error branch in ConstToPat::unevaluated_to_pat() that checks the type, but we now hit the error branch in ConstToPat::valtree_to_pat() that checks the value and runs first, because valtree_to_pat() cares about exactly the StructuralPartialEq impl that we're constraining away in this PR.

I fixed that by adding a copy of the check that unevaluated_to_pat() does, early enough that we produce that error in the right cases.

This should now be ready for review, but I’ll see if PR CI has anything to say first.

[rustbot]

Changes to the code generated for builtin derived traits.

cc @nnethercote

Some changes occurred in match checking

cc @Nadrieril

[rustbot]

r? @petrochenkov

rustbot has assigned @petrochenkov.
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

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: compiler
• compiler expanded to 73 candidates
• Random selection from 18 candidates

[theemathas]

@bors try

[rust-bors]

☀️ Try build successful (CI)
Build commit: 3857be5 (3857be5045fe74bd0f296f6f4c23db10f8857f73, parent: 31a9463c6e2794a59ce57a8f37abc6966afc2a58)

[theemathas]

@craterbot check

[craterbot]

👌 Experiment pr-156807 created and queued.
🤖 Automatically detected try build 3857be5
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🚧 Experiment pr-156807 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

[craterbot]

🎉 Experiment pr-156807 is completed!
📊 2 regressed and 1 fixed (950352 total)
📊 5376 spurious results on the retry-regressed-list.txt, consider a retry¹ if this is a significant amount.
📰 Open the summary report.

⚠️ If you notice any spurious failure please add them to the denylist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

Footnotes

1. re-run the experiment with crates=https://crater-reports.s3.amazonaws.com/pr-156807/retry-regressed-list.txt ↩

[theemathas]

The regressions are spurious and unrelated to this PR.

@rustbot label +I-lang-nominated

I am nominating because this PR is a breaking change. This PR changes the behavior of derive(PartialEq). That is, given:

#[derive(PartialEq)]
struct Thing<T>(Option<T>);

Previously, the above code will cause Thing(None) to be usable as a const pattern as long as Thing<T> implements PartialEq in some way, regardless of whether this impl came from the derive or not. This PR causes Thing(None) to be usable as a const pattern only when the relevant PartialEq came from the derive.

See the reference for documentation on what's allowed as constant patterns. See the issue being fixed for compilable demonstration code that will be broken by this PR.

[traviscross]

This makes sense to me. Good (and subtle) call, in particular, on the bound being T: PartialEq rather than T: StructuralPartialEq. Thanks to @kpreid and @theemathas on this.

@rfcbot fcp merge lang

[rust-rfcbot]

Team member @traviscross has proposed to merge this. The next step is review by the rest of the tagged team members:

• @joshtriplett
• @nikomatsakis
• @scottmcm
• @tmandry
• @traviscross

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

cc @rust-lang/lang-advisors: FCP proposed for lang, please feel free to register concerns.
See this document for info about what commands tagged team members can give me.

[scottmcm]

Note: neither of the two regressions from the crater run is real.

[theemathas]

To clarify why this bound is correct: We want StructuralPartialEq to apply exactly at the same time that the derived PartialEq applies. That way, the promise made by StructuralPartialEq is always fulfilled by the derive.

[rust-rfcbot]

🔔 This is now entering its final comment period, as per the review above. 🔔

[nikomatsakis]

@rfcbot reviewed

[tmandry]

We want StructuralPartialEq to apply exactly at the same time that the derived PartialEq applies. That way, the promise made by StructuralPartialEq is always fulfilled by the derive.

In other words, iiuc, we want StructuralPartialEq to have the same bounds as PartialEq, so that the impls apply for the same types. That makes sense to me.

@rfcbot reviewed

[rust-rfcbot]

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

[kpreid]

@rustbot label -S-waiting-on-t-lang +S-waiting-on-review