Fix HIR visit order #61572
Merged
Fix HIR visit order #61572
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
|
r? @varkor (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors r+ |
|
📌 Commit ea20e1c008079d318aa10f0d24e06ac22f14fdc9 has been approved by |
bors
added
S-waiting-on-bors
|
The original order is correct for built-in operators. This looks like it would cause the same issue for something like |
|
@bors r- |
bors
added
S-waiting-on-author
|
Let's make sure we add a test for @matthewjasper's example. |
|
Unfortunately, my previous approach turned out to be completely incompatible with the fact that |
|
@varkor: This should be ready for review |
|
@Aaron1011 Are you able to test this PR against #61579 (and maybe #61731)? |
Centril
reviewed
Jun 11, 2019
| @@ -0,0 +1,28 @@ | |||
| #![feature(generators)] | |||
There was a problem hiding this comment.
I really don't like having a bunch of numbered issues in run-pass (instead of in ui/) with zero description of what this is supposed to test...
EDIT: This sounded a bit harsh; What I'm critiquing here is our regular practices around numbered issues/issue-XX.rs.
There was a problem hiding this comment.
I think this test should be moved to the other generator tests wherever they are.
|
This inconsistency seems like a bug to me. cc @rust-lang/lang |
This is #28160 if I'm not mistaken. I didn't follow this closely in the last couple of years, but I expected the order to be unified together with introduction of NLL and two-phase borrows. |
Centril
added
I-nominated
T-lang
|
(The |
|
We discussed this on Thursday's language team meeting. Based on the understanding that:
we feel comfortable with landing this specific PR. However, we would like discuss and settle the execution order of |
|
ping @Centril @Aaron1011 What needs to be done to land this? I'd appreciate it if you could up the priority on this one since it is an ICE which prevents compilation with no obvious workaround (and in particular, it means we can't compile one of our projects). |
|
@nrc I read all the comments above as resolved. @varkor's only comment was
and a test has since been added. The language team was okay with this going through, and the MIR ICE tests that were asked for are actually unrelated issues solved by #61872. With that in mind, I think this is just waiting on @varkor to r+. |
Aaron1011
force-pushed
the
fix/generator-ref
branch
from
b2f18f4
to
93aa60b
Compare
|
@Centril: I've moved the test as you requested. |
varkor
suggested changes
Jun 23, 2019
src/librustc/middle/region.rs
Outdated
| // to store any scopes we encounter when visiting the inenr expressions | ||
| // of the AssignOp. Once we finish visiting the inner expressions, we pop | ||
| // off the inner Vec, and process the Scopes it contains. | ||
| // This allows us to handle nested AssignOps - while a terrible idea, |
There was a problem hiding this comment.
(while a terrible idea — citation needed 😁)
| let live_across_yield = scope.map(|s| { | ||
| self.region_scope_tree.yield_in_scope(s).and_then(|yield_data| { | ||
|
|
||
|
|
| *{ yield; x } += match String::new() { _ => 0 }; | ||
| }; | ||
|
|
||
| // Please don't ever actually write something like this |
src/librustc/middle/region.rs
Outdated
| debug!("resolve_expr - enabling pessimistic_yield, was previously {}", | ||
| prev_pessimistic); | ||
|
|
||
| visitor.fixup_scopes.push(vec![]); |
There was a problem hiding this comment.
Instead of using a Vec<Vec<_>>, can we use a single Vec<_> as a stack, and just iterate over the new Scopes?
There was a problem hiding this comment.
This way, it would be easier to combine fixup_scopes and pessimistic_yield too.
There was a problem hiding this comment.
That would require me to also keep track of the starting index for new scopes. I think using a Vec<Vec<_>> makes the intention clear, and avoids any off-by-one errors that might otherwise occur.
There was a problem hiding this comment.
We'd only need to store the starting indices in a local variable, rather than a visitor, which should make things clear, I think. I imagine it'll reduce allocation too.
| @@ -947,12 +960,108 @@ fn resolve_expr<'tcx>(visitor: &mut RegionResolutionVisitor<'tcx>, expr: &'tcx h | |||
| } | |||
| } | |||
|
|
|||
| let prev_pessimistic = visitor.pessimistic_yield; | |||
|
|
|||
| // Ordinarily, we can rely on the visit order of HIR intravisit | |||
There was a problem hiding this comment.
This is a very clear explanation 👍
Co-Authored-By: varkor <github@varkor.com>
|
@varkor: I've eliminated the |
|
@bors r+ |
|
📌 Commit 770655a has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit 770655a with merge ab178dc3cf45860d357b9789c8ed4bdcc035b82c... |
|
💔 Test failed - checks-travis |
bors
added
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 |
|
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Jun 25, 2019
Fix HIR visit order Fixes #61442 When rustc::middle::region::ScopeTree computes its yield_in_scope field, it relies on the HIR visitor order to properly compute which types must be live across yield points. In order for the computed scopes to agree with the generated MIR, we must ensure that expressions evaluated before a yield point are visited before the 'yield' expression. However, the visitor order for ExprKind::AssignOp was incorrect. The left-hand side of a compund assignment expression is evaluated before the right-hand side, but the right-hand expression was being visited before the left-hand expression. If the left-hand expression caused a new type to be introduced (e.g. through a deref-coercion), the new type would be incorrectly seen as occuring *after* the yield point, instead of before. This leads to a mismatch between the computed generator types and the MIR, since the MIR will correctly see the type as being live across the yield point. To fix this, we correct the visitor order for ExprKind::AssignOp to reflect the actual evaulation order.
|
☀️ Test successful - checks-travis, status-appveyor |
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.
Fixes #61442
When rustc::middle::region::ScopeTree computes its yield_in_scope
field, it relies on the HIR visitor order to properly compute which
types must be live across yield points. In order for the computed scopes
to agree with the generated MIR, we must ensure that expressions
evaluated before a yield point are visited before the 'yield'
expression.
However, the visitor order for ExprKind::AssignOp
was incorrect. The left-hand side of a compund assignment expression is
evaluated before the right-hand side, but the right-hand expression was
being visited before the left-hand expression. If the left-hand
expression caused a new type to be introduced (e.g. through a
deref-coercion), the new type would be incorrectly seen as occuring
after the yield point, instead of before. This leads to a mismatch
between the computed generator types and the MIR, since the MIR will
correctly see the type as being live across the yield point.
To fix this, we correct the visitor order for ExprKind::AssignOp
to reflect the actual evaulation order.