Stream: wg-async-foundations

Topic: pr #67911 - duplicate closure regions


Aaron Hill (Jan 07 2020 at 18:44, on Zulip):

@nikomatsakis let me know when you're available to talk about PR https://github.com/rust-lang/rust/pull/67911

nikomatsakis (Jan 07 2020 at 18:45, on Zulip):

@Aaron Hill ok, I can't prob chat too much now, but one immediate question I had was whether we believe this to be primarily a matter of diagnostics? I guess I have to re-read your original description. Is it the case that the error (with your PR) is processed and reported in the closuer's owner, rather than within the closure?

Aaron Hill (Jan 07 2020 at 18:46, on Zulip):

I believe it's reported within the closure itself

nikomatsakis (Jan 07 2020 at 18:46, on Zulip):

before and after?

Aaron Hill (Jan 07 2020 at 18:46, on Zulip):

yes

Aaron Hill (Jan 07 2020 at 18:46, on Zulip):

The current diagnostic isn't "wrong", exactly - it's just suboptimal, since the computed upper bound is overly conservative

Aaron Hill (Jan 07 2020 at 18:47, on Zulip):

Specifically, the code gives up trying to find an upper bound, since it doesn't know that some of the regions involved are actually "the same"

Aaron Hill (Jan 07 2020 at 18:47, on Zulip):

Regarding your comment on the PR - I'm not sure that I understand how erasing regions affects this

Aaron Hill (Jan 07 2020 at 18:48, on Zulip):

If we were to erase regions earlier, wouldn't we still need to somehow remember which regions are actually the same?

Aaron Hill (Jan 07 2020 at 18:50, on Zulip):

e.g. code like fn bar<'a>(first: &'a u8, second: &'a u8) { || do_something(first, second); } needs to know that 'first' and 'second' have the same region

Aaron Hill (Jan 07 2020 at 18:52, on Zulip):

To answer your earlier question - yes, I believe this is entirely a matter of diagnostics. The current non-diagnostic code seems perfectly correct - however, the information we need ('more precise' knowledge of region relationships) is not available while we're checking the closure. Changing the diagnostic code seemed to be the simplest way of resolving this

nikomatsakis (Jan 07 2020 at 18:56, on Zulip):

e.g. code like fn bar<'a>(first: &'a u8, second: &'a u8) { || do_something(first, second); } needs to know that 'first' and 'second' have the same region

it doesn't know that now, right?

nikomatsakis (Jan 07 2020 at 18:56, on Zulip):

Ideally, it would not know that

nikomatsakis (Jan 07 2020 at 18:56, on Zulip):

By "it" here, I specifically mean the closure

Aaron Hill (Jan 07 2020 at 18:57, on Zulip):

the closure doesn't know now

nikomatsakis (Jan 07 2020 at 18:57, on Zulip):

The outer fn item does remember those details (i.e., the things that were hand-written by the user)

Aaron Hill (Jan 07 2020 at 18:57, on Zulip):

right

nikomatsakis (Jan 07 2020 at 18:57, on Zulip):

the closure would communicate back that they must be equal (if that is required locally)

nikomatsakis (Jan 07 2020 at 18:57, on Zulip):

and then the outer fn item would determine if that can be made to be true

nikomatsakis (Jan 07 2020 at 18:57, on Zulip):

that's the intended design, at least

nikomatsakis (Jan 07 2020 at 18:57, on Zulip):

in part because it's not easy to tell if they are equal -- we don't know at the point where we are building MIR, or we won't know

nikomatsakis (Jan 07 2020 at 18:58, on Zulip):

because of subtyping etc

nikomatsakis (Jan 07 2020 at 18:58, on Zulip):

consider e.g.

nikomatsakis (Jan 07 2020 at 18:58, on Zulip):
fn foo(x: &'a u32, y: &'a u32) {
    let x1 = x;
    let y1 = y;
    || ...(x1, y1)...
}
nikomatsakis (Jan 07 2020 at 18:59, on Zulip):

here, the type of the captured value needs to be inferred

nikomatsakis (Jan 07 2020 at 18:59, on Zulip):

(but we don't want to be doing that inference until borrowck/polonius etc)

nikomatsakis (Jan 07 2020 at 18:59, on Zulip):

anyway I have to go and understand how it is that your PR improves diagnostics specifically I guess

Aaron Hill (Jan 07 2020 at 19:01, on Zulip):

@nikomatsakis: See here: https://github.com/rust-lang/rust/blob/56446fef49d73212f63ea7aa0680d5d602f19b9a/src/librustc_mir/borrow_check/diagnostics/explain_borrow.rs#L332

Aaron Hill (Jan 07 2020 at 19:01, on Zulip):

Being able to compute a correct upper bound allows this branch to succeed

Aaron Hill (Jan 07 2020 at 19:01, on Zulip):

which allows us to create a BorrowExplanation::MustBeValidFor instead of a BorrowExplanation::Unexplained

Aaron Hill (Jan 07 2020 at 19:02, on Zulip):

the BorrowExplanation gets checked later on when producing the message, and allows us to emit a more specific diagnostic

Aaron Hill (Jan 07 2020 at 19:02, on Zulip):

w.r.t erasing regions - if type check were to erase the regions, where would the original relationships between the regions be stored?

Aaron Hill (Jan 07 2020 at 19:03, on Zulip):

Would the closure parent still have the information needed to check the propagated constraints, or would that be done elsewhere?

Aaron Hill (Jan 14 2020 at 18:37, on Zulip):

@nikomatsakis let me know when you're available to talk more about this

Aaron Hill (Jan 14 2020 at 18:38, on Zulip):

Is there a branch somewhere that contains code for erasing regions during typecheck? Or a description somewhere of what those changes will look like?

Aaron Hill (Jan 14 2020 at 18:41, on Zulip):

If I understand correctly, typecheck will erase the regions (so Closure { upvar_a: Foo<'a>, upvar_b: Bar<'b> } becomes Closure { upvar_a: Foo<'erased>, upvar_b: Bar<'erased>. Then, borrowcheck will create fresh inference variables: Closure { upvar_a: Foo<_#1r>, upvar_b: Bar<_#2r>

Aaron Hill (Jan 14 2020 at 18:42, on Zulip):

If that's the case, I think we could still propagate the necessary information for diagnostics. When we erase regions, we could mark which 'positions' (given by the order in which the regions are visited) correspond to the same region

Aaron Hill (Jan 14 2020 at 18:43, on Zulip):

e.g. for Closure { first: Foo<'a>, second: Bar<'a>, third: Foo<'b> } we would record something like identical_regions: [ {0, 1}, {2} ]

Aaron Hill (Jan 14 2020 at 18:44, on Zulip):

During borrowcheck, we would associate this information with the freshly created region variables

Aaron Hill (Jan 14 2020 at 18:46, on Zulip):

The only other solution I can think of would be to use the context of the parent item when running the diagnostic code for closure errors. That would give us access to the 'identical' region information - however, I think we would also lose all information about 'interior' regions in the closure

Aaron Hill (Jan 14 2020 at 18:47, on Zulip):

The borrowcheck region error code seems quite complicated, so I think it would require a large amount of effort to prevent any diagnostic regressions

lqd (Jan 14 2020 at 19:40, on Zulip):

Is there a branch somewhere that contains code for erasing regions during typecheck? Or a description somewhere of what those changes will look like?

IIUC one of the first steps is https://github.com/rust-lang/rust/pull/67681, and the plan is currently being worked on

Last update: Jul 02 2020 at 19:40UTC