Expr walking structural match (reopened) #67088
Conversation
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
…s occurring in patterns. Incorporated review feedback: moved the HIR traversal to `rustc_mir::hair::structural_match`.
…n the lint explicitly.
added omitted expected-warning annotations
pnkfelix
force-pushed
the
issue-62614-expr-walking-structural-match
branch
from
6dce14b to
c37b8b3
Compare
|
(It’s the same PR as before, just rebased and with fixed |
|
I think we should have a language team meeting about this before merging the PR. It feels like the language is being changed here but without an explicit design / goal / ... communicated. |
|
@Centril I'm happy to discuss it with the lang team. I've interpreted this work as being part of a long-term linguistic bug fix to structural match; its still part of the But since this PR is changing that lint from allow-by-default back to warn-by-default, it certainly falls under the T-lang team purview. |
pnkfelix
added
I-nominated
T-lang
|
☔ The latest upstream changes (presumably #67532) made this pull request unmergeable. Please resolve the merge conflicts. |
|
We discussed this at last night's T-lang meeting. The basic consensus was that the lang team generally felt like we should invest more effort in trying to get the MIR-based (i.e. const-qualification) PR #67343 up to the point where we could land it, rather than landing this PR and then having to maintain multiple similar systems, even if this PR today has better diagnostic quality than #67343. Therefore, I am going to close this PR, and focus my attention on trying to help get PR #67343 to the point where it is good enough to land Iwhich we currently believe is largely a matter of improving the diagnostics and getting it to use the lint infrastructure rather than be a hard error). |
…h, r=pnkfelix Const qualification for `StructuralEq` Furthers rust-lang#62411. Resolves rust-lang#62614. The goal of this PR is to implement the logic in rust-lang#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 rust-lang#67088 and emits the proper warnings for the corner cases. This PR does not handle rust-lang#65466, which would require that we be [more aggressive](https://github.com/rust-lang/rust/blob/42abbd8878d3b67238f3611b0587c704ba94f39c/src/librustc_mir_build/hair/pattern/const_to_pat.rs#L126-L130) 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 rust-lang#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 rust-lang#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
Long awaited fix to #62614; reopened version of PR #66120
The structural-match check is meant to ensure that consts used in patterns only have types that
derive(PartialEq)(see RFC 1445).Unfortunately the initial code to enforce structural-match was not quite correct, which led to the creation of the indirect_structural_match compatibility lint; see #62411
Doubly unfortunately the initial code to enforce the lint was not quite correct, which led to it being downgraded to allow-by-default and the filing of #62614.
This PR implements a scheme I outlined on #62614: Instead of basing the analysis solely on the type of the given const, it first attempts to prove the const adheres to the structural-match rules by analysis of the HIR representing the const's right-hand side (RHS). It only falls back to analyzing the type when it encounters a sub-expression on the RHS that it deems too complicated to analyze, such as
const fninvocations; in such cases it then resorts to a type-based analysis.Follow-up work is to do this as a MIR const-qualif pass rather than walking the HIR.
Fix #62614
Fix #65466