Const qualification for `StructuralEq` #67343

ecstatic-morse · 2019-12-16T05:01:01Z

Furthers #62411. Resolves #62614.

The goal of this PR is to implement the logic in #67088 on the MIR instead of the HIR. It uses the Qualif trait to track StructuralPartialEq/StructuralEq in the final value of a const. Then, if we encounter a constant during HAIR lowering whose value may not be structurally matchable, we emit the indirect_structural_match lint.

This PR contains all the tests present in #67088 and emits the proper warnings for the corner cases. This PR does not handle #65466, which would require that we be more aggressive when checking matched types for PartialEq. I think that should be done separately.

Because this works on MIR and uses dataflow, this PR should accept more cases than #67088. Notably, the qualifs in the final value of a const are encoded cross-crate, so matching on a constant whose value is defined in another crate to be Option::<TyWithCustomEqImpl>::None should work. Additionally, if a const has branching/looping, we will only emit the warning if any possible control flow path could result in a type with a custom PartialEq impl ending up as the final value of a const. I'm not sure how #67088 handled this.

AFAIK, it's not settled that these are the semantics we actually want: it's just how the Qualif framework happens to work. If the cross-crate part is undesirable, it would be quite easy to change the result of mir_const_qualif().custom_eq to true before encoding it in the crate metadata. This way, other crates would have to assume that all publicly exported constants may not be safe for matching.

r? @pnkfelix
cc @eddyb

ecstatic-morse · 2020-03-17T18:53:44Z

This handles #62614 properly, but still ICEs on #65466.

src/test/ui/consts/const_in_pattern/reject_non_structural.rs

ecstatic-morse · 2020-04-07T19:21:10Z

@pnkfelix I've updated the PR description and this is now ready for review.

ecstatic-morse · 2020-04-07T20:12:11Z

src/test/ui/consts/const_in_pattern/reject_non_structural.stderr

+LL |     match Some(NoDerive) { NoDerive::ASSOC => dbg!(NoDerive::ASSOC), _ => panic!("whoops"), };
+   |                            ^^^^^^^^^^^^^^^
+
+error: to use a constant of type `NoDerive` in a pattern, `NoDerive` must be annotated with `#[derive(PartialEq, Eq)]`


Note that these error messages are repeated for some reason. This does not happen on nightly, or when I run the test using rustc +stage1.

Maybe we don't deduplicate errors when running compiletest? Tests like ui/consts/match_ice.rs also have repeated output on master.

ecstatic-morse · 2020-04-29T04:00:50Z

@bors r=pnkfelix

I believe I've addressed everything above that needed immediate attention. One warn is now a bug and the other is a debug, since it is relatively common.

bors · 2020-04-29T04:00:51Z

📌 Commit e4c650c has been approved by pnkfelix

bors · 2020-04-29T13:59:27Z

⌛ Testing commit e4c650c with merge 36d13cb...

bors · 2020-04-29T17:13:26Z

☀️ Test successful - checks-azure
Approved by: pnkfelix
Pushing 36d13cb to master...

rust-highfive · 2020-04-29T17:14:37Z

📣 Toolstate changed by #67343!

Tested on commit 36d13cb.
Direct link to PR: #67343

🎉 rustc-dev-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m).

@JohnTitor

Tested on commit rust-lang/rust@36d13cb. Direct link to PR: <rust-lang/rust#67343> 🎉 rustc-dev-guide on linux: test-fail → test-pass (cc @JohnTitor @amanjeev @spastorino @mark-i-m).

nnethercote · 2020-05-19T04:37:39Z

As expected, this was a perf regression of up to 6% on many-assoc-items and up to 2% on a few others, mostly html5ever.

rust-highfive assigned pnkfelix Dec 16, 2019

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 16, 2019

ecstatic-morse force-pushed the qualif-structural-match branch from 748dfd0 to 75514ef Compare December 16, 2019 05:16