Stream: t-compiler/wg-nll

Topic: #52742


davidtwco (Aug 06 2018 at 17:31, on Zulip):

With the PR for #52739 approved, figured I'd open a new topic (instead of issue-52739) to discuss #52742.

davidtwco (Aug 06 2018 at 17:31, on Zulip):

@nikomatsakis

nikomatsakis (Aug 06 2018 at 17:33, on Zulip):

+1

nikomatsakis (Aug 06 2018 at 17:33, on Zulip):

@davidtwco what error do we get with your branch?

nikomatsakis (Aug 06 2018 at 17:33, on Zulip):

for that example

nikomatsakis (Aug 06 2018 at 17:33, on Zulip):

can you paste it in there?

nikomatsakis (Aug 06 2018 at 17:33, on Zulip):

oh, actually, i'm going to guess it's the same?

nikomatsakis (Aug 06 2018 at 17:34, on Zulip):

probably we assume that all EarlyBound regions are named and not anonymous?

nikomatsakis (Aug 06 2018 at 17:34, on Zulip):

that strikes me as a bug in the "nice region error" code

davidtwco (Aug 06 2018 at 17:34, on Zulip):

Same as issue, yeah: https://gist.github.com/davidtwco/3f8c293d4455bbbc8de7988eabe53f55

nikomatsakis (Aug 06 2018 at 17:35, on Zulip):

I think @davidtwco that this function is wrong

nikomatsakis (Aug 06 2018 at 17:35, on Zulip):

in particular this line

davidtwco (Aug 06 2018 at 17:36, on Zulip):

I was just looking at that, then at ReEarlyBound to see if there were functions on it that would identify it as a '_.

nikomatsakis (Aug 06 2018 at 17:36, on Zulip):

we should check the name, yeah

nikomatsakis (Aug 06 2018 at 17:36, on Zulip):

when that code was written, '_ was not a thing

davidtwco (Aug 06 2018 at 17:36, on Zulip):

I'll throw a branch together with that.

nikomatsakis (Aug 06 2018 at 17:36, on Zulip):

not sure what name we use there

davidtwco (Aug 06 2018 at 17:37, on Zulip):

I assume that other parts of our region inference error handling might make the same assumption about ReEarlyBound always being named.

nikomatsakis (Aug 06 2018 at 17:37, on Zulip):

it appears to be the result of this

        let lifetime_name = |def_id| {
            tcx.hir.name(tcx.hir.as_local_node_id(def_id).unwrap()).as_interned_str()
        };
nikomatsakis (Aug 06 2018 at 17:38, on Zulip):

I think we will use keywords::UnderscoreLifetime.name().as_interned_str() @davidtwco

nikomatsakis (Aug 06 2018 at 17:38, on Zulip):

i.e., that is what name will be for a '_ or other anonymous lifetime

nikomatsakis (Aug 06 2018 at 17:38, on Zulip):

that appears in an impl header

davidtwco (Aug 06 2018 at 17:39, on Zulip):

:thumbs_up:

nikomatsakis (Aug 06 2018 at 17:40, on Zulip):

from here, in case you are curious (link to source)

nikomatsakis (Aug 06 2018 at 19:04, on Zulip):

@Matthew Jasper what do you think of these proposed changes to your PR? (I'm sharing this topic because it seems relevant enough)

davidtwco (Aug 06 2018 at 19:08, on Zulip):

(deleted)

Matthew Jasper (Aug 06 2018 at 19:09, on Zulip):

@nikomatsakis Both look good.

nikomatsakis (Aug 06 2018 at 19:10, on Zulip):

@Matthew Jasper ok, cool...wasn't sure if you'd want to split out into a distinct PR

nikomatsakis (Aug 06 2018 at 19:10, on Zulip):

but I think it'll be simple enough

davidtwco (Aug 06 2018 at 19:16, on Zulip):

@nikomatsakis Submitted #53124 for #52742.

nikomatsakis (Aug 06 2018 at 19:17, on Zulip):

@davidtwco great! that is more the message I expected; I wouldn't say it fixes 52742 though. More like "exposes" it

nikomatsakis (Aug 06 2018 at 19:18, on Zulip):

seems like good progress regardless

davidtwco (Aug 06 2018 at 19:18, on Zulip):

Should I change it to "Part of #52742"?

nikomatsakis (Aug 06 2018 at 19:18, on Zulip):

one easy thing might be to fix the&self handling to use the "pretty-printed type"

nikomatsakis (Aug 06 2018 at 19:18, on Zulip):

yeah

nikomatsakis (Aug 06 2018 at 19:18, on Zulip):

e.g.,

error: unsatisfied lifetime constraints
  --> $DIR/issue-52742.rs:25:9
   |
LL |     fn take_bar(&mut self, b: Bar<'_>) {
   |                 ---------         -- let's call this `'1`
   |                 |
   |                 has type `&mut Foo<'1, '_>`
LL |         self.y = b.z
   |         ^^^^^^^^^^^^ requires that `'1` must outlive `'2`
nikomatsakis (Aug 06 2018 at 19:18, on Zulip):

which would be better

nikomatsakis (Aug 06 2018 at 19:20, on Zulip):

I think my ideal though might be to highlight the impl itself:

error: unsatisfied lifetime constraints
  --> $DIR/issue-52742.rs:25:9
   |
LL |  impl Foo<'_, '_> {
  |            -- let's call this `'2`
LL |     fn take_bar(&mut self, b: Bar<'_>) {
   |                                   -- let's call this `'1`
LL |         self.y = b.z
   |         ^^^^^^^^^^^^ requires that `'1` must outlive `'2`
nikomatsakis (Aug 06 2018 at 19:20, on Zulip):

something like that

nikomatsakis (Aug 06 2018 at 19:21, on Zulip):

that would be a bit harder :)

nikomatsakis (Aug 06 2018 at 19:21, on Zulip):

thoughts?

davidtwco (Aug 06 2018 at 19:21, on Zulip):

I think I prefer annotating the function rather than the impl - the locality of it all helps I think - might just be me though.

nikomatsakis (Aug 06 2018 at 19:22, on Zulip):

yeah, I'm not entirely sure which I prefer

nikomatsakis (Aug 06 2018 at 19:22, on Zulip):

another interesting case is when you have a named lifetime

nikomatsakis (Aug 06 2018 at 19:22, on Zulip):

e.g., imagine

davidtwco (Aug 06 2018 at 19:23, on Zulip):

Are those parameters on the impl not intended to be inferred from the struct defn in future?

nagisa (Aug 06 2018 at 19:23, on Zulip):

I’d kinda prefer if '_s were replaced in-place as well, it would make reading the error in-a-glance way easier. Don’t need to scan around for the “lets call this x”.

nikomatsakis (Aug 06 2018 at 19:23, on Zulip):
error: unsatisfied lifetime constraints
  --> $DIR/issue-52742.rs:25:9
   |
LL |  impl Foo<'tcx, '_> {
  |            ----`'tcx` declared here
LL |     fn take_bar(&mut self, b: Bar<'_>) {
   |                                   -- let's call this `'1`
LL |         self.y = b.z
   |         ^^^^^^^^^^^^ requires that `'1` must outlive `'tcx`
nikomatsakis (Aug 06 2018 at 19:23, on Zulip):

I’d kinda prefer if '_s were replaced in-place as well, it would make reading the error in-a-glance way easier. Don’t need to scan around for the “lets call this x”.

I think this is an interesting idea -- editing the source txt -- but I'd prefer to hold off on it

nikomatsakis (Aug 06 2018 at 19:23, on Zulip):

in particular, we are working now on identifying the spans etc

nikomatsakis (Aug 06 2018 at 19:24, on Zulip):

once we have the labels all working as we want, transitioning to editing the source text seems like an orthogonal step

nagisa (Aug 06 2018 at 19:24, on Zulip):

yes, can always be implemented as a improvement once everything lands at all.

nikomatsakis (Aug 06 2018 at 19:24, on Zulip):

Are those parameters on the impl not intended to be inferred from the struct defn in future?

they are not inferred from the struct definition, no.. but they may not require explicit declarations

nikomatsakis (Aug 06 2018 at 19:24, on Zulip):

e.g., like my second example

nikomatsakis (Aug 06 2018 at 19:24, on Zulip):

but that feature still hasn't gotten enough usage for me to feel confident about its fate

nikomatsakis (Aug 06 2018 at 19:25, on Zulip):

anyway, I'd be happy with either the "fully annotated self type" or "select the impl" version to start

nikomatsakis (Aug 06 2018 at 19:25, on Zulip):

but I'd like to get rid of all the "lifetime '1 appears in foo" sort of messages at least

nikomatsakis (Aug 06 2018 at 19:25, on Zulip):

which are horribly unspecific

davidtwco (Aug 06 2018 at 19:27, on Zulip):

I reckon I could probably use the existing "has type" logic. I'd just need to detect when we are looking at self in the code where the "lifetime appears in" message logic and call that function.

nikomatsakis (Aug 06 2018 at 19:28, on Zulip):

right

nikomatsakis (Aug 06 2018 at 19:28, on Zulip):

I think right now what happens

nikomatsakis (Aug 06 2018 at 19:28, on Zulip):

is that we fail to "match" the Ty<'tcx> and the HIR type

nikomatsakis (Aug 06 2018 at 19:28, on Zulip):

I imagine we could — in this case — fallback to the "has type" sort of display

nikomatsakis (Aug 06 2018 at 19:28, on Zulip):

that was my intention, anyway

nikomatsakis (Aug 06 2018 at 19:29, on Zulip):

thinking a bit more about it, I think one appealing point about highlighting the impl is that -- if you wanted to change the lifetime in the self type -- it shows you the code you would have to edit to do so

nikomatsakis (Aug 06 2018 at 19:29, on Zulip):

I guess we could give both, too

nikomatsakis (Aug 06 2018 at 19:29, on Zulip):

anyway, changing the "fallback" to "has type" seems like a good step regardless

nikomatsakis (Aug 06 2018 at 19:29, on Zulip):

probably helps with some other cases

nikomatsakis (Aug 06 2018 at 19:30, on Zulip):

hybrid:

error: unsatisfied lifetime constraints
  --> $DIR/issue-52742.rs:25:9
   |
LL |  impl Foo<'_, '_> {
  |            -- let's call this `'2`
LL |     fn take_bar(&mut self, b: Bar<'_>) {
   |                 ---------         -- let's call this `'1`
   |                 |
   |                 has type `&mut Foo<'2, '_>`
LL |         self.y = b.z
   |         ^^^^^^^^^^^^ requires that `'1` must outlive `'2`
davidtwco (Aug 06 2018 at 19:30, on Zulip):

What's the best way to check if the argument_hir_ty: &hir::Ty I have is self?

nikomatsakis (Aug 06 2018 at 19:30, on Zulip):

I don't think you need to do that actually

davidtwco (Aug 06 2018 at 19:31, on Zulip):

Just fallback to "has type" in all cases?

davidtwco (Aug 06 2018 at 19:31, on Zulip):

Or from some other criteria?

nikomatsakis (Aug 06 2018 at 19:31, on Zulip):

all cases

nikomatsakis (Aug 06 2018 at 19:31, on Zulip):

here is the function I am talking about link

nikomatsakis (Aug 06 2018 at 19:32, on Zulip):

you can see in my comment what I wanted it to look like :)

nikomatsakis (Aug 06 2018 at 19:32, on Zulip):

anyway, this is the fallback here

davidtwco (Aug 06 2018 at 19:33, on Zulip):

Ah, I was looking at give_name_if_we_can_match_hir_ty_from_argument because that has the message has "appears in this type" rather than "appears in this argument".

nikomatsakis (Aug 06 2018 at 19:39, on Zulip):

oh well maybe I am wrong too :)

nikomatsakis (Aug 06 2018 at 19:40, on Zulip):

oh hmm there is this fallback too...

nikomatsakis (Aug 06 2018 at 19:41, on Zulip):

maybe I wound up with 'double fallback'? :)

nikomatsakis (Aug 06 2018 at 19:41, on Zulip):

@davidtwco I think that this code should return None

nikomatsakis (Aug 06 2018 at 19:41, on Zulip):

and then the "arguments code" I was talking about should be modified to use the "has type" formulation

davidtwco (Aug 06 2018 at 19:42, on Zulip):

:thumbs_up:

nikomatsakis (Aug 06 2018 at 19:43, on Zulip):

@davidtwco want me to hold off on https://github.com/rust-lang/rust/pull/53124 then? (i.e., try to make those changes in this PR?)

davidtwco (Aug 06 2018 at 19:46, on Zulip):

Yeah, I'll hopefully get that in tonight @nikomatsakis

nikomatsakis (Aug 06 2018 at 19:46, on Zulip):

ok

davidtwco (Aug 06 2018 at 21:10, on Zulip):

@nikomatsakis That's the fallback changed.

davidtwco (Aug 06 2018 at 21:10, on Zulip):

Interestingly, only affected that single case.

nikomatsakis (Aug 06 2018 at 21:11, on Zulip):

what is "that"?

nikomatsakis (Aug 06 2018 at 21:11, on Zulip):

did you update PR?

nikomatsakis (Aug 06 2018 at 21:12, on Zulip):

ah, I see

nikomatsakis (Aug 06 2018 at 21:12, on Zulip):

still, very nice

nikomatsakis (Aug 06 2018 at 21:12, on Zulip):

that mostly means we need more tests I guess ;)

nikomatsakis (Aug 06 2018 at 21:13, on Zulip):

@davidtwco if you are looking for more things to do =) https://github.com/rust-lang/rust/issues/51175 are kind of interesting

nikomatsakis (Aug 06 2018 at 21:13, on Zulip):

in each case, it seems like we pick a bad span to highlight

nikomatsakis (Aug 06 2018 at 21:13, on Zulip):

would have to dig in to see why

davidtwco (Aug 06 2018 at 21:14, on Zulip):

Sounds good, I'll assign it.

Matthew Jasper (Aug 06 2018 at 21:15, on Zulip):

I think I have that fixed (or at least improved) locally (I was working on something else, which is why I didn't assign it)

davidtwco (Aug 06 2018 at 21:16, on Zulip):

I'll unassign it.

Jake Goulding (Aug 06 2018 at 23:34, on Zulip):

maybe I wound up with 'double fallback'? :)

Careful, if you get to triple fallback the processor will reset...

davidtwco (Aug 09 2018 at 07:33, on Zulip):

@nikomatsakis PR for this failed on merge testing in AppVeyor - not particularly sure why. I've seen it and will take a look.

davidtwco (Aug 09 2018 at 09:53, on Zulip):

So, I can reproduce the failure locally after a rebase.

davidtwco (Aug 09 2018 at 09:55, on Zulip):

Seems like it isn't really anything to do with this PR, something another PR has changed.

davidtwco (Aug 09 2018 at 09:56, on Zulip):

So I'm just going to do a --bless and push since I assume the output is intended given that the other PR that changed it got through review.

davidtwco (Aug 09 2018 at 09:57, on Zulip):

Even though it changes the test case for this change (and others) - the execution never seems to get to output this diagnostic this PR changed.

davidtwco (Aug 09 2018 at 09:58, on Zulip):
 failures:
 ---- [ui] ui\nll\issue-52742.rs stdout ----
 diff of stderr:
-   error: unsatisfied lifetime constraints
-     --> $DIR/issue-52742.rs:25:9
+   error[E0106]: missing lifetime specifiers
+     --> $DIR/issue-52742.rs:23:10
3      |
-   LL |     fn take_bar(&mut self, b: Bar<'_>) {
-      |                 ---------         -- let's call this `'1`
-      |                 |
-      |                 has type `&mut Foo<'_, '2>`
-   LL |         self.y = b.z
-      |         ^^^^^^^^^^^^ requires that `'1` must outlive `'2`
+   LL | impl Foo<'_, '_> {
+      |          ^^ expected 2 lifetime parameters
10
11  error: aborting due to previous error
12
+   For more information about this error, try `rustc --explain E0106`.
13

This was the test case we specifically added to test &mut Foo<'_, '2> - should we add another since it no longer does that?

nikomatsakis (Aug 09 2018 at 09:59, on Zulip):

@davidtwco see https://github.com/rust-lang/rust/pull/53124/files#r208872417

davidtwco (Aug 09 2018 at 09:59, on Zulip):

Oh, thanks!

davidtwco (Aug 09 2018 at 10:04, on Zulip):

Fixed that PR, passes locally. Should be good to r+ again once Travis confirms.

davidtwco (Aug 09 2018 at 12:44, on Zulip):

PR is all green again.

Last update: Nov 21 2019 at 23:45UTC