Stream: t-compiler

Topic: #57362 confusing errors after universe transition


lqd (Jan 16 2019 at 19:06, on Zulip):

@nikomatsakis I have some simple in-progress work that basically results in this diff for dtolnay's original issue #57362, part of Matthew's #57642, and a blessed ui test. My beginner-itude makes me wonder whether this result is good or bad or stupid ugly, and how to improve it in the latter cases. (bbl, there's no rush :)

nikomatsakis (Jan 16 2019 at 20:01, on Zulip):

@lqd I was wondering this morning whether we ought to be taking another path in the case of this error

nikomatsakis (Jan 16 2019 at 20:01, on Zulip):

as in, maybe the framing of "mismatched types" is ungreat

nikomatsakis (Jan 16 2019 at 20:01, on Zulip):

though of course those results look good

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

with NLLs these specific errors we've been looking at recently are "higher-rank subtype" errors rather than "mismatched types" (IIRC; but maybe not all of them ofc), so you mean framing these differently with the current errors (coming from AST borrowck right now IIUC) ? or most likely a general approach for these higher-rank errors (maybe linked to incompatible universes)

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

(if these results look interesting I can open a PR tomorrow so that it's better tested and can be reviewed, as I'm not sure how :fire: p-high is and if this issue impacts the release/beta train processes happening for the new release)

nikomatsakis (Jan 16 2019 at 21:02, on Zulip):

what I meant very concretely was trying to invoke the try_report_placeholder_conflict code from the nice-region-errors module

nikomatsakis (Jan 16 2019 at 21:02, on Zulip):

just thinking about this case, not the NLL case yet

nikomatsakis (Jan 16 2019 at 21:02, on Zulip):

it reports errors like this

nikomatsakis (Jan 16 2019 at 21:02, on Zulip):
    // error[E0308]: implementation of `Foo` does not apply to enough lifetimes
    //   --> /home/nmatsakis/tmp/foo.rs:12:5
    //    |
    // 12 |     all::<&'static u32>();
    //    |     ^^^^^^^^^^^^^^^^^^^ lifetime mismatch
    //    |
    //    = note: Due to a where-clause on the function `all`,
    //    = note: `T` must implement `...` for any two lifetimes `'1` and `'2`.
    //    = note: However, the type `T` only implements `...` for some specific lifetime `'2`.
nikomatsakis (Jan 16 2019 at 21:02, on Zulip):

those kind of just seem better

nikomatsakis (Jan 16 2019 at 21:02, on Zulip):

I'm a bit curious why those errors aren't firing

lqd (Jan 16 2019 at 21:05, on Zulip):

oh interesting indeed, I will try to find more information on why they don't

nikomatsakis (Jan 16 2019 at 21:06, on Zulip):

maybe we don't enter via report_region_errors? I'm not really sure

nikomatsakis (Jan 16 2019 at 21:06, on Zulip):

I'd just run the test with RUST_LOG and maybe sprinkle some more debug! to find out

nikomatsakis (Jan 16 2019 at 21:06, on Zulip):

ok, gotta run

nikomatsakis (Jan 16 2019 at 21:06, on Zulip):

thanks @lqd! =)

lqd (Jan 16 2019 at 21:06, on Zulip):

IIRC we don't enter through report_region_errors indeed

lqd (Jan 16 2019 at 21:08, on Zulip):

I remember it being from report_and_explain_type_error but will check again tomorrow (or maybe the report_region_errors is higher in the stack)

lqd (Jan 16 2019 at 21:09, on Zulip):

at some point it's a subsupconflict and ends up here somehow

lqd (Jan 16 2019 at 21:12, on Zulip):

(I will trace it later today and report here)

lqd (Jan 17 2019 at 10:13, on Zulip):

quick correction until I finish my modifications, we indeed go though report_region_errors which detects a SubSupConflict and it seems try_report_placeholder_conflict doesn't find it applicable

lqd (Jan 17 2019 at 10:15, on Zulip):

(I think it's in the asymetric testing of subtype + placeholder, maybe that it only accepts subtype errors in the "sub" region I think, while this is in the "sup" one)

lqd (Jan 17 2019 at 10:43, on Zulip):

@nikomatsakis if I add this missing case of checking that the origin of the sup placeholder is a subtype (if I understand this correctly I think 2 match cases might be missing in total from here to make these errors fire via the nice-error-regions) try_report_placeholder_conflict will fire the error, resulting in this for the specific code in the issue (it seems the existing message code was written with the sub subtype in mind, so might need changing or reversing the expected/found values in some cases)

nikomatsakis (Jan 17 2019 at 13:41, on Zulip):

@lqd interesting

nikomatsakis (Jan 17 2019 at 13:41, on Zulip):

I definitely prefer the overall structure this way

nikomatsakis (Jan 17 2019 at 13:41, on Zulip):

but the message is confusing, I think, in two ways

nikomatsakis (Jan 17 2019 at 13:41, on Zulip):

first off, you are correct about the expected/actual being reversed

nikomatsakis (Jan 17 2019 at 13:42, on Zulip):

but secondly, I think the phrasing doesn't work

nikomatsakis (Jan 17 2019 at 13:42, on Zulip):

e.g. if we just reverse expected/found we get

error: implementation of `Trait` is not general enough
   --> src/main.rs:13:7
    |
 13 |     a.f();
    |       ^
    |
    = note: `for<'r> fn(&'r _)` must implement `Trait`
    = note: but `fn(&u8)` only implements `Trait`
nikomatsakis (Jan 17 2019 at 13:42, on Zulip):

the position of the "only" feels wrong

lqd (Jan 17 2019 at 13:43, on Zulip):

yeah

lqd (Jan 17 2019 at 13:44, on Zulip):

it probably works better for the expected sub case and non higher-rank cases

nikomatsakis (Jan 17 2019 at 13:46, on Zulip):
error: implementation of `Trait` is not general enough
   --> src/main.rs:13:7
    |
 13 |     a.f();
    |       ^
    |
    = note: `Trait` must be implemented for `for<'r> fn(&'r _)`
    = note: but `Trait` is only implemented for `fn(&u8)`
nikomatsakis (Jan 17 2019 at 13:47, on Zulip):

that's probably what I would want to see

nikomatsakis (Jan 17 2019 at 13:47, on Zulip):

man it's delicate

nikomatsakis (Jan 17 2019 at 13:47, on Zulip):

hmm

nikomatsakis (Jan 17 2019 at 13:47, on Zulip):

I wonder if this ordering is correct in those cases where the "placeholders" don't appear in the types/traits

nikomatsakis (Jan 17 2019 at 13:47, on Zulip):

i.e., don't we sometimes have something like "for any lifetime '0"

nikomatsakis (Jan 17 2019 at 13:48, on Zulip):

we could also certainly detect the case of a trait with no generics (like this one)

nikomatsakis (Jan 17 2019 at 13:48, on Zulip):

in which case this ordering is always correct

nikomatsakis (Jan 17 2019 at 13:48, on Zulip):

I shuold think

nikomatsakis (Jan 17 2019 at 13:48, on Zulip):

you up to do some experimentation like that?

nikomatsakis (Jan 17 2019 at 13:48, on Zulip):

I'm kind of excited by the idea of higher-ranked errors being comprehensible

nikomatsakis (Jan 17 2019 at 13:49, on Zulip):

they've always been terrible

lqd (Jan 17 2019 at 13:49, on Zulip):

you up to do some experimentation like that?

sure :)

lqd (Jan 17 2019 at 13:51, on Zulip):

(it will surely require leveling up my understanding of higher-ranked topics and errors and I might have to ask questions if that's ok)

nikomatsakis (Jan 17 2019 at 13:52, on Zulip):

but of course

nikomatsakis (Jan 17 2019 at 13:52, on Zulip):

I wonder if we should land some kind of "stem the bleeding" PR first

lqd (Jan 17 2019 at 13:52, on Zulip):

we definitely can

nikomatsakis (Jan 17 2019 at 13:52, on Zulip):

e.g., just the change you made above plus reverse the expected/found maybe?

nikomatsakis (Jan 17 2019 at 13:52, on Zulip):

(or the changes you showed at first?)

centril (Jan 17 2019 at 13:53, on Zulip):

Adjusting Niko's sample error, I'd emit something like:

error: implementation of `Trait` is not general enough
   --> src/main.rs:13:7
    |
 13 |     a.f();
    |       ^
    |
    = note: `Trait` must be implemented for `for<'r> fn(&'r _)`
    = note: but `Trait` is only implemented for `fn(&'a u8)` for a specific `'a`
lqd (Jan 17 2019 at 13:53, on Zulip):

both could work tbh, I'll try running the test suite on the 2nd one to check if there's any problems elsewhere

nikomatsakis (Jan 17 2019 at 13:53, on Zulip):

thanks

nikomatsakis (Jan 17 2019 at 13:53, on Zulip):

I think I prefer using the "specialized" errors

nikomatsakis (Jan 17 2019 at 13:53, on Zulip):

if we get the expected/found right

nikomatsakis (Jan 17 2019 at 13:54, on Zulip):

since it's closer to what we want eventually, and I think they're still more "on point"

nikomatsakis (Jan 17 2019 at 13:54, on Zulip):

this is not really about a "mismatched type"

lqd (Jan 17 2019 at 13:54, on Zulip):

alright I'll continue on this path so at least we can see the self type in the error soon, and then we can iterate towards having them improved

lqd (Jan 17 2019 at 16:54, on Zulip):

when the subtype origin is on the sup placeholder, the expected/found traitrefs aren't always what I'd expect, or rather, the more general of the two can be either one, while I believe try_report_placeholder_conflict currently expects the more general one to be expected. For example, the code in #57362 results, like we mentioned earlier, in the inverted

error: implementation of `Trait` is not general enough
  --> src/main.rs:13:7
   |
13 |     a.f();
   |       ^
   |
   = note: `fn(&u8)` must implement `Trait`
   = note: but `for<'r> fn(&'r _)` only implements `Trait`

while Matthew's examples from #57642 seem ok

error: implementation of `X` is not general enough
  --> src/main.rs:44:13
   |
44 |     let x = <fn (&())>::make_g();
   |             ^^^^^^^^^^^^^^^^^^
   |
   = note: `for<'r> fn(&'r ())` must implement `X`
   = note: but `fn(&'0 ())` only implements `X` for the lifetime `'0`

error: implementation of `Y` is not general enough
  --> src/main.rs:49:13
   |
49 |     let x = <fn (&())>::make_f();
   |             ^^^^^^^^^^^^^^^^^^
   |
   = note: `for<'r> fn(&'r ())` must implement `Y`
   = note: but `fn(_)` only implements `Y`

should I be looking for something to compute which traitref is actually the more general @nikomatsakis ?

lqd (Jan 17 2019 at 17:04, on Zulip):

(or could it also be a bug somewhere downstream from / in the lexical_region_resolve module for these higher ranked conflicts?)

lqd (Jan 17 2019 at 18:20, on Zulip):

(or possibly linked to the other universe-related issues)

nikomatsakis (Jan 17 2019 at 22:25, on Zulip):

@lqd that sounds right. This is probably because we wind up encoding 'a = 'b as the pair of 'a: 'b and 'b: 'a

nikomatsakis (Jan 17 2019 at 22:26, on Zulip):

hmm

nikomatsakis (Jan 17 2019 at 22:27, on Zulip):

I'm not sure why we are getting it backwards really, maybe there is some code higher up that is not using expected/actual the way we expect

lqd (Jan 17 2019 at 22:27, on Zulip):

the sub subregion origin is a ExprTypeIsNotInScope(fn(&()) in the correct case, and I wondered if it was related (probably not)

lqd (Jan 17 2019 at 22:29, on Zulip):

example dumping the error + expected/found in this inverted case

DEBUG 2019-01-17T22:33:12Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholder_conflict(error=Some(SubSupConflict('_#1r, MiscVariable(src/main.rs:13:7: 13:8), ParameterInScope(MethodCall, src/main.rs:13:5: 13:10), ReScope(Node(18)), Subtype(TypeTrace(ObligationCause { span: src/main.rs:13:7: 13:8, body_id: NodeId(85), code: MiscObligation })), RePlaceholder(Placeholder { universe: U7, name: BrAnon(0) }))))
DEBUG 2019-01-17T22:33:12Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholder_conflict - SubSupConflict 3.5, expected=<fn(&u8) as Trait>, found=<for<'r> fn(&'r _) as Trait>
lqd (Jan 18 2019 at 14:13, on Zulip):

I wonder if this results from evaluating the <fn(&u8) as Trait> candidate as OkModuloRegions; when trying to confirm it, we check At.eq(expected=<fn(&u8) as Trait>, actual=<for<'r> fn(&'r _) as Trait>) which makes the candidate "expected"

nikomatsakis (Jan 18 2019 at 14:41, on Zulip):

@lqd where do we do that check?

nikomatsakis (Jan 18 2019 at 14:41, on Zulip):

that is, where is the confirmation occurring exactly?

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

let me paste you a bit of the log

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

so here is the pick_method on fn(&u8)

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

(it's a bit big, but smaller than the 9K lines I'm looking at)

nikomatsakis (Jan 18 2019 at 14:44, on Zulip):

:)

nikomatsakis (Jan 18 2019 at 14:44, on Zulip):

yeah

nikomatsakis (Jan 18 2019 at 14:45, on Zulip):

man, I'd love to find a better way to handle logging

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

especially with probes, snapshots, rollbacks

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

I wonder if having this evaluated as EvaluatedToOkModuloRegions will make the code downstream think fn(&u8) as Trait is indeed expected

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

which I think is following this log, while "confirming" this candidate

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

eg later DEBUG 2019-01-18T14:14:51Z: rustc::traits::select: match_impl(impl_def_id=DefId(0/0:5 ~ universe_bug[ac27]::{{impl}}[0]), obligation=Obligation(predicate=Binder(TraitPredicate(<fn(&u8) as Trait>)),depth=0), impl_trait_ref=<for<'r> fn(&'r _) as Trait>, skol_obligation_trait_ref=<fn(&u8) as Trait>)

nikomatsakis (Jan 18 2019 at 14:49, on Zulip):

I wonder if having this evaluated as EvaluatedToOkModuloRegions will make the code downstream think fn(&u8) as Trait is indeed expected

I don't think this is quite right

nikomatsakis (Jan 18 2019 at 14:50, on Zulip):

that is, the result of evaluation wouldn't really affect that

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

oh

nikomatsakis (Jan 18 2019 at 14:50, on Zulip):

it would just be used to select which canidate we conirm

nikomatsakis (Jan 18 2019 at 14:50, on Zulip):

however, we might have expected/actual "backwards"

nikomatsakis (Jan 18 2019 at 14:50, on Zulip):

conceivably

nikomatsakis (Jan 18 2019 at 14:50, on Zulip):

let me briefly skim code, one sec

nikomatsakis (Jan 18 2019 at 14:51, on Zulip):

ok, so the relevant bit is this:

        let InferOk { obligations, .. } = self.infcx
            .at(&obligation.cause, obligation.param_env)
            .eq(skol_obligation_trait_ref, impl_trait_ref)
            .map_err(|e| debug!("match_impl: failed eq_trait_refs due to `{}`", e))?;
        nested_obligations.extend(obligations);
nikomatsakis (Jan 18 2019 at 14:51, on Zulip):

from match_impl

nikomatsakis (Jan 18 2019 at 14:51, on Zulip):

which iirc implies that the "thing we are trying to prove" is going to be called the "expected"

nikomatsakis (Jan 18 2019 at 14:51, on Zulip):

and the impl is "actual"

nikomatsakis (Jan 18 2019 at 14:51, on Zulip):

now it is also quite possible that we lose track of that later on

nikomatsakis (Jan 18 2019 at 14:52, on Zulip):

but does that match what the error message code seems to expect?

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

I think it does, but not always but it's still unclear :/

nikomatsakis (Jan 18 2019 at 14:57, on Zulip):

hmm

nikomatsakis (Jan 18 2019 at 14:57, on Zulip):

as in, in some examples it seems to work, but others not?

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

then the interesting part of the log following this was a bit later

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

as in, in some examples it seems to work, but others not?

yeah :/

nikomatsakis (Jan 18 2019 at 14:58, on Zulip):

can you send me one example where it seems to fit and one where it doesn't?

nikomatsakis (Jan 18 2019 at 14:58, on Zulip):

more importantly I guess the one where it doesn't

nikomatsakis (Jan 18 2019 at 14:59, on Zulip):

(sorry, I know if I scroll up I will probably see them...)

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

sure

nikomatsakis (Jan 18 2019 at 14:59, on Zulip):

also, do you have a branch I can build?

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

no worries :)

lqd (Jan 18 2019 at 15:00, on Zulip):

yeah will push a branch soon with the added debug! + the nice-error-region change just after getting the examples

lqd (Jan 18 2019 at 15:06, on Zulip):

1) good example + error
2) bad example + error
branch incoming shortly

lqd (Jan 18 2019 at 15:21, on Zulip):

the branch

lqd (Jan 23 2019 at 17:00, on Zulip):

@nikomatsakis (sorry to bother you) do you by any chance have pointers on where to look to debug this possible expected/actual inversion ?

nikomatsakis (Jan 23 2019 at 17:09, on Zulip):

@lqd thanks for the reminder

nikomatsakis (Jan 23 2019 at 17:12, on Zulip):

I have an errand to run but will try to get back t you after that

lqd (Jan 23 2019 at 17:12, on Zulip):

awesome, thank you

nikomatsakis (Jan 23 2019 at 20:53, on Zulip):

ok, @lqd, let's talk :)

lqd (Jan 23 2019 at 20:53, on Zulip):

:)

lqd (Jan 23 2019 at 20:54, on Zulip):

I'm looking at the branch + log traces to remember a bit of everything

nikomatsakis (Jan 23 2019 at 20:55, on Zulip):

me too

nikomatsakis (Jan 23 2019 at 20:57, on Zulip):

I guess I better grab your branch

lqd (Jan 23 2019 at 20:57, on Zulip):

I can upload the 2 traces as well

nikomatsakis (Jan 23 2019 at 20:58, on Zulip):

building your branch now

lqd (Jan 23 2019 at 21:00, on Zulip):

at some point we mentioned https://rust-lang.zulipchat.com/#narrow/stream/131828-t-compiler/topic/.2357362.20confusing.20errors.20after.20universe.20transition/near/156373149 but there wasn't something immediately obvious to me about it in the traces (but that could be because of the Display format rather than Debug)

lqd (Jan 23 2019 at 21:00, on Zulip):

that is match_impl: failed logs

lqd (Jan 23 2019 at 21:00, on Zulip):

but could very well be important as you mentioned

nikomatsakis (Jan 23 2019 at 21:01, on Zulip):

hmm

nikomatsakis (Jan 23 2019 at 21:01, on Zulip):

I probably just have to look a bit at the logs

Jake Goulding (Jan 23 2019 at 21:08, on Zulip):

I probably just have to look a bit at the logs

lqd (Jan 23 2019 at 21:09, on Zulip):

what I was looking at was the confirm_impl_candidate for <fn(&u8) as Trait> which starts a snapshot and a bit later ends up making a subregion whose trace values are expected: <fn(&u8) as Trait>, found: <for<'r> fn(&'r _) as Trait> which commits since the result is Ok

nikomatsakis (Jan 23 2019 at 21:12, on Zulip):

yeah I mean this line does look a bit suspicious:

        let InferOk { obligations, .. } = self.infcx
            .at(&obligation.cause, obligation.param_env)
            .eq(skol_obligation_trait_ref, impl_trait_ref)
            .map_err(|e| debug!("match_impl: failed eq_trait_refs due to `{}`", e))?;
nikomatsakis (Jan 23 2019 at 21:12, on Zulip):

in particular, I think that the first argument to eq is the "expected" thing

nikomatsakis (Jan 23 2019 at 21:12, on Zulip):

in this case, that is the thing we are trying to prove

nikomatsakis (Jan 23 2019 at 21:12, on Zulip):

and I believe the "actual" is the type from the impl

nikomatsakis (Jan 23 2019 at 21:12, on Zulip):

not sure if that's what the error later suggests, let me check

nikomatsakis (Jan 23 2019 at 21:13, on Zulip):

well

nikomatsakis (Jan 23 2019 at 21:13, on Zulip):

it looks right

nikomatsakis (Jan 23 2019 at 21:14, on Zulip):

I wonder if it gets turned around somehow

lqd (Jan 23 2019 at 21:14, on Zulip):

and only in some cases but not all

lqd (Jan 23 2019 at 21:15, on Zulip):

or maybe some miscommunication at the boundaries of what we're trying to prove, and the errors we need to emit later

nikomatsakis (Jan 23 2019 at 21:17, on Zulip):

:/

lqd (Jan 23 2019 at 21:19, on Zulip):

eg proving the trait refs work out here, since one is a subtype of the other, but when we later check if the subregions are valid in the higher rank case, it turns out this previous result can't be reused as is

nikomatsakis (Jan 23 2019 at 21:23, on Zulip):

@lqd sorry for not having a lot of data, build is taking a while of course

nikomatsakis (Jan 23 2019 at 21:23, on Zulip):

I'm kind of doing a few other things while it goes, i'll ping you back in a bit

lqd (Jan 23 2019 at 21:23, on Zulip):

sure, no problem :)

nikomatsakis (Jan 23 2019 at 21:31, on Zulip):

ok that finally finished

nikomatsakis (Jan 23 2019 at 21:35, on Zulip):

um so @lqd

Alexander Regueiro (Jan 23 2019 at 21:35, on Zulip):

I'm kind of doing a few other things while it goes, i'll ping you back in a bit

does that include building my branch heh? ;-) maybe you can see what's going on from the debug logs though.

nikomatsakis (Jan 23 2019 at 21:35, on Zulip):

(it does not :) but that's a good idea, I should kick that off)

lqd (Jan 23 2019 at 21:35, on Zulip):

I wonder if it's a problem more at the error generation point, e.g. in some of these SubSupConflict cases we should be looking for the more general of the 2 expected/found

nikomatsakis (Jan 23 2019 at 21:35, on Zulip):

So, @lqd, the error message -- looking more closely -- I think it's actually correct

nikomatsakis (Jan 23 2019 at 21:36, on Zulip):

or am I missing something

lqd (Jan 23 2019 at 21:36, on Zulip):

/me mindblown

nikomatsakis (Jan 23 2019 at 21:36, on Zulip):

that is, in expected-seems-bad.rs, we have:

nikomatsakis (Jan 23 2019 at 21:36, on Zulip):
impl<T> Trait for fn(&T)
nikomatsakis (Jan 23 2019 at 21:36, on Zulip):

i.e., it is implemented for for<'a> fn(&'a T)

nikomatsakis (Jan 23 2019 at 21:36, on Zulip):

and the error message says:

but `for<'r> fn(&'r _)` only implements `Trait`
nikomatsakis (Jan 23 2019 at 21:37, on Zulip):

that is pretty bad phrasing, but it is trying to say "there is an implementation, but for the wrong type"

nikomatsakis (Jan 23 2019 at 21:37, on Zulip):

and indeed the type it lists is the one that it is implemented for

nikomatsakis (Jan 23 2019 at 21:38, on Zulip):

so maybe the problem is just that the error message is really ungreat?

nikomatsakis (Jan 23 2019 at 21:38, on Zulip):

we have this logic that tries to detect how often the placeholder occured

lqd (Jan 23 2019 at 21:38, on Zulip):

what does the _placeholder stand for here ?

nikomatsakis (Jan 23 2019 at 21:39, on Zulip):

'some type'

lqd (Jan 23 2019 at 21:39, on Zulip):

but not u8

nikomatsakis (Jan 23 2019 at 21:39, on Zulip):

yeah I don't know why that is unresolved

lqd (Jan 23 2019 at 21:40, on Zulip):

the ungreatness + unresolvedness seems to be why we were confused almost from the start about the ordering

nikomatsakis (Jan 23 2019 at 21:40, on Zulip):

maybe we just need to invoke the inference method that refereshes those

nikomatsakis (Jan 23 2019 at 21:40, on Zulip):

resolve_type_vars_if_possible

lqd (Jan 23 2019 at 21:41, on Zulip):

it kinda improves from the "expected type Trait found type Trait" but not by much :)

nikomatsakis (Jan 23 2019 at 21:41, on Zulip):

in any case, I think when we see the has_vid flag being None

lqd (Jan 23 2019 at 21:41, on Zulip):

I can try calling these yeah

nikomatsakis (Jan 23 2019 at 21:42, on Zulip):

we want to print something different

nikomatsakis (Jan 23 2019 at 21:42, on Zulip):

like, "but {} is implemented for {}"

nikomatsakis (Jan 23 2019 at 21:42, on Zulip):

to put the emphasis on the self type

nikomatsakis (Jan 23 2019 at 21:42, on Zulip):

ideally, we would also call out the "free" lifetime from the other type

nikomatsakis (Jan 23 2019 at 21:42, on Zulip):

I'm not 100% sure how to do that

nikomatsakis (Jan 23 2019 at 21:43, on Zulip):

oh, we should be able to do that

nikomatsakis (Jan 23 2019 at 21:43, on Zulip):

so, I see this in the debug logs

DEBUG 2019-01-23T21:32:18Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholder_conflict(error=Some(SubSupConflict('_#1r, MiscVariable(/home/nmatsakis/tmp/expected-seems-bad.rs:13:7: 13:8), Param\
eterInScope(MethodCall, /home/nmatsakis/tmp/expected-seems-bad.rs:13:5: 13:10), ReScope(Node(18)), Subtype(TypeTrace(ObligationCause { span: /home/nmatsakis/tmp/expected-seems-bad.rs:13:7: 13:8, body_id: NodeId(85), code: MiscObligatio\
n })), RePlaceholder(Placeholder { universe: U7, name: BrAnon(0) }))))
nikomatsakis (Jan 23 2019 at 21:43, on Zulip):

ugh that's hard to read

nikomatsakis (Jan 23 2019 at 21:43, on Zulip):

the key thing is that the conflict involves '_#1r

nikomatsakis (Jan 23 2019 at 21:44, on Zulip):

(oh, btw, when you dump logs, are you using -Zverbose?)

lqd (Jan 23 2019 at 21:44, on Zulip):

I was not :/

nikomatsakis (Jan 23 2019 at 21:44, on Zulip):

it's...kind of crucial

nikomatsakis (Jan 23 2019 at 21:44, on Zulip):

the expected trait-ref, in turn, is`<fn(&'_#1r u8) as Trait>

nikomatsakis (Jan 23 2019 at 21:44, on Zulip):

(in particular, it causes the full detail of lifetimes to be printed

lqd (Jan 23 2019 at 21:45, on Zulip):

yeah, seems useful when trying to debug higher-ranked lifetimes :)

nikomatsakis (Jan 23 2019 at 21:45, on Zulip):

our pretty printing system is quite messed up

nikomatsakis (Jan 23 2019 at 21:45, on Zulip):

in my opinion {:?} should always print this info

nikomatsakis (Jan 23 2019 at 21:45, on Zulip):

but oh well

nikomatsakis (Jan 23 2019 at 21:46, on Zulip):

I'm doing another build to dump out a bit more information...

lqd (Jan 23 2019 at 21:46, on Zulip):

(I'll make sure we have this in rustc-guide if we haven't already)

nikomatsakis (Jan 23 2019 at 21:47, on Zulip):

thanks

nikomatsakis (Jan 23 2019 at 21:47, on Zulip):

anyway, this is kind of good, I think it just means we have to tweak the output a bit

lqd (Jan 23 2019 at 21:47, on Zulip):

oh, we should be able to do that

did you mean modifying the message formatting in try_report_placeholders_trait w.r.t the vid here ?

nikomatsakis (Jan 23 2019 at 21:48, on Zulip):

yes

nikomatsakis (Jan 23 2019 at 21:48, on Zulip):

what I specifically meant is:

nikomatsakis (Jan 23 2019 at 21:48, on Zulip):

I would like to print something like

nikomatsakis (Jan 23 2019 at 21:49, on Zulip):
// error: implementation of `Trait` is not general enough
//   --> src/main.rs:13:7
//    |
// 13 |     a.f();
//    |       ^
//    |
//    = note: `Trait` would have to be implemented for the type `fn(&'0 u8)`, for some lifetime `'0`
//    = note: but `Trait` is actually implemented for `for<'r> fn(&'r u8)`
nikomatsakis (Jan 23 2019 at 21:49, on Zulip):

or..something like that

nikomatsakis (Jan 23 2019 at 21:49, on Zulip):

I forget what the suggestions were before

nikomatsakis (Jan 23 2019 at 21:49, on Zulip):

anyway, the key thing is .. how do we know to highlight that '0 region?

nikomatsakis (Jan 23 2019 at 21:49, on Zulip):

but the connection to '_#1r gives us a key to latch on to

lqd (Jan 23 2019 at 21:49, on Zulip):

something like that

nikomatsakis (Jan 23 2019 at 21:50, on Zulip):

yeah, except this is the reverse case

nikomatsakis (Jan 23 2019 at 21:50, on Zulip):

but I like the "some specific" phrasing

lqd (Jan 23 2019 at 21:50, on Zulip):

yeah the examples were made while still being confused by the ordering/message

nikomatsakis (Jan 23 2019 at 21:51, on Zulip):

do you feel at all unblocked? :)

lqd (Jan 23 2019 at 21:51, on Zulip):

thanks a lot, I can now try to change the error messages in try_report_placeholders_trait, and also check what effect resolve_type_vars_if_possible will have for them

nikomatsakis (Jan 23 2019 at 21:51, on Zulip):

ok

lqd (Jan 23 2019 at 21:52, on Zulip):

I'll work on that and report back whenever I'm at at an interesting point (or open a PR directly if it's interesting enough)

nikomatsakis (Jan 23 2019 at 21:52, on Zulip):

well I've got a build going with a bit of extra debug info

nikomatsakis (Jan 23 2019 at 21:53, on Zulip):

so maybe I'll dump some more notes if I have anything

nikomatsakis (Jan 23 2019 at 21:53, on Zulip):

else feel free to ping with questions =)

nikomatsakis (Jan 23 2019 at 21:53, on Zulip):

(if needed)

lqd (Jan 23 2019 at 21:53, on Zulip):

awesome, thanks again niko :)

nikomatsakis (Jan 23 2019 at 21:53, on Zulip):

/me gets back to trying to draft up all hands meeting agendas :)

lqd (Jan 23 2019 at 21:57, on Zulip):

(I wonder who else has experience with this so I wouldn't have to bother you per se, seems likely maybe felix or scalexm have worked in these areas)

nikomatsakis (Jan 23 2019 at 22:08, on Zulip):

maybe @scalexm, though this code I added pretty recently

nikomatsakis (Jan 23 2019 at 22:13, on Zulip):

some logs from try_report_placeholders_trait

DEBUG 2019-01-23T22:11:40Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholders_trait(
    vid=Some('_#1r),
    sub_placeholder=None,
    sup_placeholder=Some(RePlaceholder(Placeholder { universe: U7, name:
BrAnon(0) })),
    trait_def_id=DefId(0/0:3 ~ expected_seems_bad[317d]::Trait[0]),
    expected_substs=[fn(&'_#1r u8)],
    actual_substs=[for<'r> fn(&ReLateBound(DebruijnIndex(0), BrNamed(crate0:DefIndex(0:0), 'r)) _#6t)])
DEBUG 2019-01-23T22:11:40Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholders_trait: has_vid=None
DEBUG 2019-01-23T22:11:40Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholders_trait: has_sub=None
DEBUG 2019-01-23T22:11:40Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholders_trait: has_sup=None
DEBUG 2019-01-23T22:11:40Z: rustc::infer::error_reporting::nice_region_error::placeholder_error: try_report_placeholders_trait: self_ty_has_vid=false
nikomatsakis (Jan 23 2019 at 22:13, on Zulip):

given that all those flags are None we probably have to add some add'l test :)

nikomatsakis (Jan 23 2019 at 22:15, on Zulip):

I guess something like self_ty_has_vid but testing the expected type

lqd (Jan 23 2019 at 22:16, on Zulip):

yeah

lqd (Jan 23 2019 at 22:16, on Zulip):

feels like interesting progress can be made here so it's nice

lqd (Jan 24 2019 at 17:16, on Zulip):

(thanks for updating #57362 @nikomatsakis sorry I wasn't able to do it in time for the meeting)

nikomatsakis (Jan 24 2019 at 17:18, on Zulip):

no worries

lqd (Jan 25 2019 at 13:24, on Zulip):

@pnkfelix re: your GH comment, FWIW as of now I'm not even sure how to turn the currently displayed type fn(&u8) into fn(&'0 u8) :) I'll have the updated blessed tests diff soon, if you'd like to check my current progress (it'll be in the direction niko mentioned in the update, but not there yet)

pnkfelix (Jan 25 2019 at 13:25, on Zulip):

Hmm, I am pretty sure that there are other diagnostics that do this sort of substitution, let me see

lqd (Jan 25 2019 at 13:27, on Zulip):

oh yeah I assumed so, just didn't know which, and how general they'd be for this use case

lqd (Jan 25 2019 at 13:39, on Zulip):

btw @pnkfelix (boy am I glad you're back, Paris TZ FTW ;) I had to add the InferCtxt to NiceRegionError since the trait refs type variables are not resolved in the error messages we're talking about, my question: since NiceRegionError already uses the TyCtxt, which is usually from the InferCtxt, would you rather I remove the tcx and access it everywhere through the InferCtxt I added instead of just adding it ?

pnkfelix (Jan 25 2019 at 13:39, on Zulip):

yes

lqd (Jan 25 2019 at 13:40, on Zulip):

ok, more like a self.tcx() helper or switch uses to self.infcx.tcx ?

pnkfelix (Jan 25 2019 at 13:40, on Zulip):

that is, don't pass around two parameters when one will do. Especially when it wouldn't make sense to ever use a different value for the derivable parameter

pnkfelix (Jan 25 2019 at 13:41, on Zulip):

I don't personally care about which pattern (self.tcx() vs self.infcx.tcx vs impl Deref for Self -- yep, that comes up too as a way to automatically get all the methods)

pnkfelix (Jan 25 2019 at 13:41, on Zulip):

I guess self.tcx() will look nicest in the diff.

lqd (Jan 25 2019 at 13:42, on Zulip):

ok I will do the latter then, merci :)

lqd (Jan 25 2019 at 14:10, on Zulip):

@pnkfelix the WIP PR results look like this for some cases missed by our current tests, a mix of #57362 and #57642. (GH reversed the ordering of the files in the gist) and I'll have the blessed output in a short while, but it will showcase more of the concern in your GH comment

pnkfelix (Jan 25 2019 at 14:15, on Zulip):

Ah "actually" is a better word than "only" here

pnkfelix (Jan 25 2019 at 14:16, on Zulip):

(well... maybe its a better word. I think.)

lqd (Jan 25 2019 at 14:21, on Zulip):

I think it would be interesting to achieve a "good enough" message soon-ish (since it's a p-high issue) ? and then build on that to improve these higher-ranked lifetime errors later on (both for this 2015 case, and the related 2018 case)

lqd (Jan 25 2019 at 14:47, on Zulip):

@pnkfelix and here's the previously mentioned diff: the blessed diff is not a big improvement (or maybe not at all) compared to what we have now, what do you think ?

lqd (Jan 25 2019 at 18:47, on Zulip):

@nikomatsakis :wave: I feel I've progressed enough that we can start iterating on the message wording on GH, with CI checks, so I've opened #57901. Crucially, it doesn't yet handle the case you wanted to clarify: fn(&u8) into fn(&'0 u8) for the specific lifetime '0 (as I don't know how to do that yet).

nikomatsakis (Jan 25 2019 at 18:48, on Zulip):

ok

nikomatsakis (Jan 25 2019 at 18:48, on Zulip):

that's awesome!

nikomatsakis (Jan 25 2019 at 18:48, on Zulip):

let me take a look

lqd (Jan 25 2019 at 18:49, on Zulip):

yeah, take a look before saying that's awesome :D

nikomatsakis (Jan 25 2019 at 18:49, on Zulip):

so @lqd I see this in the tests:

 but `X` is actually implemented for the type `fn(&'0 ())`, for the specific lifetime `'0`

I thought you said that wasn't working?

nikomatsakis (Jan 25 2019 at 18:49, on Zulip):

is that an "aspirational" stderr file?

lqd (Jan 25 2019 at 18:49, on Zulip):

no it's the "good case" from Matthew's issue ;)

nikomatsakis (Jan 25 2019 at 18:49, on Zulip):

(if so, I think it's best to always make the stderr files match the current output, it helps when reviewing)

nikomatsakis (Jan 25 2019 at 18:49, on Zulip):

ah, ok

nikomatsakis (Jan 25 2019 at 18:49, on Zulip):

must be a different test

lqd (Jan 25 2019 at 18:50, on Zulip):

yeah :/

nikomatsakis (Jan 25 2019 at 18:50, on Zulip):

which is the test that looks bad :)

nikomatsakis (Jan 25 2019 at 18:50, on Zulip):

oh I see

nikomatsakis (Jan 25 2019 at 18:50, on Zulip):

the first error in that file?

lqd (Jan 25 2019 at 18:50, on Zulip):

yeah

lqd (Jan 25 2019 at 18:50, on Zulip):

I can surely make it clearer

lqd (Jan 25 2019 at 18:52, on Zulip):

or work out where it should probably better belong, maybe the ui/hrtb cases for instance

nikomatsakis (Jan 25 2019 at 18:53, on Zulip):

ok I mean

nikomatsakis (Jan 25 2019 at 18:53, on Zulip):

it's definitely far, far better than before

nikomatsakis (Jan 25 2019 at 18:53, on Zulip):

I think it is confusing to a "newbie" now

nikomatsakis (Jan 25 2019 at 18:53, on Zulip):

but e.g. dtolnay could probably decrypt it ;)

lqd (Jan 25 2019 at 18:53, on Zulip):

haha

nikomatsakis (Jan 25 2019 at 18:53, on Zulip):

that said, let me build your branch and poke at it

lqd (Jan 25 2019 at 18:54, on Zulip):

yay

lqd (Jan 25 2019 at 18:57, on Zulip):

(I have to go back home in a couple minutes but will be back in a short while -- to fix the potentially many mistakes I've made here :)

lqd (Jan 25 2019 at 19:46, on Zulip):

@centril thanks for the review! I'll let you and niko decide the wording and then implement it if that's ok. I plan to work on these kinds of errors more after this PR (which is more to fix the current confusingitude — yes that is a word maybe) and also on the similar ones in the NLL cases, so we can have them all improved over time

centril (Jan 25 2019 at 19:47, on Zulip):

@lqd thanks for the PR :slight_smile: -- a clear improvement!

lqd (Jan 25 2019 at 19:48, on Zulip):

in any case we'll make them better for sure

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

btw @lqd / @pnkfelix the word "actually" was a stroke of genius

lqd (Jan 25 2019 at 19:51, on Zulip):

it was your stroke of genius!

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

hahaha

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

ok well

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

what can I say, I love my wording

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

Breaking news:

Past-Niko is able to write sentences that Future-Niko understands

centril (Jan 25 2019 at 19:54, on Zulip):

Which Future-Niko in the multiverse tho? ;)

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

I think it was NUPRL research which basically said "6 universes should be enough for everyone" so who knows what the other 4 Nikos are even capable of

nikomatsakis (Jan 25 2019 at 20:08, on Zulip):

@lqd hope you don't mind, I'm poking at your branch

nikomatsakis (Jan 25 2019 at 20:08, on Zulip):

I think we can fix that one case fairly easily

nikomatsakis (Jan 25 2019 at 20:09, on Zulip):

easier though for me to just try it than to explain it :P

lqd (Jan 25 2019 at 20:11, on Zulip):

I don't mind at all — and yeah, understandable :) I'll look at your commits so I can learn a bit more about the errors/diagnostics API (and I hadn't noticed, of course tidy failed)

nikomatsakis (Jan 25 2019 at 20:22, on Zulip):

@lqd current output with my commit:

error: implementation of `Trait` is not general enough
  --> issue-57362-1.rs:20:7
   |
20 |     a.f(); //~ ERROR not general enough
   |       ^
   |
   = note: `Trait` would have to be implemented for the type `fn(&'0 u8)`, for some specific lifetime `'0`
   = note: but `Trait` is actually implemented for the type `for<'r> fn(&'r u8)`
lqd (Jan 25 2019 at 20:23, on Zulip):

yay

nikomatsakis (Jan 25 2019 at 20:23, on Zulip):

let me run with --bless

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

@lqd pushed

lqd (Jan 25 2019 at 20:34, on Zulip):

awesome, thanks a lot @nikomatsakis

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

I guess you can see what I did

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

btw I removed this actual.or(expected)

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

I'm not sure why that was there but I think it could lead to bad output / is wrong

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

well, it probably didn't really do anything bad

nikomatsakis (Jan 25 2019 at 20:35, on Zulip):

but just didn't make sense

lqd (Jan 25 2019 at 20:35, on Zulip):

oh

nikomatsakis (Jan 25 2019 at 20:35, on Zulip):

the idea of the highlight methods is that they take an Option<usize> to say "if this is Some, replace the given region with that"

nikomatsakis (Jan 25 2019 at 20:35, on Zulip):

the reason it probably didn't do anything

nikomatsakis (Jan 25 2019 at 20:35, on Zulip):

is because if actual_has_vid is None

lqd (Jan 25 2019 at 20:35, on Zulip):

I was expecting this to highlight the vid whether it was in the actual self ty or the expected self ty

nikomatsakis (Jan 25 2019 at 20:35, on Zulip):

it would do that

nikomatsakis (Jan 25 2019 at 20:36, on Zulip):

but

nikomatsakis (Jan 25 2019 at 20:36, on Zulip):

the stuff we print in the closure itself

nikomatsakis (Jan 25 2019 at 20:36, on Zulip):

is only from the actual self ty

nikomatsakis (Jan 25 2019 at 20:36, on Zulip):

we're not printing anything from the expected self ty

nikomatsakis (Jan 25 2019 at 20:36, on Zulip):

so, if actual is None, then vid just won't appear

nikomatsakis (Jan 25 2019 at 20:36, on Zulip):

so there is no point to highlighting it

nikomatsakis (Jan 25 2019 at 20:37, on Zulip):

(and, what's worse, if we did somehow find it, then we would print '0, but because the match was matching on just actual, we wouldn't have printed an explanation of what '0 represents)

nikomatsakis (Jan 25 2019 at 20:37, on Zulip):

does that make sense?

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

true, at some point I did have a similar structure the theself_ty_has_vidin the latter match but it didn't make sense

lqd (Jan 25 2019 at 20:38, on Zulip):

yeah it does, thanks a lot for the explanation

nikomatsakis (Jan 25 2019 at 20:38, on Zulip):

anyway the main thing I changed was -- in the case where has_sub and has_sup are both None -- to check expected_has_vid

nikomatsakis (Jan 25 2019 at 20:38, on Zulip):

the code pattern there is a bit obtuse I guess

nikomatsakis (Jan 25 2019 at 20:38, on Zulip):

I wonder if it would be cleaner to move the maybe_highlight into the match arms

nikomatsakis (Jan 25 2019 at 20:38, on Zulip):

I think this way was a bit less reptition

nikomatsakis (Jan 25 2019 at 20:39, on Zulip):

but basically it like sets up the highlights, and then the inner matches actually check whether the highlights did anything

lqd (Jan 25 2019 at 20:39, on Zulip):

I see

nikomatsakis (Jan 25 2019 at 20:39, on Zulip):

but if we moved the highlights into the match arms (so that we only invoke them if they might do something), then e.g. we would have to have duplicate for cases like this

                    (Some(n), _) | (_, Some(n)) => {
                        err.note(&format!(
                            "`{}` would have to be implemented for the type `{}`, \
                             for any lifetime `'{}`",
                            expected_trait_ref,
                            expected_trait_ref.self_ty(),
                            n,
                        ));
                    }
lqd (Jan 25 2019 at 20:39, on Zulip):

I wondered the same about the maybe_highlight

nikomatsakis (Jan 25 2019 at 20:39, on Zulip):

and also between that arm and the other arms

nikomatsakis (Jan 25 2019 at 20:39, on Zulip):

but it seems kind of confusing the way it is

nikomatsakis (Jan 25 2019 at 20:40, on Zulip):

it certainly took me a minute to remember what the heck I was doing

lqd (Jan 25 2019 at 20:40, on Zulip):

it's a bit tricky

nikomatsakis (Jan 25 2019 at 20:41, on Zulip):

feel free to play with it :)

lqd (Jan 25 2019 at 20:41, on Zulip):

there can be a lot of different cases

nikomatsakis (Jan 25 2019 at 20:41, on Zulip):

yeah

lqd (Jan 25 2019 at 20:41, on Zulip):

I wondered about something

nikomatsakis (Jan 25 2019 at 20:41, on Zulip):

maybe I should at least add a comment

lqd (Jan 25 2019 at 20:42, on Zulip):

the case that was added was when the subtype was in the sup region

lqd (Jan 25 2019 at 20:42, on Zulip):

but I wondered if it was possible to have another missing case here, I'm not sure it does happen

lqd (Jan 25 2019 at 20:42, on Zulip):

I'll explain a bit more

nikomatsakis (Jan 25 2019 at 20:43, on Zulip):

but I wondered if it was possible to have another missing case here, I'm not sure it does happen

seems probable :)

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

we can have sub/sup placeholders + sub/sup region "subtype" origin, we handled the subtype origin for the sub region (and thus, the 2 cases of sub and sup placeholders), and now we added the subtype origin for the sup region placeholder case

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

and I wondered if we could have a sup region placeholder but a subtype origin on the sub region — I'm not sure if that makes sense

nikomatsakis (Jan 25 2019 at 20:48, on Zulip):

maybe :)

lqd (Jan 25 2019 at 20:48, on Zulip):

:)

lqd (Jan 25 2019 at 20:49, on Zulip):

I'll try running tests and panicking if that happens

lqd (Jan 25 2019 at 20:50, on Zulip):

what do you expect remains to be done for the PR btw ? maybe I can tackle some of centril's comments

nikomatsakis (Jan 25 2019 at 20:51, on Zulip):

ok I was just skimming your changes

nikomatsakis (Jan 25 2019 at 20:51, on Zulip):

and I see what you are asking

nikomatsakis (Jan 25 2019 at 20:51, on Zulip):

these structs are a bit confusing :P

nikomatsakis (Jan 25 2019 at 20:51, on Zulip):

not sure if you looked at the code that is producing them

nikomatsakis (Jan 25 2019 at 20:51, on Zulip):

it boils down to us walking a graph of constraints

nikomatsakis (Jan 25 2019 at 20:52, on Zulip):

the idea is that you have something like

SUB <= vid <= SUP

where SUB <= SUP is not true

nikomatsakis (Jan 25 2019 at 20:52, on Zulip):

and the "origins" are the description of the immediate edges connecting to SUB and SUP

lqd (Jan 25 2019 at 20:52, on Zulip):

I've seen some of it when I was trying to locate the unexpected expected/actual inversion

nikomatsakis (Jan 25 2019 at 20:53, on Zulip):

I don't really think this is the right way to think about it per se.

nikomatsakis (Jan 25 2019 at 20:53, on Zulip):

If nothing else, the origin that is immediately attached to the SUB edge may not be the most interesting one

nikomatsakis (Jan 25 2019 at 20:53, on Zulip):

but anyway it's what the code does now and it seems to work relatively ok in practice

lqd (Jan 25 2019 at 20:54, on Zulip):

oh alright

nikomatsakis (Jan 25 2019 at 20:54, on Zulip):

I'm not really sure I guess how you would get a sup-placeholder where the trait-refs are not immediately attached to it

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

I think the answer probably is "that can probably happen"

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

it might be interesting to remove this case:

            Some(RegionResolutionError::SubSupConflict(
                vid,
                _,
                SubregionOrigin::Subtype(TypeTrace {
                    cause,
                    values: ValuePairs::TraitRefs(ExpectedFound { expected, found }),
                }),
                _,
                _,
                sup_placeholder @ ty::RePlaceholder(_),
            )) if expected.def_id == found.def_id =>

and see what changes

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

since that's kind of the inverse scenario

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

but I'd not want to block this PR on it

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

tbh the PR is looking prety good to me

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

I didn't read it all over though

nikomatsakis (Jan 25 2019 at 20:55, on Zulip):

and I didn't read @centril's comments

nikomatsakis (Jan 25 2019 at 20:56, on Zulip):

let me read it now

lqd (Jan 25 2019 at 20:56, on Zulip):

I will experiment with these 2 cases and then maybe try add a missing arm if there happens to be one

lqd (Jan 25 2019 at 20:56, on Zulip):

but then again, it will depend on our current tests

nikomatsakis (Jan 25 2019 at 21:07, on Zulip):

ok, I reviewed

lqd (Jan 25 2019 at 21:07, on Zulip):

I'll try the reordering you mention!

nikomatsakis (Jan 25 2019 at 21:07, on Zulip):

I also responded to @centril's comments, I don't know that I agree on some of the changes they suggested

nikomatsakis (Jan 25 2019 at 21:07, on Zulip):

though I do agree that there are problems

nikomatsakis (Jan 25 2019 at 21:07, on Zulip):

this is just a hard error

nikomatsakis (Jan 25 2019 at 21:08, on Zulip):

anyway, I'm happy with the progress, I think we should land the PR, but it would probably be good to keep thinking about how to improve

lqd (Jan 25 2019 at 21:08, on Zulip):

not sure how to check if the '0 only appears in the self types yet

nikomatsakis (Jan 25 2019 at 21:08, on Zulip):

if you want to try the reordering though I'm :+1:

nikomatsakis (Jan 25 2019 at 21:08, on Zulip):

@lqd this is what the self_ty_has_vid is doing

lqd (Jan 25 2019 at 21:08, on Zulip):

oh yeah duh

nikomatsakis (Jan 25 2019 at 21:08, on Zulip):

i.e., if has-vid is true but self-ty-has-vid is false

nikomatsakis (Jan 25 2019 at 21:08, on Zulip):

then it only appears in the substs

nikomatsakis (Jan 25 2019 at 21:08, on Zulip):

we probably need two of those though

lqd (Jan 25 2019 at 21:09, on Zulip):

I even added an "expected_self_ty_has_vid" at some point but didn't know how to use it

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

i.e., for actual + expected ty

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

so..many...cases...

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

would be nice to reduce the rightward drift

lqd (Jan 25 2019 at 21:09, on Zulip):

indeed

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

@centril's suggestion for a fn could help

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

i.e., we could do the maybe_replace calls

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

and then have the inner logic in a helper fn

lqd (Jan 25 2019 at 21:10, on Zulip):

I'll try this as well, it shouldn't be that long

lqd (Jan 25 2019 at 21:10, on Zulip):

I'm guessing next week should still be ok to land

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

(still unsure how pressing P-high is)

nikomatsakis (Jan 25 2019 at 21:21, on Zulip):

it looks like this is a 1.33 regression

nikomatsakis (Jan 25 2019 at 21:21, on Zulip):

so we have a bit of time before it hits stable

lqd (Jan 25 2019 at 21:23, on Zulip):

ok great, I'm hopeful I can get it done next week

lqd (Jan 25 2019 at 21:24, on Zulip):

(seems like the more time-consuming parts are already there)

centril (Jan 25 2019 at 21:25, on Zulip):

@nikomatsakis re. refactoring out to functions, I think especially in a big project and repo such as this its good to be proactive on splitting things out; otherwise it will inevitably lead to huge files and huge functions as people add and add more bits

centril (Jan 25 2019 at 21:26, on Zulip):

the last changes made the PR nicer :slight_smile:

I'd still change to impl<T> Trait for for<'a> fn(&'a T) { but that's about it

nikomatsakis (Jan 25 2019 at 21:28, on Zulip):

@centril seems fine. I like it for reducing the rightward drift, regardless. (Note that this file is specific to this particular error message, as well, so it's already fairly narrow.)

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

the last changes made the PR nicer :)

:)

lqd (Jan 25 2019 at 21:34, on Zulip):

thanks to niko ofc I had nothing to do with those :joy:

lqd (Jan 26 2019 at 20:58, on Zulip):

@nikomatsakis I've updated #57901 to only do the inversion whenever the sub/sup placeholders appear in the self type, so that the ordering is untouched in the common cases, and (AFAICT always) making it the closest to the placeholder regions. It does make the message read better to me as well. I've also extracted the notes-handling part to a helper fn like @centril mentioned. Overall, I'm satisfied with the results for now — especially compared to expected Trait, found Trait :) — and as we talked about, we'll continue improving these errors over time. Have a nice weekend you two :wave:

Matthew Jasper (Jan 27 2019 at 16:12, on Zulip):

@lqd Have you seen #57875? What error does it give with your PR?

lqd (Jan 27 2019 at 16:13, on Zulip):

let me check

lqd (Jan 27 2019 at 16:17, on Zulip):

it looks not to change anything on this example

lqd (Jan 27 2019 at 16:17, on Zulip):

I'll investigate this issue because it might be the last missing case we expected but didn't see happening

lqd (Jan 27 2019 at 16:18, on Zulip):

so that specific example doesn't go through the expected new placeholder errors, much like dtolnay's example didn't

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

@Matthew Jasper thanks for the link

Matthew Jasper (Jan 27 2019 at 16:19, on Zulip):

OK

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

seems related indeed

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

@Matthew Jasper as you expected #57875 is completely related, it is a SubSupConflict, and both regions are placeholders, with Subtype origins. However, try_report_placeholder_conflictin nice_region_errors::placeholder_errors (link to the similar case which is already handled) in general only deals with TraitRefs and these expected/found values are PolyTraitRefs here. Would you expect handling PolyTraitRefs in error messages in this module being vastly different from just TraitRefs ?

Matthew Jasper (Jan 27 2019 at 18:32, on Zulip):

Hmm. I think you'll have to ask @nikomatsakis about that. I would have thought that placeholder regions shouldn't appear in binders like that.

lqd (Jan 27 2019 at 18:36, on Zulip):

yeah :/ alright thanks Matthew, maybe it can easily be added to the PR even though it doesn't look like it at first glance; we'll see soon.

Matthew Jasper (Jan 27 2019 at 18:51, on Zulip):

Actually maybe I'm misunderstanding what's going on, if it's not too much trouble, could you debug print the RegionResolutionError with -Zverbose passed to the compiler?

lqd (Jan 27 2019 at 18:56, on Zulip):

sure

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

@Matthew Jasper a bit hard to read but here it is — the error output lacks the origins' TypeTrace.values

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

so they're printed in the later lines

Matthew Jasper (Jan 27 2019 at 19:12, on Zulip):

That was pretty much what I first thought, so I'm not really sure what's going on.

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

we'll see with niko but at least we've done a little initial investigation to see how it's different from the other similar issues

nikomatsakis (Jan 28 2019 at 21:00, on Zulip):

@lqd hmm I think that handling a poly-trait-ref and a trait-ref doesn't have to be .. particularly different here. But I guess I would try to handle it in a separate PR.

nikomatsakis (Jan 28 2019 at 21:01, on Zulip):

Regarding your existing changes, they look great!

nikomatsakis (Jan 28 2019 at 21:01, on Zulip):

The only nit I can see is that I might be inclined to change from "the specific lifetime '1" to "some specific lifetime"

nikomatsakis (Jan 28 2019 at 21:01, on Zulip):

somehow the word "the" makes the number thing that much more confusing

lqd (Jan 28 2019 at 21:01, on Zulip):

yeah that was the only thing I was wondering about, the "some" vs "the specific"

lqd (Jan 28 2019 at 21:02, on Zulip):

(apart from handling PolyTraitRefs ofc)

lqd (Jan 28 2019 at 21:03, on Zulip):

so we'd remove the number ?

lqd (Jan 28 2019 at 21:03, on Zulip):

Felix had similar feedback over the wording I think yeah

nikomatsakis (Jan 28 2019 at 21:04, on Zulip):

so we'd remove the number ?

I didn't mean to remove the number

nikomatsakis (Jan 28 2019 at 21:04, on Zulip):

I just meant that the phrase "the specific lifetime '1" doesn't seem great; I guess I prefer "some specific lifetime '1"

lqd (Jan 28 2019 at 21:04, on Zulip):

oh :)

nikomatsakis (Jan 28 2019 at 21:04, on Zulip):

but I'm not in love with the numbers

nikomatsakis (Jan 28 2019 at 21:04, on Zulip):

I just don't have a better idea really

nikomatsakis (Jan 28 2019 at 21:05, on Zulip):

and I think if we were going to change it, I'd want to change it for the NLL errors too

lqd (Jan 28 2019 at 21:06, on Zulip):

at least always having "some specific lifetime" would be consistent, and we can then work on the wording "holistically" I expect, for these and the NLL ones

lqd (Jan 28 2019 at 21:07, on Zulip):

I'll go update the message now

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

this should probably say "some specific lifetime '1" as well, instead of just "some lifetime '1"

lqd (Jan 28 2019 at 22:22, on Zulip):

(I've pushed the reworded blessed tests)

Last update: Nov 16 2019 at 02:45UTC