generator_interior: Count match pattern bindings as borrowed for the whole guard expression #97029
Conversation
|
@nikomatsakis and I talked about this in a call today. This doesn't seem like quite the right fix, but unfortunately there's not a way to observe it breaking. This will probably cause problems in the future though, so it's worth solving this correctly now. We think the best option is when entering a pattern guard, count all the bound variables in the pattern as immutably borrowed for the lifetime of the guard. That should be simpler and more precise than the way this PR currently works. We also found what seems to be a potential way to make UB, but @nikomatsakis is filing a separate bug on that. |
|
Niko just filed the related bug: #97092 |
| // Subtle: MIR desugaring introduces immutable borrows for each pattern | ||
| // binding when lowering pattern guards to ensure that the guard does not | ||
| // modify the scrutinee. | ||
| if has_guard { |
There was a problem hiding this comment.
The tests pass with or without this change, so arguably we should remove it. That said, this feels like the more correct behavior, so I think we should leave it in.
There was a problem hiding this comment.
Hmm. Yes, it seems correct, but it's interesting that the tests pass either way! There is a certain amount of redundancy, it seems, between the analysis of what is borrowed etc and the "what types appear in the generator".
I think perhaps we could observe the difference with this test, but maybe not?
I think this will compile successfully:
#![feature(generators)]
fn is_send<T: Send>(_: T) { }
fn main() {
is_send(async move {
let x = std::rc::Rc::new(22);
//match x { ref y if foo().await => () }
drop(x);
foo().await;
});
}
async fn foo() -> bool {
true
}but if you remove the comment from //match, it will not. I'm curious what happens if you remove this line here?
I guess that to really eliminate this redundancy we would drive the "do we need to include &T" check based on some set generated by this code (e.g., "is lvalue borrowed" or whatever).
There was a problem hiding this comment.
The test you provided does compile successfully. When I uncomment the match line, it gives a compiler error saying that the async block is not Send due to y and x being borrowed across the await point.
The behavior is the same with and without the if has_guard { ... } section in expr_use_visitor.rs.
| /// As such, we need to track these borrows and record them despite of the fact | ||
| /// that they may succeed the said yield point in the post-order. | ||
| guard_bindings: SmallVec<[SmallVec<[HirId; 4]>; 1]>, | ||
| guard_bindings_set: HirIdSet, |
| // Subtle: MIR desugaring introduces immutable borrows for each pattern | ||
| // binding when lowering pattern guards to ensure that the guard does not | ||
| // modify the scrutinee. | ||
| if has_guard { |
There was a problem hiding this comment.
Hmm. Yes, it seems correct, but it's interesting that the tests pass either way! There is a certain amount of redundancy, it seems, between the analysis of what is borrowed etc and the "what types appear in the generator".
I think perhaps we could observe the difference with this test, but maybe not?
I think this will compile successfully:
#![feature(generators)]
fn is_send<T: Send>(_: T) { }
fn main() {
is_send(async move {
let x = std::rc::Rc::new(22);
//match x { ref y if foo().await => () }
drop(x);
foo().await;
});
}
async fn foo() -> bool {
true
}but if you remove the comment from //match, it will not. I'm curious what happens if you remove this line here?
I guess that to really eliminate this redundancy we would drive the "do we need to include &T" check based on some set generated by this code (e.g., "is lvalue borrowed" or whatever).
|
r=me @eholk -- I think it's ok to land as is, but I'm also ok if you want to try a bit of further cleanup |
|
I think it makes sense to go ahead and merge this and investigate the test case you suggested in a followup. I'll try to look into it tomorrow or sometime this week. @bors r=nikomatsakis |
|
@bors r+ |
|
📌 Commit d9e370c49ed81bc0cf1ae326084e4408f18fe9e5 has been approved by |
|
⌛ Testing commit d9e370c49ed81bc0cf1ae326084e4408f18fe9e5 with merge 8e3d2f943b6318e0c99d867be53bd8af5ecbe9cb... |
This comment has been minimized.
This comment has been minimized.
|
💔 Test failed - checks-actions |
Thanks to @tmiasko for this one!
This reverts commit 0d270b5e9f48268735f9a05462df65c9d1039855.
This is subsumed by the new changes that count pattern variables as bound for the whole guard expression.
d9e370c
to
7d1dbdf
Compare
|
@bors r+ |
|
📌 Commit 7d1dbdf has been approved by |
|
☀️ Test successful - checks-actions |
|
Finished benchmarking commit (f24ef2e): comparison url. Instruction countThis benchmark run did not return any relevant results for this metric. Max RSS (memory usage)Results
CyclesResults
If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf. @rustbot label: -perf-regression Footnotes |
The test case
yielding-in-match-guard.rswas failing with-Zdrop-trackingenabled. The reason is that the copy of a local (y) was not counted as a borrow in typeck, while MIR did consider this as borrowed.The correct thing to do here is to count pattern bindings are borrowed for the whole guard. Instead, what we were doing is to record the type at the use site of the variable and check if the variable comes from a borrowed pattern. Due to the fix for #57017, we were considering too small of a scope for this variable, which meant it was not counted as borrowed.
Because we now unconditionally record the borrow, rather than only for bindings that are used, this PR is also able to remove a lot of the logic around match bindings that was there before.
r? @nikomatsakis