Combine lifetime parameters when instantiating default methods

[edwardw]

When instantiating trait default methods for certain implementation,
typeck correctly combined type parameters from trait bound with those
from method bound, but didn't do so for lifetime parameters. Applies
the same logic to lifetime parameters.

Closes #13204

[flaper87]

cc @nikomatsakis

[flaper87]

main needs to be pub for check-fast tests to run this code

[luqmana]

check-fast was removed as of #13288.

[flaper87]

Damn, I missed that! Thanks @luqmana

[nikomatsakis]

This test does fail, though not with the error message I was expecting:

// Copyright 2012-2014 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.

// Test that when instantiating trait default methods, typeck handles
// lifetime parameters defined on the method bound correctly.

pub trait Foo {
    fn bar<'a, I: Iterator<&'a ()>>(&self, it: I, f: |&'a int|) -> uint {
        let mut xs = it.filter(|_| true);
        xs.len()
    }
}

pub struct Baz;

impl Foo for Baz {
    // When instantiating `Foo::bar` for `Baz` here, typeck used to
    // ICE due to the lifetime parameter of `bar`.
}

fn main() {
    let x = Baz;
    let y = vec!((), (), ());
    assert_eq!(x.bar(y.iter()), 3);
}

[nikomatsakis]

@edwardw let me re-read the code again and make sure I'm remembering what it is for properly. I may be confused.

[nikomatsakis]

@edwardw I think I was confused. This is one of the very few cases in which it makes sense to substitute to a bound region. Hence, I think your patch is correct. I expect the test case failure I was seeing is just #12856. Though, now that I think of it, I'm a bit surprised that you didn't hit it in your example. That may be another bug.

[edwardw]

I see. I also revised the patch a little bit. Please review.

[nikomatsakis]

So, I think the patch is correct as written, but i'm still not happy about it, because it is repeating the knowledge about the index adjustment. This is of course what we do for types but that's not a great thing.

To make this more DRY, one option is to add the (adjusted) index into the RegionParameterDef -- that's certainly an improvement, but it still leaves knowledge distributed between the code in collect and resolve_lifetimes.

A better approach is to have a routine that can compute the correct index from "first principles". This probably requires @pnkfelix's changes from #13261. The idea would be to walk up from the definition to examine the context in which it occurs. This same routine could then be used by both resolve_lifetimes and the code in coherence.

I guess though that even if we do this, the result is still not totally DRY, since there is code that computes the combined substitution which currently concatenates vectors. So maybe it's not worth the effort to make the change I propose. I'd like to just remove this notion of concatenating vectors altogether in favor of something more obvious and less annoying, and that'd be the REAL right fix.

[edwardw]

Interesting challenge. I reckon the purpose of a monotonic index in ty::ty_param and ty::ReEarlyBound is to distinguish type and lifetime parameters originated from trait, implementation and method bounds; they may have same name so resolving by name won't work.

So,

pub struct substs {
    pub self_ty: Option<ty::t>,
    pub tps: DefIdMap<t>,    // instead of Vec<t>
    pub regions: RegionSubsts,
}
pub enum RegionSubsts {
    ErasedRegions,
    NonerasedRegions(NodeMap<Region>)    // instead of OwnedSlice<Region>
}

may work without vector concatenation at all. A little bit daunting to touch such a core data structure.