Replace CanonicalVar with DebruijnIndex #52984

fabric-and-ink · 2018-08-02T15:29:36Z

Close #49887

rust-highfive · 2018-08-02T15:29:48Z

r? @michaelwoerister

(rust_highfive has picked a reviewer for you, use r? to override)

michaelwoerister · 2018-08-02T15:49:42Z

Thanks for the PR, @fabric-and-ink! This looks like something that @nikomatsakis will want to review.

r? @nikomatsakis

fabric-and-ink · 2018-08-02T16:12:15Z

The test suite will fail with a few errors that I don't understand.

Edit: Well, locally they failed... there were a few errors in the debuginfo suite... strange.

fabric-and-ink · 2018-08-02T17:11:17Z

I don't follow... CanonicalVar is being removed.

varkor · 2018-08-02T18:14:50Z

Sorry, I was thinking of CanonicalVarValues along with CanonicalVar. This looks good to me.

fabric-and-ink · 2018-08-03T16:30:55Z

Ok. @nikomatsakis probably has a plan about CanonicalVarValues et.al.

nikomatsakis

See below for some thoughts. Trying to decide whether to merge as is or not.

nikomatsakis · 2018-08-07T12:05:38Z

src/librustc/infer/canonical/canonicalizer.rs

@@ -385,7 +385,7 @@ impl<'cx, 'gcx, 'tcx> Canonicalizer<'cx, 'gcx, 'tcx> {
            // direct linear search of `var_values`.
            if let Some(idx) = var_values.iter().position(|&k| k == kind) {
                // `kind` is already present in `var_values`.
-                CanonicalVar::new(idx)
+                DebruijnIndex::new(idx)


Hmm, this doesn't seem right. To really do this properly, we have to track the number of binders and adjust the debruijn index accordingly.

So, for example, if we are canonicalizing something like this:

for<'a> fn(&'a _)

then there is one binder in scope (from the fn) and hence I would expect the debruijn index to be adusted.

OTOH, I suppose that — at present — these canonical variables effectively occupy a "separate universe" from late-bound regions and things, and hence it doesn't make sense to adjust for the number of binders in scope. I think that's why this code all works fine.

Hmm, so maybe the PR is right "as is", at least as a stepping stone to the final intended setup.

Ok. Let me know when you have made a decision. I suppose a gradual but not strictly correct approach wouldn't be too bad...

For a correct solution I would probably need a bit of mentoring, since I am not sure if I understand the problem completely.

nikomatsakis · 2018-08-10T21:17:40Z

I had a brief discussion with @scalexm today and we got to thinking about how best to integrate this stuff. Our thought was that we should probably try to "go with the flow" --

Use debruijn indices to represent "binding levels" and not individual variables, as the compiler already does
Introduce some kind of TyBound(DebruijnIndex, BoundTyIndex) that represents a "bound type"
- It would use the debrujin index to select binding level, and the usize to distinguish different types at that same level

Canonical variables would thus change from TyCanonical to TyBound -- the CanonicalVar would change to BoundTyIndex. The idea would be that canonical things are things bound at a particular binding level.

fabric-and-ink · 2018-08-14T15:04:35Z

Ok!

bors · 2018-08-22T20:07:01Z

☔ The latest upstream changes (presumably #53581) made this pull request unmergeable. Please resolve the merge conflicts.

TimNN · 2018-09-04T17:16:32Z

Ping from triage @fabric-and-ink: What are your plans for this PR? It looks like some change have been requested and this PR needs a rebase.

fabric-and-ink · 2018-09-04T19:04:58Z

Didn't forget about it. There have been some holidays and lots of other stuff. I will continue to work on it hopefully next weekend.

fabric-and-ink · 2018-09-08T17:13:44Z

@nikomatsakis You mentioned that I should rename CanonicalVar to BoundTyIndex. There are several closely related types to CanonicalVar:

CanonicalVarInfo and CanonicalVarInfos
CanonicalVarKind
CanonicalVarValues
SmallCanonicalVarValues

I would suggest to rename them to BoundTy.... What do you think?

Edit: Talked to Niko, will do that.

bors · 2018-09-26T20:52:25Z

☔ The latest upstream changes (presumably #54453) made this pull request unmergeable. Please resolve the merge conflicts.

fabric-and-ink · 2018-09-30T16:10:23Z

I replaced CanonicalTy(...) with BoundTy(BoundTy) where BoundTy { level: DebruijnIndex, var: BoundTyIndex }. Since the BoundTyIndex is calculated here (called from here) this calculation now has to produce a DebruijnIndex along with a BoundTyIndex. How would I determine its value in this function? Am I heading in the right direction?

bors · 2018-10-01T13:00:59Z

☔ The latest upstream changes (presumably #54711) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2018-10-02T20:33:54Z

@fabric-and-ink I think this is probably in the right direction but I've been kind of busy with other things. I think I'm going to hand off to @scalexm

r? @scalexm

scalexm · 2018-10-09T14:34:29Z

@fabric-and-ink I think we can start with the assumption that types live in a different universe than lifetimes -- as @nikomatsakis wrote in a previous comment -- and since we don't support binding over types yet, you can just use 0 for the DebruijnIndex.

fabric-and-ink · 2018-10-14T17:35:29Z

@scalexm I changed CanonicalTy to BoundTy and adapted fn canonical_var(...) accordingly. Changes in this area seem to be very far-reaching and I am not sure where to stop. (And I need to catch up to upstream regularly.)

bors · 2018-10-15T20:03:13Z

☔ The latest upstream changes (presumably #54858) made this pull request unmergeable. Please resolve the merge conflicts.

scalexm

@fabric-and-ink This seems good, I just find it weird that we use BoundTyInfo / BoundTyKind to describe something that could be a type or a region.

Last thing, could you add some assertions like debug_assert_eq!(ty::INNERMOST, b.level) everywhere a BoundTy is effectively used? So that it'll be harder to forget about changing the logic when we move to non-zero binding levels.

fabric-and-ink · 2018-10-16T09:51:30Z

@scalexm Your right, I didn't think of that. Do you have a suggestion for a better name? Should I revert it to the old name?

scalexm · 2018-10-16T09:53:28Z

@fabric-and-ink Yes I'd say just change the name for things that only relate to types. I don't think that leaving some things named Canonical... is a problem, especially if they are defined in the canonical module :)

fabric-and-ink · 2018-10-16T10:00:48Z

Ok :)

…nt type

fabric-and-ink · 2018-10-20T17:18:00Z

@scalexm I updated my PR. Please have a look and let me know if something is missing. 🙂 (I left out the renaming of the bitflag. Wasn't sure about that.)

scalexm · 2018-10-21T13:36:00Z

@fabric-and-ink Ok it seems good enough like that. We’ll eventually get rid of ReCanonical as well, so I guess we’ll rename the bitflag at that moment then.

scalexm · 2018-10-21T13:37:18Z

@bors r+

bors · 2018-10-21T13:37:18Z

📌 Commit 2f41c0d has been approved by scalexm

bors · 2018-10-21T14:42:41Z

⌛ Testing commit 2f41c0d with merge 0e2f912...

Replace CanonicalVar with DebruijnIndex Close #49887

bors · 2018-10-21T17:26:10Z

☀️ Test successful - status-appveyor, status-travis
Approved by: scalexm
Pushing 0e2f912 to master...

rust-highfive assigned michaelwoerister Aug 2, 2018

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 2, 2018

rust-highfive assigned nikomatsakis and unassigned michaelwoerister Aug 2, 2018

This comment has been minimized.

Sign in to view

nikomatsakis reviewed Aug 7, 2018

View reviewed changes

pietroalbini added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 27, 2018

fabric-and-ink changed the title ~~Replace CanonicalVar with DebruijnIndex~~ WIP: Replace CanonicalVar with DebruijnIndex Sep 8, 2018

rust-highfive assigned scalexm and unassigned nikomatsakis Oct 2, 2018

fabric-and-ink changed the title ~~WIP: Replace CanonicalVar with DebruijnIndex~~ Replace CanonicalVar with DebruijnIndex Oct 14, 2018

scalexm reviewed Oct 16, 2018

View reviewed changes

fabric-and-ink added 2 commits October 20, 2018 18:39

Rename CanonicalVar to BoundTyIndex

2581bdc

Rename InferTy::CanonicalTy to BoundTy and add DebruijnIndex to varia…

2f41c0d

…nt type

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Oct 21, 2018

bors added a commit that referenced this pull request Oct 21, 2018

Auto merge of #52984 - fabric-and-ink:remove-canonical-var, r=scalexm

0e2f912

Replace CanonicalVar with DebruijnIndex Close #49887

bors mentioned this pull request Oct 21, 2018

Give OpTy access to locals for priroda #55179

Merged

bors merged commit 2f41c0d into rust-lang:master Oct 21, 2018

bors mentioned this pull request Oct 21, 2018

enforce user annotations in closure signatures #55229

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace CanonicalVar with DebruijnIndex #52984

Replace CanonicalVar with DebruijnIndex #52984

fabric-and-ink commented Aug 2, 2018

rust-highfive commented Aug 2, 2018

michaelwoerister commented Aug 2, 2018

fabric-and-ink commented Aug 2, 2018 •

edited

Loading

This comment has been minimized.

fabric-and-ink commented Aug 2, 2018 •

edited

Loading

varkor commented Aug 2, 2018

fabric-and-ink commented Aug 3, 2018

nikomatsakis left a comment

nikomatsakis Aug 7, 2018

fabric-and-ink Aug 8, 2018

nikomatsakis commented Aug 10, 2018

fabric-and-ink commented Aug 14, 2018

bors commented Aug 22, 2018

TimNN commented Sep 4, 2018

fabric-and-ink commented Sep 4, 2018

fabric-and-ink commented Sep 8, 2018 •

edited

Loading

bors commented Sep 26, 2018

fabric-and-ink commented Sep 30, 2018 •

edited

Loading

bors commented Oct 1, 2018

nikomatsakis commented Oct 2, 2018

scalexm commented Oct 9, 2018 •

edited

Loading

fabric-and-ink commented Oct 14, 2018 •

edited

Loading

bors commented Oct 15, 2018

scalexm left a comment

fabric-and-ink commented Oct 16, 2018 •

edited

Loading

scalexm commented Oct 16, 2018

fabric-and-ink commented Oct 16, 2018

fabric-and-ink commented Oct 20, 2018 •

edited

Loading

scalexm commented Oct 21, 2018

scalexm commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

Replace CanonicalVar with DebruijnIndex #52984

Replace CanonicalVar with DebruijnIndex #52984

Conversation

fabric-and-ink commented Aug 2, 2018

rust-highfive commented Aug 2, 2018

michaelwoerister commented Aug 2, 2018

fabric-and-ink commented Aug 2, 2018 • edited Loading

This comment has been minimized.

fabric-and-ink commented Aug 2, 2018 • edited Loading

varkor commented Aug 2, 2018

fabric-and-ink commented Aug 3, 2018

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Aug 7, 2018

Choose a reason for hiding this comment

fabric-and-ink Aug 8, 2018

Choose a reason for hiding this comment

nikomatsakis commented Aug 10, 2018

fabric-and-ink commented Aug 14, 2018

bors commented Aug 22, 2018

TimNN commented Sep 4, 2018

fabric-and-ink commented Sep 4, 2018

fabric-and-ink commented Sep 8, 2018 • edited Loading

bors commented Sep 26, 2018

fabric-and-ink commented Sep 30, 2018 • edited Loading

bors commented Oct 1, 2018

nikomatsakis commented Oct 2, 2018

scalexm commented Oct 9, 2018 • edited Loading

fabric-and-ink commented Oct 14, 2018 • edited Loading

bors commented Oct 15, 2018

scalexm left a comment

Choose a reason for hiding this comment

fabric-and-ink commented Oct 16, 2018 • edited Loading

scalexm commented Oct 16, 2018

fabric-and-ink commented Oct 16, 2018

fabric-and-ink commented Oct 20, 2018 • edited Loading

scalexm commented Oct 21, 2018

scalexm commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

bors commented Oct 21, 2018

fabric-and-ink commented Aug 2, 2018 •

edited

Loading

fabric-and-ink commented Aug 2, 2018 •

edited

Loading

fabric-and-ink commented Sep 8, 2018 •

edited

Loading

fabric-and-ink commented Sep 30, 2018 •

edited

Loading

scalexm commented Oct 9, 2018 •

edited

Loading

fabric-and-ink commented Oct 14, 2018 •

edited

Loading

fabric-and-ink commented Oct 16, 2018 •

edited

Loading

fabric-and-ink commented Oct 20, 2018 •

edited

Loading