Stream: wg-traits

Topic: adding context to `TypeFamily` chalk#328


nikomatsakis (Feb 14 2020 at 19:25, on Zulip):

Right now, we use the name TypeFamily

nikomatsakis (Feb 14 2020 at 19:25, on Zulip):

We've been talking about renaming that to TypeInterner or maybe TypeContext

nikomatsakis (Feb 14 2020 at 19:25, on Zulip):

The task here is going to involve threading around local variables, so we go from

fn foo<TF: TypeFamily>()

to

fn foo<TF: TypeFamily>(tf: &TF) { ... }
nikomatsakis (Feb 14 2020 at 19:26, on Zulip):

it seems good to decide what the name of this local variable should be

nikomatsakis (Feb 14 2020 at 19:26, on Zulip):

tf .. doesn't strike me as ideal

nikomatsakis (Feb 14 2020 at 19:26, on Zulip):

interner: &TypeInterner is maybe good, if a bit long

nikomatsakis (Feb 14 2020 at 19:26, on Zulip):

tcx: &TypeContext is very rustc-like, but tcx has been confusing there to new folks for some time

Jack Huey (Feb 14 2020 at 19:26, on Zulip):

either that or passing the context as an argument into the functions

nikomatsakis (Feb 14 2020 at 19:27, on Zulip):

I don't get it :)

nikomatsakis (Feb 14 2020 at 19:27, on Zulip):

that is, that sounds like what I wrote

Jack Huey (Feb 14 2020 at 19:27, on Zulip):

oh, one sec

nikomatsakis (Feb 14 2020 at 19:28, on Zulip):

BTW, filed chalk#328

Jack Huey (Feb 14 2020 at 19:28, on Zulip):

I mean, so instead of intern_ty(ty: TyData<Self>) it could be instead intern_ty(ctx: Context, ty: TyData<Self>)

Jack Huey (Feb 14 2020 at 19:28, on Zulip):

and so on

nikomatsakis (Feb 14 2020 at 19:29, on Zulip):

I expected

nikomatsakis (Feb 14 2020 at 19:29, on Zulip):

fn intern_ty(&self, ty: TyData<Self>) -> Ty<Self>

nikomatsakis (Feb 14 2020 at 19:29, on Zulip):

but you could do

nikomatsakis (Feb 14 2020 at 19:29, on Zulip):

fn intern_ty(cx: Self::Context, ty: TyData<Self>) -> Ty<Self>

nikomatsakis (Feb 14 2020 at 19:29, on Zulip):

which is perhaps what you mean

nikomatsakis (Feb 14 2020 at 19:29, on Zulip):

BTW, filed chalk#328

I intentionally left this quite minimal while we hammer out the destination

nikomatsakis (Feb 14 2020 at 19:30, on Zulip):

I don't have a strong opinion about &self vs Self::Context except that &self feels slightly simpler

nikomatsakis (Feb 14 2020 at 19:30, on Zulip):

in particular

nikomatsakis (Feb 14 2020 at 19:30, on Zulip):

if you have fn foo<TF>(interner: &TF, ...), and you call it like foo(interner, ...)

nikomatsakis (Feb 14 2020 at 19:30, on Zulip):

then the value of TF can be inferred from the argument

nikomatsakis (Feb 14 2020 at 19:31, on Zulip):

in contrast, today (and with the associated type design), you might sometimes need to do foo::<TF>(...) to explicitly specify TF

nikomatsakis (Feb 14 2020 at 19:31, on Zulip):

since if you have fn foo<TF>(interner: &TF::Interner, ..) then the type of the argument doesn't really tell you what TF is, only what its interner is

Jack Huey (Feb 14 2020 at 19:32, on Zulip):

I don't know which way would be better

nikomatsakis (Feb 14 2020 at 19:35, on Zulip):

I don't know for sure either, I'd be inclined to start with &self and see if we hit some problems

nikomatsakis (Feb 14 2020 at 19:36, on Zulip):

(for the reasons I gave)

Jack Huey (Feb 14 2020 at 19:37, on Zulip):

Either way, I actually think this is probably a blocker for integrating chalk-solve/chalk-ir into rustc

nikomatsakis (Feb 14 2020 at 19:37, on Zulip):

Yes, I expect so

nikomatsakis (Feb 14 2020 at 19:38, on Zulip):

see comment

nikomatsakis (Feb 14 2020 at 19:39, on Zulip):

I'm trying to decide if there is some "trick" to going about this. I think I would probably just add the &self and start running cargo check --all

nikomatsakis (Feb 14 2020 at 19:39, on Zulip):

I'm wondering if this is a good "starting issue" or not. It's going to be some work, that's for sure.

nikomatsakis (Feb 14 2020 at 19:39, on Zulip):

OTOH, it's the kind of bug where you just get it to compile, and things should "just work"

nikomatsakis (Feb 14 2020 at 19:40, on Zulip):

there will be a few things that need to be refactored, e.g. I think we have some FromIterator impls that invoke TF::intern and those won't work

Jack Huey (Feb 14 2020 at 19:40, on Zulip):

I mean, self.data() is called in a bunch of places in debug, to start

nikomatsakis (Feb 14 2020 at 19:40, on Zulip):

note that I did not modify data

nikomatsakis (Feb 14 2020 at 19:40, on Zulip):

(yet) :)

nikomatsakis (Feb 14 2020 at 19:40, on Zulip):

but yeah, you're right, we should discus how we'll solve that

nikomatsakis (Feb 14 2020 at 19:40, on Zulip):

in rustc we use thread-local data to get the tcx

Jack Huey (Feb 14 2020 at 19:41, on Zulip):

debug already uses tls in some places

nikomatsakis (Feb 14 2020 at 19:41, on Zulip):

yes

nikomatsakis (Feb 14 2020 at 19:41, on Zulip):

in slightly different way

Jack Huey (Feb 14 2020 at 19:41, on Zulip):

so that's not too strange

nikomatsakis (Feb 14 2020 at 19:41, on Zulip):

but it could proabbly be adapted to this purpose

nikomatsakis (Feb 14 2020 at 19:41, on Zulip):

it'd just become more imporant than ever :)

nikomatsakis (Feb 14 2020 at 19:41, on Zulip):

i.e., right now you get "kinda usable" debug output without good TLS

nikomatsakis (Feb 14 2020 at 19:41, on Zulip):

but it would become quite less

Jane Lusby (Feb 14 2020 at 19:41, on Zulip):

okay, its good to know this is a blocker for other stuff on integration

nikomatsakis (Feb 14 2020 at 19:42, on Zulip):

@Jane Lusby I'm going to spend a bit of time enumerating a coupel of other concrete steps, but I definitely think this is one we should try to do this sprint, whether it be you or someone else (maybe me...)

Jane Lusby (Feb 14 2020 at 19:42, on Zulip):

okay

Jane Lusby (Feb 14 2020 at 19:42, on Zulip):

I'm definitely gonna try to do it

nikomatsakis (Feb 14 2020 at 19:42, on Zulip):

ok great!

Jane Lusby (Feb 14 2020 at 19:42, on Zulip):

ill let you know quickly if I feel like im spining my wheels though

Jack Huey (Feb 14 2020 at 19:42, on Zulip):

Or (and I don't know how feasible this is), but don't have Ty (and others) implement Debug, only TyData

nikomatsakis (Feb 14 2020 at 19:42, on Zulip):

one thing we can do if you want

nikomatsakis (Feb 14 2020 at 19:43, on Zulip):

Jack Huey said:

Or (and I don't know how feasible this is), but don't have Ty (and others) implement Debug, only TyData

maybe we should spin the "debug" conversation to a side topic

nikomatsakis (Feb 14 2020 at 19:43, on Zulip):

nikomatsakis said:

one thing we can do if you want

I was going to say, @Jane Lusby, if you want we could try a "pair session" to get started

nikomatsakis (Feb 14 2020 at 19:43, on Zulip):

sometimes that's useful

Jack Huey (Feb 14 2020 at 19:43, on Zulip):

maybe, but I think it is slightly relevant

Jack Huey (Feb 14 2020 at 19:43, on Zulip):

but we can hold for another time :)

nikomatsakis (Feb 14 2020 at 19:44, on Zulip):

they are related, but (a) I don't think this step depends on it and (b) we have to do this change, it's not like it's optional

Jane Lusby (Feb 14 2020 at 19:44, on Zulip):

whats your availability look like @nikomatsakis ?

nikomatsakis (Feb 14 2020 at 19:44, on Zulip):

this afternoon I'm available; next week would be harder. I've blocked out mondays/fridays in general though

Jane Lusby (Feb 14 2020 at 19:44, on Zulip):

okay

nikomatsakis (Feb 14 2020 at 19:45, on Zulip):

next week I'm on vacation mon/fri is part of the problem :) but I'll be around tue-thu in the mornings (but I think you are on west coast US?)

Jane Lusby (Feb 14 2020 at 19:45, on Zulip):

im at work until 5pm pst so I cant do any voice chat things until then

Jane Lusby (Feb 14 2020 at 19:45, on Zulip):

yea

Jane Lusby (Feb 14 2020 at 19:45, on Zulip):

west coast

Jane Lusby (Feb 14 2020 at 19:45, on Zulip):

tues and thurs are good for me because I'm wfh and often not busy

Jane Lusby (Feb 14 2020 at 19:46, on Zulip):

I'm gonna attempt to start on this without the pair programming but ill msg you this afternoon if I feel like it would help

nikomatsakis (Feb 14 2020 at 19:46, on Zulip):

Sounds good

Jane Lusby (Feb 14 2020 at 20:05, on Zulip):

nikomatsakis said:

We've been talking about renaming that to TypeInterner or maybe TypeContext

Fwiw, interner gets the meaning across best for me, and is the only one that I think makes it obvious what its doing, so I vote for this over tf or tcx, even if interner is longer to write, its not that long >_>

Jane Lusby (Feb 14 2020 at 20:08, on Zulip):

also

Jane Lusby (Feb 14 2020 at 20:08, on Zulip):
impl<TF: TypeFamily> Ty<TF> {
    pub fn new(data: impl CastTo<TyData<TF>>) -> Self {
        Ty {
            interned: TF::intern_ty(data.cast()),
        }
    }
Jane Lusby (Feb 14 2020 at 20:08, on Zulip):

presumably this requires I construct an instance of the TF, should I add a TF: Default bound here?

Jane Lusby (Feb 14 2020 at 20:09, on Zulip):

or is this gonna pass in a TF as self also

Jane Lusby (Feb 14 2020 at 20:09, on Zulip):

or not self, just an arg

nikomatsakis (Feb 14 2020 at 20:10, on Zulip):

@Jane Lusby you want to pass in an instance of TF

nikomatsakis (Feb 14 2020 at 20:10, on Zulip):

at least in the chalk-ir code, you will never create one, always be given it

Jane Lusby (Feb 14 2020 at 20:10, on Zulip):

okay

nikomatsakis (Feb 14 2020 at 20:10, on Zulip):

this is precisely the kind of pattern where we cheated a bit out of convenience :)

nikomatsakis (Feb 14 2020 at 20:11, on Zulip):

feel free, if you think it would be nicer, to experiment with changing patterns here and there

nikomatsakis (Feb 14 2020 at 20:11, on Zulip):

e.g., to make interner.new_ty(data) instead of Ty::new(interner, data) or something

nikomatsakis (Feb 14 2020 at 20:11, on Zulip):

although that particular thing doesn't look better to me

Jane Lusby (Feb 14 2020 at 20:11, on Zulip):

i would rather get more familiar with the codebase before I try to make decisions about what patterns to use

nikomatsakis (Feb 14 2020 at 20:11, on Zulip):

yeah

Jane Lusby (Feb 14 2020 at 20:11, on Zulip):

im going to go ahead and rename TF to interner everywhere as I'm moving along

nikomatsakis (Feb 14 2020 at 20:11, on Zulip):

the main thing I'm concerned about is I think there are some cases where we use FromIterator and collect and that will have to be rewritten

Jane Lusby (Feb 14 2020 at 20:12, on Zulip):

what should the parameter for generics be

Jane Lusby (Feb 14 2020 at 20:12, on Zulip):

just I?

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

Jane Lusby said:

im going to go ahead and rename TF to interner everywhere as I'm moving along

maybe we should just do this as a first PR

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

before anything else

Jane Lusby (Feb 14 2020 at 20:12, on Zulip):

okay

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

I would definitely recommend that

Jane Lusby (Feb 14 2020 at 20:12, on Zulip):

that sounds good

Jane Lusby (Feb 14 2020 at 20:12, on Zulip):

okay

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

otherwise you'll get trippe up in rebasing hell or something

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

as far as the initials..

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

should it be Interner or TypeInterner

nikomatsakis (Feb 14 2020 at 20:12, on Zulip):

I guess it interns more than types

nikomatsakis (Feb 14 2020 at 20:13, on Zulip):

maybe I: Interner is kind of nice, actually

nikomatsakis (Feb 14 2020 at 20:13, on Zulip):

simple

Jane Lusby (Feb 14 2020 at 20:13, on Zulip):

or actually

nikomatsakis (Feb 14 2020 at 20:13, on Zulip):

one thing is that there is also TargetTypeFamily, but TargetInterner is ok

Jane Lusby (Feb 14 2020 at 20:13, on Zulip):

TI?

Jane Lusby (Feb 14 2020 at 20:13, on Zulip):

okay

Jane Lusby (Feb 14 2020 at 20:13, on Zulip):

i

Jane Lusby (Feb 14 2020 at 20:13, on Zulip):

I and Interner

nikomatsakis (Feb 14 2020 at 20:13, on Zulip):

I could go either way but I'm kind of leaning towards "less is more"

detrumi (Feb 14 2020 at 20:14, on Zulip):

We should avoid TI for Interner, because that gets confusing for TargetInterner

Jane Lusby (Feb 14 2020 at 20:15, on Zulip):

its colliding with a few I: IntoIterator calls

Jane Lusby (Feb 14 2020 at 20:15, on Zulip):

i guess ill make those II

Jane Lusby (Feb 14 2020 at 20:15, on Zulip):

this is fine

nikomatsakis (Feb 14 2020 at 20:16, on Zulip):

detrumi said:

We should avoid TI for Interner, because that gets confusing for TargetInterner (TTI?)

I think it should be I: Interner or TI: TypeInterner

Jane Lusby (Feb 14 2020 at 20:17, on Zulip):

i went with I: Interner

Jane Lusby (Feb 14 2020 at 20:27, on Zulip):

this is a very large change...

nikomatsakis (Feb 14 2020 at 20:27, on Zulip):

yeah it's gonna be a pain

Jane Lusby (Feb 14 2020 at 20:27, on Zulip):

/me worried that every existing pr is gonna explode

nikomatsakis (Feb 14 2020 at 20:27, on Zulip):

this is why I'm wondering if I should find you something smaller to start with :)

nikomatsakis (Feb 14 2020 at 20:27, on Zulip):

oh well you're jus doing the rename now

nikomatsakis (Feb 14 2020 at 20:28, on Zulip):

that's not so bad

Jane Lusby (Feb 14 2020 at 20:28, on Zulip):

yea

nikomatsakis (Feb 14 2020 at 20:28, on Zulip):

:P

Jane Lusby (Feb 14 2020 at 20:28, on Zulip):

just wait till you see the PR

nikomatsakis (Feb 14 2020 at 20:28, on Zulip):

it'll touch a lot of files ...

Jane Lusby (Feb 14 2020 at 20:28, on Zulip):

i may have gone overboard

detrumi (Feb 14 2020 at 20:28, on Zulip):

Nah it'll be fine, all open PRs just have to rebase

nikomatsakis (Feb 14 2020 at 20:29, on Zulip):

it's fine. you'll just RUIN EVERYONE'S WORK

nikomatsakis (Feb 14 2020 at 20:29, on Zulip):

:P

nikomatsakis (Feb 14 2020 at 20:29, on Zulip):

(and we'll be grateful for it)

Jane Lusby (Feb 14 2020 at 20:35, on Zulip):

Screenshot-from-2020-02-14-12-35-23.png

Jane Lusby (Feb 14 2020 at 20:35, on Zulip):

https://github.com/rust-lang/chalk/pull/329/files

Jane Lusby (Feb 14 2020 at 20:36, on Zulip):

welp

Jane Lusby (Feb 14 2020 at 20:36, on Zulip):

i did the easy part

Jane Lusby (Feb 14 2020 at 20:36, on Zulip):

yall have fun with the merge conflicts

Jane Lusby (Feb 14 2020 at 20:36, on Zulip):

looool

detrumi (Feb 14 2020 at 20:37, on Zulip):

You missed some _TTF's

Jane Lusby (Feb 14 2020 at 20:38, on Zulip):

aah, tyty

Jane Lusby (Feb 14 2020 at 20:39, on Zulip):

fixed

Jane Lusby (Feb 14 2020 at 21:08, on Zulip):

so I can just slap interner variables into every fn here but I'm not sure if that should be done so I'm gonna put function signatures that I'm adding interner to here for double checking

Jane Lusby (Feb 14 2020 at 21:08, on Zulip):
impl<I: Interner> TyData<I> {
    pub fn intern(self, interner: &I) -> Ty<I> {
        Ty::new(interner, self)
    }
}
Jane Lusby (Feb 14 2020 at 21:09, on Zulip):

this leaked into this, but im not sure i should be adding interner to the fn sig here on this Folder trait

Jane Lusby (Feb 14 2020 at 21:09, on Zulip):
    fn fold_free_var_ty(&mut self, depth: usize, binders: usize) -> Fallible<Ty<TI>> {
        if self.forbid_free_vars() {
            panic!("unexpected free variable with depth `{:?}`", depth)
        } else {
            Ok(TyData::<TI>::BoundVar(depth + binders).intern())
        }
    }
Jane Lusby (Feb 14 2020 at 21:11, on Zulip):

going ahead with adding interner to both, and I've decided to prefer having interner before other args other than self

nikomatsakis (Feb 14 2020 at 21:24, on Zulip):

@Jane Lusby for folders

nikomatsakis (Feb 14 2020 at 21:24, on Zulip):

I think that the Folder needs a fn interner(&self) -> &I function

nikomatsakis (Feb 14 2020 at 21:24, on Zulip):

and probably a fn target_interner(&self) -> &TI function as well

nikomatsakis (Feb 14 2020 at 21:25, on Zulip):

I guess, in your PR, probably only the latter

Jane Lusby (Feb 14 2020 at 21:25, on Zulip):

aaaah

nikomatsakis (Feb 14 2020 at 21:25, on Zulip):

that way, the fold methods can use folder.target_interner()

Jane Lusby (Feb 14 2020 at 21:25, on Zulip):

yea i'd just been doing

Jane Lusby (Feb 14 2020 at 21:25, on Zulip):

interner: &TI, everywhere

Jane Lusby (Feb 14 2020 at 21:25, on Zulip):

and sometimes it wasnt obvious based on the usage

Jane Lusby (Feb 14 2020 at 21:25, on Zulip):

which one would be needed

nikomatsakis (Feb 14 2020 at 21:25, on Zulip):

sorry, should have had Zulip on my screen

Jane Lusby (Feb 14 2020 at 21:25, on Zulip):

its okay

Jane Lusby (Feb 14 2020 at 21:40, on Zulip):

@nikomatsakis what about ToParameter?

Jane Lusby (Feb 14 2020 at 21:40, on Zulip):
pub trait ToParameter {
    /// Utility for converting a list of all the binders into scope
    /// into references to those binders. Simply pair the binders with
    /// the indices, and invoke `to_parameter()` on the `(binder,
    /// index)` pair. The result will be a reference to a bound
    /// variable of appropriate kind at the corresponding index.
    fn to_parameter<I: Interner>(&self) -> Parameter<I>;
}

impl<'a> ToParameter for (&'a ParameterKind<()>, usize) {
    fn to_parameter<I: Interner>(&self) -> Parameter<I> {
        let &(binder, index) = self;
        match *binder {
            ParameterKind::Lifetime(_) => LifetimeData::BoundVar(index).intern().cast(),
            ParameterKind::Ty(_) => TyData::BoundVar(index).intern().cast(),
        }
    }
}
nikomatsakis (Feb 14 2020 at 21:40, on Zulip):

ah, that :)

Jane Lusby (Feb 14 2020 at 21:40, on Zulip):

it neets an interner for BoundVar(index).intern(<interner>)

nikomatsakis (Feb 14 2020 at 21:40, on Zulip):

yeah

nikomatsakis (Feb 14 2020 at 21:40, on Zulip):

it should take a &I I think

Jane Lusby (Feb 14 2020 at 21:40, on Zulip):

as an arg into to_parameter?

nikomatsakis (Feb 14 2020 at 21:41, on Zulip):

right

Jane Lusby (Feb 14 2020 at 21:41, on Zulip):

ack

Jane Lusby (Feb 14 2020 at 21:41, on Zulip):

@nikomatsakis same thing here?

Jane Lusby (Feb 14 2020 at 21:41, on Zulip):
impl<I: Interner> AssociatedTyDatum<I> {
    /// Returns the associated ty's bounds applied to the projection type, e.g.:
    ///
    /// ```notrust
    /// Implemented(<?0 as Foo>::Item<?1>: Sized)
    /// ```
    ///
    /// these quantified where clauses are in the scope of the
    /// `binders` field.
    pub fn bounds_on_self(&self) -> Vec<QuantifiedWhereClause<I>> {
Jane Lusby (Feb 14 2020 at 21:41, on Zulip):

this uses to_parameters

Jane Lusby (Feb 14 2020 at 21:41, on Zulip):

and intern directly

Jane Lusby (Feb 14 2020 at 21:42, on Zulip):

both of which need an interner

nikomatsakis (Feb 14 2020 at 21:43, on Zulip):

yeah

Jane Lusby (Feb 14 2020 at 21:49, on Zulip):

theres gonna be a lot of these repeat confirmations @nikomatsakis sorry

Jane Lusby (Feb 14 2020 at 21:49, on Zulip):
pub fn push_auto_trait_impls<I: Interner>(
    builder: &mut ClauseBuilder<'_, I>,
    auto_trait_id: TraitId<I>,
    struct_id: StructId<I>,
) {
Jane Lusby (Feb 14 2020 at 21:49, on Zulip):

add arg?

Jane Lusby (Feb 14 2020 at 21:49, on Zulip):

this is a free fn in clauses.rs in chalk-solve

nikomatsakis (Feb 14 2020 at 21:50, on Zulip):

hmm

nikomatsakis (Feb 14 2020 at 21:50, on Zulip):

probably yes :)

Jane Lusby (Feb 14 2020 at 21:50, on Zulip):

lol okay

nikomatsakis (Feb 14 2020 at 21:50, on Zulip):

though I wonder if builder should store a &I

nikomatsakis (Feb 14 2020 at 21:50, on Zulip):

wait, @Jane Lusby, did you complete the rename?

nikomatsakis (Feb 14 2020 at 21:51, on Zulip):

I guess you must've :)

Jane Lusby (Feb 14 2020 at 21:51, on Zulip):

yes

Jane Lusby (Feb 14 2020 at 21:51, on Zulip):

the PR is waiting for approval

nikomatsakis (Feb 14 2020 at 21:51, on Zulip):

(you could open a PR for that..if you didn't already)

Jane Lusby (Feb 14 2020 at 21:51, on Zulip):

im working on top of it

nikomatsakis (Feb 14 2020 at 21:51, on Zulip):

oh, ok

Jane Lusby (Feb 14 2020 at 21:51, on Zulip):

https://github.com/rust-lang/chalk/pull/329

nikomatsakis (Feb 14 2020 at 21:52, on Zulip):

I'm merging that;)

Jane Lusby (Feb 14 2020 at 21:52, on Zulip):

awesome :)

nikomatsakis (Feb 14 2020 at 21:52, on Zulip):

(I did page down throgh it, looked good to me)

nikomatsakis (Feb 14 2020 at 21:52, on Zulip):

and I think Interner was the right choice, it reads really naturally now

Jane Lusby (Feb 14 2020 at 21:52, on Zulip):

:blush:

Jane Lusby (Feb 14 2020 at 21:53, on Zulip):

im going to go ahead and add an interner fn to ClauseBuilder

Jane Lusby (Feb 14 2020 at 21:53, on Zulip):

because I think thats easy to undo later

nikomatsakis (Feb 14 2020 at 21:53, on Zulip):

yeah I suspect that is right

Jane Lusby (Feb 14 2020 at 21:53, on Zulip):

and it lets me avoid a ton of work by just putting unimplemented() in the initial def

Jane Lusby (Feb 14 2020 at 21:55, on Zulip):

okay what about CoherenceSolver

Jane Lusby (Feb 14 2020 at 21:55, on Zulip):

doesn't appear I have a ClauseBuilder available here

Jane Lusby (Feb 14 2020 at 21:56, on Zulip):
    fn disjoint(&self, lhs: &ImplDatum<I>, rhs: &ImplDatum<I>) -> bool {

needs access to an interner for its call to compatible

Jane Lusby (Feb 14 2020 at 21:57, on Zulip):

gonna just go with the same approach as ClauseBuilder for same reason

Jane Lusby (Feb 14 2020 at 21:57, on Zulip):

actually this only uses the interner in one place i think?

nikomatsakis (Feb 14 2020 at 21:57, on Zulip):

hmm so

nikomatsakis (Feb 14 2020 at 21:57, on Zulip):

I am thinking that the RustIrDatabase trait

nikomatsakis (Feb 14 2020 at 21:57, on Zulip):

should maybe have a fn interner(&self) -> &I method

Jane Lusby (Feb 14 2020 at 21:58, on Zulip):

okay

nikomatsakis (Feb 14 2020 at 21:58, on Zulip):

that seems right anyway

nikomatsakis (Feb 14 2020 at 21:58, on Zulip):

to be honest, we could even want trait RustIrDatabase: Interner

nikomatsakis (Feb 14 2020 at 21:58, on Zulip):

but let's not do that now

Jane Lusby (Feb 14 2020 at 21:59, on Zulip):

ack

Jane Lusby (Feb 14 2020 at 21:59, on Zulip):

what about InferenceTable

Jane Lusby (Feb 14 2020 at 21:59, on Zulip):
pub(crate) struct InferenceTable<I: Interner> {
    unify: ena::unify::InPlaceUnificationTable<EnaVariable<I>>,
    vars: Vec<EnaVariable<I>>,
    max_universe: UniverseIndex,
}
Jane Lusby (Feb 14 2020 at 22:00, on Zulip):

needs one for

    pub(crate) fn instantiate_binders_universally<T>(
        &mut self,
        arg: impl IntoBindersAndValue<Value = T>,
    ) -> T::Result
    where
        T: Fold<I>,
    {
nikomatsakis (Feb 14 2020 at 22:00, on Zulip):

Hmm

nikomatsakis (Feb 14 2020 at 22:00, on Zulip):

you know, side note

nikomatsakis (Feb 14 2020 at 22:00, on Zulip):

we've been passing &I everywhere

nikomatsakis (Feb 14 2020 at 22:01, on Zulip):

but we could've made it Interner: Copy..no, I think &I is probably right

nikomatsakis (Feb 14 2020 at 22:01, on Zulip):

I think I would not store it in InferenceTable

Jane Lusby (Feb 14 2020 at 22:01, on Zulip):

:3

Jane Lusby (Feb 14 2020 at 22:01, on Zulip):

so just take as an arg for instantiate...

nikomatsakis (Feb 14 2020 at 22:01, on Zulip):

I'm wondering if we will regret that

nikomatsakis (Feb 14 2020 at 22:01, on Zulip):

but let's try it

Jane Lusby (Feb 14 2020 at 22:01, on Zulip):

okay

Jane Lusby (Feb 14 2020 at 22:01, on Zulip):

im going to git add rn

Jane Lusby (Feb 14 2020 at 22:01, on Zulip):

so its easy to undo

Jane Lusby (Feb 14 2020 at 22:03, on Zulip):
impl<I: Interner> GoalExt<I> for Goal<I> {
    /// Returns a canonical goal in which the outermost `exists<>` and
    /// `forall<>` quantifiers (as well as implications) have been
    /// "peeled" and are converted into free universal or existential
    /// variables. Assumes that this goal is a "closed goal" which
    /// does not -- at present -- contain any variables. Useful for
    /// REPLs and tests but not much else.
    fn into_peeled_goal(self) -> UCanonical<InEnvironment<Goal<I>>> {
Jane Lusby (Feb 14 2020 at 22:03, on Zulip):

need an interner here

Jane Lusby (Feb 14 2020 at 22:03, on Zulip):

arg?

nikomatsakis (Feb 14 2020 at 22:04, on Zulip):

yes

Jane Lusby (Feb 14 2020 at 22:06, on Zulip):

aaand what about

Jane Lusby (Feb 14 2020 at 22:06, on Zulip):
impl<'t, I: Interner> Unifier<'t, I> {
Jane Lusby (Feb 14 2020 at 22:06, on Zulip):
    fn unify_binders<T, R>(
        &mut self,
        a: impl IntoBindersAndValue<Value = T> + Copy + Debug,
        b: impl IntoBindersAndValue<Value = T> + Copy + Debug,
    ) -> Fallible<()>
    where
        T: Fold<I, Result = R>,
        R: Zip<I> + Fold<I, Result = R>,
    {
nikomatsakis (Feb 14 2020 at 22:07, on Zulip):

I was wondering when you would get to that one

nikomatsakis (Feb 14 2020 at 22:07, on Zulip):

that should have a &'t I in Unifier

nikomatsakis (Feb 14 2020 at 22:07, on Zulip):

for sure

Jane Lusby (Feb 14 2020 at 22:07, on Zulip):

okay

Jane Lusby (Feb 14 2020 at 22:08, on Zulip):
impl<I: Interner> EnaVariable<I> {
    /// Convert this inference variable into a type. When using this
    /// method, naturally you should know from context that the kind
    /// of this inference variable is a type (we can't check it).
    pub(crate) fn to_ty(self) -> Ty<I> {
        self.var.to_ty::<I>()
    }
Jane Lusby (Feb 14 2020 at 22:08, on Zulip):

arg?

nikomatsakis (Feb 14 2020 at 22:08, on Zulip):

yes

Jane Lusby (Feb 14 2020 at 22:09, on Zulip):
impl<I: Interner> ParameterEnaVariableExt<I> for ParameterEnaVariable<I> {
    fn to_parameter(self) -> Parameter<I> {
        match self {
            ParameterKind::Ty(v) => v.to_ty().cast(),
            ParameterKind::Lifetime(v) => v.to_lifetime().cast(),
        }
    }
}
Jane Lusby (Feb 14 2020 at 22:09, on Zulip):

assuming arg

Jane Lusby (Feb 14 2020 at 22:10, on Zulip):

and by extension adding an arg to

Jane Lusby (Feb 14 2020 at 22:10, on Zulip):
    pub(crate) fn fresh_subst(
        &mut self,
        interner: &I,
Jane Lusby (Feb 14 2020 at 22:10, on Zulip):

in InferenceTable

nikomatsakis (Feb 14 2020 at 22:11, on Zulip):

wait

Jane Lusby (Feb 14 2020 at 22:11, on Zulip):

awe shit this is spreading like a contagion in InferenceTable fn args

nikomatsakis (Feb 14 2020 at 22:11, on Zulip):

I mean, it has an arg already

nikomatsakis (Feb 14 2020 at 22:11, on Zulip):

but I guess you already added it

Jane Lusby (Feb 14 2020 at 22:11, on Zulip):

only on

Jane Lusby (Feb 14 2020 at 22:11, on Zulip):

yea

Jane Lusby (Feb 14 2020 at 22:11, on Zulip):

i just added that

Jane Lusby (Feb 14 2020 at 22:11, on Zulip):

sorry

Jane Lusby (Feb 14 2020 at 22:11, on Zulip):

been posting pre changes then randomly did one post :sweat_smile:

nikomatsakis (Feb 14 2020 at 22:12, on Zulip):

:)

nikomatsakis (Feb 14 2020 at 22:12, on Zulip):

Jane Lusby said:

awe shit this is spreading like a contagion in InferenceTable fn args

yeah .. I'm not too surprised ..

nikomatsakis (Feb 14 2020 at 22:12, on Zulip):

it might be that inference table should have a reference

nikomatsakis (Feb 14 2020 at 22:12, on Zulip):

just kind of annoying

Jane Lusby (Feb 14 2020 at 22:12, on Zulip):

hmm

nikomatsakis (Feb 14 2020 at 22:13, on Zulip):

I wonder actually

nikomatsakis (Feb 14 2020 at 22:13, on Zulip):

I feel like I want I to be Clone

Jane Lusby (Feb 14 2020 at 22:13, on Zulip):
impl<T, I> CanonicalExt<T, I> for Canonical<T>
where
    T: HasInterner<Interner = I>,
    I: Interner,
{
    /// Maps the contents using `op`, but preserving the binders.
    ///
    /// NB. `op` will be invoked with an instantiated version of the
    /// canonical value, where inference variables (from a fresh
    /// inference context) are used in place of the quantified free
    /// variables. The result should be in terms of those same
    /// inference variables and will be re-canonicalized.
    fn map<OP, U>(self, op: OP) -> Canonical<U::Result>
    where
        OP: FnOnce(T::Result) -> U,
        T: Fold<I>,
        U: Fold<I>,
        U::Result: HasInterner<Interner = I>,
    {
        // Subtle: It is only quite rarely correct to apply `op` and
        // just re-use our existing binders. For that to be valid, the
        // result of `op` would have to ensure that it re-uses all the
        // existing free variables and in the same order. Otherwise,
        // the canonical form would be different: the variables might
        // be numbered differently, or some may not longer be used.
        // This would mean that two canonical values could no longer
        // be compared with `Eq`, which defeats a key invariant of the
        // `Canonical` type (indeed, its entire reason for existence).
        let mut infer = InferenceTable::new();
        let snapshot = infer.snapshot();
        let instantiated_value = infer.instantiate_canonical(&self);
        let mapped_value = op(instantiated_value);
        let result = infer.canonicalize(&mapped_value);
        infer.rollback_to(snapshot);
        result.quantified
    }
}
Jane Lusby (Feb 14 2020 at 22:13, on Zulip):

this needs an interner

nikomatsakis (Feb 14 2020 at 22:13, on Zulip):

I don't know what type it would map to in rust-analyzer

nikomatsakis (Feb 14 2020 at 22:13, on Zulip):

in rustc it'd be no problem for it to be Copy and to be passed around by value everywhere

nikomatsakis (Feb 14 2020 at 22:13, on Zulip):

but I think rust-analyzer that might not be true

nikomatsakis (Feb 14 2020 at 22:14, on Zulip):

so we'd prefer to thread it as parameters

Jane Lusby (Feb 14 2020 at 22:14, on Zulip):

ack

nikomatsakis (Feb 14 2020 at 22:14, on Zulip):

rather than store it in InferenceTable, since I think those can be relatively long-lived

Jane Lusby (Feb 14 2020 at 22:14, on Zulip):

lets default to & for now then

Jane Lusby (Feb 14 2020 at 22:15, on Zulip):

this CanonicalExt fn still needs an interner tho

Jane Lusby (Feb 14 2020 at 22:15, on Zulip):

gonna do as arg

nikomatsakis (Feb 14 2020 at 22:16, on Zulip):

seems right

Jane Lusby (Feb 14 2020 at 22:16, on Zulip):

and im gonna toss the refernece in InferenceTable

Jane Lusby (Feb 14 2020 at 22:16, on Zulip):

see how that goes

Jane Lusby (Feb 14 2020 at 22:19, on Zulip):

oooor maybe not

Jane Lusby (Feb 14 2020 at 22:19, on Zulip):

Screenshot-from-2020-02-14-14-19-16.png

nikomatsakis (Feb 14 2020 at 22:20, on Zulip):

hmm not sure what's up with that

Florian Diebold (Feb 14 2020 at 22:24, on Zulip):

nikomatsakis said:

I don't know what type it would map to in rust-analyzer

I'd expect it to be this

Jane Lusby (Feb 14 2020 at 22:24, on Zulip):

i just backtracked to threading via args nbd

Jane Lusby (Feb 14 2020 at 22:25, on Zulip):
impl<I: Interner> Folder<I> for OccursCheck<'_, '_, I> {
Jane Lusby (Feb 14 2020 at 22:25, on Zulip):

this raises a minor question

Jane Lusby (Feb 14 2020 at 22:25, on Zulip):

we have interner() from folder and self.unifier.interner from OccursCheck

nikomatsakis (Feb 14 2020 at 22:26, on Zulip):

is the question "which one to use"?

nikomatsakis (Feb 14 2020 at 22:26, on Zulip):

it shouldn't matter

Jane Lusby (Feb 14 2020 at 22:26, on Zulip):

gonna do self.interner

nikomatsakis (Feb 14 2020 at 22:27, on Zulip):

I think the expectation is that the interner will be threaded around rather broadly

Jane Lusby (Feb 14 2020 at 22:27, on Zulip):

on the assumption that this will be implemented as self.unifier.interner

nikomatsakis (Feb 14 2020 at 22:27, on Zulip):

right

Jane Lusby (Feb 14 2020 at 22:27, on Zulip):
impl<I: Interner> context::Context for SlgContext<I> {
    type CanonicalGoalInEnvironment = Canonical<InEnvironment<Goal<I>>>;
    type CanonicalExClause = Canonical<ExClause<Self>>;
    type UCanonicalGoalInEnvironment = UCanonical<InEnvironment<Goal<I>>>;
    type UniverseMap = UniverseMap;
    type InferenceNormalizedSubst = Substitution<I>;
    type Solution = Solution<I>;
    type InferenceTable = TruncatingInferenceTable<I>;
    type Environment = Environment<I>;
    type DomainGoal = DomainGoal<I>;
    type Goal = Goal<I>;
    type BindersGoal = Binders<Goal<I>>;
    type Parameter = Parameter<I>;
    type ProgramClause = ProgramClause<I>;
    type ProgramClauses = Vec<ProgramClause<I>>;
    type CanonicalConstrainedSubst = Canonical<ConstrainedSubst<I>>;
    type CanonicalAnswerSubst = Canonical<AnswerSubst<I>>;
    type GoalInEnvironment = InEnvironment<Goal<I>>;
    type Substitution = Substitution<I>;
    type RegionConstraint = InEnvironment<Constraint<I>>;
    type Variance = ();
Jane Lusby (Feb 14 2020 at 22:27, on Zulip):

thats a signature and a half

Jane Lusby (Feb 14 2020 at 22:27, on Zulip):

lol

Jane Lusby (Feb 14 2020 at 22:28, on Zulip):
    fn identity_constrained_subst(
        goal: &UCanonical<InEnvironment<Goal<I>>>,
    ) -> Canonical<ConstrainedSubst<I>> {
Jane Lusby (Feb 14 2020 at 22:28, on Zulip):

need an interner here

nikomatsakis (Feb 14 2020 at 22:28, on Zulip):

hmm

nikomatsakis (Feb 14 2020 at 22:29, on Zulip):

I'm coming back again btw to the question of whether we should be allowed to take ownership of Interner :)

nikomatsakis (Feb 14 2020 at 22:29, on Zulip):

because I would say that SlgContext<I> could own an I

Jane Lusby (Feb 14 2020 at 22:30, on Zulip):

```pub(crate) struct SlgContextOps<'me, I: Interner> {
program: &'me dyn RustIrDatabase<I>,


Jane Lusby (Feb 14 2020 at 22:30, on Zulip):

just noticed we have a db

nikomatsakis (Feb 14 2020 at 22:31, on Zulip):

Florian Diebold said:

I'd expect it to be this

I'm not sure about this though, because I think that these types ought to be types that are not tied to the 'a lifetime there

nikomatsakis (Feb 14 2020 at 22:31, on Zulip):

Jane Lusby said:

just noticed we have a db

ah, yes

Jane Lusby (Feb 14 2020 at 22:32, on Zulip):

that requires me adding a &self arg to the identity_constraitned_subst fn

Jane Lusby (Feb 14 2020 at 22:33, on Zulip):

wait no

Jane Lusby (Feb 14 2020 at 22:33, on Zulip):

its the Ops that has the db

Jane Lusby (Feb 14 2020 at 22:33, on Zulip):

not the self

nikomatsakis (Feb 14 2020 at 22:33, on Zulip):

yeah, but the method could maybe move to Ops

Jack Huey (Feb 14 2020 at 22:33, on Zulip):

I check zulip and there's 132 unread messages on wg-traits, my goodness

Jane Lusby (Feb 14 2020 at 22:33, on Zulip):

yea,

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

im basically being a human language based keyboard for niko rn

nikomatsakis (Feb 14 2020 at 22:34, on Zulip):

I'm going to have to go in a bit :)

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

okay

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

should I stop now or should we keep going until you have to go?

nikomatsakis (Feb 14 2020 at 22:34, on Zulip):

but I do want to sit with @Florian Diebold -- or maybe just read over code a bit -- because I think we should settle this question of whether we can own the I

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

i'm beginning to think

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

that pair programming idea is probably a good one

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

lets plan on meeting up after my work is done?

Jane Lusby (Feb 14 2020 at 22:34, on Zulip):

i have some work I should probably do anyways

Jack Huey (Feb 14 2020 at 22:35, on Zulip):

I can also answer some questions if I'm around when you ask them :)

Jane Lusby (Feb 14 2020 at 22:37, on Zulip):

i think its okay if we wait a few hours

Jane Lusby (Feb 14 2020 at 22:37, on Zulip):

but if niko isnt around ill definitely bother you

nikomatsakis (Feb 14 2020 at 22:37, on Zulip):

I have to go offline now, but @Jane Lusby seems like you made a lot of progress in any case :)

Jane Lusby (Feb 14 2020 at 22:37, on Zulip):

^_^

Jane Lusby (Feb 14 2020 at 22:52, on Zulip):

K gonna take a break for now, I pushed my changes so far to https://github.com/rust-lang/chalk/pull/330/files

Jane Lusby (Feb 14 2020 at 22:52, on Zulip):

so feel free to take a look with it / leave suggestions / comments @Jack Huey

Jack Huey (Feb 14 2020 at 22:53, on Zulip):

Sure I'll take a look :)

Jack Huey (Feb 14 2020 at 22:57, on Zulip):

pasted image rebasing on the TF -> I changes

Jane Lusby (Feb 14 2020 at 22:59, on Zulip):

im so sorry :sob:

Jack Huey (Feb 14 2020 at 22:59, on Zulip):

it's okay

Jack Huey (Feb 14 2020 at 23:00, on Zulip):

at least that file (debug) was super easy because I just has to accept all current and find/replace Formatter with Formatter<'_>

Jane Lusby (Feb 14 2020 at 23:38, on Zulip):

:D

nikomatsakis (Feb 15 2020 at 00:16, on Zulip):

@Jane Lusby (still afk, but I was going to say that if we can't pull off a pair session, next best thing is to push to a branch)

nikomatsakis (Feb 15 2020 at 00:17, on Zulip):

Also, I gave it some thought, and I think that the I parameter in rust-analyzer should map to DB, @Florian Diebold

Jane Lusby (Feb 15 2020 at 00:25, on Zulip):

Well I already pushed a branch so we're good to go either way

Jane Lusby (Feb 15 2020 at 00:38, on Zulip):

so yea @nikomatsakis im planning on setting aside time for working on this PR tonight, I can basically start at any time now. Just lmk when or if you're not gonna be available tonight lmk ahead of time so I can head home.

Jane Lusby (Feb 15 2020 at 01:02, on Zulip):

on second thought, I just realized I forgot today is valentines day, so I'm gonna have to raincheck, but I will hopefully have time to work on this on monday, if you need to get it done before then please feel free to take the branch I pushed and just finish it up.

Jane Lusby (Feb 18 2020 at 22:04, on Zulip):

@nikomatsakis / @Jack Huey lmk if either of you have time to help me power through the rest of the PR, today after work works well for me

Jack Huey (Feb 18 2020 at 23:00, on Zulip):

I can help :)

Jack Huey (Feb 18 2020 at 23:03, on Zulip):

I skimmed through your PR so far. Nothing seemed obviously wrong. But it's so incomplete that I didn't really want to think too hard about it

Jane Lusby (Feb 18 2020 at 23:17, on Zulip):

thats fair

Jane Lusby (Feb 18 2020 at 23:17, on Zulip):

im finishing up another PR real quick then im gonna dig back into it

Jane Lusby (Feb 18 2020 at 23:18, on Zulip):

as it stands I don't really feel like its a PR I could easily do on my own because I just don't know the layout of the project well enough to know which structs should have references to the interner added to them and which ones should get it via args to their fns

Jane Lusby (Feb 18 2020 at 23:18, on Zulip):

so its pretty much just me asking "pass as arg here?" over and over again

Jack Huey (Feb 18 2020 at 23:20, on Zulip):

makes sense

Jane Lusby (Feb 19 2020 at 00:09, on Zulip):

okay so heres where I left off @Jack Huey

Jane Lusby (Feb 19 2020 at 00:09, on Zulip):
pub(crate) struct SlgContext<I: Interner> {
    max_size: usize,
    /// The expected number of answers for a solution.
    /// Only really sseful for tests, since `make_solution`
    /// will panic if the number of cached answers does not
    /// equal this when a solution is made.
    expected_answers: Option<usize>,
    phantom: PhantomData<I>,
}
Jane Lusby (Feb 19 2020 at 00:09, on Zulip):

should slgcontext have a reference or something to the Interner added

Jane Lusby (Feb 19 2020 at 00:10, on Zulip):

right now Unifiers have refernces added

Jack Huey (Feb 19 2020 at 00:10, on Zulip):

I'm gonna go with no

Jane Lusby (Feb 19 2020 at 00:10, on Zulip):

Folders and RustIRDatabase have methods for accessing one that are currently unimplemented

Jane Lusby (Feb 19 2020 at 00:10, on Zulip):

alright, so just thread thru in args

Jack Huey (Feb 19 2020 at 00:10, on Zulip):

If anything, it would go on ContextOps/SlgContextOps

Jane Lusby (Feb 19 2020 at 00:11, on Zulip):

yea its already in SlgContextOpts technically

Jane Lusby (Feb 19 2020 at 00:11, on Zulip):

via the irdatabase member

Jane Lusby (Feb 19 2020 at 00:11, on Zulip):

but i need it in fn identity_constrained_subst(

Jack Huey (Feb 19 2020 at 00:11, on Zulip):

the functions in Context may need to be moved to SlgContextOps

Jane Lusby (Feb 19 2020 at 00:11, on Zulip):

which is a method of Context impled on the SlgContext

Jane Lusby (Feb 19 2020 at 00:11, on Zulip):

ooh

Jane Lusby (Feb 19 2020 at 00:12, on Zulip):

whats the difference between the two pieces

Jane Lusby (Feb 19 2020 at 00:12, on Zulip):

(trying to know how I would decide which fns to move)

Jack Huey (Feb 19 2020 at 00:12, on Zulip):

SlgContext functions don't have a &self

Jane Lusby (Feb 19 2020 at 00:12, on Zulip):

other than "if it needs access to the context it should be in ops"

Jack Huey (Feb 19 2020 at 00:12, on Zulip):

basically only that

Jane Lusby (Feb 19 2020 at 00:12, on Zulip):

okay

Jane Lusby (Feb 19 2020 at 00:12, on Zulip):

welp that makes it easy

Jack Huey (Feb 19 2020 at 00:13, on Zulip):

identity_constrained_subst makes sense to be in ContextOps since where it's called, ContextOps is Self

Jane Lusby (Feb 19 2020 at 00:21, on Zulip):

yea that worked out

Jane Lusby (Feb 19 2020 at 00:21, on Zulip):

what about this one

Jane Lusby (Feb 19 2020 at 00:21, on Zulip):
impl<I: Interner> context::UnificationOps<SlgContext<I>> for TruncatingInferenceTable<I> {
    fn instantiate_binders_universally(&mut self, arg: &Binders<Goal<I>>) -> Goal<I> {
        self.infer.instantiate_binders_universally(arg)
    }

    fn instantiate_binders_existentially(&mut self, arg: &Binders<Goal<I>>) -> Goal<I> {
        self.infer.instantiate_binders_existentially(arg)
    }
Jane Lusby (Feb 19 2020 at 00:21, on Zulip):

instantiate_binders_universally needs an interner

Jack Huey (Feb 19 2020 at 00:23, on Zulip):

Probably on TruncatingInferenceTable

Jack Huey (Feb 19 2020 at 00:23, on Zulip):

ooh that's gonna be tough

Jack Huey (Feb 19 2020 at 00:25, on Zulip):

maybe just on the functions

Jane Lusby (Feb 19 2020 at 00:27, on Zulip):

need to figure out how to get the interner type available in the trait signature

Jane Lusby (Feb 19 2020 at 00:27, on Zulip):

because its a parameter on the impl right now

Jack Huey (Feb 19 2020 at 00:27, on Zulip):

right, that's why I said it's gonna be tough

Jane Lusby (Feb 19 2020 at 00:27, on Zulip):

i noticed the HasInterner trait earlier

Jane Lusby (Feb 19 2020 at 00:28, on Zulip):

should I just add a HasInterner bound on UnificationOps trait

Jane Lusby (Feb 19 2020 at 00:28, on Zulip):
pub trait UnificationOps<C: Context>: HasInterner {
    // Used by: simplify
    fn instantiate_binders_universally(&mut self, interner: &Self::Interner, arg: &C::BindersGoal) -> C::Goal;
Jack Huey (Feb 19 2020 at 00:29, on Zulip):

probably

Jane Lusby (Feb 19 2020 at 00:30, on Zulip):

looks like that wont work

Jane Lusby (Feb 19 2020 at 00:31, on Zulip):

HasInterner is part of chalk_ir

Jane Lusby (Feb 19 2020 at 00:31, on Zulip):

and thats not a dep of chalk_engine i guess

Jane Lusby (Feb 19 2020 at 00:31, on Zulip):

I will just add an associated type

Jack Huey (Feb 19 2020 at 00:33, on Zulip):

ah right

Jack Huey (Feb 19 2020 at 00:33, on Zulip):

this is gonna be tough

Jane Lusby (Feb 19 2020 at 00:34, on Zulip):

yea this is breaking down pretty bad

Jack Huey (Feb 19 2020 at 00:34, on Zulip):

My first thought was to put it on TruncatingInferenceTable, but you would need a lifetime, which you can't have because it would need to be a GAT

Jane Lusby (Feb 19 2020 at 00:34, on Zulip):

the impl for Forest has no Interner on it

Jane Lusby (Feb 19 2020 at 00:35, on Zulip):

and we use dyn InferenceTables

Jane Lusby (Feb 19 2020 at 00:35, on Zulip):

which require that we specify the associated type

Jane Lusby (Feb 19 2020 at 00:36, on Zulip):

could the Context define the interner?

Jack Huey (Feb 19 2020 at 00:37, on Zulip):

nope, because Context is in chalk-engine

Jane Lusby (Feb 19 2020 at 00:37, on Zulip):

i dont follow

Jack Huey (Feb 19 2020 at 00:37, on Zulip):

no Interner in the engine

Jane Lusby (Feb 19 2020 at 00:38, on Zulip):

it looks like it worked

Jane Lusby (Feb 19 2020 at 00:38, on Zulip):

let me show you what I meant

Jane Lusby (Feb 19 2020 at 00:39, on Zulip):

im sure im explaining this poorly

Jane Lusby (Feb 19 2020 at 00:39, on Zulip):
pub trait UnificationOps<C: Context> {
    // Used by: simplify
    fn instantiate_binders_universally(
        &mut self,
        interner: &C::Interner,
        arg: &C::BindersGoal,
    ) -> C::Goal;
Jane Lusby (Feb 19 2020 at 00:39, on Zulip):

i added the Interner associated type to the Context trait

Jane Lusby (Feb 19 2020 at 00:40, on Zulip):

and when Context is defined everything is parameterized on the interner

Jane Lusby (Feb 19 2020 at 00:40, on Zulip):
impl<I: Interner> context::Context for SlgContext<I> {
    type CanonicalGoalInEnvironment = Canonical<InEnvironment<Goal<I>>>;
    type CanonicalExClause = Canonical<ExClause<Self>>;
    type UCanonicalGoalInEnvironment = UCanonical<InEnvironment<Goal<I>>>;
    type UniverseMap = UniverseMap;
    type InferenceNormalizedSubst = Substitution<I>;
    type Solution = Solution<I>;
    type InferenceTable = TruncatingInferenceTable<I>;
    type Environment = Environment<I>;
    type DomainGoal = DomainGoal<I>;
    type Goal = Goal<I>;
    type BindersGoal = Binders<Goal<I>>;
    type Parameter = Parameter<I>;
    type ProgramClause = ProgramClause<I>;
    type ProgramClauses = Vec<ProgramClause<I>>;
    type CanonicalConstrainedSubst = Canonical<ConstrainedSubst<I>>;
    type CanonicalAnswerSubst = Canonical<AnswerSubst<I>>;
    type GoalInEnvironment = InEnvironment<Goal<I>>;
    type Substitution = Substitution<I>;
    type RegionConstraint = InEnvironment<Constraint<I>>;
    type Variance = ();
    type Interner = I;
Jane Lusby (Feb 19 2020 at 00:40, on Zulip):

so I added that last line for Interner = I

Jack Huey (Feb 19 2020 at 00:41, on Zulip):

well that works

Jane Lusby (Feb 19 2020 at 00:41, on Zulip):

doing the same thing on this fn

Jane Lusby (Feb 19 2020 at 00:41, on Zulip):
impl<C: Context> Forest<C> {
    /// Simplifies an HH goal into a series of positive domain goals
    /// and negative HH goals. This operation may fail if the HH goal
    /// includes unifications that cannot be completed.
    pub(super) fn simplify_hh_goal(
        interner: &C::Interner,
        infer: &mut dyn InferenceTable<C>,
        subst: C::Substitution,
        initial_environment: C::Environment,
        initial_hh_goal: HhGoal<C>,
    ) -> Fallible<ExClause<C>> {
Jane Lusby (Feb 19 2020 at 00:42, on Zulip):

i added an interner arg

Jane Lusby (Feb 19 2020 at 00:42, on Zulip):

lmk if you think it should be a member of the Forest instead

Jane Lusby (Feb 19 2020 at 00:42, on Zulip):

or maybe a getter fn on Context

Jane Lusby (Feb 19 2020 at 00:43, on Zulip):

if it were a getter fn on Context that would make this all very easy

Jack Huey (Feb 19 2020 at 00:44, on Zulip):

on ContextOps?

Jane Lusby (Feb 19 2020 at 00:46, on Zulip):

On either, but that would probably be the best

Jane Lusby (Feb 19 2020 at 00:46, on Zulip):

like here for example

Jane Lusby (Feb 19 2020 at 00:46, on Zulip):
    fn push_initial_strands(&mut self, context: &impl ContextOps<C>, table: TableIndex) {
        // Instantiate the table goal with fresh inference variables.
        let table_goal = self.tables[table].table_goal.clone();
        let (infer, subst, environment, goal) = context.instantiate_ucanonical_goal(&table_goal);
        self.push_initial_strands_instantiated(context, table, infer, subst, environment, goal);
    }
Jane Lusby (Feb 19 2020 at 00:46, on Zulip):

we have a ContextOps arg already

Jane Lusby (Feb 19 2020 at 00:48, on Zulip):

oh duh we already have the getter on ContextOps technically

Jane Lusby (Feb 19 2020 at 00:48, on Zulip):

via its program member

Jack Huey (Feb 19 2020 at 00:49, on Zulip):

yeah

Jack Huey (Feb 19 2020 at 00:49, on Zulip):

adding a getting to ContextOps is probably okay

Jane Lusby (Feb 19 2020 at 00:50, on Zulip):

yea, doing that, cannot access the program member when the context type is behind the generic

Jane Lusby (Feb 19 2020 at 00:53, on Zulip):

damnit

Jane Lusby (Feb 19 2020 at 00:53, on Zulip):
impl<C: HasInterner + Context> HasInterner for ExClause<C> {
    type Interner = C::Interner;
}
Jane Lusby (Feb 19 2020 at 00:53, on Zulip):

conflict

Jane Lusby (Feb 19 2020 at 00:53, on Zulip):
impl<C: HasInterner + Context> HasInterner for ExClause<C> {
    type Interner = <C as HasInterner>::Interner;
}
Jane Lusby (Feb 19 2020 at 00:54, on Zulip):

assuming this is Okay

Jack Huey (Feb 19 2020 at 00:54, on Zulip):

yep

Jack Huey (Feb 19 2020 at 00:55, on Zulip):

theoretically, they're the same

Jane Lusby (Feb 19 2020 at 00:55, on Zulip):

Okay thats done i think!

Jane Lusby (Feb 19 2020 at 00:55, on Zulip):

the slg stuff at least

Jane Lusby (Feb 19 2020 at 00:56, on Zulip):
    fn aggregate_application_tys(
        &mut self,
        apply1: &ApplicationTy<I>,
        apply2: &ApplicationTy<I>,
    ) -> Ty<I> {
Jane Lusby (Feb 19 2020 at 00:56, on Zulip):

this needs an interner

Jane Lusby (Feb 19 2020 at 00:56, on Zulip):
struct AntiUnifier<'infer, I: Interner> {
    infer: &'infer mut InferenceTable<I>,
    universe: UniverseIndex,
}
Jane Lusby (Feb 19 2020 at 00:56, on Zulip):

arg?

Jack Huey (Feb 19 2020 at 00:56, on Zulip):

probably a field

Jane Lusby (Feb 19 2020 at 00:57, on Zulip):

oh okay

Jack Huey (Feb 19 2020 at 00:57, on Zulip):

btw about to be on mobile. So I can probably still respond, but can't check code

Jack Huey (Feb 19 2020 at 00:57, on Zulip):

(unless you post it here :P)

Jane Lusby (Feb 19 2020 at 01:00, on Zulip):

okay

Jane Lusby (Feb 19 2020 at 01:00, on Zulip):
impl<I: Interner> context::ResolventOps<SlgContext<I>> for TruncatingInferenceTable<I> {
    /// Applies the SLG resolvent algorithm to incorporate a program
    /// clause into the main X-clause, producing a new X-clause that
    /// must be solved.
    ///
    /// # Parameters
    ///
    /// - `goal` is the goal G that we are trying to solve
    /// - `clause` is the program clause that may be useful to that end
    fn resolvent_clause(
        &mut self,
        environment: &Environment<I>,
        goal: &DomainGoal<I>,
        subst: &Substitution<I>,
        clause: &ProgramClause<I>,
    ) -> Fallible<ExClause<SlgContext<I>>> {
Jane Lusby (Feb 19 2020 at 01:00, on Zulip):

need an interner here

Jane Lusby (Feb 19 2020 at 01:00, on Zulip):

arg im guessing

Jane Lusby (Feb 19 2020 at 01:01, on Zulip):

that worked well

Jack Huey (Feb 19 2020 at 01:01, on Zulip):

Probably same UnificationOps

Jane Lusby (Feb 19 2020 at 01:03, on Zulip):

okay

Jane Lusby (Feb 19 2020 at 01:03, on Zulip):
    fn merge_answer_into_strand(&mut self, strand: &mut Strand<C>) -> RootSearchResult<()> {
Jane Lusby (Feb 19 2020 at 01:03, on Zulip):

this needs an interner

Jane Lusby (Feb 19 2020 at 01:04, on Zulip):

should I add a ContextOps arg?

Jane Lusby (Feb 19 2020 at 01:05, on Zulip):

or can Strand somehow pass it along

Jane Lusby (Feb 19 2020 at 01:05, on Zulip):

going ahead and adding a context arg

Jane Lusby (Feb 19 2020 at 01:08, on Zulip):

okay i think thats that path done

Jane Lusby (Feb 19 2020 at 01:08, on Zulip):
struct Truncater<'infer, I: Interner> {
    infer: &'infer mut InferenceTable<I>,
    current_size: usize,
    max_size: usize,
    overflow: bool,
}

impl<'infer, I: Interner> Truncater<'infer, I> {
    fn new(infer: &'infer mut InferenceTable<I>, max_size: usize) -> Self {
        Truncater {
            infer,
            current_size: 0,
            max_size,
            overflow: false,
        }
    }

    fn overflow(&mut self, pre_size: usize) -> Ty<I> {
        self.overflow = true;
        self.current_size = pre_size + 1;
        let universe = self.infer.max_universe();
        self.infer.new_variable(universe).to_ty()
    }
}
Jane Lusby (Feb 19 2020 at 01:08, on Zulip):

need an interner for overflow

Jane Lusby (Feb 19 2020 at 01:08, on Zulip):

arg?

Jane Lusby (Feb 19 2020 at 01:08, on Zulip):

this thing impls Folder

Jane Lusby (Feb 19 2020 at 01:08, on Zulip):
    fn interner(&self) -> &I {
        unimplemented!()
    }

    fn target_interner(&self) -> &I {
        unimplemented!()
    }
Jane Lusby (Feb 19 2020 at 01:09, on Zulip):

I can probably just use that

Jack Huey (Feb 19 2020 at 01:09, on Zulip):

Yep

Jack Huey (Feb 19 2020 at 01:09, on Zulip):

Good so far!

Jane Lusby (Feb 19 2020 at 01:10, on Zulip):

Jane Lusby (Feb 19 2020 at 01:10, on Zulip):

impl<I: Interner> FoldInputTypes for AliasEq<I> {
fn fold(&self, accumulator: &mut Vec<Ty<I>>) {
TyData::Alias(self.alias.clone()).intern().fold(accumulator);
self.ty.fold(accumulator);
}
}

Jane Lusby (Feb 19 2020 at 01:10, on Zulip):

gdi

Jane Lusby (Feb 19 2020 at 01:10, on Zulip):
```impl<I: Interner> FoldInputTypes for AliasEq<I> {
    fn fold(&self, accumulator: &mut Vec<Ty<I>>) {
        TyData::Alias(self.alias.clone()).intern().fold(accumulator);
        self.ty.fold(accumulator);
    }
}
Jane Lusby (Feb 19 2020 at 01:10, on Zulip):

close enough

Jane Lusby (Feb 19 2020 at 01:10, on Zulip):

this needs an interner

Jane Lusby (Feb 19 2020 at 01:10, on Zulip):

arg on fold? getter fn on trait?

Jack Huey (Feb 19 2020 at 01:11, on Zulip):

Uh

Jane Lusby (Feb 19 2020 at 01:11, on Zulip):

I only have 2 more errors left atm

Jack Huey (Feb 19 2020 at 01:11, on Zulip):

Getter on trait probably

Jane Lusby (Feb 19 2020 at 01:11, on Zulip):

okay

Jack Huey (Feb 19 2020 at 01:12, on Zulip):

Err, maybe rags

Jack Huey (Feb 19 2020 at 01:12, on Zulip):

Arg

Jane Lusby (Feb 19 2020 at 01:13, on Zulip):

oooo shit a lot of ppl implement this trait

Jane Lusby (Feb 19 2020 at 01:13, on Zulip):

none of the other impls need an interner

Jane Lusby (Feb 19 2020 at 01:13, on Zulip):

at least with the change I've made so far

Jack Huey (Feb 19 2020 at 01:14, on Zulip):

Oof

Jane Lusby (Feb 19 2020 at 01:14, on Zulip):

just gonna do an associated fn on the type

Jane Lusby (Feb 19 2020 at 01:15, on Zulip):

knowing its wrong

Jane Lusby (Feb 19 2020 at 01:15, on Zulip):

and punt on the issue for a hot second

Jack Huey (Feb 19 2020 at 01:15, on Zulip):

Yeah, that's fine

Jane Lusby (Feb 19 2020 at 01:16, on Zulip):

ooh shit!

Jane Lusby (Feb 19 2020 at 01:16, on Zulip):

we got to the borrow checker pass

Jack Huey (Feb 19 2020 at 01:17, on Zulip):

:raised_hands:

Jane Lusby (Feb 19 2020 at 01:19, on Zulip):

okay

Jane Lusby (Feb 19 2020 at 01:19, on Zulip):
impl RustIrDatabase<ChalkIr> for ChalkDatabase {
Jane Lusby (Feb 19 2020 at 01:19, on Zulip):

I have to impl the interner getter for RustIrDatabase

Jane Lusby (Feb 19 2020 at 01:19, on Zulip):

but theres no interner type here

Jane Lusby (Feb 19 2020 at 01:19, on Zulip):

presumably ChalkDatabase will own the interner or something?

Jane Lusby (Feb 19 2020 at 01:19, on Zulip):

(not sure)

Jane Lusby (Feb 19 2020 at 01:20, on Zulip):

ChalkIr

Jack Huey (Feb 19 2020 at 01:20, on Zulip):

Probably

Jane Lusby (Feb 19 2020 at 01:21, on Zulip):
trait LowerParameterMap {
    fn synthetic_parameters(&self) -> Option<chalk_ir::ParameterKind<chalk_ir::Identifier>>;
    fn declared_parameters(&self) -> &[ParameterKind];
    fn all_parameters(&self) -> Vec<chalk_ir::ParameterKind<chalk_ir::Identifier>> {
        self.synthetic_parameters()
            .into_iter()
            .chain(self.declared_parameters().iter().map(|id| id.lower()))
            .collect()

        /* TODO: switch to this ordering, but adjust *all* the code to match

        self.declared_parameters()
            .iter()
            .map(|id| id.lower())
            .chain(self.synthetic_parameters()) // (*) see below
            .collect()
         */
    }

    fn parameter_refs(&self) -> Vec<chalk_ir::Parameter<ChalkIr>> {
        self.all_parameters()
            .anonymize()
            .iter()
            .zip(0..)
            .map(|p| p.to_parameter())
            .collect()
    }
Jane Lusby (Feb 19 2020 at 01:22, on Zulip):

parameter_refs needs an Interner

Jack Huey (Feb 19 2020 at 01:22, on Zulip):

Maybe getter on trait

Jane Lusby (Feb 19 2020 at 01:22, on Zulip):

okay

Jack Huey (Feb 19 2020 at 01:22, on Zulip):

Brb

Jane Lusby (Feb 19 2020 at 01:23, on Zulip):
trait LowerTy {
    fn lower(&self, env: &Env) -> LowerResult<chalk_ir::Ty<ChalkIr>>;
}

impl LowerTy for Ty {
    fn lower(&self, env: &Env) -> LowerResult<chalk_ir::Ty<ChalkIr>> {
        match *self {
            Ty::Id { name } => match env.lookup_type(name)? {
                TypeLookup::Struct(id) => {
                    let k = env.struct_kind(id);
                    if k.binders.len() > 0 {
                        Err(RustIrError::IncorrectNumberOfTypeParameters {
                            identifier: name,
                            expected: k.binders.len(),
                            actual: 0,
                        })
                    } else {
                        Ok(chalk_ir::TyData::Apply(chalk_ir::ApplicationTy {
                            name: chalk_ir::TypeName::Struct(id),
                            substitution: chalk_ir::Substitution::empty(),
                        })
                        .intern())
Jane Lusby (Feb 19 2020 at 01:23, on Zulip):

alrighty

Jane Lusby (Feb 19 2020 at 01:24, on Zulip):

gonna afk for a bit / work on other things

Jane Lusby (Feb 19 2020 at 01:25, on Zulip):

msg me when you're back

Jack Huey (Feb 19 2020 at 01:27, on Zulip):

Back

Jack Huey (Feb 19 2020 at 01:27, on Zulip):

@Jane Lusby

Jack Huey (Feb 19 2020 at 01:28, on Zulip):

Probably a function arg there

Jane Lusby (Feb 19 2020 at 01:54, on Zulip):

okay

Jane Lusby (Feb 19 2020 at 01:55, on Zulip):

@Jack Huey

Jane Lusby (Feb 19 2020 at 01:55, on Zulip):
impl LowerProgram for Program {
    fn lower(&self) -> LowerResult<LoweredProgram> {
Jack Huey (Feb 19 2020 at 02:20, on Zulip):

Maybe same?

Jane Lusby (Feb 19 2020 at 02:22, on Zulip):

alrighty

Jane Lusby (Feb 19 2020 at 02:24, on Zulip):

I'm gonna assume the same and just keep slapping interner args on all the lowering stuff

Jane Lusby (Feb 19 2020 at 02:24, on Zulip):

like this

Jane Lusby (Feb 19 2020 at 02:24, on Zulip):
impl LowerWhereClauseVec for [QuantifiedWhereClause] {
    fn lower(&self, env: &Env) -> LowerResult<Vec<chalk_ir::QuantifiedWhereClause<ChalkIr>>> {
        self.iter()
            .flat_map(|wc| match wc.lower(env) {
                Ok(v) => v.into_iter().map(Ok).collect(),
                Err(e) => vec![Err(e)],
            })
            .collect()
    }
}
Jack Huey (Feb 19 2020 at 02:26, on Zulip):

Yeah that's good

Jack Huey (Feb 19 2020 at 02:26, on Zulip):

Can always change it if needed

Jane Lusby (Feb 19 2020 at 02:39, on Zulip):

@Jack Huey ```rust

Jane Lusby (Feb 19 2020 at 02:39, on Zulip):
fn program_ir(db: &impl LoweringDatabase) -> Result<Arc<Program>, ChalkError> {
    let text = db.program_text();
    Ok(Arc::new(chalk_parse::parse_program(&text)?.lower(interner)?))
}
Jane Lusby (Feb 19 2020 at 02:39, on Zulip):

need an interner there

Jane Lusby (Feb 19 2020 at 02:39, on Zulip):

i initially slapped it on as an arg

Jane Lusby (Feb 19 2020 at 02:40, on Zulip):

but that propogated back up and caused lifetime issues

Jack Huey (Feb 19 2020 at 02:40, on Zulip):

Maybe on LoweringDatabase then?

Jane Lusby (Feb 19 2020 at 02:42, on Zulip):

also i had absolutely no idea this syntax was allowed

Jane Lusby (Feb 19 2020 at 02:42, on Zulip):

impling trait items as free fns

Jane Lusby (Feb 19 2020 at 02:44, on Zulip):
    fn program_ir<'a>(&self, interner: &'a ChalkIr) -> Result<Arc<Program>, ChalkError>;
Jane Lusby (Feb 19 2020 at 02:44, on Zulip):

it doesn't want to accept this

Jane Lusby (Feb 19 2020 at 02:45, on Zulip):
chalk-integration/src/query.rs|31 col 41 error 261| use of undeclared lifetime name `'a`
||    |
|| 31 |     fn program_ir<'a>(&self, interner: &'a ChalkIr) -> Result<Arc<Program>, ChalkError>;
||    |                                         ^^ undeclared lifetime
|| For more information about this error, try `rustc --explain E0261`.
Jane Lusby (Feb 19 2020 at 02:45, on Zulip):

can you not declare lifetimes on associated fns for traits

Jack Huey (Feb 19 2020 at 02:46, on Zulip):

Hmm

Jack Huey (Feb 19 2020 at 02:46, on Zulip):

Weird

Jack Huey (Feb 19 2020 at 02:46, on Zulip):

I have to get off for the night though

Jane Lusby (Feb 19 2020 at 02:46, on Zulip):

okay

Jane Lusby (Feb 19 2020 at 03:01, on Zulip):

eyyy, i got it compiling

Jane Lusby (Feb 19 2020 at 03:01, on Zulip):

with liberal usage of unimplemented!()

Jane Lusby (Feb 19 2020 at 03:01, on Zulip):

the lowering stuff is almost certainly wrong

Jane Lusby (Feb 19 2020 at 03:01, on Zulip):

i mostly yoloed it

Jane Lusby (Feb 19 2020 at 03:02, on Zulip):

and I intentionally hacked it so src/main.rs wouldn't need to know about the Interner

Jane Lusby (Feb 19 2020 at 03:04, on Zulip):

@nikomatsakis this is probably far enough along where you can do code review and leave comments rather than doing synchronous help via chat

Jane Lusby (Feb 19 2020 at 03:04, on Zulip):

https://github.com/rust-lang/chalk/pull/330/files

Jane Lusby (Feb 19 2020 at 03:06, on Zulip):

the changes in chalk-integration are the most suspect / in need of review

Jane Lusby (Feb 19 2020 at 03:06, on Zulip):

so thats probably a good place to start the code review

Jane Lusby (Feb 19 2020 at 03:07, on Zulip):

I haven't bothered making the tests compile yet

detrumi (Feb 19 2020 at 07:57, on Zulip):

In lowering.rs, you're passing both the interner and the env to most functions. Shouldn't we add the interner to Env?

Jack Huey (Feb 19 2020 at 14:28, on Zulip):

Yeah, would work well

Jane Lusby (Feb 19 2020 at 18:25, on Zulip):

k moved it into env

Jane Lusby (Feb 19 2020 at 18:53, on Zulip):

ready for more review, I left a comment on the AliasEq stuff, don't know how to move forward on that, and I have a bunch of interner() -> &I { unimplemented!() } fns that I need to deal with

Jane Lusby (Feb 19 2020 at 18:53, on Zulip):

so guidance on how to deal with those would be a good next step

Jack Huey (Feb 19 2020 at 18:57, on Zulip):

Just replied to your comment re. AliasEq. As tough as it is (since the other impls of that trait don't need an Interner), I do think providing the interner as an arg is the right choice

Jack Huey (Feb 19 2020 at 18:58, on Zulip):

As far as the unimplemented!()s, most will probably just coming from a new field on the struct

Jane Lusby (Feb 22 2020 at 00:23, on Zulip):

Most important question of all

Jane Lusby (Feb 22 2020 at 00:23, on Zulip):

who should own the interner?

Jane Lusby (Feb 22 2020 at 00:23, on Zulip):

I feel like if I know where the concrete references to the interner are coming from a lot of this will fall into place

Jane Lusby (Feb 22 2020 at 00:24, on Zulip):

btw gonna start adding interner arg to the trait that AliasEq needs it on

Jane Lusby (Feb 22 2020 at 00:41, on Zulip):

K i pushed a version that adds the interner to the FoldInputTypes trait

Jane Lusby (Feb 22 2020 at 00:41, on Zulip):

I think right now the main things left to do are start actually populating the various items I added interner() fns to with actual references to interner

nikomatsakis (Feb 24 2020 at 17:40, on Zulip):

@Jane Lusby very sorry for disappearing last week

nikomatsakis (Feb 24 2020 at 17:40, on Zulip):

let me see if I can catch up...

nikomatsakis (Feb 24 2020 at 17:40, on Zulip):

is chalk#330 the latest?

Jane Lusby (Feb 24 2020 at 17:53, on Zulip):

@nikomatsakis yea, I believe its completely up to date with the changes I've made

Jane Lusby (Feb 24 2020 at 17:53, on Zulip):

comments from above are where I left off

nikomatsakis (Feb 24 2020 at 17:53, on Zulip):

great, ok

nikomatsakis (Feb 24 2020 at 17:54, on Zulip):

I think the question of "who should own" is definitely key-- I also think that probably it should be firmly borrowed and temporary, even though that's less convenient

nikomatsakis (Feb 24 2020 at 17:54, on Zulip):

but I think it'll fit best into the salsa model

Jane Lusby (Feb 24 2020 at 17:54, on Zulip):

firmly borrowed?

nikomatsakis (Feb 24 2020 at 17:54, on Zulip):

heh, not sure what "firmly" means :)

Jane Lusby (Feb 24 2020 at 17:54, on Zulip):

yea me neither

Jane Lusby (Feb 24 2020 at 17:54, on Zulip):

lol

nikomatsakis (Feb 24 2020 at 17:55, on Zulip):

well I mean that it should be passed in as a parameter and we shouldn't be able to clone it

nikomatsakis (Feb 24 2020 at 17:55, on Zulip):

or take ownership of it

nikomatsakis (Feb 24 2020 at 17:55, on Zulip):

so I guess "strictly"

nikomatsakis (Feb 24 2020 at 17:55, on Zulip):

but let me check out the PR and see how it feels

nikomatsakis (Feb 24 2020 at 17:55, on Zulip):

this means that potentially "long lived" data structures shouldn't have an Interner in their fields

Jane Lusby (Feb 24 2020 at 17:56, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 17:56, on Zulip):

if you search for instances of interner(&self) that will give you a good idea of who currently would need to borrow it with the guesses we took at how to thread it through

Jack Huey (Feb 24 2020 at 18:20, on Zulip):

(removed)

Jack Huey (Feb 24 2020 at 18:25, on Zulip):

Yes, so the interner, I assume, would get passed in at solve

Jack Huey (Feb 24 2020 at 18:25, on Zulip):

similar to RustIrDatabase

Jane Lusby (Feb 24 2020 at 18:35, on Zulip):

so LoadedProgram should own an interner?

Jane Lusby (Feb 24 2020 at 18:35, on Zulip):

also, what about the target_interner

Jane Lusby (Feb 24 2020 at 18:36, on Zulip):

the fold trait needed two interners, not sure how the second one gets threaded through

Jack Huey (Feb 24 2020 at 18:38, on Zulip):

I would have to double check. There's two (main) places in the current code where we are "creating" an interner: the tests, and the repl

Jack Huey (Feb 24 2020 at 18:38, on Zulip):

can't say for sure about either until I look at the code, which I can't do until later

Jack Huey (Feb 24 2020 at 18:38, on Zulip):

As far as target_interner, no clue. Will have to look later

Jane Lusby (Feb 24 2020 at 18:53, on Zulip):

okay so I'm trying to thread an interner from LoadedProgram to Env in lowering.rs

Jane Lusby (Feb 24 2020 at 18:53, on Zulip):

and it requires that i add a lifetime param to Program

Jane Lusby (Feb 24 2020 at 18:53, on Zulip):

which is being difficult...

Jane Lusby (Feb 24 2020 at 18:56, on Zulip):

I think the salsa stuff might be responsible

Jane Lusby (Feb 24 2020 at 18:57, on Zulip):

but no matter what I do I get errors saying the lifetimes are undeclared

Jack Huey (Feb 24 2020 at 18:59, on Zulip):

let me look at this a bit later and I'll get back to you

Jane Lusby (Feb 24 2020 at 19:00, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 19:00, on Zulip):

i was gonna just run it through cargo expand to see what the proc macro was generating

Jane Lusby (Feb 24 2020 at 19:02, on Zulip):

I'm hoping its as simple as forgetting to parse / re-add the generic parameters when its transforming the trait definition

Jane Lusby (Feb 24 2020 at 19:02, on Zulip):

which is something I'm pretty sure I can fix

Jane Lusby (Feb 24 2020 at 19:03, on Zulip):
fn program_ir(&self, key0: &'i ChalkIr) -> Result<Arc<Program<'i>>, ChalkError>;
Jane Lusby (Feb 24 2020 at 19:33, on Zulip):

I think I fixed it in salsa-macros

Jane Lusby (Feb 24 2020 at 19:33, on Zulip):

damnit apparently not

Jane Lusby (Feb 24 2020 at 19:33, on Zulip):

fff

Jane Lusby (Feb 24 2020 at 19:34, on Zulip):

apparently chalk-integration and chalk both have salsa deps

Jane Lusby (Feb 24 2020 at 19:34, on Zulip):

had to update path

Jane Lusby (Feb 24 2020 at 19:34, on Zulip):

pls work pls work pls work

Jane Lusby (Feb 24 2020 at 19:35, on Zulip):

wooo progress, custom attribute panicked :)

Jane Lusby (Feb 24 2020 at 19:37, on Zulip):

aaah shit, chalk depends on an 0.10.0 version of salsa-macros

Jane Lusby (Feb 24 2020 at 19:37, on Zulip):

and the volatile attribute was removed in 0.14 i guess

Jane Lusby (Feb 24 2020 at 19:37, on Zulip):

so this isnt even my fault

Jane Lusby (Feb 24 2020 at 19:37, on Zulip):

lol

Jane Lusby (Feb 24 2020 at 19:41, on Zulip):

aaaand now rand is fucking me over

Jane Lusby (Feb 24 2020 at 19:41, on Zulip):

fantastic

nikomatsakis (Feb 24 2020 at 19:43, on Zulip):

Jack Huey said:

similar to RustIrDatabase

if it helps, I think the interner would probably be the RustIrData in many cases -- or a wrapper around it

nikomatsakis (Feb 24 2020 at 19:43, on Zulip):

Jane Lusby said:

and it requires that i add a lifetime param to Program

I think this is probably going in the wrong direction

nikomatsakis (Feb 24 2020 at 19:44, on Zulip):

let me check the PR now though, I was afk for a bit

Jane Lusby (Feb 24 2020 at 19:44, on Zulip):

awe fk this generates a lot more code than I thought

Jane Lusby (Feb 24 2020 at 19:45, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 19:45, on Zulip):

i shall stop cargo culting and wait for guidance

Jane Lusby (Feb 24 2020 at 19:45, on Zulip):

:)

nikomatsakis (Feb 24 2020 at 19:54, on Zulip):

@Jane Lusby ok so the branch in your PR .. builds?

nikomatsakis (Feb 24 2020 at 19:55, on Zulip):

with cargo check --all at least

Jane Lusby (Feb 24 2020 at 19:55, on Zulip):

does it right now?

Jane Lusby (Feb 24 2020 at 19:55, on Zulip):

eyy cool

Jane Lusby (Feb 24 2020 at 19:55, on Zulip):

yea

nikomatsakis (Feb 24 2020 at 19:55, on Zulip):

is the question then "did I do it right"

Jane Lusby (Feb 24 2020 at 19:55, on Zulip):

thats mostly thanks to creative use of unimplemented!()

nikomatsakis (Feb 24 2020 at 19:55, on Zulip):

or "egads this feels so messy in part X"

nikomatsakis (Feb 24 2020 at 19:55, on Zulip):

ok :)

nikomatsakis (Feb 24 2020 at 19:56, on Zulip):

I see

nikomatsakis (Feb 24 2020 at 19:56, on Zulip):

I'm going to poke around a bit

Jane Lusby (Feb 24 2020 at 19:56, on Zulip):
impl<'k> Env<'k> {
    fn interner(&self) -> &ChalkIr {
        unimplemented!()
    }
}
Jane Lusby (Feb 24 2020 at 19:56, on Zulip):

probably a good place to start poking

nikomatsakis (Feb 24 2020 at 19:58, on Zulip):

I'm removing GOal::interner

nikomatsakis (Feb 24 2020 at 19:58, on Zulip):

which seems wrong

Jane Lusby (Feb 24 2020 at 19:58, on Zulip):

ok

nikomatsakis (Feb 24 2020 at 19:58, on Zulip):

(side note, I do wonder if we should trait RustIrDatabase: Interner, but I'm going to leave it as is for now)

nikomatsakis (Feb 24 2020 at 20:00, on Zulip):

to explain why it seems wrong: Goals are a "data structure" that gets allocated in the interner, they don't own the interner

nikomatsakis (Feb 24 2020 at 20:01, on Zulip):

same is probably true of Env<'k> =)

nikomatsakis (Feb 24 2020 at 20:01, on Zulip):

oh, no, that's just used during lowering

Jane Lusby (Feb 24 2020 at 20:01, on Zulip):

i see i see

nikomatsakis (Feb 24 2020 at 20:01, on Zulip):

I guess that for now we can change ChalkIr to just struct ChalkIr;

nikomatsakis (Feb 24 2020 at 20:02, on Zulip):

and return this global constant

nikomatsakis (Feb 24 2020 at 20:09, on Zulip):

@Jane Lusby I'm debating the best way to give feedback here :)

nikomatsakis (Feb 24 2020 at 20:09, on Zulip):

I guess I can leave comments on the PR

Jane Lusby (Feb 24 2020 at 20:09, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 20:09, on Zulip):

im fiddling with the interner around goal

nikomatsakis (Feb 24 2020 at 20:10, on Zulip):

actually

nikomatsakis (Feb 24 2020 at 20:10, on Zulip):

what I think I will do is push some commits with // NDM containing a few notes

Jane Lusby (Feb 24 2020 at 20:12, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 20:20, on Zulip):

so I see the changes you made, now we have interners all throughout chalk integration it seems

nikomatsakis (Feb 24 2020 at 20:20, on Zulip):

OK, I just pushed a commit with a bunch of comments

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

one for each unimplemented!

Jane Lusby (Feb 24 2020 at 20:21, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 20:21, on Zulip):

looking for that

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

the basic pattern is this

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

there are structs that correspond to "some operation"

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

e.g., Canonicalization and Truncation

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

those structs should store a &I and implement Folder

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

but they will often need to get it from an additional argument on the method that constructs the struct

nikomatsakis (Feb 24 2020 at 20:21, on Zulip):

(which will propagate, but hopefully not too far...)

nikomatsakis (Feb 24 2020 at 20:22, on Zulip):

there are a few other cases, like Substitution<I>, where we were trying to get away with just having one struct that represents part of the IR and implements TypeFolder. That doesn't work I guess.

nikomatsakis (Feb 24 2020 at 20:22, on Zulip):

The alternative would be to modify the fold_with methods to thread the interner down

nikomatsakis (Feb 24 2020 at 20:22, on Zulip):

instead of having a fn interner(&self) method in TypeFolder

nikomatsakis (Feb 24 2020 at 20:22, on Zulip):

but I think it's probably better this way

Jane Lusby (Feb 24 2020 at 20:25, on Zulip):

okay

Jane Lusby (Feb 24 2020 at 20:25, on Zulip):

so when you say which will propogate

Jane Lusby (Feb 24 2020 at 20:26, on Zulip):

you mean just add interner args onto fns that call fns like shift_in until you get to some place where you can access a concrete interner

Jane Lusby (Feb 24 2020 at 20:26, on Zulip):

either as an arg thats already passed or from the db or something

Jane Lusby (Feb 24 2020 at 20:27, on Zulip):

or another folder that has its own self.interner() method

nikomatsakis (Feb 24 2020 at 20:28, on Zulip):

yeah

nikomatsakis (Feb 24 2020 at 20:28, on Zulip):

basically

nikomatsakis (Feb 24 2020 at 20:28, on Zulip):

I would do them one at a time :)

nikomatsakis (Feb 24 2020 at 20:29, on Zulip):

I've not read in depth apart from searching for unimplemented!, though my spot checks all seemed to look roughly like what I expect

Jane Lusby (Feb 24 2020 at 20:30, on Zulip):
impl<I: Interner> Ty<I> {
...
    pub fn needs_shift(&self) -> bool {
        let ty = self.clone();
        ty != ty.shifted_in(1)
    }
Jane Lusby (Feb 24 2020 at 20:30, on Zulip):

looks like I need to add an arg here

nikomatsakis (Feb 24 2020 at 20:33, on Zulip):

another case where a visitor would be nice, @Jack Huey =)

nikomatsakis (Feb 24 2020 at 20:33, on Zulip):

Jane Lusby said:

looks like I need to add an arg here

yes

nikomatsakis (Feb 24 2020 at 20:33, on Zulip):

though that's quite annoying

nikomatsakis (Feb 24 2020 at 20:33, on Zulip):

I mean mostly because we shouldn't need to do any interning here

nikomatsakis (Feb 24 2020 at 20:33, on Zulip):

I don't think it's called in a ton of places..?

Jane Lusby (Feb 24 2020 at 20:36, on Zulip):

I'm still working on the first unimplemented call

Jane Lusby (Feb 24 2020 at 20:36, on Zulip):

lol

Jane Lusby (Feb 24 2020 at 20:36, on Zulip):

its spreading a lot

Jane Lusby (Feb 24 2020 at 20:36, on Zulip):

its not bad

Jane Lusby (Feb 24 2020 at 20:36, on Zulip):

like not deeply

Jane Lusby (Feb 24 2020 at 20:36, on Zulip):

but it seems like this touches a lot of places, thankfully most already have interners threaded to them

Jane Lusby (Feb 24 2020 at 20:37, on Zulip):
impl<I: Interner> Zipper<I> for AnswerSubstitutor<'_, I> {
    fn zip_tys(&mut self, answer: &Ty<I>, pending: &Ty<I>) -> Fallible<()> {
        if let Some(pending) = self.table.normalize_shallow(self.interner, pending) {
            return Zip::zip_with(self, answer, &pending);
        }
Jane Lusby (Feb 24 2020 at 20:37, on Zulip):

this one doesn't though

Jane Lusby (Feb 24 2020 at 20:38, on Zulip):

also this one is a conflicting mut and shared borrow

Jane Lusby (Feb 24 2020 at 20:38, on Zulip):
    fn fold_ty(&mut self, ty: &Ty<I>, binders: usize) -> Fallible<Ty<I>> {
        if let Some(normalized_ty) = self.infer.normalize_shallow(self.interner(), ty) {
            return self.fold_ty(&normalized_ty, binders);
        }
nikomatsakis (Feb 24 2020 at 21:36, on Zulip):

@Jane Lusby hmm

nikomatsakis (Feb 24 2020 at 21:36, on Zulip):

probably self.interner would work fine there?

nikomatsakis (Feb 24 2020 at 21:36, on Zulip):

i.e., the problem is using the method vs the field

Jane Lusby (Feb 24 2020 at 23:13, on Zulip):

@nikomatsakis im a little confused by how to handle the apply method for Substitution

Jane Lusby (Feb 24 2020 at 23:14, on Zulip):

I added a SubstFolder type and moved the Folder impl to that type instead of Substitution

Jane Lusby (Feb 24 2020 at 23:14, on Zulip):

and gave it a reference to the Substitution and Interner that it needs to operate

Jane Lusby (Feb 24 2020 at 23:14, on Zulip):

but I'm not sure how to use that from apply or how to use apply throughout the rest of the code

Last update: Feb 25 2020 at 04:20UTC