Stream: t-compiler/wg-nll

Topic: #21114 non-intuitive rvalue lifetimes


pnkfelix (Sep 19 2018 at 14:03, on Zulip):

So I've been looking at the examples from #21114, and I had a thought

pnkfelix (Sep 19 2018 at 14:03, on Zulip):

a lot of the discussion centers around trying to make the examples compile, e.g. by changing our rvalue lifetime rules

pnkfelix (Sep 19 2018 at 14:04, on Zulip):

but I was just now wondering: If we could emit a more targetted diagnostic, focused on this case

pnkfelix (Sep 19 2018 at 14:04, on Zulip):

that could provide links to the explanation of why this is happening, and the standard workarounds

pnkfelix (Sep 19 2018 at 14:05, on Zulip):

That seems like it would be a marked improvement over today, where you just see "does not live long enough" without enough context to understand why...

pnkfelix (Sep 19 2018 at 14:13, on Zulip):

(I'm thinking in particular of targeting the diagnostic to the scenario where you have

{
   ...;
   TAIL
}; // semi, or at end of `-> ()` fn

so the result of the block is unused and thus dropped. We don't have to change language semantics to suggest to the user that they could put a semi immediately after TAIL if we see that the extended lifetime is causing a "does not live long enough" error for some temp created within TAIL.)

pnkfelix (Sep 19 2018 at 14:20, on Zulip):

In essence: I'm suggesting that while the status quo is annoying, what makes it especially annoying is tracking down the issue when it arises. Exceptionally so for newcomers. If the compiler feedback guided one to blindly insert the semi-colon, then some people might gripe momentarily as they type it in, but I doubt we'd see bug reports.

Jake Goulding (Sep 19 2018 at 14:36, on Zulip):

I doubt we'd see bug reports.

Wouldn't that be a net negative end result? People are subtly annoyed, but not enough to tell us. When they talk to friends they say "oh, Rust has weird magical problems with stuff, I don't even remember what, so I don't recommend using it"

pnkfelix (Sep 19 2018 at 14:38, on Zulip):

Okay well maybe that was flippant of me. A more positive way to phrase it: this could cause people to shift their viewpoint over whether this is a bug.

pnkfelix (Sep 19 2018 at 14:39, on Zulip):

The drop order for these temporaries is observable, and thus the presence of that semicolon immediately after TAIL does matter, even when the type is TAIL: ().

pnkfelix (Sep 19 2018 at 14:39, on Zulip):

(I mean the order is observable with respect to the locals bound in the block)

pnkfelix (Sep 19 2018 at 14:42, on Zulip):

as demonstrated e.g. on this playpen

pnkfelix (Sep 19 2018 at 14:43, on Zulip):

And sure, some people may still get frustrated and abandon ship

pnkfelix (Sep 19 2018 at 14:44, on Zulip):

but the diagnostics in the compiler, especially the help explanations, try hard to counteract that...

pnkfelix (Sep 19 2018 at 14:46, on Zulip):

Part of my response here is influenced by my past experience attempting to propose changes to the dynamic semantics of drop

pnkfelix (Sep 19 2018 at 14:49, on Zulip):

because a change to the rvalue lifetimes in this scenario is, in some ways, a more limited version of what was being proposed for RFC 210. And there was a lot of fear there about silently breaking existing code.

simulacrum (Sep 19 2018 at 15:20, on Zulip):

I do think that telling people 'maybe insert a ; here' and pointing to some longer form documentation in --explain for that error could be a good way of 'solving' this

simulacrum (Sep 19 2018 at 15:21, on Zulip):

We could even say something like "drop order changes", possibly even providing that order to the user

Jake Goulding (Sep 19 2018 at 16:06, on Zulip):

to shift their viewpoint over whether this is a bug

Which is a crucial thing, I believe. Is it a bug? (I know that bug vs feature request is a fuzzy line). Your discussion of observable effects seems to indicate that it is not a bug, but a hard fact within the current set of rules. If that's the case, I agree there are two primary options: improve diagnostics and/or change the rules. NLL is an example of changing the rules, but there was also effort put into improving diagnostics for AST borrowck before MIR borrowck could exist.

I think it's mostly your phrasing that is throwing me, which sounds like "let's pull a fast one on the users so they stop complaining". I'm :100: on improving diagnostics to explain a rule that is non-intuitive, however.

pnkfelix (Sep 19 2018 at 16:07, on Zulip):

yes, I think my phrasing was poor.

pnkfelix (Sep 19 2018 at 16:12, on Zulip):

When I wrote:

... some people might gripe momentarily as they type it in, but I doubt we'd see bug reports.

I was half-thinking of cases like my own griping here, which was actually a case where the borrow-checker is right to reject my code.

nikomatsakis (Sep 19 2018 at 18:00, on Zulip):

my feeling @pnkfelix is that we ought to start with a diganostic, yes

nikomatsakis (Sep 19 2018 at 18:00, on Zulip):

makes a lot of sense

nikomatsakis (Sep 19 2018 at 18:00, on Zulip):

it's something we can do in the short term

nikomatsakis (Sep 19 2018 at 18:00, on Zulip):

that said, I think maybe longer term we should see if we can make some targeted changes here— but you're not wrong that changing drop order is a scary thing

nikomatsakis (Sep 19 2018 at 18:01, on Zulip):

(before doing it, I would want to try and assess impact, e.g. by devising some sort of lint to detect cases where we think it would maybe be significant and doing crater runs)

simulacrum (Sep 19 2018 at 19:27, on Zulip):

@Jake Goulding FWIW, "NLL is an example of changing the rules" isn't really true -- that is, NLL should have no effects on runtime execution -- unlike changes to drop order

Jake Goulding (Sep 19 2018 at 19:35, on Zulip):

Perhaps my wording wasn't the clearest. What I mean is that pre-NLL, it was expected that certain sound code would not compile. It was not a bug that that code didn't compile, but it was unfortunate. The rules were overly conservative and then NLL "changed the rules".

It sounds similar to this case, as presumably there's code that is sound w.r.t these conditions at the end of blocks but the compiler doesn't allow it. (I assume this because I don't imagine that someone is trying to change the rules to allow for unsoundness). @pnkfelix saying " change to the rvalue lifetimes" sounds like "a change to the rules"

Jake Goulding (Sep 19 2018 at 19:35, on Zulip):

NLL is literally a change to the rules of what the compiler allows though...

pnkfelix (Sep 19 2018 at 19:36, on Zulip):

In truth NLL did change a few of the rules of the dynamic semantics

Jake Goulding (Sep 19 2018 at 19:37, on Zulip):

Perhaps y'all are meaning a different set of rules than what I mean, but I cannot figure out how to state what I have stated in a different manner :-)

Jake Goulding (Sep 19 2018 at 19:40, on Zulip):

Now, if @simulacrum means that "the changes we call NLL" and "the proposed changes to rvalue lifetimes " would have different amounts / kinds of end-user behavior, that's totally possible; I don't know anything about that (other than what @pnkfelix just spilled :wink: )

simulacrum (Sep 19 2018 at 19:41, on Zulip):

Yes, I think different amounts is what I meant to imply; NLL is intended to be mostly or all 'transparent' as I understand it

pnkfelix (Sep 21 2018 at 19:54, on Zulip):

(in any case I am looking into trying to improve the diagnostics here now)

pnkfelix (Sep 21 2018 at 21:46, on Zulip):

So, for this example (play)

fn main() {
    {
        let mut _thing1 = D(Box::new("thing1"));
        // D("other").next(&_thing1).end()
        D(&_thing1).end()
    }

    ;
}

what do people think of this new diagnostic output i've managed to hack up:

error[E0597]: `_thing1` does not live long enough
  --> ../../issue-21114-demo-borrow-of-temp.rs:7:11
   |
7  |         D(&_thing1).end()
   |           ^^^^^^^^ borrowed value does not live long enough
8  |     }
   |     - `_thing1` dropped here while still borrowed
9  |
10 |     ;
   |     - borrow later used here, when temporary is dropped
   |
note: temporary is formed when evaluating this expresion
  --> ../../issue-21114-demo-borrow-of-temp.rs:7:9
   |
7  |         D(&_thing1).end()
   |         ^^^^^^^^^^^
   = note: temporary is part of a block-tail expression. Consider adding semicolon to drop its temporaries sooner
pnkfelix (Sep 21 2018 at 21:49, on Zulip):

I recognize there is likely to be overlap/conflicts between some of what I've done here and @mikhail-m1 's work on #54164. But hopefully it will be more synergistic rather than actual conflicts...

nikomatsakis (Sep 21 2018 at 21:51, on Zulip):

@pnkfelix I like it -- I like in particular calling out the expression where temporary is formed. That said, I think it's a bit misleading here. In particular, the temporary is formed because of end being an &self method, I suppose?

nikomatsakis (Sep 21 2018 at 21:51, on Zulip):

I think we should work harder to call that out

nikomatsakis (Sep 21 2018 at 21:51, on Zulip):

that's one of the more confusing things

pnkfelix (Sep 21 2018 at 21:51, on Zulip):

end being a &self method rather than a self method, you mean?

pnkfelix (Sep 21 2018 at 21:52, on Zulip):

(i mean, a temporary is formed in the latter case; it just gets moved into end() and dies sooner...)

pnkfelix (Sep 21 2018 at 21:53, on Zulip):

but it is an interesting idea, to tie in the relevance of the signature of end

pnkfelix (Sep 21 2018 at 21:53, on Zulip):

(The playpen link I provided provides the full context, as you might expect; so that's a better place to play around with the example.)

pnkfelix (Sep 21 2018 at 21:54, on Zulip):

Anyway I'm pretty happy with even this smallish hack that I've gotten working

pnkfelix (Sep 21 2018 at 21:56, on Zulip):

Perhaps amazingly ... this change doesn't seem to have affected our test suite ???

pnkfelix (Sep 21 2018 at 21:57, on Zulip):

as in... we aren't testing the behavior on this test, even in [ui (nll)] ???

pnkfelix (Sep 21 2018 at 21:57, on Zulip):

wait no, strike that

davidtwco (Sep 21 2018 at 21:58, on Zulip):

I'd suggest explaining "a block-tail expression" or changing that wording.

davidtwco (Sep 21 2018 at 21:58, on Zulip):

What's the old diagnostic for that look like? I'll just look at the playground link.

nikomatsakis (Sep 21 2018 at 21:59, on Zulip):

(i mean, a temporary is formed in the latter case; it just gets moved into end() and dies sooner...)

my point is that the underlined thing — D(&_thing1) — is the value that goes into the temporary, but not the reason we make a temporary

pnkfelix (Sep 21 2018 at 21:59, on Zulip):

(I misread my test output; it never got to [ui (nll)], due to problems with my test set up)

nikomatsakis (Sep 21 2018 at 21:59, on Zulip):

and in particular the reason that we are making a temporary is not visible

pnkfelix (Sep 21 2018 at 22:00, on Zulip):

okay. Its late and I have to go, but this provides interesting food for thought

pnkfelix (Sep 21 2018 at 22:00, on Zulip):

I'll try to explore whether I can encode a bit more information here

pnkfelix (Sep 21 2018 at 22:01, on Zulip):

but I don't know if I agree that the diagnostic as written is "misleading" per se

pnkfelix (Sep 21 2018 at 22:01, on Zulip):

I.e. its not claiming to say why the temporary is made ...

davidtwco (Sep 21 2018 at 22:01, on Zulip):

For me, not knowing why that error is happening, reading that error doesn't make it much clearer.

nikomatsakis (Sep 21 2018 at 22:01, on Zulip):

I don't disagree

nikomatsakis (Sep 21 2018 at 22:01, on Zulip):

but

pnkfelix (Sep 21 2018 at 22:02, on Zulip):

its just saying "a temporary was made. Here's the expression that produced "it" -- though as you point out, it would be more acccurrte to say that's the expression used to initialize it, I guess...

nikomatsakis (Sep 21 2018 at 22:02, on Zulip):

it took me a while to understand it

nikomatsakis (Sep 21 2018 at 22:02, on Zulip):

I kept thinking "but &_thing1 shouldn't make a temporary..."

pnkfelix (Sep 21 2018 at 22:02, on Zulip):

I didn't highlight &_thing1 alone

nikomatsakis (Sep 21 2018 at 22:02, on Zulip):

yes, and that also confused me

nikomatsakis (Sep 21 2018 at 22:02, on Zulip):

until I realized what was going on :)

pnkfelix (Sep 21 2018 at 22:03, on Zulip):

It would perhaps be nice to provide the highlight of the bigger expression sooner

pnkfelix (Sep 21 2018 at 22:03, on Zulip):

perhaps via multispan magic?

pnkfelix (Sep 21 2018 at 22:03, on Zulip):

not really sure if that API is right thing to use here...

pnkfelix (Sep 21 2018 at 22:04, on Zulip):

@davidtwco part of the problem is that there is a confluence of many factors here

nikomatsakis (Sep 21 2018 at 22:04, on Zulip):
error[E0597]: `_thing1` does not live long enough
  --> ../../issue-21114-demo-borrow-of-temp.rs:7:11
   |
7  |         D(&_thing1).end()
   |           ^^^^^^^^ borrowed value does not live long enough
8  |     }
   |     - `_thing1` dropped here while still borrowed
9  |
10 |     ;
   |     - borrow later used here, when temporary is dropped
   |
note: temporary is formed because this is an `&self` method
  --> ../../issue-21114-demo-borrow-of-temp.rs:7:9
   |
7  |         D(&_thing1).end()
   |                     ^^^
   = note: temporary is part of a block-tail expression. Consider adding semicolon to drop its temporaries sooner
nikomatsakis (Sep 21 2018 at 22:04, on Zulip):

I think that's better, but not quite there

pnkfelix (Sep 21 2018 at 22:04, on Zulip):

wait, surely you would want us to still highlight the whole expression ... ?

pnkfelix (Sep 21 2018 at 22:04, on Zulip):

I thought identifying it as D(&_thing1) was important

nikomatsakis (Sep 21 2018 at 22:04, on Zulip):

I'd prefer not, but maybe we would in practice

pnkfelix (Sep 21 2018 at 22:05, on Zulip):

hmm

nikomatsakis (Sep 21 2018 at 22:05, on Zulip):

I guess I don't know

pnkfelix (Sep 21 2018 at 22:05, on Zulip):

maybe in the common case the expression would be unwieldy and just noise...

pnkfelix (Sep 21 2018 at 22:05, on Zulip):

my micro-test may be misleading me as to the utility of the message

pnkfelix (Sep 21 2018 at 22:06, on Zulip):

but part of the reason why I think its important to highlight the expression

pnkfelix (Sep 21 2018 at 22:06, on Zulip):

is the fact that it implements Drop

pnkfelix (Sep 21 2018 at 22:06, on Zulip):

obviously we could alternatively highlight the impl Drop for D

nikomatsakis (Sep 21 2018 at 22:07, on Zulip):

hmm

nikomatsakis (Sep 21 2018 at 22:07, on Zulip):

i may not have the full picture of the example in question

pnkfelix (Sep 21 2018 at 22:07, on Zulip):

i definitely recommend playing with it in the playpen

pnkfelix (Sep 21 2018 at 22:07, on Zulip):

every piece is necessary...

nikomatsakis (Sep 21 2018 at 22:08, on Zulip):

I guess I am thinking a bit more generally

nikomatsakis (Sep 21 2018 at 22:08, on Zulip):

I think in general our temporary rules are a big source of confusion

nikomatsakis (Sep 21 2018 at 22:08, on Zulip):

and I think it is not helpful that the reason there is a temporary is often "hidden"

nikomatsakis (Sep 21 2018 at 22:08, on Zulip):

(though even &foo() is probably not obvious to most folks)

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

The example, to my untrained eye, would be confused by more diagnostics - I read it and think "oh, we're creating a D that has a borrow in it, and we're returning that but the borrow won't live long enough". It feels like something similar to:

struct Q<'a>(&'a u32);
fn x(_y: &u32) -> Q {
    let y = 3;
    Q(&y)
}

At least with this example, I don't understand why the error is confusing, because I'd need to understand what is happening at a much deeper level than I do to realise the error isn't right? I think that's what I think?

pnkfelix (Sep 21 2018 at 22:15, on Zulip):

well, maybe the answer is I should try out my branch on some of the examples from #21114

pnkfelix (Sep 21 2018 at 22:15, on Zulip):

because there are plenty of cases of tail-exprs where our users get mad about having don't understand why they have to add ;

pnkfelix (Sep 21 2018 at 22:16, on Zulip):

e.g. note that in the code I wrote, the block isn't returning a D. Its returning ().

pnkfelix (Sep 21 2018 at 22:16, on Zulip):

and so people say "why is the borrow lasting so long?"

pnkfelix (Sep 21 2018 at 22:17, on Zulip):

and the answer is "because the D you created gets killed after locals in the block are dropped." But then you get into "well why is that happening?"

pnkfelix (Sep 21 2018 at 22:17, on Zulip):

and the answer then is "our r-value lifetime rules"

davidtwco (Sep 21 2018 at 22:22, on Zulip):

Perhaps a good way to explain that is to "expand" the confusing part, have a little note at the bottom with a code snippet that splits the confusing statement into multiple statements that approximates how the compiler understands the code and is more obviously wrong to someone who has an intuition for the borrow rules? Not sure if that would work, just a thought.

pnkfelix (Sep 21 2018 at 22:31, on Zulip):

by the way, one of the few tests that this change did affect was issue-47646.stderr

pnkfelix (Sep 21 2018 at 22:32, on Zulip):

I linked to that commit, 0957ede5027c0bffe208904998675a17bfd4cd59 , in particular, because it looks to me like that PR might have regressed the diagnostics that @nikomatsakis carefully plotted out for #47646 ?

pnkfelix (Sep 21 2018 at 22:32, on Zulip):

@Santiago Pastorino ^

pnkfelix (Sep 21 2018 at 22:33, on Zulip):

(I'll leave a note on PR #51889 about this)

pnkfelix (Sep 21 2018 at 22:36, on Zulip):

its not hard to "fix"; or at least, a fix will be a natural consequence of the branch I'm playing with for #21114. But I'm just curious about whether this was deliberate or accidental.

pnkfelix (Sep 21 2018 at 23:01, on Zulip):

I ended up pushing the current state to a branch. I'm a little shocked at how few of our tests needed to be updated: https://github.com/pnkfelix/rust/commits/issue-21114-semi-on-tail-diagnostic

pnkfelix (Sep 21 2018 at 23:02, on Zulip):

(the test updates are on this commit in particular)

pnkfelix (Sep 21 2018 at 23:04, on Zulip):

Also, this test shows a clear case where the blind suggestion to add a semicolon is bogus

pnkfelix (Sep 21 2018 at 23:04, on Zulip):

(as I alluded to in the commit message of the parent commit to that one linked above)

pnkfelix (Sep 21 2018 at 23:06, on Zulip):

I may change it to only suggest adding the semi-colon if the tail expression has type (). Not sure yet.

pnkfelix (Sep 21 2018 at 23:07, on Zulip):

(because then that won't catch the case where the context surrounding the block-expression in question is itself supplying the ; that causes the returned value to be discarded... but maybe that's okay, as long as we highlight that semicolon when we presumably say "temporary is dropped here")

Santiago Pastorino (Sep 22 2018 at 03:45, on Zulip):

@pnkfelix cool that you bring this up

Santiago Pastorino (Sep 22 2018 at 03:45, on Zulip):

to be honest I do not remember why we changed that

Santiago Pastorino (Sep 22 2018 at 03:45, on Zulip):

/cc @nikomatsakis

nikomatsakis (Sep 24 2018 at 15:11, on Zulip):

its not hard to "fix"; or at least, a fix will be a natural consequence of the branch I'm playing with for #21114. But I'm just curious about whether this was deliberate or accidental.

it wasn't deliberate, I don't think.

Last update: Nov 22 2019 at 00:05UTC