Stream: wg-traits

Topic: bound-tys


scalexm (Oct 11 2018 at 16:47, on Zulip):

@nikomatsakis I have a question, why do we need InferTy::CanonicalTy in rustc? Couldn't we just re-use InferTy::TyVar / InferTy::IntVar / InferTy::FloatVar?

nikomatsakis (Oct 11 2018 at 18:01, on Zulip):

we could, but then things are less robust

nikomatsakis (Oct 11 2018 at 18:01, on Zulip):

I would rather use BoundVar

nikomatsakis (Oct 11 2018 at 18:01, on Zulip):

and keep TyVar to represent free types

nikomatsakis (Oct 11 2018 at 18:01, on Zulip):

(which reminds me, I have to get back to that chalk PR)

nikomatsakis (Oct 11 2018 at 18:01, on Zulip):

(in particular, if we move to bind-var, we should be able to share the substitution machinery etc)

scalexm (Oct 11 2018 at 18:29, on Zulip):

Ok, I'm currently trying to move to BoundTy in rustc

scalexm (Oct 11 2018 at 18:29, on Zulip):

my current definition is the following:

pub enum TyKind {
    ...
    Bound(DebruijnIndex, BoundTy),
}

pub struct BoundTy {
    pub idx: u32,
    pub kind: BoundTyKind,
}

pub enum BoundTyKind {
    Anon,
    Param(InternedString),
}
scalexm (Oct 11 2018 at 18:30, on Zulip):

@nikomatsakis what do you think of it?

nikomatsakis (Oct 11 2018 at 18:32, on Zulip):

the role of BoundTyKind is basically for printouts?

nikomatsakis (Oct 11 2018 at 18:33, on Zulip):

seems good, anyway

scalexm (Oct 11 2018 at 18:35, on Zulip):

@nikomatsakis yes it's for printouts

scalexm (Oct 12 2018 at 11:20, on Zulip):

@nikomatsakis I'm having trouble figuring out how we will unify bound tys and regions in SubstFolder

scalexm (Oct 12 2018 at 11:20, on Zulip):

as far as I understand, SubstFolder only substitutes early bound regions

scalexm (Oct 12 2018 at 11:21, on Zulip):

however, we cannot use early bound regions to express bound regions in a "general way", we rather need to use late bound regions ... which are substituted through another routine

nikomatsakis (Oct 15 2018 at 13:10, on Zulip):

@scalexm yes. I was imagining that we would treat "bound tys" as "late-bound" and use those other routines to do the substitution

nikomatsakis (Oct 15 2018 at 13:10, on Zulip):

is there a time when we have to "mix"?

scalexm (Oct 15 2018 at 13:12, on Zulip):

@nikomatsakis ok then in that case, I don't think so

scalexm (Oct 16 2018 at 09:31, on Zulip):

@nikomatsakis do we want the BoundTy variant in enum TyKind or rather in enum InferTy?

scalexm (Oct 16 2018 at 09:31, on Zulip):

also if we change CanonicalTy to BoundTy, what about ReCanonical? Do we still want that region variant? Couldn't we use ReLateBound?

nikomatsakis (Oct 16 2018 at 09:43, on Zulip):

I think it should go in TyKind probably, and I also think that ReCanonical can go away, yes

scalexm (Oct 23 2018 at 18:02, on Zulip):

@nikomatsakis what should I do about the HAS_CANONICAL_VARS flag? Can I just remove it?

nikomatsakis (Oct 23 2018 at 18:24, on Zulip):

@scalexm I suppose so

scalexm (Oct 23 2018 at 21:01, on Zulip):

@nikomatsakis I think I'm seeing the end, but now that I moved from ReCanonical to ReLateBound I hit this assertion: https://github.com/rust-lang/rust/blob/master/src/librustc/infer/region_constraints/mod.rs#L692-L697 (called from InferCtxt::make_query_response)

scalexm (Oct 23 2018 at 21:02, on Zulip):

I did not try to understand what is happening in that function yet

nikomatsakis (Oct 24 2018 at 10:00, on Zulip):

I'll take a look in a bit :)

nikomatsakis (Oct 24 2018 at 10:02, on Zulip):

@scalexm can you point me at your diffs and/or open a WIP PR?

nikomatsakis (Oct 24 2018 at 10:02, on Zulip):

sounds like you are missing a substitution or something somewhere

scalexm (Oct 24 2018 at 10:07, on Zulip):

@nikomatsakis yes that's what I'm thinking too

scalexm (Oct 24 2018 at 10:08, on Zulip):

this is the full diff: https://github.com/rust-lang/rust/compare/master...scalexm:bound-ty

scalexm (Oct 24 2018 at 10:08, on Zulip):

but the problem is with the last commit: https://github.com/rust-lang/rust/commit/8051a2be5f9a9190974cbebe877672b6081cf7ce (which tries to remove ReCanonical)

scalexm (Oct 24 2018 at 10:19, on Zulip):

It may be the canonical substitution that is wrong (I just tweaked the original one, but eventually I’d like to extend ty::fold::RegionReplacer to replace both bound regions and types, and use that for canonical substitution as well)

nikomatsakis (Oct 24 2018 at 16:26, on Zulip):

oh @scalexm I forgot to look at this

nikomatsakis (Oct 24 2018 at 16:26, on Zulip):

did you figure it out?

nikomatsakis (Oct 24 2018 at 16:26, on Zulip):

also, it so happens that the code I wrote now is trying to canonicalize a value that contains a canonical vaule

nikomatsakis (Oct 24 2018 at 16:26, on Zulip):

annoying that your branch has not already landed :P

scalexm (Oct 24 2018 at 16:26, on Zulip):

@nikomatsakis not yet, I'm still trying to debug

nikomatsakis (Oct 24 2018 at 16:27, on Zulip):

ok if you want I can start up a local build and try to trace it

nikomatsakis (Oct 24 2018 at 16:27, on Zulip):

the commit is big enough it's hard for me to "just see" :)

scalexm (Oct 24 2018 at 16:31, on Zulip):

@nikomatsakis yes that might be helpful

nikomatsakis (Oct 24 2018 at 16:32, on Zulip):

ok starting

nikomatsakis (Oct 24 2018 at 16:32, on Zulip):

er, where is the branch?

nikomatsakis (Oct 24 2018 at 16:32, on Zulip):

ah, bound-ty on your repo

scalexm (Oct 24 2018 at 16:32, on Zulip):

yep

scalexm (Oct 24 2018 at 17:32, on Zulip):

@nikomatsakis oh ok I think I found something

scalexm (Oct 24 2018 at 17:33, on Zulip):

https://github.com/scalexm/rust/blob/8051a2be5f9a9190974cbebe877672b6081cf7ce/src/librustc/infer/canonical/query_response.rs#L502-L504

scalexm (Oct 24 2018 at 17:33, on Zulip):

the skip_binder is wrong, because "canonical" late bound regions are adjusted with the binder

nikomatsakis (Oct 24 2018 at 18:00, on Zulip):

@scalexm ah

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

indeed, should be not so hard to fix I wouldn't think tho

nikomatsakis (Oct 24 2018 at 18:10, on Zulip):

so I now see the failure, but maybe I won't look any further @scalexm ?

scalexm (Oct 24 2018 at 18:19, on Zulip):

@nikomatsakis the build with the fix is still not finished yet but I think this is good yes

nikomatsakis (Oct 24 2018 at 18:21, on Zulip):

nice

scalexm (Oct 24 2018 at 20:04, on Zulip):

@nikomatsakis is it fine to rename Binder::no_late_bound_regions to Binder::no_escaping_vars?

scalexm (Oct 24 2018 at 20:06, on Zulip):

or no_escaping_bound_vars

nikomatsakis (Oct 24 2018 at 20:06, on Zulip):

seems fine

Alexander Regueiro (Oct 24 2018 at 22:17, on Zulip):

hey @nikomatsakis. you round?

scalexm (Oct 24 2018 at 22:38, on Zulip):

@nikomatsakis PR up: https://github.com/rust-lang/rust/pull/55330

scalexm (Oct 24 2018 at 22:40, on Zulip):

I have a test failing locally: src/test/run-pass/rustc-rust-log.rs, but it's weird because compiletest tells me that it encountered an explicit panic, whereas when I run RUST_LOG=debug rustc src/test/run-pass/rustc-rust-log.rs with my stage 1 compiler, everything is fine

nikomatsakis (Oct 25 2018 at 13:14, on Zulip):

that is weird; sometimes it's due to // compile-flags, maybe?

nikomatsakis (Oct 25 2018 at 13:15, on Zulip):

hmm maybe not

nikomatsakis (Oct 25 2018 at 13:15, on Zulip):

I think you can use -vv or something to see the exact command being executed

nikomatsakis (Oct 25 2018 at 13:15, on Zulip):

@scalexm I left a review of your PR

scalexm (Oct 25 2018 at 14:34, on Zulip):

@nikomatsakis Ok fixed

scalexm (Oct 25 2018 at 14:35, on Zulip):

did you have a look at the perf run: https://perf.rust-lang.org/compare.html?start=f99911a4a0bead7dd1f9ef2f90442844434cc391&end=2b86a36d9530d0a26855c5a8ec6759e21fce2efc ? I'm not sure how to interpret it

scalexm (Oct 25 2018 at 14:47, on Zulip):

I was just worried about degrading the perfs of canonical substitution because we lose the inlining of the var replacement machinery, now that this is done through the fld_t and fld_r function pointers

nikomatsakis (Oct 25 2018 at 15:35, on Zulip):

did you have a look at the perf run: https://perf.rust-lang.org/compare.html?start=f99911a4a0bead7dd1f9ef2f90442844434cc391&end=2b86a36d9530d0a26855c5a8ec6759e21fce2efc ? I'm not sure how to interpret it

yes, it looked like "no difference" to me

scalexm (Oct 30 2018 at 17:28, on Zulip):

@nikomatsakis question abound bound vars in the environment query (same as param_env but for chalk)

scalexm (Oct 30 2018 at 17:29, on Zulip):

currently the result of this query contains type params / early bound regions coming from the generics of the def_id

scalexm (Oct 30 2018 at 17:29, on Zulip):

should I replace them with bound vars instead?

scalexm (Oct 30 2018 at 17:30, on Zulip):

I guess that when we call this query, we next replace these bound vars with placeholder types/regions corresponding to the universally instantiated params

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

good question, yes

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

@scalexm I think I expected them to just stay roughly as they are to start; they are effectively placeholders, but in U0, so we can just treat them like any other nominal type I think? I guess this is not true around negative queries though

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

that said, I would like to move to a place where we model them as bound-types

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

in a uniform way

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

ah well — I see that we would want to replace them with bound types when creating (e.g.) program clauses

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

but I guess that's somewhat separate

scalexm (Oct 30 2018 at 18:26, on Zulip):

@nikomatsakis yes so currently I replaced every type parameters in program clauses with bound vars

scalexm (Oct 30 2018 at 18:27, on Zulip):

(Bound at index 0 with the Clause::ForAll variant)

scalexm (Oct 30 2018 at 18:27, on Zulip):

So I’m going to do the same with environment, which will now return a ty::Binder<Environment>

scalexm (Oct 30 2018 at 18:29, on Zulip):

Also now thanks to bound types I can also use them as « anonymous » types to express some built-in program clauses that were not writable before

scalexm (Oct 30 2018 at 18:29, on Zulip):

e.g. forall<‘a, T> { T: ‘a :- FromEnv(&’a T) }

nikomatsakis (Oct 30 2018 at 18:33, on Zulip):

ok

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

oh, @scalexm, not urgent, but it occurs to me that since we can now represent bound-types, we could probably cleanup some of the trait object code, so that we can e.g. use a BoundTy to represent the "unknown self type"

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

basically a exists<T> {...} binder

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

@nikomatsakis ok

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

Btw you told me once that placeholder types would be useful for the arbitrary self type RFC

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

More info on that?

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

Because I’m going to add placeholder types on top of your universes refactor 3, so that I can implement most of the chalk engine routines

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

Ah, well, yes

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

The PR in question has not landed though I hope it will soon

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

I haven't heard from the author, @mikeyhew, lately though

scalexm (Nov 01 2018 at 18:05, on Zulip):

@nikomatsakis btw niko do you think we could maybe set a priority for https://github.com/rust-lang/rust/pull/55305 and https://github.com/rust-lang/rust/pull/55330 (once rebased on top of the former)?

scalexm (Nov 01 2018 at 18:05, on Zulip):

I've started to write code that depends on both already, but I'm not very at ease with git and rebasing on top of multiple branches

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

seems reasonable, yes..

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

I gave 55305 p=1, @scalexm

mikeyhew (Nov 01 2018 at 22:03, on Zulip):

@nikomatsakis I replied to your comment yesterday... we are talking about https://github.com/rust-lang/rust/pull/54383, right?

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

@mikeyhew so you did :)

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

I was talking about that, yes. @scalexm has added proper support for bound-types into rustc -- at least, I think it landed? I sort of forget. I don't think we're quite at that point where we can use them for that PR yet, but it's a prime example where we would want to.

scalexm (Nov 02 2018 at 14:57, on Zulip):

quick question @nikomatsakis , if you are in this else branch: https://github.com/rust-lang/rust/blob/master/src/librustc/infer/canonical/canonicalizer.rs#L400-L402

scalexm (Nov 02 2018 at 14:57, on Zulip):

won't you miss some placeholder regions? I see that CanonicalizeQueryResponse canonicalizes placeholder regions, but has self.any() == false

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

@nikomatsakis create_substs_for_ast_trait_ref is probalby what I want to edit then... but not sure how

mikeyhew (Nov 02 2018 at 17:15, on Zulip):

@nikomatsakis ok cool, I look forward to using them. For the record, the only reason we need the U type is because using dyn Trait directly causes query cycles

mikeyhew (Nov 02 2018 at 17:24, on Zulip):

Does that mean that ExistentialPredicate would be replaced with Binder<Predicate> or something like that?

mikeyhew (Nov 02 2018 at 17:26, on Zulip):

Sorry that was a reply to this comment, looks like the "reply" button isn't very useful

oh, @scalexm, not urgent, but it occurs to me that since we can now represent bound-types, we could probably cleanup some of the trait object code, so that we can e.g. use a BoundTy to represent the "unknown self type"

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

@mikeyhew yes I guess so

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

won't you miss some placeholder regions? I see that CanonicalizeQueryResponse canonicalizes placeholder regions, but has self.any() == false

@scalexm it's quite possible that code is now out of sync :(

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

@nikomatsakis ok I just added HAS_RE_PLACEHOLDER in needs_canonical_flags

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

yes, seems good

scalexm (Nov 02 2018 at 18:24, on Zulip):

also, is universe canonicalization even a thing in rustc? Since we canonicalize everything to bound vars

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

@scalexm at present we are not canonicalizing universes, but I plan to do it at some point, the code is mostly set up for it

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

well in particular we are not "compressing" unused universes

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

(or renumbering)

scalexm (Nov 02 2018 at 18:33, on Zulip):

@nikomatsakis mmh ok, but I'm not sure where in the code would we be doing that, if I look in chalk this basically happens in two steps: first canonicalizing replaces inference variables with canonical bound vars and leaves placeholder variables untouched, second u-canonicalizing renumbers placeholder variables

scalexm (Nov 02 2018 at 18:33, on Zulip):

anyway this is something that can wait

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

@scalexm yeah just ignore it for now :P I think we would just do it in the canoncialize_query call though (it's not something you want to do for canonicalize_response)

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

I don't think the chalk setup is ideal really

scalexm (Nov 02 2018 at 21:02, on Zulip):

@nikomatsakis I see that rustc's inference context tracks universes for type variables already, and I'm going to track them in canonicalization as well, but the inference table only tracks universes for unresolved type variables

scalexm (Nov 02 2018 at 21:03, on Zulip):

so I have two possible strategies: either make the inference context track universes for all variables (by e.g. adding a field to TypeVariableData), either try to resolve the variable during canonicalization (if it fails we know that the inference context gives the universe, if it succeeds replace the inference variable with the resolved value instead)

scalexm (Nov 02 2018 at 21:16, on Zulip):

well anyway I'll go for the new field in TypeVariableData, seems simpler

scalexm (Nov 02 2018 at 21:24, on Zulip):

ah no that wouldn't work since the universe indices change during unification

scalexm (Nov 02 2018 at 21:24, on Zulip):

ok so I'll go for the second strategy then lol

Last update: Nov 12 2019 at 15:50UTC