Stream: wg-traits

Topic: implied-bounds


scalexm (Oct 29 2018 at 20:40, on Zulip):

@nikomatsakis re

trait Foo<T: Send> { ... }
trait Bar<U, T>: Foo<T> { ... }
scalexm (Oct 29 2018 at 20:41, on Zulip):

to me this is no different than

trait Clone { }
trait Copy: Clone { }

trait Bar where Self: Copy { }
scalexm (Oct 29 2018 at 20:41, on Zulip):

if we make the first example ill-formed under the pretext that Self: Foo<T> is ill-formed, so should we do with the second example Self: Copy

scalexm (Oct 29 2018 at 20:42, on Zulip):

so to me, in traits we don't need to check any where clauses, but impls must always enforce the full set of transitive bounds

nikomatsakis (Oct 30 2018 at 15:21, on Zulip):

ok I see -- yes @scalexm you make a persuasive case :)

nikomatsakis (Oct 30 2018 at 15:21, on Zulip):

we currently do have some sort of WF checks here

nikomatsakis (Oct 30 2018 at 15:21, on Zulip):

however

nikomatsakis (Oct 30 2018 at 15:22, on Zulip):

but I think that -- under an implied bounds scheme -- they are largely silly

scalexm (Oct 30 2018 at 15:43, on Zulip):

@nikomatsakis yes

scalexm (Oct 30 2018 at 15:43, on Zulip):

If you check the WF chapter I juste wrote in rustc-guide

scalexm (Oct 30 2018 at 15:44, on Zulip):

For trait decls, the only things I check are that types appearing in the where clauses are WF

nikomatsakis (Oct 30 2018 at 17:10, on Zulip):

@scalexm ok I will consult. Of course, today this gets an error

trait Foo<T: Send> { }
trait Bar<T>: Foo<T> { }
scalexm (Oct 30 2018 at 17:11, on Zulip):

@nikomatsakis yes :) but since the WF chapter is in the "new-style" trait solving section, I wrote things as how I think they should be tomorrow

nikomatsakis (Oct 30 2018 at 17:12, on Zulip):

yep yep

nikomatsakis (Oct 30 2018 at 17:12, on Zulip):

I'm not saying that to "contradict" you, just noting

nikomatsakis (Oct 30 2018 at 17:12, on Zulip):

it feels maybe relevant to the trait alias PR that @Alexander Regueiro has been working on

nikomatsakis (Oct 30 2018 at 17:12, on Zulip):

I am basically thinking that we should make trait aliases "fit" the rest of the system as cleanly as we can

nikomatsakis (Oct 30 2018 at 17:12, on Zulip):

which might mean inheriting its limitations for the time being

nikomatsakis (Oct 30 2018 at 17:13, on Zulip):

but I am debating if that is silly

nikomatsakis (Nov 01 2018 at 17:24, on Zulip):

Picking this up again, @scalexm, I basically can't come up with any reason that we should impose limitations on trait aliases (cc @Alexander Regueiro) that they be "consistent". In other words, it seems ok to permit trait Foo<T: Send { }; trait Bar<T> = Foo<T>;, and not require trait Bar<T: Send> = Foo<T>.

The way this is implemented, you basically wind up with roughly the equivalent of

impl<Self: Foo<T>> Bar<T> for Self { .. }

I was trying to figure out if there could be problems like those from #43784 — if so, I don't see it yet.

nikomatsakis (Nov 01 2018 at 17:25, on Zulip):

it does feel a touch suspicious, but otoh we already require that the impl of Foo<T> must be able to prove that anything implied by the trait is true (and we elaborate those, thanks to your fix for #43784), so by proving Foo<T> we ought to be able to rely on the Foo impl to prove that T: Send, I think?

nikomatsakis (Nov 01 2018 at 17:25, on Zulip):

(And I guess the other key point is that the Foo impl can't rely on the Bar impl, because it has a Self: Foo<T> requirement — that was the real problem in #43784 from what I see.)

scalexm (Nov 01 2018 at 17:30, on Zulip):

@nikomatsakis under the implied bounds setting your translation seems harmless yes

scalexm (Nov 01 2018 at 17:31, on Zulip):

however in rustc, the T: Send is not even elaborated I think

nikomatsakis (Nov 01 2018 at 17:31, on Zulip):

well, we are presently doing that for trait aliases

nikomatsakis (Nov 01 2018 at 17:31, on Zulip):

but not other traits

nikomatsakis (Nov 01 2018 at 17:31, on Zulip):

I'm not sure if we should keep doing it

nikomatsakis (Nov 01 2018 at 17:32, on Zulip):

(just for "consistency")

scalexm (Nov 01 2018 at 17:32, on Zulip):

in that case that might be problematic because my fix for #43784 only proves elaborated bounds, i.e. supertrait and bounds on assoc types

nikomatsakis (Nov 01 2018 at 17:32, on Zulip):

but I guess the same all applies to trait Foo: Send { } trait Bar = Foo;

nikomatsakis (Nov 01 2018 at 17:32, on Zulip):

in that case that might be problematic because my fix for #43784 only prove elaborated bounds, i.e. supertrait and bounds on assoc types

well, that same function you use to elaborate

nikomatsakis (Nov 01 2018 at 17:32, on Zulip):

is extended to treat trait aliases differently I think?

nikomatsakis (Nov 01 2018 at 17:32, on Zulip):

hmm

scalexm (Nov 01 2018 at 17:32, on Zulip):

@nikomatsakis in that case that would be ok

nikomatsakis (Nov 01 2018 at 17:32, on Zulip):

but in this case, the T: Send comes from a normal trait..

nikomatsakis (Nov 01 2018 at 17:33, on Zulip):

I can certainly create the example, I dont' think it "went wrong" when I tried it, but maybe I did some step wrong

nikomatsakis (Nov 01 2018 at 17:34, on Zulip):

well, basically, we would only elaborate Bar<T> to Foo<T>

nikomatsakis (Nov 01 2018 at 17:34, on Zulip):

so actually know that X: Bar<T> does not imply that T: Send to us

nikomatsakis (Nov 01 2018 at 17:34, on Zulip):

(but also I think you cannot prove that X: Bar<T> unless T: Send)

scalexm (Nov 01 2018 at 17:34, on Zulip):

when you say that the trait alias is translated to impl<Self: Foo<T>> Bar<T> for Self { .. }, I guess there is also a trait Bar<T>: Foo<T> or something?

nikomatsakis (Nov 01 2018 at 17:35, on Zulip):

correct

scalexm (Nov 01 2018 at 17:35, on Zulip):

ok

nikomatsakis (Nov 01 2018 at 17:35, on Zulip):

the funny bit is that this trait

nikomatsakis (Nov 01 2018 at 17:35, on Zulip):

would be an error today

scalexm (Nov 01 2018 at 17:35, on Zulip):

yes

nikomatsakis (Nov 01 2018 at 17:35, on Zulip):

which is why I am tempted (for consistency) to enforce WF conditions

nikomatsakis (Nov 01 2018 at 17:35, on Zulip):

otoh, I can't see why it's important to do so

scalexm (Nov 01 2018 at 17:36, on Zulip):

yeah maybe we should enforce them until full implied bounds are implemented

nikomatsakis (Nov 01 2018 at 17:41, on Zulip):

so basically we would check that, assuming the (elaborated) trait alias bounds hold, all the traits bounds are well-formed, just as we do for traits

nikomatsakis (Nov 01 2018 at 17:42, on Zulip):

hence trait Foo = Bar is fine (because Self: Bar => Self: Bar), but trait Foo<T> = Bar<T>; trait Bar<T: Send> { } is not, because elaborated(Self: Bar<T>) = [Self: Bar<T>]

scalexm (Nov 01 2018 at 17:43, on Zulip):

yes, also I see in the RFC that bounds on type variables are basically left as unresolved

scalexm (Nov 01 2018 at 17:44, on Zulip):

so yeah I'd go for just use the normal elaboration scheme for now

Alexander Regueiro (Nov 01 2018 at 17:44, on Zulip):

@nikomatsakis I'm not sure I fully understand the above, but I think it kind of depends on whether we want to consider this behaviour as part of the wider implied bounds semantics.

nikomatsakis (Nov 01 2018 at 17:47, on Zulip):

Right I think @Alexander Regueiro what @scalexm and I are thinking is that — at least for this PR — it'd be better to make things "consistent" with everything else

nikomatsakis (Nov 01 2018 at 17:47, on Zulip):

(which basically means adding a little code in wfcheck)

Alexander Regueiro (Nov 01 2018 at 17:47, on Zulip):

so enforce the bounds?

Alexander Regueiro (Nov 01 2018 at 17:47, on Zulip):

yeah

Alexander Regueiro (Nov 01 2018 at 17:47, on Zulip):

makes sense to me

nikomatsakis (Nov 01 2018 at 17:47, on Zulip):

right

Alexander Regueiro (Nov 01 2018 at 17:48, on Zulip):

how do we go about that? is it during selection?

Alexander Regueiro (Nov 01 2018 at 17:50, on Zulip):

@nikomatsakis

nikomatsakis (Nov 01 2018 at 17:55, on Zulip):

no, it is in wfcheck.rs

nikomatsakis (Nov 01 2018 at 17:56, on Zulip):

more specifically, @Alexander Regueiro, probably just need to do the same thing for traits and trait aliases here

Alexander Regueiro (Nov 01 2018 at 18:01, on Zulip):

oh right

Alexander Regueiro (Nov 01 2018 at 18:02, on Zulip):

never touched this part of the codebase before

Alexander Regueiro (Nov 01 2018 at 18:02, on Zulip):

let me have a look though

Alexander Regueiro (Nov 01 2018 at 18:03, on Zulip):

let's see if that does the trick...

Alexander Regueiro (Nov 01 2018 at 18:09, on Zulip):

@nikomatsakis incidentally, how does gate-test-X work?

nikomatsakis (Nov 01 2018 at 18:10, on Zulip):

hmm :) I sort of forget

Alexander Regueiro (Nov 01 2018 at 18:13, on Zulip):

@nikomatsakis yep that works. will push in a second, then you can have a look and r+ hopefully!

nikomatsakis (Nov 01 2018 at 18:16, on Zulip):

@Alexander Regueiro what do you think about the idea of having a comprehensive list of tests and the files they are in?

Probably living on the tracking issue? Maybe we should make an associated issue for "implementation", since that issue has various comments on it.

nikomatsakis (Nov 01 2018 at 18:16, on Zulip):

I was also thinking it would be nice to add some comments to the rustc-guide

nikomatsakis (Nov 01 2018 at 18:16, on Zulip):

I'd like to have an issue with a checklist describing the open issues we've encountered here

nikomatsakis (Nov 01 2018 at 18:16, on Zulip):

I can do some of that legwork if you want

nikomatsakis (Nov 01 2018 at 18:16, on Zulip):

we've basically got a lot of "state" in our heads right now about what works, what doesn't, and why, and I'd like to get that captured before I forget :)

Alexander Regueiro (Nov 01 2018 at 18:20, on Zulip):

@nikomatsakis sounds fair enough. I've just pushed to the branch now. everything should be passing now. :-) and yes, I'd appreciate if you'd start with some of the legwork... but I'm here to help. especially if you're unsure of anything I've written.

Alexander Regueiro (Nov 01 2018 at 18:20, on Zulip):

that WF checking was refreshingly trivial

Alexander Regueiro (Nov 01 2018 at 18:21, on Zulip):

@nikomatsakis I'm going to have a look at https://github.com/rust-lang/rust/issues/24010 now too... since it impacts on trait aliases as well.

nikomatsakis (Nov 01 2018 at 18:24, on Zulip):

@Alexander Regueiro yes! that would be great. I don't think there's any "deep reason" that this doesn't work

nikomatsakis (Nov 01 2018 at 18:24, on Zulip):

though I've not looked at it in a while

Alexander Regueiro (Nov 01 2018 at 18:25, on Zulip):

@nikomatsakis also, if you want me to split up the tests in the trait-alias-bounds file into multiple files, I could do that. (the trait-alias-object-types file is already pretty small). just let me know how you'd like to do the splitting, since I'm not totally sure

Alexander Regueiro (Nov 01 2018 at 18:26, on Zulip):

yeah, I agree, the reason doesn't seem to involve the underlying mechanics of the trait system

nikomatsakis (Nov 01 2018 at 18:27, on Zulip):

@nikomatsakis also, if you want me to split up the tests in the trait-alias-bounds file into multiple files, I could do that. (the trait-alias-object-types file is already pretty small). just let me know how you'd like to do the splitting, since I'm not totally sure

ok, I will have to take a look. Usually the lint for me is if there are lots of supporting types that are only used in one small subpart

Alexander Regueiro (Nov 01 2018 at 18:29, on Zulip):

@nikomatsakis that sounds reasonable, but I was hoping to make a more "conceptual" separation.

Alexander Regueiro (Nov 01 2018 at 18:29, on Zulip):

anyway, prioritise things in the order you want

Alexander Regueiro (Nov 01 2018 at 18:30, on Zulip):

if you want to start documenting things in the tracking issue and whatnot first, go for that

Alexander Regueiro (Nov 01 2018 at 18:30, on Zulip):

or indeed review my PR :-)

Alexander Regueiro (Nov 01 2018 at 18:30, on Zulip):

CI hasn't finish quite yet, mind you

Alexander Regueiro (Nov 01 2018 at 19:41, on Zulip):

@nikomatsakis strange test failure. seen it yet?

nikomatsakis (Nov 01 2018 at 20:56, on Zulip):

I see it now, let me take a look

Alexander Regueiro (Nov 01 2018 at 21:02, on Zulip):

sure

nikomatsakis (Nov 01 2018 at 21:07, on Zulip):

I have a hunch, @Alexander Regueiro, that the problem lies in this code, which applies I think also to TraitAlias

nikomatsakis (Nov 01 2018 at 21:07, on Zulip):

I haven't tested that yet though :)

nikomatsakis (Nov 01 2018 at 21:08, on Zulip):

perhaps the same is true here?

nikomatsakis (Nov 01 2018 at 21:08, on Zulip):

ugh, there are more places

nikomatsakis (Nov 01 2018 at 21:08, on Zulip):

here

nikomatsakis (Nov 01 2018 at 21:09, on Zulip):

(the second place is something else, I have to think about a test case that exposes the problem)

nikomatsakis (Nov 01 2018 at 21:09, on Zulip):

in any case I think we should find some way to write this code differently (that is, the 1st and 3rd links)

nikomatsakis (Nov 01 2018 at 21:09, on Zulip):

instead of testing if this is a trait and adjusting by 1, it would be nice if we could "query" the generics_of the def-id or something

nikomatsakis (Nov 01 2018 at 21:10, on Zulip):

I think we could replace that code with tcx.generics_of(def_id).has_self and it would just work

nikomatsakis (Nov 01 2018 at 21:10, on Zulip):

have to get the right def-id though :)

nikomatsakis (Nov 01 2018 at 21:11, on Zulip):

e.g., in the first case, it would be tcx.hir.local_def_id(item.id)

nikomatsakis (Nov 01 2018 at 21:11, on Zulip):

althouhg hmm that might be wrong, because has_self is also true for methods...

nikomatsakis (Nov 01 2018 at 21:11, on Zulip):

ah, but those are a different match arm

Alexander Regueiro (Nov 01 2018 at 21:12, on Zulip):

@nikomatsakis what is this code even doing?

nikomatsakis (Nov 01 2018 at 21:14, on Zulip):

/me sighs

nikomatsakis (Nov 01 2018 at 21:14, on Zulip):

actually, I think a trait alias is impossible in this case, since there are no methods "within" trait aliases (unlike traits + impls)

nikomatsakis (Nov 01 2018 at 21:14, on Zulip):

in any case, the code is computing the index of the lifetime parameters defined on the trait alias and elsewhere

nikomatsakis (Nov 01 2018 at 21:14, on Zulip):

each generic parameter (whether a lifetime or a type) gets an index

nikomatsakis (Nov 01 2018 at 21:15, on Zulip):

in the case of trait Foo, there is also an index assigned to the (implicit) Self type parameter

Alexander Regueiro (Nov 01 2018 at 21:15, on Zulip):

Ah, and it's trying to handle the index of the lifetime parameter for the self object?

nikomatsakis (Nov 01 2018 at 21:15, on Zulip):

yes, that's the problem, it is adjusting to account for that implicit Self

Alexander Regueiro (Nov 01 2018 at 21:15, on Zulip):

right

Alexander Regueiro (Nov 01 2018 at 21:15, on Zulip):

I sort of get that

nikomatsakis (Nov 01 2018 at 21:15, on Zulip):

ah way

nikomatsakis (Nov 01 2018 at 21:15, on Zulip):

er, wait

nikomatsakis (Nov 01 2018 at 21:15, on Zulip):

I think you can't call tcx.generics_of(...) here

nikomatsakis (Nov 01 2018 at 21:15, on Zulip):

because this code needs to run before generics_of

nikomatsakis (Nov 01 2018 at 21:16, on Zulip):

anyway I guess the easiest fix for now (ugh) is to add TraitAlias into the match statements (or convert if let to match)

nikomatsakis (Nov 01 2018 at 21:16, on Zulip):

it'd be nice to do something cleaner though

Alexander Regueiro (Nov 01 2018 at 21:16, on Zulip):

I can't add it to the code in the first link either?

nikomatsakis (Nov 01 2018 at 21:16, on Zulip):

but honestly resolve_lifetime is kind of a mess that we've been meaning to refactor for some time

nikomatsakis (Nov 01 2018 at 21:16, on Zulip):

I can't add it to the code in the first link either?

why not?

Alexander Regueiro (Nov 01 2018 at 21:16, on Zulip):

I'm just checking. I thought I could.

nikomatsakis (Nov 01 2018 at 21:17, on Zulip):

yeah, I think you should

Alexander Regueiro (Nov 01 2018 at 21:17, on Zulip):

I don't want to get bogged down in refactoring the whole thing heh

nikomatsakis (Nov 01 2018 at 21:17, on Zulip):

and I think that's the bug,

nikomatsakis (Nov 01 2018 at 21:17, on Zulip):

that's why it's getting those "index out of bound" errors

nikomatsakis (Nov 01 2018 at 21:17, on Zulip):

because the index is off by 1 :)

nikomatsakis (Nov 01 2018 at 21:17, on Zulip):

\Region parameter out of range when substituting in region 'a

Alexander Regueiro (Nov 01 2018 at 21:17, on Zulip):

@nikomatsakis should I be consistent and use a match in both cases though?

Alexander Regueiro (Nov 01 2018 at 21:17, on Zulip):

(perhaps factoring it out into a function)

nikomatsakis (Nov 01 2018 at 21:19, on Zulip):

yeah, a helper fn would be good

Alexander Regueiro (Nov 01 2018 at 21:22, on Zulip):

@nikomatsakis wait, why doesn't the self lifetime come first for impl items?

Alexander Regueiro (Nov 01 2018 at 21:28, on Zulip):

/me compiles

Alexander Regueiro (Nov 01 2018 at 21:49, on Zulip):

@nikomatsakis well, it seems to work... not sure why the case of ItemKind::Impl isn't needed though

Alexander Regueiro (Nov 01 2018 at 21:53, on Zulip):

anyway, see my push. ready to review I think. :-)

Alexander Regueiro (Nov 02 2018 at 03:05, on Zulip):

As for solving the other issue, I think the key to it is the create_substs_for_ast_trait_ref fn

Alexander Regueiro (Nov 02 2018 at 03:16, on Zulip):

@nikomatsakis or it might be better to make the change in instantiate_poly_trait_ref_inner... not sure. what's the diff between a mono and poly trait ref?

nikomatsakis (Nov 02 2018 at 14:22, on Zulip):

@Alexander Regueiro I'll have to look, some of those routines are general purpose. We want to find the right place that is specific to trait objects.

nikomatsakis (Nov 02 2018 at 14:22, on Zulip):

a "mono trait ref" refers to something like T: Foo<U> -- i.e., a particular trait with some specific type and lifetime parameters

nikomatsakis (Nov 02 2018 at 14:22, on Zulip):

a "poly trait ref" refers to a "higher-ranked" trait ref, e.g., for<'a> T: Foo<'a>

nikomatsakis (Nov 02 2018 at 14:22, on Zulip):

the Poly refers to the for binder

nikomatsakis (Nov 02 2018 at 14:23, on Zulip):

(so this defines many trait references)

Alexander Regueiro (Nov 02 2018 at 16:06, on Zulip):

Aha

Alexander Regueiro (Nov 02 2018 at 16:06, on Zulip):

Fair

Alexander Regueiro (Nov 02 2018 at 16:06, on Zulip):

But everything gets converted to a PolyTraitRef in the end right?

Alexander Regueiro (Nov 02 2018 at 16:08, on Zulip):

@nikomatsakis and yes, I was trying to make the change to the most specifuc site...not 100% sure where.

Alexander Regueiro (Nov 02 2018 at 17:01, on Zulip):

@nikomatsakis any ideas?

nikomatsakis (Nov 02 2018 at 18:23, on Zulip):

I sent one thought earlier but I will re-investigate

nikomatsakis (Nov 02 2018 at 20:32, on Zulip):

@scalexm

btw I asked this on that PR but maybe we can discuss it here =)

Remind me btw why we pushed the "expansion" into the lowering step, versus defining a proper WellFormed(T) goal? (And then having struct Box<T> get to assume that WellFormed(T) in its "environment"?)

scalexm (Nov 02 2018 at 20:34, on Zulip):

@nikomatsakis we had infinite branches problem at some time, not sure if it is still the case

scalexm (Nov 02 2018 at 20:36, on Zulip):

well I think we won't have them anymore now with the FromEnv / WF setup

scalexm (Nov 02 2018 at 20:41, on Zulip):

I think there were other problems maybe with associated types because they interacted with traits, I don't remember, but I think another point is that it enabled an optimization to be sound, which is that if you have type Type<...> where WC you only have WF(Type<...>) :- WC not WF(Type<...>) :- WellFormed(WC)

scalexm (Nov 02 2018 at 20:42, on Zulip):

well I'll find some time to think about it again, maybe now we can have the expansion in the logical rules again :) but if it works out, it'll be an easy change so let's keep the guide like that for now (this is how it is implemented in chalk and rustc_traits currently)

nikomatsakis (Nov 02 2018 at 20:43, on Zulip):

Yep.

Last update: Nov 12 2019 at 16:50UTC