Stream: t-compiler

Topic: leak check to universes take 2 #65232


lqd (Oct 23 2019 at 10:49, on Zulip):

since I have a little experience with region errors, I was looking at #65232's and have written down a couple of maybe-useful notes

lqd (Oct 23 2019 at 10:50, on Zulip):

especially the "expected X, found X" ones, which we did somewhat improve before in #57901 (albeit with the "little odd" wording @Matthew Jasper reported -- and which is the work you asked me about a couple weeks back Matthew, it was merged but they mostly don't show in borrowck=mir, where we'd get the "higher-ranked subtype error" most of the time IIUC), and was reported in #57362

lqd (Oct 23 2019 at 10:52, on Zulip):

in particular I looked at this test case producing this output with the PR

lqd (Oct 23 2019 at 10:53, on Zulip):

niko you mentioned maybe breaking the previous work on these errors but no we can still see it in the output.

lqd (Oct 23 2019 at 10:54, on Zulip):

in this case the difference is that there are 2 RegionResolutionErrors instead of 1:

lqd (Oct 23 2019 at 10:55, on Zulip):

did we want 2 errors here ? (or should one actually subsume/suppress the other, especially if upper bound universe conflicts partially overlap with sub-sup conflicts ?)

lqd (Oct 23 2019 at 10:57, on Zulip):

in any case, the UpperBoundUniverseConflict errors will not be using the nice_region_errors changes you added to support them in try_report_placeholder_conflict here, because try_report_nice_region_error doesn't know about them here, and also here just a bit later via try_report_named_anon_conflict (but might require the dummy sub region or making get_regions and eg try_report_named_anon_conflict -- which tries to report nice region errors before placeholder conflicts -- support optional sub/sup regions)

lqd (Oct 23 2019 at 10:58, on Zulip):

this will make the 2 errors trigger the try_report_placeholders_trait message

lqd (Oct 23 2019 at 10:59, on Zulip):

however the ObligationCause will be an item obligation, and the error message will think it's coming from a where clause, which is not technically in the surface syntax in this test but can probably be understood, or improved over time (and which I can do btw)

lqd (Oct 23 2019 at 11:04, on Zulip):

(so without other changes/improvements elsewhere, the second error

error[E0308]: mismatched types
  --> $DIR/issue-57362-2.rs:22:13
   |
LL |     let x = <fn (&())>::make_g();
   |             ^^^^^^^^^^^^^^^^^^ one type is more general than the other
   |
   = note: expected type `X`
              found type `X`

would now be, say, this)

lqd (Oct 23 2019 at 11:07, on Zulip):

now that I've finished writing up the notes, I can mention @nikomatsakis -- and if it helps I can also look at the other stderr files, to see if this little analysis applies to them as well

lqd (Oct 23 2019 at 11:43, on Zulip):

also @Matthew Jasper about your question, it seems the answer is "yes and no": there are 2 errors at this point, replacing 'x with 'static in the closure, or making 'x: 'static in the fn, will both get rid of the second error but not the first one

nikomatsakis (Nov 11 2019 at 17:41, on Zulip):

@lqd thanks for this. I started doing some digging myself. One thing that seems surprising -- you said that the "nice region errors" don't consider UpperBoundUniverseConflict, but at least on my branch it does

nikomatsakis (Nov 11 2019 at 17:41, on Zulip):

I'm actually not sure why it's not triggering

nikomatsakis (Nov 11 2019 at 17:41, on Zulip):

I'm adding a bit more debug

nikomatsakis (Nov 11 2019 at 17:41, on Zulip):

that said, I agree that we don't really want two errors here anyway, they are effectively duplicates

lqd (Nov 11 2019 at 18:21, on Zulip):

hum I did build your branch at the time to investigate, and it didn’t seem to trigger until I made the changes I mentioned here

lqd (Nov 11 2019 at 18:30, on Zulip):

to maybe be more clear, the "big match" in try_report_placeholder_conflict does handle them, but they never did get there

lqd (Nov 11 2019 at 18:31, on Zulip):

because try_report_nice_region_error didn't itself know about this new variant

lqd (Nov 11 2019 at 18:31, on Zulip):

but maybe you've updated the branch since I last saw it though, let me check

lqd (Nov 11 2019 at 18:44, on Zulip):

@nikomatsakis they'll trigger if you add the UpperBoundUniverseConflict variant here https://github.com/rust-lang/rust/blob/b99b7325e36fb14ee4391f0d3193d2ea640dabcf/src/librustc/infer/error_reporting/nice_region_error/mod.rs#L20

lqd (Nov 11 2019 at 19:22, on Zulip):

and also here https://github.com/rust-lang/rust/blob/b99b7325e36fb14ee4391f0d3193d2ea640dabcf/src/librustc/infer/error_reporting/nice_region_error/mod.rs#L80-L84

nikomatsakis (Nov 11 2019 at 20:07, on Zulip):

@lqd I think I found the bug

nikomatsakis (Nov 11 2019 at 20:07, on Zulip):

oh, I see you .. yes

nikomatsakis (Nov 11 2019 at 20:07, on Zulip):

that is the change I made, more or less :)

nikomatsakis (Nov 11 2019 at 20:07, on Zulip):

haha

nikomatsakis (Nov 11 2019 at 20:07, on Zulip):

more to the point, I removed the match altogether

lqd (Nov 11 2019 at 20:08, on Zulip):

nice :)

nikomatsakis (Nov 11 2019 at 20:08, on Zulip):

this required a few other minor changes

lqd (Nov 11 2019 at 20:09, on Zulip):

the change in get_regions might have required to use the dummy sub again

nikomatsakis (Nov 11 2019 at 20:10, on Zulip):

I made get_regions return an Option

nikomatsakis (Nov 11 2019 at 20:10, on Zulip):

and the callers just use ?

nikomatsakis (Nov 11 2019 at 20:10, on Zulip):

about to push, I was making x.py test --bless work

lqd (Nov 11 2019 at 20:10, on Zulip):

yeah that's what I was mentioning last time as well :)

lqd (Nov 11 2019 at 20:10, on Zulip):

(including those 2 exact same links you know :p)

nikomatsakis (Nov 11 2019 at 20:11, on Zulip):

sigh

lqd (Nov 11 2019 at 20:11, on Zulip):

but buried among too many text for sure

lqd (Nov 11 2019 at 20:11, on Zulip):

glad it's working now

nikomatsakis (Nov 11 2019 at 20:11, on Zulip):

now the question is

lqd (Nov 11 2019 at 20:11, on Zulip):

you'll probably have the "where clause" error mesg tho now

nikomatsakis (Nov 11 2019 at 20:11, on Zulip):

it seems like we don't get 2 errors anymore

nikomatsakis (Nov 11 2019 at 20:11, on Zulip):

I think some duplicate suppression is working? not really sure what is happening :)

lqd (Nov 11 2019 at 20:12, on Zulip):

that's fortunate :)

nikomatsakis (Nov 11 2019 at 20:12, on Zulip):

the question is ... what do I have to do to land this branch besides this

nikomatsakis (Nov 11 2019 at 20:12, on Zulip):

I think some duplicate suppression is working? not really sure what is happening :)

not 100% sure

lqd (Nov 11 2019 at 20:12, on Zulip):

were you testing https://github.com/rust-lang/rust/blob/master/src/test/ui/issues/issue-57362-2.rs ?

lqd (Nov 11 2019 at 20:14, on Zulip):

some of Matthew's concerns can probably be dealt in future PRs (which I can also work on, esp around the error messages and things like this)

lqd (Nov 11 2019 at 20:15, on Zulip):

but they still had some questions IIRC

lqd (Nov 11 2019 at 20:16, on Zulip):

maybe a crater run ?

lqd (Nov 11 2019 at 20:17, on Zulip):

(you left a list of TODO items in the PR text as well)

Matthew Jasper (Nov 11 2019 at 20:17, on Zulip):

the question is ... what do I have to do to land this branch besides this

I think that I was generally happy with it. There was a regression that I was somewhat concerned about ...

Matthew Jasper (Nov 11 2019 at 20:18, on Zulip):

src/test/ui/hrtb/issue-57639.rs
https://github.com/rust-lang/rust/pull/65232/files#diff-973f3a129f85d3c38af3cdbb4557fbc6

lqd (Nov 11 2019 at 20:32, on Zulip):

this regression seemed uncommon last time (eg it didn't trigger on the universes crater run at the time)

nikomatsakis (Nov 12 2019 at 14:49, on Zulip):

One thing I am wondering about is the coherence effects

nikomatsakis (Nov 12 2019 at 14:49, on Zulip):

I feel like there might need to be an RFC or other formal decision

nikomatsakis (Nov 12 2019 at 14:49, on Zulip):

I do think I could make a warning period, having thought about it some

nikomatsakis (Nov 12 2019 at 14:49, on Zulip):

I think I might prefer to put off the NLL work

Last update: Nov 16 2019 at 01:35UTC