Expr walking structural match #66120
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…s occurring in patterns.
…n the lint explicitly.
|
r? @eddyb (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
eddyb
reviewed
Nov 6, 2019
| // abstraction barrier for non-trivial expression; traverse | ||
| // `typeof(expr)` instead of expression itself. | ||
| debug!("SearchHirExpr switch to type analysis for expr: {:?} ty: {:?}", ex, ty); | ||
| self.search_for_structural_match_violation(ty); |
There was a problem hiding this comment.
Heh, this is a lot like const-qualification (cc @oli-obk @RalfJung @ecstatic-morse).
Maybe one day we'll have an unified system for refining a trait bound based on values.
eddyb
reviewed
Nov 6, 2019
| @@ -207,3 +301,186 @@ impl<'a, 'tcx> TypeVisitor<'tcx> for Search<'a, 'tcx> { | |||
| false | |||
| } | |||
| } | |||
|
|
|||
| struct SearchHirExpr<'a, 'tcx> { | |||
There was a problem hiding this comment.
Not sure how I feel about this code being in rustc::ty, but that's neither here nor there.
What I might expect is to find this is in rustc_mir::hair, perhaps even operating on patterns extracted from the const expressions (to avoid having to reason about "expressions as patterns" in more than one place).
However, that might actually be a cyclic dependency of sorts, so potentially there's nothing to do here, for now.
There was a problem hiding this comment.
I'll look into this. (I had thought there was some motivation for making this accessible more broadly than would be possible if it lived in rustc_mir, but I should double-check that.)
There was a problem hiding this comment.
Actually, I just realized something - are we removing that Expr -> Pat conversion / was it already removed, on the basis that if we enforce #[structural_match] we can just use == instead?
Because then I think mir_const_qualif is probably the way to go, and then we don't have to look at HIR expressions and/or try to convert them into patterns.
There was a problem hiding this comment.
We have not removed the Expr -> Pat conversion, and we cannot use == instead in all cases, at least not today due to compiler bugs and/or language limitations.
The particular case I am thinking of is that we cannot implement a blanket PartialEq over all for <'a> fn(...), but we do allow those to appear in patterns.
This leads to some cases where we ICE today, and other cases where we just rely on the Expr -> Pat conversion to deal with the codegen.
References:
eddyb
reviewed
Nov 6, 2019
| self.recur_into_const_rhs(const_def_id); | ||
| } else { | ||
| // abstraction barrer for non-local definition; | ||
| // traverse `typeof(expr)` instead. |
There was a problem hiding this comment.
mir_const_qualif gets around this by encoding the relevant bits (literally, 2 bits) cross-crate, perhaps something similar could be done here?
You could almost just use mir_const_qualif directly (and add the analysis to it), but with everything being in flux that's terrible advice for now.
There was a problem hiding this comment.
I'll file a bug suggesting this change for longer-term.
eddyb
approved these changes
Nov 6, 2019
|
(I don't understand why the test fails; I deliberately put in |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
pnkfelix
added
S-waiting-on-author
|
Ping from triage: |
|
I haven't managed to replicate the test failure locally. And meanwhile other people have been exploring how to implement this analogous to how we do |
|
I will just close this PR for now. If I decide to invest time in figuring out the warning then I'll revive it, but it no longer seems like a high priority item, and keeping the PR open is just making headaches for others. |
|
(heh, weird; I finally managed to replicate the warnings, but I don't think I did anything differently... except maybe I rebased? I wonder if that was the problem the whole time...) |
|
(reopened in #67088. I've started looking at how the const-qualif based analysis would work, thanks to @ecstatic-morse for assistance there. But progress there has been not been quite as fast as I thought it would be, so figure I'm better off trying to land this now and doing the const-qualif stuff myself as follow-up.) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Long awaited fix to #62614
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.Note: It is entirely possible that we will decide that even this analysis is not as precise as what we would like. (For example, we may wish to instead perform the analysis by inspecting the actual MIR value that you get from evaluating the const in question. However, it is not obvious to me that such an approach is superior from a UX point of view.) So I decided that there was no harm in trying to put this version up for review.