auto merge of #13164 : ktt3ja/rust/lifetime-suggestion-method, r=nmatsakis

This includes a change to the way lifetime names are generated. Say we
figure that `[#0, 'a, 'b]` have to be the same lifetimes, then instead
of just generating a new lifetime `'c` like before to replace them, we
would reuse `'a`. This is done so that when the lifetime name comes
from an impl, we don't give something that's completely off, and we
don't have to do much work to figure out where the name came from. For
example, for the following code snippet:

```rust
struct Baz<'x> {
    bar: &'x int
}

impl<'x> Baz<'x> {
    fn baz1(&self) -> &int {
        self.bar
    }
}
```

`[#1, 'x]` (where `#1` is BrAnon(1) and refers to lifetime of `&int`)
have to be marked the same lifetime. With the old method, we would
generate a new lifetime `'a` and suggest `fn baz1(&self) -> &'a int`
or `fn baz1<'a>(&self) -> &'a int`, both of which are wrong.

Author: bors

src/librustc/middle/typeck/infer/error_reporting.rs

@@ -82,6 +82,7 @@ use syntax::ast_map;
 use syntax::ast_util;
 use syntax::ast_util::name_to_dummy_lifetime;
 use syntax::owned_slice::OwnedSlice;
+use syntax::codemap;
 use syntax::parse::token;
 use syntax::print::pprust;
 use util::ppaux::UserString;
@@ -143,10 +144,12 @@ trait ErrorReportingHelpers {
                           origin: SubregionOrigin);
 
     fn give_expl_lifetime_param(&self,
-                                inputs: Vec<ast::Arg>,
-                                output: ast::P<ast::Ty>,
-                                item: ast::P<ast::Item>,
-                                generics: ast::Generics);
+                                decl: &ast::FnDecl,
+                                fn_style: ast::FnStyle,
+                                ident: ast::Ident,
+                                opt_explicit_self: Option<ast::ExplicitSelf_>,
+                                generics: &ast::Generics,
+                                span: codemap::Span);
 }
 
 impl<'a> ErrorReporting for InferCtxt<'a> {
@@ -260,6 +263,19 @@ impl<'a> ErrorReporting for InferCtxt<'a> {
             scope_id: ast::NodeId
         }
 
+        impl FreeRegionsFromSameFn {
+            fn new(sub_fr: ty::FreeRegion,
+                   sup_fr: ty::FreeRegion,
+                   scope_id: ast::NodeId)
+                   -> FreeRegionsFromSameFn {
+                FreeRegionsFromSameFn {
+                    sub_fr: sub_fr,
+                    sup_fr: sup_fr,
+                    scope_id: scope_id
+                }
+            }
+        }
+
         fn free_regions_from_same_fn(tcx: &ty::ctxt,
                                      sub: Region,
                                      sup: Region)
@@ -280,17 +296,14 @@ impl<'a> ErrorReporting for InferCtxt<'a> {
             match parent_node {
                 Some(node) => match node {
                     ast_map::NodeItem(item) => match item.node {
-                        // FIXME: handle method
                         ast::ItemFn(..) => {
-                            let fr_from_same_fn = FreeRegionsFromSameFn {
-                                sub_fr: fr1,
-                                sup_fr: fr2,
-                                scope_id: scope_id
-                            };
-                            Some(fr_from_same_fn)
+                            Some(FreeRegionsFromSameFn::new(fr1, fr2, scope_id))
                         },
                         _ => None
                     },
+                    ast_map::NodeMethod(..) => {
+                        Some(FreeRegionsFromSameFn::new(fr1, fr2, scope_id))
+                    },
                     _ => None
                 },
                 None => {
@@ -662,21 +675,28 @@ impl<'a> ErrorReporting for InferCtxt<'a> {
         let node_inner = match parent_node {
             Some(node) => match node {
                 ast_map::NodeItem(item) => match item.node {
-                    // FIXME: handling method
-                    ast::ItemFn(ref fn_decl, _, _, ref gen, _) => {
-                        Some((item, fn_decl, gen))
+                    ast::ItemFn(ref fn_decl, ref pur, _, ref gen, _) => {
+                        Some((fn_decl, gen, *pur, item.ident, None, item.span))
                     },
                     _ => None
                 },
+                ast_map::NodeMethod(m) => {
+                    Some((&m.decl, &m.generics, m.fn_style,
+                          m.ident, Some(m.explicit_self.node), m.span))
+                },
                 _ => None
             },
             None => None
         };
-        let (item, fn_decl, generics) = node_inner.expect("expect item fn");
-        let rebuilder = Rebuilder::new(self.tcx, *fn_decl,
-                                       generics, same_regions);
-        let (inputs, output, generics) = rebuilder.rebuild();
-        self.give_expl_lifetime_param(inputs, output, item, generics);
+        let (fn_decl, generics, fn_style, ident, expl_self, span)
+                                    = node_inner.expect("expect item fn");
+        let taken = lifetimes_in_scope(self.tcx, scope_id);
+        let life_giver = LifeGiver::with_taken(taken.as_slice());
+        let rebuilder = Rebuilder::new(self.tcx, *fn_decl, expl_self,
+                                       generics, same_regions, &life_giver);
+        let (fn_decl, expl_self, generics) = rebuilder.rebuild();
+        self.give_expl_lifetime_param(&fn_decl, fn_style, ident,
+                                      expl_self, &generics, span);
     }
 }
 
@@ -694,53 +714,98 @@ struct RebuildPathInfo<'a> {
 struct Rebuilder<'a> {
     tcx: &'a ty::ctxt,
     fn_decl: ast::P<ast::FnDecl>,
+    expl_self_opt: Option<ast::ExplicitSelf_>,
     generics: &'a ast::Generics,
     same_regions: &'a [SameRegions],
-    life_giver: LifeGiver,
+    life_giver: &'a LifeGiver,
     cur_anon: Cell<uint>,
     inserted_anons: RefCell<HashSet<uint>>,
 }
 
+enum FreshOrKept {
+    Fresh,
+    Kept
+}
+
 impl<'a> Rebuilder<'a> {
     fn new(tcx: &'a ty::ctxt,
            fn_decl: ast::P<ast::FnDecl>,
+           expl_self_opt: Option<ast::ExplicitSelf_>,
            generics: &'a ast::Generics,
-           same_regions: &'a [SameRegions])
+           same_regions: &'a [SameRegions],
+           life_giver: &'a LifeGiver)
            -> Rebuilder<'a> {
         Rebuilder {
             tcx: tcx,
             fn_decl: fn_decl,
+            expl_self_opt: expl_self_opt,
             generics: generics,
             same_regions: same_regions,
-            life_giver: LifeGiver::with_taken(generics.lifetimes.as_slice()),
+            life_giver: life_giver,
             cur_anon: Cell::new(0),
             inserted_anons: RefCell::new(HashSet::new()),
         }
     }
 
-    fn rebuild(&self) -> (Vec<ast::Arg>, ast::P<ast::Ty>, ast::Generics) {
+    fn rebuild(&self)
+               -> (ast::FnDecl, Option<ast::ExplicitSelf_>, ast::Generics) {
+        let mut expl_self_opt = self.expl_self_opt;
         let mut inputs = self.fn_decl.inputs.clone();
         let mut output = self.fn_decl.output;
         let mut ty_params = self.generics.ty_params.clone();
+        let mut kept_lifetimes = HashSet::new();
         for sr in self.same_regions.iter() {
             self.cur_anon.set(0);
             self.offset_cur_anon();
             let (anon_nums, region_names) =
                                 self.extract_anon_nums_and_names(sr);
-            let lifetime = self.life_giver.give_lifetime();
+            let (lifetime, fresh_or_kept) = self.pick_lifetime(&region_names);
+            match fresh_or_kept {
+                Kept => { kept_lifetimes.insert(lifetime.name); }
+                _ => ()
+            }
+            expl_self_opt = self.rebuild_expl_self(expl_self_opt, lifetime,
+                                                   &anon_nums, &region_names);
             inputs = self.rebuild_args_ty(inputs.as_slice(), lifetime,
                                           &anon_nums, &region_names);
             output = self.rebuild_arg_ty_or_output(output, lifetime,
                                                    &anon_nums, &region_names);
             ty_params = self.rebuild_ty_params(ty_params, lifetime,
                                                &region_names);
         }
-        let generated_lifetimes = self.life_giver.get_generated_lifetimes();
+        let fresh_lifetimes = self.life_giver.get_generated_lifetimes();
         let all_region_names = self.extract_all_region_names();
         let generics = self.rebuild_generics(self.generics,
-                                             generated_lifetimes,
-                                             &all_region_names, ty_params);
-        (inputs, output, generics)
+                                             &fresh_lifetimes,
+                                             &kept_lifetimes,
+                                             &all_region_names,
+                                             ty_params);
+        let new_fn_decl = ast::FnDecl {
+            inputs: inputs,
+            output: output,
+            cf: self.fn_decl.cf,
+            variadic: self.fn_decl.variadic
+        };
+        (new_fn_decl, expl_self_opt, generics)
+    }
+
+    fn pick_lifetime(&self,
+                     region_names: &HashSet<ast::Name>)
+                     -> (ast::Lifetime, FreshOrKept) {
+        if region_names.len() > 0 {
+            // It's not necessary to convert the set of region names to a
+            // vector of string and then sort them. However, it makes the
+            // choice of lifetime name deterministic and thus easier to test.
+            let mut names = Vec::new();
+            for rn in region_names.iter() {
+                let lt_name = token::get_name(*rn).get().to_owned();
+                names.push(lt_name);
+            }
+            names.sort();
+            let name = token::str_to_ident(names.get(0).as_slice()).name;
+            return (name_to_dummy_lifetime(name), Kept);
+        }
+        return (self.life_giver.give_lifetime(), Fresh);
     }
 
     fn extract_anon_nums_and_names(&self, same_regions: &SameRegions)
@@ -849,9 +914,38 @@ impl<'a> Rebuilder<'a> {
         })
     }
 
+    fn rebuild_expl_self(&self,
+                         expl_self_opt: Option<ast::ExplicitSelf_>,
+                         lifetime: ast::Lifetime,
+                         anon_nums: &HashSet<uint>,
+                         region_names: &HashSet<ast::Name>)
+                         -> Option<ast::ExplicitSelf_> {
+        match expl_self_opt {
+            Some(expl_self) => match expl_self {
+                ast::SelfRegion(lt_opt, muta) => match lt_opt {
+                    Some(lt) => if region_names.contains(&lt.name) {
+                        return Some(ast::SelfRegion(Some(lifetime), muta));
+                    },
+                    None => {
+                        let anon = self.cur_anon.get();
+                        self.inc_and_offset_cur_anon(1);
+                        if anon_nums.contains(&anon) {
+                            self.track_anon(anon);
+                            return Some(ast::SelfRegion(Some(lifetime), muta));
+                        }
+                    }
+                },
+                _ => ()
+            },
+            None => ()
+        }
+        expl_self_opt
+    }
+
     fn rebuild_generics(&self,
                         generics: &ast::Generics,
-                        add: Vec<ast::Lifetime>,
+                        add: &Vec<ast::Lifetime>,
+                        keep: &HashSet<ast::Name>,
                         remove: &HashSet<ast::Name>,
                         ty_params: OwnedSlice<ast::TyParam>)
                         -> ast::Generics {
@@ -860,7 +954,7 @@ impl<'a> Rebuilder<'a> {
             lifetimes.push(*lt);
         }
         for lt in generics.lifetimes.iter() {
-            if !remove.contains(&lt.name) {
+            if keep.contains(&lt.name) || !remove.contains(&lt.name) {
                 lifetimes.push((*lt).clone());
             }
         }
@@ -1099,29 +1193,17 @@ impl<'a> Rebuilder<'a> {
 
 impl<'a> ErrorReportingHelpers for InferCtxt<'a> {
     fn give_expl_lifetime_param(&self,
-                                inputs: Vec<ast::Arg>,
-                                output: ast::P<ast::Ty>,
-                                item: ast::P<ast::Item>,
-                                generics: ast::Generics) {
-        let (fn_decl, fn_style, ident) = match item.node {
-            // FIXME: handling method
-            ast::ItemFn(ref fn_decl, ref fn_style, _, _, _) => {
-                (fn_decl, fn_style, item.ident)
-            },
-            _ => fail!("Expect function or method")
-
-        };
-        let fd = ast::FnDecl {
-            inputs: inputs,
-            output: output,
-            cf: fn_decl.cf,
-            variadic: fn_decl.variadic
-        };
-        let suggested_fn =
-            pprust::fun_to_str(&fd, *fn_style, ident, None, &generics);
+                                decl: &ast::FnDecl,
+                                fn_style: ast::FnStyle,
+                                ident: ast::Ident,
+                                opt_explicit_self: Option<ast::ExplicitSelf_>,
+                                generics: &ast::Generics,
+                                span: codemap::Span) {
+        let suggested_fn = pprust::fun_to_str(decl, fn_style, ident,
+                                              opt_explicit_self, generics);
         let msg = format!("consider using an explicit lifetime \
                            parameter as shown: {}", suggested_fn);
-        self.tcx.sess.span_note(item.span, msg);
+        self.tcx.sess.span_note(span, msg);
     }
 
     fn report_inference_failure(&self,
@@ -1318,6 +1400,47 @@ impl Resolvable for @ty::TraitRef {
     }
 }
 
+fn lifetimes_in_scope(tcx: &ty::ctxt,
+                      scope_id: ast::NodeId)
+                      -> Vec<ast::Lifetime> {
+    let mut taken = Vec::new();
+    let parent = tcx.map.get_parent(scope_id);
+    let method_id_opt = match tcx.map.find(parent) {
+        Some(node) => match node {
+            ast_map::NodeItem(item) => match item.node {
+                ast::ItemFn(_, _, _, ref gen, _) => {
+                    taken.push_all(gen.lifetimes.as_slice());
+                    None
+                },
+                _ => None
+            },
+            ast_map::NodeMethod(m) => {
+                taken.push_all(m.generics.lifetimes.as_slice());
+                Some(m.id)
+            },
+            _ => None
+        },
+        None => None
+    };
+    if method_id_opt.is_some() {
+        let method_id = method_id_opt.unwrap();
+        let parent = tcx.map.get_parent(method_id);
+        match tcx.map.find(parent) {
+            Some(node) => match node {
+                ast_map::NodeItem(item) => match item.node {
+                    ast::ItemImpl(ref gen, _, _, _) => {
+                        taken.push_all(gen.lifetimes.as_slice());
+                    }
+                    _ => ()
+                },
+                _ => ()
+            },
+            None => ()
+        }
+    }
+    return taken;
+}
+
 // LifeGiver is responsible for generating fresh lifetime names
 struct LifeGiver {
     taken: HashSet<~str>,
@@ -1326,7 +1449,6 @@ struct LifeGiver {
 }
 
 impl LifeGiver {
-    // FIXME: `taken` needs to include names from higher scope, too
     fn with_taken(taken: &[ast::Lifetime]) -> LifeGiver {
         let mut taken_ = HashSet::new();
         for lt in taken.iter() {

src/test/compile-fail/lifetime-inference-give-expl-lifetime-param-2.rs

@@ -22,7 +22,7 @@ impl<'r> Itble<'r, uint, Range<uint>> for (uint, uint) {
 }
 
 fn check<'r, I: Iterator<uint>, T: Itble<'r, uint, I>>(cont: &T) -> bool {
-//~^ NOTE: consider using an explicit lifetime parameter as shown: fn check<'a, I: Iterator<uint>, T: Itble<'a, uint, I>>(cont: &'a T) -> bool
+//~^ NOTE: consider using an explicit lifetime parameter as shown: fn check<'r, I: Iterator<uint>, T: Itble<'r, uint, I>>(cont: &'r T) -> bool
     let cont_iter = cont.iter(); //~ ERROR: cannot infer
     let result = cont_iter.fold(Some(0u16), |state, val| {
         state.map_or(None, |mask| {