Refactor DebruijnIndex to be 0-based #50475

csmoe · 2018-05-06T04:22:53Z

Fixes #49813

rust-highfive · 2018-05-06T04:22:56Z

r? @estebank

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

rust-highfive · 2018-05-06T04:22:57Z

Warning

These commits modify submodules.

csmoe · 2018-05-06T04:24:36Z

r? @nikomatsakis

nikomatsakis · 2018-05-07T18:56:40Z

src/librustc/middle/resolve_lifetime.rs

@@ -1866,22 +1866,22 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
                trait_ref: &hir::PolyTraitRef,
                modifier: hir::TraitBoundModifier,
            ) {
-                self.binder_depth += 1;
+                self.outer_index.shifted_in(1);


Probably this should be shift_in

nikomatsakis · 2018-05-07T18:56:58Z

src/librustc/middle/resolve_lifetime.rs

            }

            fn visit_lifetime(&mut self, lifetime_ref: &hir::Lifetime) {
                if let Some(&lifetime) = self.map.defs.get(&lifetime_ref.id) {
                    match lifetime {
                        Region::LateBound(debruijn, _, _) | Region::LateBoundAnon(debruijn, _)
-                            if debruijn.index < self.binder_depth =>
+                            if debruijn.index < self.outer_index.index =>


Can we make this debruijn < self.outer_index?

nikomatsakis · 2018-05-07T19:00:16Z

Another request: Add the #[must_use] attribute to shifted_in and shifted_out

bors · 2018-05-10T12:51:36Z

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

csmoe · 2018-05-12T01:30:13Z

src/test/ui/issue-26638.stderr

-   |
-   = help: this function's return type contains a borrowed value with an elided lifetime, but the lifetime cannot be derived from the arguments
-   = help: consider giving it an explicit bounded or 'static lifetime
-


@nikomatsakis what's going wrong here? error checking is skipped for this function.

nikomatsakis · 2018-05-14T16:37:46Z

src/librustc/middle/resolve_lifetime.rs

                        {
                            self.have_bound_regions = true;
                        }
                        _ => {
                            self.lifetimes
-                                .insert(lifetime.from_depth(self.binder_depth));


OK, I suspect this is where the bug is coming from. The binder_depth in the "before code" is equal to index, but (if you look at the definition of from_depth) you will see that used to be computing adjusting by depth - 1, basically.

I think the way I would want to compute the "depth" by giving two debruijn indices. In particular, we might do something like self.outer_index.depth_from(INNERMOST), where we have:

impl DebruijnIndex { /// Assuming that these two debruijn indices are both in scope, /// returns the number of binding levels *between* them. /// This is often used if `outer` represents some starting binding /// and `self` represents the current innermost binding; it would then /// compute the number of bindings passed over so far. /// /// As the name suggests, `outer` is assumed to be the /// outermost depth of the two. fn depth_from(self, outer: DebruijnIndex) -> usize { assert!(self.index >= other.index); // or return the absolute value self.index - other.index } }

bors · 2018-05-16T01:46:58Z

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

eddyb · 2018-05-16T05:32:32Z

src/librustc/ty/sty.rs

@@ -990,7 +990,7 @@ impl<'a, 'gcx, 'tcx> ParamTy {
 pub struct DebruijnIndex {
    /// We maintain the invariant that this is never 0. So 1 indicates
    /// the innermost binder. To ensure this, create with `DebruijnIndex::new`.


This comment is wrong now, right? Also, why make the field private, especially now that there's no invariant?

Comments repairing is listed in my TODO, it'll be fixed once the key change works.
the field is private: new impl DebruijnIndex provide enough pub-methods to manipulate it.

Hmm. Maybe the external interface should use usize? Not sure if super relevant.

Co-authored-by: csmoe <35686186+csmoe@users.noreply.github.com>

nikomatsakis · 2018-05-28T23:47:33Z

@bors r+

bors · 2018-05-28T23:47:33Z

📌 Commit 783fe4f has been approved by nikomatsakis

nikomatsakis · 2018-05-28T23:47:43Z

@bors r-

nikomatsakis · 2018-05-29T00:03:50Z

@bors r+

bors · 2018-05-29T00:03:50Z

📌 Commit 783fe4f has been approved by nikomatsakis

bors · 2018-05-29T01:11:31Z

⌛ Testing commit 783fe4f with merge 5ae5361...

Refactor DebruijnIndex to be 0-based Fixes #49813

bors · 2018-05-29T03:22:08Z

☀️ Test successful - status-appveyor, status-travis
Approved by: nikomatsakis
Pushing 5ae5361 to master...

rust-highfive assigned estebank May 6, 2018

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

rust-highfive assigned nikomatsakis and unassigned estebank May 6, 2018