Stream: t-compiler/wg-nll

Topic: issue-51027-closure-region-errors


nikomatsakis (Jul 19 2018 at 17:33, on Zulip):

@David Wood so I was thinking about #51027 and what error to print

nikomatsakis (Jul 19 2018 at 17:34, on Zulip):

I sorta like something like this

error: borrowed data cannot be stored outside of its closure
  --> issue-45983.rs:17:27
   |
16 |     let mut x = None;
   |         ----- `x` is declared here, outside of the closure body
17 |     give_any(|y| x = Some(y));:
   |               -  ^^^^^^^^^^^ `y` escapes the closure body here
   |               |
   |               `y` is a reference that is only valid in the closure body
nikomatsakis (Jul 19 2018 at 17:34, on Zulip):

anyway I thought I would drop a few notes on how to identify the scenario

nikomatsakis (Jul 19 2018 at 17:34, on Zulip):

I guess we can fine-tune the wording in the PR etc

davidtwco (Jul 19 2018 at 17:34, on Zulip):

Yeah, I think that is a pretty clear error.

davidtwco (Jul 19 2018 at 17:35, on Zulip):

You mentioned how to identify the scenario briefly in the existing notes w/r/t what sub, sup and the most interesting constraint should be.

nikomatsakis (Jul 19 2018 at 17:36, on Zulip):

oh, right, I did, cool

nikomatsakis (Jul 19 2018 at 17:37, on Zulip):

this may take some refactoring, looking more at the code

nikomatsakis (Jul 19 2018 at 17:38, on Zulip):

right now, when we add a label like "let's call the lifetime of this reference '1"

nikomatsakis (Jul 19 2018 at 17:38, on Zulip):

we have code that both searches and adds the label to the diagnostic

nikomatsakis (Jul 19 2018 at 17:38, on Zulip):

I was wondering if that was the right approach

nikomatsakis (Jul 19 2018 at 17:38, on Zulip):

it is convenient because it lets us handle many kinds of scenarios

nikomatsakis (Jul 19 2018 at 17:38, on Zulip):

but if we want to tweak the langage -- e.g., to say something like "this reference is only valid during the closure body"

nikomatsakis (Jul 19 2018 at 17:39, on Zulip):

it's a bit annoying

nikomatsakis (Jul 19 2018 at 17:39, on Zulip):

might be nicer if we passed things back up -- e.g., I found the lifetime at this span etc

nikomatsakis (Jul 19 2018 at 17:39, on Zulip):

though I guess we can pass down a flavor as well

davidtwco (Jul 19 2018 at 17:39, on Zulip):

Makes sense.

nikomatsakis (Jul 19 2018 at 17:44, on Zulip):

ok I wrote some additional notes here

davidtwco (Jul 19 2018 at 17:51, on Zulip):

Great, thanks.

davidtwco (Jul 19 2018 at 19:30, on Zulip):

@nikomatsakis Am I right in thinking that -Z dump-mir=all should output a .dot file?

davidtwco (Jul 19 2018 at 19:35, on Zulip):

Hmm, seems like something strange to do with RUST_LOG and piping the output of rustc into less meant that it didn't output it.

davidtwco (Jul 19 2018 at 19:35, on Zulip):

Anyway..

nikomatsakis (Jul 19 2018 at 19:41, on Zulip):

weird

davidtwco (Jul 20 2018 at 17:05, on Zulip):

@nikomatsakis I've submitted #52572 that has the bulk of the issue solved. There are two tests mentioned in the issue that it doesn't yet handle and there's a note I've added for a change that I wasn't sure about. I had a blast working on this one, not sure why.

davidtwco (Jul 20 2018 at 17:05, on Zulip):

I don't know if it passes all the tests, I only ran the ui ones locally.

davidtwco (Jul 20 2018 at 18:17, on Zulip):

For the issue-7573.rs case that currently isn't working, I understand what is happening, but I don't know how to work out what I want to work out. We've got this mir for the closure:

fn remove_package_from_database::{{closure}}(_1: &mut [closure@src/test/ui/borrowck/issue-7573.rs:29:19: 35:6 lines_to_use:&mut std::vec::Vec<&CrateId>], _2: &CrateId) -> (){
    let mut _0: ();                      // return place
    let mut _3: ();
    let mut _4: &mut std::vec::Vec<&CrateId>;
    let mut _5: &CrateId;

    bb0: {
                                         | Live variables on entry to bb0[0]: [_1, _2]
        StorageLive(_4);                 // bb0[0]: scope 0 at src/test/ui/borrowck/issue-7573.rs:32:9: 32:21
                                         | Live variables on entry to bb0[1]: [_1, _2]
        _4 = &mut (*((*_1).0: &mut std::vec::Vec<&CrateId>)); // bb0[1]: scope 0 at src/test/ui/borrowck/issue-7573.rs:32:9: 32:21
                                         | Live variables on entry to bb0[2]: [_2, _4]
        StorageLive(_5);                 // bb0[2]: scope 0 at src/test/ui/borrowck/issue-7573.rs:32:27: 32:39
                                         | Live variables on entry to bb0[3]: [_2, _4]
        _5 = &(*_2);                     // bb0[3]: scope 0 at src/test/ui/borrowck/issue-7573.rs:32:27: 32:39
                                         | Live variables on entry to bb0[4]: [_4, _5]
        _3 = const <std::vec::Vec<T>>::push(move _4, move _5) -> [return: bb2, unwind: bb1]; // bb0[4]: scope 0 at src/test/ui/borrowck/issue-7573.rs:32:9: 32:40
        // ...more stuff...

In the above mir, _3 = const <std::vec::Vec<T>>::push(move _4, move _5) is the problematic line, it's trying to push _5 into _4 which won't work. _4 is a mutable borrow of _1 which is the captured upvar.

My current code that detects whether we're in an "escaping data" scenario using is_upvar_field_projection. In this case, with a function call, I check whether the receiver is an upvar field projection - ie . it will check whether the place for _4 is an upvar using is_upvar_field_projection.

However, _4 is a Place::Local(..), not a Place::Projection(..). So I'd need to work out (somehow) that _4 is just a mutable borrow of _1 which I can then check as normal with is_upvar_field_projection. After that, everything just falls out of the rest of the code from earlier commits. I'm not sure how to associate _4 with _1 to do this check though - I start with TerminatorKind::Call.

nikomatsakis (Jul 21 2018 at 03:43, on Zulip):

@David Wood that looks awesome!

nikomatsakis (Jul 21 2018 at 11:49, on Zulip):

@David Wood you around by any chance?

davidtwco (Jul 21 2018 at 11:51, on Zulip):

yeah, I'm around.

nikomatsakis (Jul 21 2018 at 11:52, on Zulip):

cool, I was reviewing your PR and I had a question

nikomatsakis (Jul 21 2018 at 11:52, on Zulip):

thogh I may have answered it now

nikomatsakis (Jul 21 2018 at 11:55, on Zulip):

I left a review in any case, mostly a request for some more docs

nikomatsakis (Jul 21 2018 at 11:55, on Zulip):

your code doesn't trigger on a case like this, right?

fn foo(x: &mut Vec<&u32>) {
  bar(|y| x.push(y));
}

fn bar(_: impl FnOnce(&u32)) { }
davidtwco (Jul 21 2018 at 11:55, on Zulip):

No, that's similar to the issue-7573.rs case I mentioned above.

nikomatsakis (Jul 21 2018 at 11:56, on Zulip):

ah, ok, I can take a look

davidtwco (Jul 21 2018 at 11:56, on Zulip):

I attempted to add another variant to ConstraintCategory to handle this case but I was struggling to get is_upvar_field_projection to recognize that it was infact a upvar.

nikomatsakis (Jul 21 2018 at 11:56, on Zulip):

as I mentioned elsewhere I think my desktop in Boston has stopped responding to pings though :) so it'll take me longer than usual to do a build, since now I have to run on my laptop

nikomatsakis (Jul 21 2018 at 11:56, on Zulip):

I'm feeling grumpy about it :)

davidtwco (Jul 21 2018 at 11:57, on Zulip):

I work on my laptop all the time, you get used to slow builds.

nikomatsakis (Jul 21 2018 at 11:57, on Zulip):

heh

nikomatsakis (Jul 21 2018 at 11:57, on Zulip):

the other thing is having tons of active rust working directories :)

nikomatsakis (Jul 21 2018 at 11:58, on Zulip):

I am so excited about these new errors though...

nikomatsakis (Jul 21 2018 at 12:01, on Zulip):

right now my laptop is driving itself crazy trying to build LLVM, for the most part

davidtwco (Jul 21 2018 at 12:23, on Zulip):

FYI pushed a commit that addressed the documentation comments and the function returning an unneccesary Option<..>.

davidtwco (Jul 21 2018 at 12:24, on Zulip):

Checking now whether considering only the first Deref messes with other tests.

nikomatsakis (Jul 21 2018 at 12:24, on Zulip):

I saw later that it had the same signature as the one for arguments...?

nikomatsakis (Jul 21 2018 at 12:24, on Zulip):

(I guess that was the original motivation?)

davidtwco (Jul 21 2018 at 12:25, on Zulip):

Yeah, it was. I just added a .map(..) call in that function instead of changing the signature.

davidtwco (Jul 21 2018 at 12:34, on Zulip):

@nikomatsakis Doing a deref only once still causes those duplicate errors:

---- [ui] ui/issue-27282-reborrow-ref-mut-in-guard.rs stdout ----
diff of stderr:

+       error[E0596]: cannot borrow immutable item `r` as mutable
+         --> $DIR/issue-27282-reborrow-ref-mut-in-guard.rs:26:40
+          |
+       LL |         ref mut r if { (|| { let bar = &mut *r; **bar = false; })();
+          |                                        ^^^^^^^ cannot borrow as mutable
+
1       error[E0596]: cannot borrow immutable item `*r` as mutable
2         --> $DIR/issue-27282-reborrow-ref-mut-in-guard.rs:26:24
3          |

4       LL |         ref mut r if { (|| { let bar = &mut *r; **bar = false; })();
5          |                        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot borrow as mutable
6
-       error: aborting due to previous error
+       error: aborting due to 2 previous errors
8
9       For more information about this error, try `rustc --explain E0596`.
10
nikomatsakis (Jul 21 2018 at 12:38, on Zulip):

hmm

nikomatsakis (Jul 21 2018 at 12:39, on Zulip):

ok so ... I think .. those duplicate errors are "correct" in some sense

nikomatsakis (Jul 21 2018 at 12:39, on Zulip):

well I guess I don't know

nikomatsakis (Jul 21 2018 at 12:40, on Zulip):

I'm not crazy about the recurse boolean parameter

nikomatsakis (Jul 21 2018 at 12:40, on Zulip):

but I guess I have to look at the other logic maybe

nikomatsakis (Jul 21 2018 at 12:40, on Zulip):

right now, it's all the places that are using false, I assume?

nikomatsakis (Jul 21 2018 at 12:41, on Zulip):

ah so .. ok I think I see the problem

nikomatsakis (Jul 21 2018 at 12:42, on Zulip):

there is already a "deref" built-in in some of the uses

nikomatsakis (Jul 21 2018 at 12:42, on Zulip):

e.g. this one

             Place::Projection(ref proj) => {
                 match proj.elem {
                     ProjectionElem::Deref => {
-                        if let Some(field) = self.is_upvar_field_projection(&proj.base) {
+                        let upvar_field_projection = proj.base.is_upvar_field_projection(
+                            self.mir, &self.tcx, false);
+                        if let Some(field) = upvar_field_projection {
                             let var_index = field.index();
                             let name = self.mir.upvar_decls[var_index].debug_name.to_string();
                             if self.mir.upvar_decls[var_index].by_ref {
nikomatsakis (Jul 21 2018 at 12:42, on Zulip):

if you changed that from proj.base.is_upvar_field_projection to proj.is_upvar_field_projection it would probably work

nikomatsakis (Jul 21 2018 at 12:43, on Zulip):

some of the other users are a bit less clear but I suspect the * is ok

nikomatsakis (Jul 21 2018 at 12:44, on Zulip):

that is, handling the deref case is ok

davidtwco (Jul 21 2018 at 12:46, on Zulip):

Ah, I didn't notice that, I'll go check the callsites.

nikomatsakis (Jul 21 2018 at 12:46, on Zulip):

otoh

nikomatsakis (Jul 21 2018 at 12:46, on Zulip):

it doesn't seem terrible to split

nikomatsakis (Jul 21 2018 at 12:47, on Zulip):

i.e., the current method just checks for what it says: is this a field projection (not a deref projection)

nikomatsakis (Jul 21 2018 at 12:47, on Zulip):

and you could have a wrapper is

nikomatsakis (Jul 21 2018 at 12:47, on Zulip):

is_upvar_ref

nikomatsakis (Jul 21 2018 at 12:47, on Zulip):

that also checks for deref

nikomatsakis (Jul 21 2018 at 12:47, on Zulip):

if that winds up being useful

nikomatsakis (Jul 21 2018 at 12:47, on Zulip):

but probably better to just have one :)

nikomatsakis (Jul 21 2018 at 12:48, on Zulip):

to really know whether the deref is important or not, we'd have to consider the 'mode' of the upvar, which I don't think we currently do?

nikomatsakis (Jul 21 2018 at 12:48, on Zulip):

that is, you could have x: &mut u32; move || *x and that would desugar to *self.x

davidtwco (Jul 22 2018 at 12:42, on Zulip):

Back in the correct topic again... I've done the required rebase, just doing tests locally before pushing.

nikomatsakis (Jul 22 2018 at 12:43, on Zulip):

ok let me take a look

nikomatsakis (Jul 22 2018 at 12:43, on Zulip):

I think I almost r+'d yesterday but wanted to see travis results

nikomatsakis (Jul 22 2018 at 12:43, on Zulip):

then I got distracted

nikomatsakis (Jul 22 2018 at 12:43, on Zulip):

I'm doing a bit of work this afternoon so I'll be around

davidtwco (Jul 22 2018 at 12:43, on Zulip):

Sounds good. If this PR is still fine then I've got bandwidth to take something else.

davidtwco (Jul 22 2018 at 12:44, on Zulip):

Just pushed the rebase.

nikomatsakis (Jul 22 2018 at 12:44, on Zulip):

hmm so the question what is best next thing to take on

nikomatsakis (Jul 22 2018 at 12:45, on Zulip):

I feel like there is more work on region errors

nikomatsakis (Jul 22 2018 at 12:45, on Zulip):

if you want to continue in this vein

nikomatsakis (Jul 22 2018 at 12:45, on Zulip):

they have definitely made many, many strides

davidtwco (Jul 22 2018 at 12:45, on Zulip):

Yeah, we could look into improving some of the cases this PR didn't cover.

nikomatsakis (Jul 22 2018 at 12:47, on Zulip):

I opened some issues iirc

nikomatsakis (Jul 22 2018 at 12:47, on Zulip):

one of which had what I thought was an interesting test case to improve

nikomatsakis (Jul 22 2018 at 12:48, on Zulip):

in general I think things that say "free region requires" is ungreat

davidtwco (Jul 22 2018 at 12:50, on Zulip):

Yeah, those aren't great.

nikomatsakis (Jul 22 2018 at 13:00, on Zulip):

well I'm marking your PR r+

nikomatsakis (Jul 22 2018 at 13:00, on Zulip):

if travis gets upset we'll cancel it :)

nikomatsakis (Jul 22 2018 at 13:02, on Zulip):

@David Wood what does your branch do for this example?

#![feature(nll)]

fn foo(_: impl for<'a> FnOnce(&'a u32, &u32) -> &'a u32) {
}

fn main() {
    foo(|a, b| b)
}
nikomatsakis (Jul 22 2018 at 13:02, on Zulip):

I am reminded that there were some cases where we wanted to do follow-up

nikomatsakis (Jul 22 2018 at 13:02, on Zulip):

that example came from https://github.com/rust-lang/rust/issues/52533

davidtwco (Jul 22 2018 at 13:07, on Zulip):

Still doing a build (did a check so it needs a full build because of #52565) locally but I'll let you know.

davidtwco (Jul 22 2018 at 13:42, on Zulip):

Seems like my branch didn't have an effect @nikomatsakis:

error: unsatisfied lifetime constraints
 --> test1.rs:7:16
  |
7 |     foo(|a, b| b)
  |          -  -  ^ free region requires that `'1` must outlive `'2`
  |          |  |
  |          |  lifetime `'1` appears in this argument
  |          lifetime `'2` appears in this argument
nikomatsakis (Jul 22 2018 at 13:43, on Zulip):

ok I expect that is because this is the "return value" case, and I think you disabled your branch in that case...

davidtwco (Jul 22 2018 at 13:44, on Zulip):

I don't think it was explicitly disabled for that case, just that it only handles assignments and function calls.

nikomatsakis (Jul 22 2018 at 13:46, on Zulip):

ok I see

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

well

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

this should be an assignment actually

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

but an assignment to the "return place"

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

(that is, _0)

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

if you look at the MIR, you'll see what I mean

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

"return slot place"

nikomatsakis (Jul 22 2018 at 13:47, on Zulip):

we could detect that

davidtwco (Jul 22 2018 at 13:47, on Zulip):

Ah yeah, that's categorized as a Return not an Assignment.

davidtwco (Jul 22 2018 at 13:47, on Zulip):

It'd be pretty easy to support that with the way it's done now.

nikomatsakis (Jul 22 2018 at 13:49, on Zulip):

do you want to roll that into the same PR?

nikomatsakis (Jul 22 2018 at 13:49, on Zulip):

or should we do a follow-up

davidtwco (Jul 22 2018 at 13:50, on Zulip):

A new PR is cleaner, but depends if we think that one will land quickly. No point in having another PR waiting for that to land before it can be approved and get in the queue.

nikomatsakis (Jul 22 2018 at 13:54, on Zulip):

I was gonna p=1 it

davidtwco (Jul 22 2018 at 14:06, on Zulip):

I'll experiment locally with that example see if I can get it to use the newer messages.

nikomatsakis (Jul 22 2018 at 14:17, on Zulip):

gave p=1 since travis is happy

nikomatsakis (Jul 22 2018 at 14:19, on Zulip):

I know I keep saying it, but I am quite happy with how these new region errors are turning out :)

nikomatsakis (Jul 22 2018 at 14:19, on Zulip):

it feels like we're finally getting close to "cracking the case"

davidtwco (Jul 22 2018 at 14:20, on Zulip):

Interestingly, it seems as though the return in that example is being classified as Boring.

davidtwco (Jul 22 2018 at 14:21, on Zulip):

Right now we consider anything that isn't Locations::Interesting as boring. Including Locations::Boring and Locations:All (this example has a combination of both).

davidtwco (Jul 22 2018 at 14:21, on Zulip):

So they aren't even considered for classification as anything more.

nikomatsakis (Jul 22 2018 at 15:18, on Zulip):

I'd like to remove that distintion, I think, and just move all of that logic into the classificaiton scheme

nikomatsakis (Jul 22 2018 at 15:18, on Zulip):

maybe we will still need a bit of "pre-logic" though, not sure

nikomatsakis (Jul 22 2018 at 15:19, on Zulip):

i.e., if the location alone is not enough to distinguish

davidtwco (Jul 22 2018 at 15:19, on Zulip):

It also wasn't being classified correctly as to an upvar because I expected one region to be external and the other local, whereas both were external in this case.

nikomatsakis (Jul 22 2018 at 15:43, on Zulip):

hmm so actually it's a somewhat different case

nikomatsakis (Jul 22 2018 at 15:43, on Zulip):

that is, this is not a case of "data local to the region escaping"

nikomatsakis (Jul 22 2018 at 15:43, on Zulip):

it's just a case of "data with wrong lifetime being returned"

davidtwco (Jul 22 2018 at 15:44, on Zulip):
error: borrowed data escapes outside of closure
 --> test1.rs:7:16
  |
7 |     foo(|a, b| b)
  |          -  -  ^ `b` escapes the closure body here
  |          |  |
  |          |  `b` is a reference that is only valid in the closure body
  |          `a` is declared here, outside of the closure body
davidtwco (Jul 22 2018 at 15:44, on Zulip):

I've got that, using the same labels as before. Not sure what other tests this would effect though.

nikomatsakis (Jul 22 2018 at 15:45, on Zulip):

well it seems like this message is misleading to me

nikomatsakis (Jul 22 2018 at 15:45, on Zulip):

I would expect something more like "closure was supposed to return data with lifetime '1 but it is returning data with lifetime '2"

nikomatsakis (Jul 22 2018 at 15:45, on Zulip):

that is, it's not about the data "escaping the closure body"

davidtwco (Jul 22 2018 at 15:46, on Zulip):

Yeah, I didn't adjust the messages at all.

davidtwco (Jul 22 2018 at 15:46, on Zulip):

Was just trying to get it to use that branch of the code.

nikomatsakis (Jul 22 2018 at 15:46, on Zulip):

my point is I don't think it should use that branch :)

nikomatsakis (Jul 22 2018 at 15:46, on Zulip):

this seems more like the first branch to me

nikomatsakis (Jul 22 2018 at 15:46, on Zulip):

or maybe a 3rd branch

nikomatsakis (Jul 22 2018 at 15:47, on Zulip):

i.e., to me, that branch is about a "local" lifetime escaping into a "global" lifetime

nikomatsakis (Jul 22 2018 at 15:47, on Zulip):

the other branch is about conflicts between external lifetimes

nikomatsakis (Jul 22 2018 at 15:47, on Zulip):

in particular, I don't think the message for this case needs to be so different from

nikomatsakis (Jul 22 2018 at 15:47, on Zulip):
fn foo(x: &'a u32, y: &'b u32) -> &'a u32 { y }
nikomatsakis (Jul 22 2018 at 15:47, on Zulip):

the message for that case

davidtwco (Jul 22 2018 at 15:47, on Zulip):

I was thinking we should ditch the various ConstraintCategory variants I added that are specific to upvars, and then have branches depending on whether it is a (true, false), (true, true), (false, true) and (false, false) pair for the (outlived_fr, fr) being local or not.

nikomatsakis (Jul 22 2018 at 15:48, on Zulip):

yes, that was my original intent

davidtwco (Jul 22 2018 at 15:48, on Zulip):

I see.

davidtwco (Jul 22 2018 at 15:48, on Zulip):

That does make sense.

nikomatsakis (Jul 22 2018 at 15:48, on Zulip):

the upvar assignments achieves a similar effect though

nikomatsakis (Jul 22 2018 at 15:48, on Zulip):

but I think i'd rather do it based on region category ultimately

nikomatsakis (Jul 22 2018 at 15:49, on Zulip):

(do you want to give that a try in the original PR)

nikomatsakis (Jul 22 2018 at 15:49, on Zulip):

?

davidtwco (Jul 22 2018 at 15:51, on Zulip):

I'll see what I come up with locally and we can see if the PR has landed by the time I've got anything.

nikomatsakis (Jul 22 2018 at 15:54, on Zulip):

+1

Last update: Nov 21 2019 at 23:50UTC