Stream: wg-traits

Topic: #58305 "small fixes to chalk" PR


nikomatsakis (Feb 22 2019 at 20:50, on Zulip):

@scalexm you have (WIP) in the title of https://github.com/rust-lang/rust/pull/58305, is there a reason not to r+?

scalexm (Feb 22 2019 at 20:52, on Zulip):

@nikomatsakis there is at least one, in the bug I fixed in the unification code (where I needed to swap a_scopes and b_scopes), I think I also need to flip the variance, what do you think?

nikomatsakis (Feb 22 2019 at 20:52, on Zulip):

that part certainly looked unfortunate

nikomatsakis (Feb 22 2019 at 20:52, on Zulip):

mm yes I guess you do

nikomatsakis (Feb 22 2019 at 20:52, on Zulip):

it feels maybe like we should encapsulate that to a method?

nikomatsakis (Feb 22 2019 at 20:52, on Zulip):

I'd rather we don't have to flip at all, of course

scalexm (Feb 22 2019 at 20:52, on Zulip):

Do you have another solution in mind? The whole thing seems a bit fragile

nikomatsakis (Feb 22 2019 at 20:53, on Zulip):

I didn't look very closely but I guess the idea is that the logic would basically be identical either way?

nikomatsakis (Feb 22 2019 at 20:53, on Zulip):

i.e., you want some code to handle the "type/var" case or whatever

scalexm (Feb 22 2019 at 20:53, on Zulip):

Right

scalexm (Feb 22 2019 at 20:53, on Zulip):

Regardless of the sides of the two types

nikomatsakis (Feb 22 2019 at 20:55, on Zulip):

Yeah

nikomatsakis (Feb 22 2019 at 20:55, on Zulip):

I'll have to pull up the code and look a bit more

nikomatsakis (Feb 22 2019 at 20:55, on Zulip):

It doesn't feel great

scalexm (Feb 22 2019 at 20:55, on Zulip):

Other than that, I was hoping to be able to do some more work on that PR, namely trying to implement the rules for trait objects directly in rustc, and also trying to investigate the inference error I described in the PR description

nikomatsakis (Feb 22 2019 at 20:56, on Zulip):

OK

scalexm (Feb 22 2019 at 20:57, on Zulip):

But I guess we could merge it once we have figured out how to cleanly fix the unification bug, there are some useful additions already in that PR

nikomatsakis (Feb 22 2019 at 20:57, on Zulip):

I am +1 on experimenting around the rules for trait objects, though I remain .. not fully satisfied with the solutions on the table thus far.

nikomatsakis (Feb 22 2019 at 20:57, on Zulip):

Yeah, it seems like it would be better to merge and work in a separate PR

nikomatsakis (Feb 22 2019 at 20:58, on Zulip):

Not sure if you saw, but in the "structure of the working group" topic I've been trying to draw up a kind of "master list" of actionable things around traits. One of the things I wanted to do was to sketch out the next steps around chalk integration too. I know we've been over this many times. :) Sorry

scalexm (Feb 22 2019 at 20:59, on Zulip):

@nikomatsakis well, personally I think I am about satisfied with the solution I described for trait objects :) there are still not handled like « existential » types but I don’t really see how we could do that, but yes we can definitely leave that question open and experiment around it until we have some more experience

scalexm (Feb 22 2019 at 21:00, on Zulip):

I saw the topic but did not read it yet

nikomatsakis (Feb 22 2019 at 21:00, on Zulip):

it may be that I just need to really devote some time to understanding it better

nikomatsakis (Feb 22 2019 at 21:00, on Zulip):

I mean I'm not dissatisfied :)

nikomatsakis (Feb 22 2019 at 21:00, on Zulip):

I could easily see that the rules you are proposing are indeed the ones we wind up with

scalexm (Mar 09 2019 at 20:54, on Zulip):

@nikomatsakis we should discuss a solution about how best to make relate_ty_var insensible to the direction of the relation (e.g. not having to swap scopes / variance just to be able to correctly reuse that function)

scalexm (Mar 09 2019 at 20:54, on Zulip):

that's probably the only blocker to #58305

scalexm (Mar 09 2019 at 20:55, on Zulip):

after that, I'm still unsure how much progress we want to do on the "chalk in rustc" side and what direction we should take

scalexm (Mar 09 2019 at 20:55, on Zulip):

e.g. maybe we should be focusing on improving the chalk crate so that we can share more code, like to lowering stuff etc

scalexm (Mar 09 2019 at 20:56, on Zulip):

or else focus on the other problems we encountered like handling lifetime requirements which come from unification, or the "ambiguity" bug for open goals ?T: Trait (goal selection order etc)

scalexm (Mar 09 2019 at 20:59, on Zulip):

plus adding things like more builtin impls etc so that the new trait solver in rustc can compile more programs

nikomatsakis (Mar 20 2019 at 15:44, on Zulip):

we should discuss a solution about how best to make relate_ty_var insensible to the direction of the relation (e.g. not having to swap scopes / variance just to be able to correctly reuse that function)

@scalexm to be clear, we're talking about this code, right?

                    // We swap the order of `a` and `b` in the call to
                    // `relate_ty_var` below, so swap the corresponding scopes
                    // as well.
                    std::mem::swap(&mut self.a_scopes, &mut self.b_scopes);
                    let res = self.relate_ty_var(vid, a);
                    std::mem::swap(&mut self.a_scopes, &mut self.b_scopes);
res
nikomatsakis (Mar 20 2019 at 15:44, on Zulip):

I note that the PR doesn't swap variance

nikomatsakis (Mar 20 2019 at 15:45, on Zulip):

I don't quite remember the history there

nikomatsakis (Mar 20 2019 at 15:45, on Zulip):

I guess I should check the PR comments

scalexm (Mar 20 2019 at 15:45, on Zulip):

@nikomatsakis yes, that’s why it shouldn’t be merged as is :)

nikomatsakis (Mar 20 2019 at 15:46, on Zulip):

there is at least one, in the bug I fixed in the unification code (where I needed to swap a_scopes and b_scopes), I think I also need to flip the variance, what do you think?

ah, right, you were talking adding code to flip the variance =) heh, I remember now, sorry

nikomatsakis (Mar 20 2019 at 15:46, on Zulip):

Looking more at the relate_ty_var code, I think there are other problems

nikomatsakis (Mar 20 2019 at 15:46, on Zulip):

When it comes to variance

nikomatsakis (Mar 20 2019 at 15:47, on Zulip):

Notably, this line assumes "invariant", I believe.

scalexm (Mar 20 2019 at 15:47, on Zulip):

Yes, notably I think I’m doing an equate or something regardless of variance

scalexm (Mar 20 2019 at 15:47, on Zulip):

Right

nikomatsakis (Mar 20 2019 at 15:47, on Zulip):

So, one thing we could certainly do

nikomatsakis (Mar 20 2019 at 15:47, on Zulip):

is to refactor the code to be "generic" with respect to whether vid comes from A or what

nikomatsakis (Mar 20 2019 at 15:48, on Zulip):

that doesn't seem that hard to do

scalexm (Mar 20 2019 at 15:49, on Zulip):

In the NLL code, do you happen to use something else than « invariant »? Because I think the equate was there already before my unification PR where I refactored this code

nikomatsakis (Mar 20 2019 at 15:49, on Zulip):

I believe this code only used invariance before

nikomatsakis (Mar 20 2019 at 15:49, on Zulip):

/me thinks

nikomatsakis (Mar 20 2019 at 15:49, on Zulip):

Certainly when I first wrote it, that was true

nikomatsakis (Mar 20 2019 at 15:49, on Zulip):

but I may have changed it

nikomatsakis (Mar 20 2019 at 15:49, on Zulip):

and overlooked this detail :)

nikomatsakis (Mar 20 2019 at 15:49, on Zulip):

I don't recall who added the variance code to this -- I did?

scalexm (Mar 20 2019 at 15:50, on Zulip):

Variance was already there indeed when I refactored the code

nikomatsakis (Mar 20 2019 at 15:50, on Zulip):

OK. Then it's probably a bug in NLL

nikomatsakis (Mar 20 2019 at 15:50, on Zulip):

But I have to check

nikomatsakis (Mar 20 2019 at 15:52, on Zulip):

Well, one bug at a time

nikomatsakis (Mar 20 2019 at 15:52, on Zulip):

Let me experiment with a refactoring here, I can push it to your branch?

scalexm (Mar 20 2019 at 15:52, on Zulip):

Sure

nikomatsakis (Mar 20 2019 at 15:52, on Zulip):

Also, I think that whole match is wrong in the same way

nikomatsakis (Mar 20 2019 at 15:52, on Zulip):

in other words, the lazy norm case also assumes invariant

nikomatsakis (Mar 20 2019 at 15:53, on Zulip):

but we should file a bug about this

scalexm (Mar 20 2019 at 16:01, on Zulip):

I just checked, the equate bug was not pre-existing in NLL actually; and it is still not a bug because NLL will never relate two type variables (because of forbid_inference_vars())

nikomatsakis (Mar 20 2019 at 16:07, on Zulip):

@scalexm I was wondering about that (the forbid-inference-vars part of it)

nikomatsakis (Mar 20 2019 at 16:07, on Zulip):

but I couldn't quite remember when we had lifted that restriction

nikomatsakis (Mar 20 2019 at 16:08, on Zulip):

in general this code feels messier than I would like

nikomatsakis (Mar 20 2019 at 16:08, on Zulip):

but let's talk about that later

nikomatsakis (Mar 20 2019 at 16:08, on Zulip):

I've got a refactored version close to working now

scalexm (Mar 20 2019 at 16:14, on Zulip):

I lifted that restriction when I re-used that unification code for chalk, because unification in chalk must deal with inference variables in both sides of the relation

scalexm (Mar 20 2019 at 16:21, on Zulip):

But yes I agree it’s a bit messy

nikomatsakis (Mar 20 2019 at 16:48, on Zulip):

@scalexm I pushed two commits to your branch

nikomatsakis (Mar 20 2019 at 16:48, on Zulip):

took me longer than expected because I wound up doing two iterations, but I like this final version, feels pretty clean

nikomatsakis (Mar 20 2019 at 16:48, on Zulip):

could maybe use a few more comments

nikomatsakis (Mar 20 2019 at 16:51, on Zulip):

ok, forced pushed a few more comments

nikomatsakis (Mar 20 2019 at 16:52, on Zulip):

if you are happy with that, then I think I am happy with the PR overall

scalexm (Mar 20 2019 at 16:55, on Zulip):

For the equate thing, could we maybe add a method on VidValuePair, something like relate_vars which just does the right thing depending on the variance?

scalexm (Mar 20 2019 at 16:58, on Zulip):

@nikomatsakis I agree relate_projection_ty does not need to deal with scopes anyway

nikomatsakis (Mar 20 2019 at 17:20, on Zulip):

For the equate thing, could we maybe add a method on VidValuePair, something like relate_vars which just does the right thing depending on the variance?

what we have to do, I think, is to push a new constraint -- basically ?A <: ?B is something we can't make progress on until we know something about ?A or ?B

nikomatsakis (Mar 20 2019 at 17:20, on Zulip):

this is an instance of that more general problem that also afflicts ?T: Sized

nikomatsakis (Mar 20 2019 at 17:21, on Zulip):

I am thinking that i would like to dig more -- perhaps next week over zoom? -- into how the chalk-engine works

nikomatsakis (Mar 20 2019 at 17:21, on Zulip):

so we can discuss how to incorporate that concept

nikomatsakis (Mar 20 2019 at 17:21, on Zulip):

e.g., SLG solving

nikomatsakis (Mar 20 2019 at 17:21, on Zulip):

this would require me to remember some stuff :)

nikomatsakis (Mar 20 2019 at 17:21, on Zulip):

which is .. probably god

scalexm (Mar 20 2019 at 17:30, on Zulip):

@nikomatsakis Ah yes I see

scalexm (Mar 20 2019 at 17:30, on Zulip):

I’m happy with your refactoring anyway

scalexm (Mar 20 2019 at 17:30, on Zulip):

So we can merge it

nikomatsakis (Mar 20 2019 at 17:53, on Zulip):

maybe it needs a rebase?

scalexm (Mar 20 2019 at 19:09, on Zulip):

@nikomatsakis rebased

nikomatsakis (Mar 20 2019 at 19:11, on Zulip):

left a nit @scalexm but otherwise r=me

scalexm (Mar 20 2019 at 19:17, on Zulip):

@nikomatsakis ok I removed these lines

Last update: Nov 12 2019 at 16:40UTC