Stream: t-compiler/wg-nll

Topic: #57374 higher ranked subtype errors


lqd (Jan 11 2019 at 13:59, on Zulip):

so I started looking at this during my lunchbreak: I'll summarize what I think I understood (or more likely "misunderstood", heh). The "bad" error comes from check_bound_universal_region. We're checking '10 when we notice the error that the bb0/0 Location is present in its scc and emit an error where we think this "'10: '0" (IIUC) constraint is coming from, and we might have needed (if we wanted to mimick the AST borrowck behaviour) to emit a TypeError::RegionsPlaceholderMismatch placeholder failure error ("one type is more general than the other") between the 2 regions ? (however, in order to do that I'm not sure where to get a SubregionOrigin::Subtype constraint origin, or the TypeTrace needed by InferCtxt::report_and_explain_type_error)

nikomatsakis (Jan 11 2019 at 21:09, on Zulip):

@lqd that sounds about right

nikomatsakis (Jan 11 2019 at 21:09, on Zulip):

we probably can't get exactly that information

nikomatsakis (Jan 11 2019 at 21:09, on Zulip):

I'm not sure if I recall what information we can get

lqd (Jan 11 2019 at 21:32, on Zulip):

I'll try to find out

lqd (Jan 11 2019 at 21:43, on Zulip):

(but if somehow you recall, maybe while working on the similar "one type is more general than the other" error #57362, please let me know ;)

lqd (Jan 14 2019 at 18:07, on Zulip):

ok so after more hours looking at this: I'm not sure how/if to reuse the parts of rustc::infer::error_reporting and TypeError::RegionsPlaceholderMismatch in region_infer's error reporting. The 4-5 directions I've tried seemed to ultimately require some information I don't know how to get or whether it can be gotten (hitting these dead-ends is more indicative that I couldn't make them build/work rather than them not being the correct direction, possibly after some modifications, of course).

lqd (Jan 14 2019 at 18:08, on Zulip):

While I know I can manually craft an error message which adds notes and such, I'm not sure how it should look in the handful of cases in our tests (whether it should look more like the "one type is more general than the other" or "expected concrete lifetime, found bound lifetime parameter 'a" errors). So I unfortunately haven't made a lot of progress per se, apart from maybe knowing a bit more about what didn't immediately work out of the box

lqd (Jan 14 2019 at 18:09, on Zulip):

(or maybe noticing the span returned from find_outlives_blame_span seems to do subtly different things in these cases, as we can see in the hr-fn-aaa-as-aba test's output; or that I'm not sure whether disabling leak-check actually has an impact on these errors)

nikomatsakis (Jan 15 2019 at 19:25, on Zulip):

@lqd ok let me see here

nikomatsakis (Jan 15 2019 at 19:25, on Zulip):

sorry I've faile to give you much actionable advice yet

lqd (Jan 15 2019 at 19:27, on Zulip):

oh no worries !

nikomatsakis (Jan 15 2019 at 19:27, on Zulip):

the other thing is that there is the other bug

nikomatsakis (Jan 15 2019 at 19:27, on Zulip):

I have some better idea what is needed there

nikomatsakis (Jan 15 2019 at 19:27, on Zulip):

I should leave some notes

nikomatsakis (Jan 15 2019 at 19:28, on Zulip):

it might be a good starter issue :P

lqd (Jan 15 2019 at 19:28, on Zulip):

haha :) sure why not

nikomatsakis (Jan 15 2019 at 19:28, on Zulip):

I am referring to https://github.com/rust-lang/rust/issues/57362

nikomatsakis (Jan 15 2019 at 19:28, on Zulip):

let me try to leave a few notes on the issue

lqd (Jan 15 2019 at 19:29, on Zulip):

awesome, thank you

nikomatsakis (Jan 15 2019 at 20:34, on Zulip):

@lqd some notes here

nikomatsakis (Jan 15 2019 at 20:34, on Zulip):

I don't really know precisely how I would fix it

nikomatsakis (Jan 15 2019 at 20:34, on Zulip):

but I think that's the "lay of the land" in terms of what's causing the problem

nikomatsakis (Jan 15 2019 at 20:34, on Zulip):

I guess I can imagine a few different things to tweak

lqd (Jan 15 2019 at 20:36, on Zulip):

it does overlap nicely with the investigation I had been doing for the higher ranked subtyping errors

lqd (Jan 15 2019 at 20:37, on Zulip):

thanks for the notes, I'll dig into this part and see what stumbling blocks I find in order to construct such a message :)

lqd (Jan 30 2019 at 12:58, on Zulip):

@pnkfelix re: GH comment yeah my understanding is #57374 is specific to MIR borrowck, while #57362 was the dual in AST borrowck, and there's another similar one for non-NLLs #57875 which IIUC should be a dupe of #57374 under NLLs (but haven't investigated yet)

pnkfelix (Jan 30 2019 at 13:06, on Zulip):

yeah I had just had an idle hope that whatever fixed #57362 would apply to #57374. But it seems pretty obvious in hindsight based on skimming PR #57362 that it was independent.

lqd (Jan 30 2019 at 13:12, on Zulip):

if you have any ideas on how I might "generalize" the PR to handle #57875 as well (the problem there being very similar, but with PolyTraitRefs instead of TraitRefs) -- or how to go at fixing #57374, it would also ofc be very helpful :)

nikomatsakis (Jan 30 2019 at 20:07, on Zulip):

@lqd right, so, now that you dug into the "error" code for non-NLL, we can come back to this point I guess =)

lqd (Jan 30 2019 at 20:07, on Zulip):

we definitely can :)

lqd (Jan 30 2019 at 20:09, on Zulip):

I feel like it'll be similar for NLLs ?

lqd (Jan 30 2019 at 20:16, on Zulip):

hum I feel I can investigate this better, and experiment with it, now that I've seen the non-NLL scaffolding

nikomatsakis (Jan 30 2019 at 20:33, on Zulip):

it should be similar, the main question is how to get the information about the traits being solved. I can't recall what we have and what we don't off hand

lqd (Jan 30 2019 at 20:45, on Zulip):

yeah, after looking a bit at it, I remembered getting this information or knowing which info we had access to, was the area where I had the most difficulty. Maybe a roundabount way of finding the PlaceholderRegion's BoundRegion elsewhere. Or maybe it'd need to be added.

lqd (Jan 31 2019 at 11:39, on Zulip):

hey @pnkfelix :wave: -- I've noticed you're also assigned to #57374, and wondered if you could give me a couple pointers as to how/where to find some of the information we need to display the correct error message in this case (maybe some expected/found TraitRefs if it's anything like #57362) from check_bound_universal_region ? I have a Placeholder and a couple erroneous regions and everything I've tried has turned up a dead-end; I'm not sure where to look anymore :)

pnkfelix (Jan 31 2019 at 11:40, on Zulip):

let me look

lqd (Jan 31 2019 at 11:40, on Zulip):

thank you

pnkfelix (Jan 31 2019 at 11:47, on Zulip):

(hmm, I hadn't previously noticed that NLL is highlighting the type annotation for let x (while without NLL we are highlighting the use of x on the RHS of the let y statement)

pnkfelix (Jan 31 2019 at 11:48, on Zulip):

I don't have any immediate answers for you, but I've been sort of wandering around looking for something to poke at, so I might as well keep looking at this with you. :smiley:

lqd (Jan 31 2019 at 11:49, on Zulip):

yeah I had noticed find_outlives_blame_span seemed to do subtly different things for these error cases, e.g. this test has both

pnkfelix (Jan 31 2019 at 11:57, on Zulip):

so, I don't know if this helps

pnkfelix (Jan 31 2019 at 11:57, on Zulip):

but I do notice that the RUST_LOG debug output indicates that best_blame_constraint is being fed three things on the path

pnkfelix (Jan 31 2019 at 11:58, on Zulip):

(the path being the constraint path between regions)

pnkfelix (Jan 31 2019 at 11:59, on Zulip):

and one of the three things is an assignment at bb0[8], _3 = _1;

pnkfelix (Jan 31 2019 at 11:59, on Zulip):

and _3 has the type for<'a> fn(&'a ())

pnkfelix (Jan 31 2019 at 11:59, on Zulip):

while _1 has the type fn(&())

pnkfelix (Jan 31 2019 at 12:00, on Zulip):

so its possible one might try to reconstruct the useful diagnostic from that

lqd (Jan 31 2019 at 12:00, on Zulip):

I had difficulty finding out how to go from the regions themselves to the types and def_id (maybe substs as well) they came from

lqd (Jan 31 2019 at 12:01, on Zulip):

but maybe these are accessible from the path

pnkfelix (Jan 31 2019 at 12:03, on Zulip):

you might also consider generalizing the callback that we pass into best_blame_constraint

pnkfelix (Jan 31 2019 at 12:04, on Zulip):

well, maybe "generalizing" is the wrong word

pnkfelix (Jan 31 2019 at 12:04, on Zulip):

"arbitrarily changing" :)

lqd (Jan 31 2019 at 12:06, on Zulip):

alright I'll poke in this direction and see what happens, thanks Felix!

pnkfelix (Jan 31 2019 at 12:06, on Zulip):

(I want to better understand what's happening in this code path anyway, so I'll keep looking even if I don't keep posting notes here.)

lqd (Jan 31 2019 at 19:56, on Zulip):

thanks to Felix's pointers I've made a bit of progress: I think the problem revolves around this part of best_blame_constraint in our case we have a path starting with an Assignment constraint but followed by a TypeAnnotation, which will be immediately chosen as the best one (and the reason why the span seems incorrect for example). The same will happen with other categories, say, a function call argument. I tried returning the Assignment in this case, just so that I could access the Tys of the locals, and see the fallout. It didn't seem to change our existing tests (so it's likely there are other similar cases to uncover here)

lqd (Jan 31 2019 at 21:20, on Zulip):

(err, I meant changing find_outlives_blame_span to return the assignment didn't seem to make tests fail: changing best_blame_constraint itself (eg the piece of code I linked) instead would impact tests for sure)

pnkfelix (Feb 27 2019 at 21:14, on Zulip):

to be clear: did returning the Assignment at least help the particular case of #57374 itself? Or did it not even have an affect on that test (which to my knowledge is not part of our existing test suite)

lqd (Feb 27 2019 at 21:18, on Zulip):

I'll have to retry this and get back to you to be sure about the details, but it was not "the solution" for sure. It just pointed to the correct span which seems like only a smaller part of the problem, but not the error itself. in particular I remember thinking, "ok I can now see this MIR assignment should have been an error before getting to this point, so how do I reemit the same error ?"

lqd (Mar 05 2019 at 18:43, on Zulip):

omfg after 2 months, I finally have a lead on this

lqd (Mar 05 2019 at 22:13, on Zulip):

@nikomatsakis do you remember what you were looking for here ?

lqd (Mar 06 2019 at 12:41, on Zulip):

nvm, I'll add it to my notes and send them to the both of you for guidance

lqd (Mar 06 2019 at 14:32, on Zulip):

ok @pnkfelix so here's the (long-ish) summary of my understanding so far, and with that done I don't think I can make any more progress on this (maybe I should unassign myself from the issue until then)

pnkfelix (Mar 06 2019 at 14:33, on Zulip):

okay thanks for writing this up

pnkfelix (Mar 06 2019 at 14:33, on Zulip):

I wonder if it is worthwhile to post this in a comment (hidden in a <details> block) on the issue itself...

pnkfelix (Mar 06 2019 at 14:34, on Zulip):

in any case, yes, please do unassign yourself for now then

lqd (Mar 06 2019 at 14:34, on Zulip):

there are questions and meanderings in there

pnkfelix (Mar 06 2019 at 14:34, on Zulip):

yeah I can understand being hesitant to post that in a github comment

lqd (Mar 06 2019 at 14:35, on Zulip):

maybe after you vet it, let's say

pnkfelix (Mar 06 2019 at 14:39, on Zulip):

my immediate reaction is that points 1 ("The span points at the type of x.") and 2 ("The span can be incorrect in other similar cases") seem of general interest and to the point. Point 3 may be useful but it definitely meanders; I'd like to figure out a way to condense it before transcribing to the issue.

lqd (Mar 06 2019 at 14:39, on Zulip):

depending on how long we'll keep the leak check, this may be less important than P-high

lqd (Mar 06 2019 at 14:41, on Zulip):

yeah I had kept point 3 because of the similarity of the path with an existing test

pnkfelix (Mar 06 2019 at 14:41, on Zulip):

(To be clear, I'm not saying to kill off point 3. It is interesting, I think.)

lqd (Mar 06 2019 at 14:43, on Zulip):

yeah, just more information on subtly different things

lqd (Mar 06 2019 at 14:43, on Zulip):

@pnkfelix sorry but I seem to have lost writing rights or something on the repo so can't unassign myself, if you could please do it

pnkfelix (Mar 06 2019 at 14:44, on Zulip):

oh sure, no problem

lqd (Mar 06 2019 at 14:44, on Zulip):

thanks

pnkfelix (Mar 06 2019 at 14:44, on Zulip):

you mind linking to your notes from the ticket, at least?

lqd (Mar 06 2019 at 14:44, on Zulip):

yeah no problem

pnkfelix (Mar 06 2019 at 14:45, on Zulip):

(just so I don't have to go digging through the zulip log to find it)

lqd (Mar 06 2019 at 14:46, on Zulip):

done

nikomatsakis (Mar 06 2019 at 19:45, on Zulip):

@lqd btw, sorry for dropping the ball, can you point me to the latest thing I should be reading? :P

lqd (Mar 06 2019 at 19:45, on Zulip):

no worries

lqd (Mar 06 2019 at 19:46, on Zulip):

it's a bit long but here's a summary of my explorations / meandering

lqd (Mar 06 2019 at 19:49, on Zulip):

(it also has some questions I had, which are both tied to understanding the current behaviour and the reasons behind it, and what it is we want to do here, and ofc how to do it)

lqd (Mar 26 2019 at 10:52, on Zulip):

@pnkfelix have you been able to look at this btw ?

pnkfelix (Mar 26 2019 at 10:52, on Zulip):

no

pnkfelix (Mar 26 2019 at 10:54, on Zulip):

given the return of the leak-check, I think in my subconscious this bug has become P-medium (even though the actual bug is still marked P-high)

lqd (Mar 26 2019 at 10:54, on Zulip):

I was typing the same thing

lqd (Mar 26 2019 at 10:54, on Zulip):

I'm still unsure on how long the check is supposed to stay though

lqd (Mar 26 2019 at 10:55, on Zulip):

and there are other issues involving higher rank subtypes as well

pnkfelix (Mar 26 2019 at 11:02, on Zulip):

sure

Last update: Nov 21 2019 at 14:55UTC