generator_interior: Count match pattern bindings as borrowed for the whole guard expression

[eholk]

The test case yielding-in-match-guard.rs was failing with -Zdrop-tracking enabled. 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

[eholk]

@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.

[eholk]

Niko just filed the related bug: #97092

[eholk]

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.

[nikomatsakis]

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).

[eholk]

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.

[nikomatsakis]

Yay for simpler

[nikomatsakis]

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).

[nikomatsakis]

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

[eholk]

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

[nikomatsakis]

@bors r+

[bors]

📌 Commit d9e370c49ed81bc0cf1ae326084e4408f18fe9e5 has been approved by nikomatsakis

[bors]

⌛ Testing commit d9e370c49ed81bc0cf1ae326084e4408f18fe9e5 with merge 8e3d2f943b6318e0c99d867be53bd8af5ecbe9cb...

[bors]

💔 Test failed - checks-actions

[nikomatsakis]

@bors r+

[bors]

📌 Commit 7d1dbdf has been approved by nikomatsakis

[bors]

⌛ Testing commit 7d1dbdf with merge f24ef2e...

[bors]

☀️ Test successful - checks-actions
Approved by: nikomatsakis
Pushing f24ef2e to master...

[rust-timer]

Finished benchmarking commit (f24ef2e): comparison url.

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

• Primary benchmarks: no relevant changes found
• Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	0	2	0	2	0
mean2	N/A	2.3%	N/A	-1.9%	N/A
max	N/A	3.0%	N/A	-2.1%	N/A

Cycles

Results

• Primary benchmarks: 😿 relevant regression found
• Secondary benchmarks: mixed results

	Regressions 😿 (primary)	Regressions 😿 (secondary)	Improvements 🎉 (primary)	Improvements 🎉 (secondary)	All 😿 🎉 (primary)
count1	1	1	0	1	1
mean2	4.2%	1.8%	N/A	-1.9%	4.2%
max	4.2%	1.8%	N/A	-1.9%	4.2%

If you disagree with this performance assessment, please file an issue in rust-lang/rustc-perf.

@rustbot label: -perf-regression

Footnotes

1. number of relevant changes ↩ ↩²

2. the arithmetic mean of the percent change ↩ ↩²