Only consider yields coming after the expressions when computing generator interiors #44392
Conversation
|
(rust_highfive has picked a reviewer for you, use r? to override) |
| } | ||
|
|
||
| impl<'a, 'gcx, 'tcx> InteriorVisitor<'a, 'gcx, 'tcx> { | ||
| fn record(&mut self, ty: Ty<'tcx>, scope: Option<region::Scope>, expr: Option<&'tcx Expr>) { | ||
| use syntax_pos::DUMMY_SP; | ||
|
|
||
| if self.fcx.tcx.sess.verbose() { | ||
| let span = scope.map_or(DUMMY_SP, |s| s.span(self.fcx.tcx, &self.region_scope_tree)); |
| } | ||
|
|
||
| impl<'a, 'gcx, 'tcx> InteriorVisitor<'a, 'gcx, 'tcx> { | ||
| fn record(&mut self, ty: Ty<'tcx>, scope: Option<region::Scope>, expr: Option<&'tcx Expr>) { | ||
| use syntax_pos::DUMMY_SP; | ||
|
|
||
| if self.fcx.tcx.sess.verbose() { | ||
| let span = scope.map_or(DUMMY_SP, |s| s.span(self.fcx.tcx, &self.region_scope_tree)); | ||
| self.fcx.tcx.sess.span_warn(span, &format!("temporary scope for node id {:?}", expr)); |
There was a problem hiding this comment.
Also, this is not what verbose is meant to be used for. Can you just remove this, or else use debug!?
| self.region_scope_tree.yield_in_scope(s) | ||
| self.region_scope_tree.yield_in_scope(s).and_then(|(span, expr_count)| { | ||
| // Check if the span in the region comes after the expression | ||
| if expr_count > self.expr_count { |
There was a problem hiding this comment.
This makes me pretty uncomfortable. It's unclear that a "count" of the number of expressions is a good way of judging control-flow. I'd feel better if we used just the statements of a block: this would make simple stuff like
fn bar(baz: String) -> impl Generator<Yield=(), Return=()> {
move || {
yield drop(&baz);
}
}
work fine, but wouldn't get the 'intra-statement' ordering. This seems .. kind of ok? Or do you think it's not good enough?
There was a problem hiding this comment.
I suspect we'd want to be more flexible here. await! will probably be nested inside expressions, unlike plain yield which doesn't have a useful result.
| impl<'a, 'gcx, 'tcx> Visitor<'tcx> for InteriorVisitor<'a, 'gcx, 'tcx> { | ||
| fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { | ||
| NestedVisitorMap::None | ||
| } | ||
|
|
||
| fn visit_body(&mut self, _body: &'tcx Body) { |
There was a problem hiding this comment.
Why are you recurring into closures? Can't you consider each closure separately?
There was a problem hiding this comment.
I realized that closures / generators aren't visited if nested_visit_map returns None, so I just removed this code.
| } | ||
|
|
||
| impl<'a, 'gcx, 'tcx> InteriorVisitor<'a, 'gcx, 'tcx> { | ||
| fn record(&mut self, ty: Ty<'tcx>, scope: Option<region::Scope>, expr: Option<&'tcx Expr>) { | ||
| use syntax_pos::DUMMY_SP; | ||
|
|
||
| if self.fcx.tcx.sess.verbose() { | ||
| let span = scope.map_or(DUMMY_SP, |s| s.span(self.fcx.tcx, &self.region_scope_tree)); | ||
| self.fcx.tcx.sess.span_warn(span, &format!("temporary scope for node id {:?}", expr)); |
| self.expr_count += 1; | ||
|
|
||
| if self.fcx.tcx.sess.verbose() { | ||
| self.fcx.tcx.sess.span_warn(expr.span, &format!("node id {:?}", expr.id)); |
| let scope = self.region_scope_tree.temporary_scope(expr.hir_id.local_id); | ||
|
|
|
|
||
| fn bar(baz: String) -> impl Generator<Yield=(), Return=()> { | ||
| move || { | ||
| yield drop(&baz); |
There was a problem hiding this comment.
I'm afraid you can do
struct DropBomb;
impl Drop for DropBomb {
fn drop(&mut self) {
println!("splat!");
}
}
fn bar() -> impl Generator<Yield=(), Return=()> {
move || {
yield &DropBomb;
}
}Because yield is currently not a terminating scope, this would only get dropped after the generator returns. If DropBomb was e.g. !Send, this can obviously cause trouble.
This can be solved by making yield a terminating scope, which would force temporaries in all of its arguments to be destroyed before it is reached (search for terminating_scope in region).
There was a problem hiding this comment.
We may want to allow generators to return references to locals, in which case having DropBomb is live across the suspension point would be useful. I changed the ordering so the suspension point comes after the yield subexpression making &DropBomb and DropBomb a part of the generator interior.
src/librustc/middle/region.rs
Outdated
| @@ -735,6 +751,8 @@ fn resolve_stmt<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, stmt: | |||
| fn resolve_expr<'a, 'tcx>(visitor: &mut RegionResolutionVisitor<'a, 'tcx>, expr: &'tcx hir::Expr) { | |||
| debug!("resolve_expr(expr.id={:?})", expr.id); | |||
|
|
|||
| visitor.expr_count += 1; | |||
There was a problem hiding this comment.
I find this mechanism somewhat hacky (traversing the CFG in preorder to gather this ordering). It would be nice to at least justify why it works:
- execution can't go backwards in node postorder, except through a backedge (aka loop)
- if node A is preorder-before node B, but node B is postorder-before node A, then A is a parent of B. This comes from the definition of a DFS.
Therefore, assuming that expr A is preorder-after a yield, we want to show that the results of expr A can't be used "over" the yield. There are 2 cases:
- if expr A is postorder-after the yield, then it can only reach the yield across a loop it is in. But a loop is a terminating scope and drops all temporaries created within it.
- if the yield is postorder-after expr A, then the yield is a parent of expr A. If we make yields a terminating scope (see the borrow-in-yield-expr example), then the terminating scope would destroy the temporary created by expr A.
There was a problem hiding this comment.
I'm still a bit uncomfortable with the hackiness here.
There was a problem hiding this comment.
Oops. Assignments are evaluated RTL, so you can do
*{ yield; x } = &y;And &y would be stored over the yield.
Maybe use the old CFG and do dataflow over it? That would be ugly, but work.
There was a problem hiding this comment.
Visitor goes over subexpressions in RTL, so this actually works
Now there's this (archaic in incorrect but correct-in-spirit) comment, which justifies the property:
rust/src/librustc/hir/intravisit.rs
Lines 29 to 34 in d93036a
|
Note that MIR regionck won't much improve the situation - we'll still want to figure out interior types before we finish type-checking. |
|
I think the pre-order numbering annoys my aesthetic sense more than the CFG preorder traversal. If you can find a nice way of doing this without matching numbers between the 2 visitors, that would make me more comfortable. The CFG ordering thing looks fairly easy to guarantee going forward, and because we need something like it to avoid MIR regionck dependency cycles, we probably want that. |
alexcrichton
added
the
S-waiting-on-author
|
☔ The latest upstream changes (presumably #43742) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Can you elaborate btw on what is wrong with the comment regarding the traversal being a "reverse-post-order on the CFG"? I think on IRC you said it was impossible to do, but I'm not a priori sure why that should be. Regarding your explanation in this comment: what you wrote makes sense, but I'm feeling a bit confused about some of the particulars of your terminology. For example, when you talk about a "pre-order", do you mean on HIR tree or on the underlying control-flow-graph we will generate? I sort of assume that you meant the tree. But of course the property we are interested in is most naturally expressed in terms of the CFG: at the point where the yield occurs, what temporaries may still be live? I guess I am still trying to work out the best way to understand the property we are proving here. Clearly we are trying to relate the visit order over the HIR to the eventual execution paths in the CFG. I think it'd be helpful to me to try and state those properties in terms of CFG nodes and relate them to the HIR. For example, a given HIR node has "parts" of its execution occuring at different times relative to its children, so I'm not 100% sure what it means for execution to go backwards or forwards relative to the postorder on the HIR tree. So maybe we can use some terminology like this: Let us use In this case, I believe that some basic properties like this hold (unless I'm overlooking something):
But I'm not quite sure where to go from here yet. =) |
|
I'm not too concerned about the soundness of this approach. We could add a check in borrowck which catches borrows across yields and the sanity check in the generator transformation will catch types live across suspension points. So if this is wrong, we could ICE instead of generating bad code. I'm not sure if this is the approach I'd like long term for calculating generator interiors types. I want to generate MIR during typeck, possibly run drop elaboration on that, and use a liveness analysis on that MIR which is aware of (NLL) borrows. |
First, this is a visitor, which makes a tree of calls to Actually, I think a post-order traversal of the tree is compatible with an RPO traversal of the CFG, so that's actually correct.
pre-order/post-order indeed referred to pre-order/post-order in the tree. By inspection of the CFG, CFG edges only go "forward" in the post-order tree traversal, except for loop back edges (e.g. if we see that tree post-order is actually RPO, then the only edges that go against RPO are loop back edges). Such a back-edge going over a temporary would erase it. |
We can map CFG nodes to their respective HIR node. Let call that map We may care about which HIR nodes are "currently being executed". Because HIR nodes can't overlap, we can define What we care about here is the set of CFG nodes during which "borrowck operations" take place. Let's call that |
|
Now with a proof. Turns out we don't need the backedge property after all. |
arielb1
force-pushed
the
yield-order
branch
2 times, most recently
from
b2dd1cf to
d3dd847
Compare
|
(Just to connect to the issue that @alex opened against futures-await, this does not fix alexcrichton/futures-await#18 ) |
| /// we might visit `U` without ever getting to `D`). | ||
| /// 3. However, we guarantee that at each HIR point, each | ||
| /// binding/temporary is always either always storage-live | ||
| /// or always storage-dead. This is what is being guaranteed |
There was a problem hiding this comment.
This is true modulo moves, I suppose.
There was a problem hiding this comment.
storage-live does not consider moves.
src/librustc/middle/region.rs
Outdated
| /// at yield-points at these indexes. | ||
| /// | ||
| /// Let's show that: let `D` be our binding/temporary and `U` be our | ||
| /// other HIR node, with `HIR-postorder(U) < HIR-postorder(D)`. |
There was a problem hiding this comment.
I think this comment would be improved with a simple example. Let me try to work one out live here:
Consider this Rust program. We want to check that, at the time in which yield x executes, the temporary storing 22 has not been initialized, because we don't want to store the 22 in our generator state (if it helps, imagine that 22 is rather some value with a destructor that has to execute).
foo(3, yield x, bar(&22))This results in a tree like so:
- HA: call
- HB:
foo - HC:
3 - HD:
yield- HE:
x
- HE:
- HF:
bar- HG:
&- HH:
22
- HH:
- HG:
- HB:
With the following traversal orders:
- Preorder traversal: HA, HB, HC, HD, HE, HF, HG, HH
- Postorder traversal: HB, HC, HE, HD, HH, HG, HF, HA
Therefore, here, we will let D be the HIR node HH, which creates a temporary whose scope is equal to HA. Then let U be the HIR node HD where the yield occurs. You can see that HD appears before HH in the post-order traversal. We therefore surmise that the temporary created by HH must not yet have been initialized.
To see why this is correct, consider:
- (your bullet points here)
There was a problem hiding this comment.
(I found it really helpful when reading the later logic to relate to that example)
src/librustc/middle/region.rs
Outdated
| /// | ||
| /// Then: | ||
| /// 1. From the ordering guarantee of HIR visitors (see | ||
| /// `rustc::hir::intravisit`), `D` does not dominate `U`. |
There was a problem hiding this comment.
All we know is that U comes before D in the postorder, therefore we know that U may end before D ends in execution order. From this we are concluding that End(D) does not dominate End(U), right? That makes sense.
There is a kind of unwritten assumption that the value produced by D is not produced until End(D), I think, right? (Which makes sense.)
There was a problem hiding this comment.
There is a kind of unwritten assumption that the value produced by
Dis not produced untilEnd(D), I think, right? (Which makes sense.)
Yeah.
| @@ -12,12 +12,10 @@ | |||
|
|
|||
| fn foo(_a: (), _b: &bool) {} | |||
|
|
|||
| // Some examples that probably *could* be accepted, but which we reject for now. | |||
|
|
|||
| fn bar() { | |||
There was a problem hiding this comment.
I think a comment would be good here to explain the purpose of this test.
For example:
// Test that a yield occuring in the list of arguments *before* a borrow
// does not cause a borrowck error about having a live borrow.
Sorta obvious from the name but still useful sometimes.
|
@arielb1 changed the code so that only yields after the expression is considered. It makes sense that the result of |
|
I thought |
|
And |
|
I think desugared placement new just makes this ugly. It creates a "desugared" |
|
We could not desugar box I think because of type inference interactions. I sort of forget but I think maybe @eddyb had a good idea in that respect ... |
…opes in bindings.
I don't think the "quasi-postorder" travesal could cause any issues, but there's no reason for it to stay broken.
arielb1
force-pushed
the
yield-order
branch
from
d3dd847 to
2384dd9
Compare
|
@bors r+ |
|
📌 Commit 2384dd9 has been approved by |
Only consider yields coming after the expressions when computing generator interiors When looking at the scopes which temporaries of expressions can live for during computation of generator interiors, only consider yields which appear after the expression in question in the HIR.
|
☀️ Test successful - status-appveyor, status-travis |
When looking at the scopes which temporaries of expressions can live for during computation of generator interiors, only consider yields which appear after the expression in question in the HIR.