Emit warning when lifetime names are shadowed.

This is not technically a [breaking-change], but it will be soon, so
you should update your code. Typically, shadowing is accidental, and
the shadowing lifetime can simply be removed. This frequently occurs
in constructor patterns:

```rust
// Old:
impl<'a> SomeStruct<'a> { fn new<'a>(..) -> SomeStruct<'a> { ... } }

// Should be:
impl<'a> SomeStruct<'a> { fn new(..) -> SomeStruct<'a> { ... } }
```

Otherwise, you should rename the inner lifetime to something
else. Note though that lifetime elision frequently applies:

```rust
// Old
impl<'a> SomeStruct<'a> {
    fn get<'a>(x: &'a self) -> &'a T { &self.field }
}

// Should be:
impl<'a> SomeStruct<'a> {
    fn get(x: &self) -> &T { &self.field }
}
``

2 files changed, 144 insertions, 53 deletions

diff --git a/src/librustc/middle/resolve_lifetime.rs b/src/librustc/middle/resolve_lifetime.rs
index 48d6ac847d8..1923142be9e 100644
--- a/src/librustc/middle/resolve_lifetime.rs
+++ b/src/librustc/middle/resolve_lifetime.rs
@@ -90,38 +90,41 @@ pub fn krate(sess: &Session, krate: &ast::Crate, def_map: &DefMap) -> NamedRegio
 
 impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
     fn visit_item(&mut self, item: &ast::Item) {
-        match item.node {
-            ast::ItemFn(..) => {
-                // Fn lifetimes get added in visit_fn below:
-                self.with(RootScope, |this| visit::walk_item(this, item));
-            }
-            ast::ItemMod(..) |
-            ast::ItemMac(..) |
-            ast::ItemForeignMod(..) |
-            ast::ItemStatic(..) |
-            ast::ItemConst(..) => {
-                // These sorts of items have no lifetime parameters at all.
-                self.with(RootScope, |this| visit::walk_item(this, item));
-            }
-            ast::ItemTy(_, ref generics) |
-            ast::ItemEnum(_, ref generics) |
-            ast::ItemStruct(_, ref generics) |
-            ast::ItemTrait(_, ref generics, _, _, _) => {
-                // These kinds of items have only early bound lifetime parameters.
-                let lifetimes = &generics.lifetimes;
-                self.with(EarlyScope(subst::TypeSpace, lifetimes, &ROOT_SCOPE), |this| {
-                    this.check_lifetime_defs(lifetimes);
+        // Items always introduce a new root scope
+        self.with(RootScope, |_, this| {
+            match item.node {
+                ast::ItemFn(..) => {
+                    // Fn lifetimes get added in visit_fn below:
                     visit::walk_item(this, item);
-                });
-            }
-            ast::ItemImpl(_, ref generics, _, _, _) => {
-                // Impls have both early- and late-bound lifetimes.
-                self.visit_early_late(subst::TypeSpace, generics, |this| {
-                    this.check_lifetime_defs(&generics.lifetimes);
+                }
+                ast::ItemMod(..) |
+                ast::ItemMac(..) |
+                ast::ItemForeignMod(..) |
+                ast::ItemStatic(..) |
+                ast::ItemConst(..) => {
+                    // These sorts of items have no lifetime parameters at all.
                     visit::walk_item(this, item);
-                })
+                }
+                ast::ItemTy(_, ref generics) |
+                ast::ItemEnum(_, ref generics) |
+                ast::ItemStruct(_, ref generics) |
+                ast::ItemTrait(_, ref generics, _, _, _) => {
+                    // These kinds of items have only early bound lifetime parameters.
+                    let lifetimes = &generics.lifetimes;
+                    let early_scope = EarlyScope(subst::TypeSpace, lifetimes, &ROOT_SCOPE);
+                    this.with(early_scope, |old_scope, this| {
+                        this.check_lifetime_defs(old_scope, lifetimes);
+                        visit::walk_item(this, item);
+                    });
+                }
+                ast::ItemImpl(_, ref generics, _, _, _) => {
+                    // Impls have both early- and late-bound lifetimes.
+                    this.visit_early_late(subst::TypeSpace, generics, |this| {
+                        visit::walk_item(this, item);
+                    })
+                }
             }
-        }
+        });
     }
 
     fn visit_fn(&mut self, fk: visit::FnKind<'v>, fd: &'v ast::FnDecl,
@@ -129,9 +132,9 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
         match fk {
             visit::FkItemFn(_, generics, _, _) |
             visit::FkMethod(_, generics, _) => {
-                self.visit_early_late(
-                    subst::FnSpace, generics,
-                    |this| visit::walk_fn(this, fk, fd, b, s))
+                self.visit_early_late(subst::FnSpace, generics, |this| {
+                    visit::walk_fn(this, fk, fd, b, s)
+                })
             }
             visit::FkFnBlock(..) => {
                 visit::walk_fn(self, fk, fd, b, s)
@@ -145,8 +148,8 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
                 // Careful, the bounds on a closure/proc are *not* within its binder.
                 visit::walk_ty_param_bounds_helper(self, &c.bounds);
                 visit::walk_lifetime_decls_helper(self, &c.lifetimes);
-                self.with(LateScope(&c.lifetimes, self.scope), |this| {
-                    this.check_lifetime_defs(&c.lifetimes);
+                self.with(LateScope(&c.lifetimes, self.scope), |old_scope, this| {
+                    this.check_lifetime_defs(old_scope, &c.lifetimes);
                     for argument in c.decl.inputs.iter() {
                         this.visit_ty(&*argument.ty)
                     }
@@ -155,10 +158,10 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
             }
             ast::TyBareFn(ref c) => {
                 visit::walk_lifetime_decls_helper(self, &c.lifetimes);
-                self.with(LateScope(&c.lifetimes, self.scope), |this| {
+                self.with(LateScope(&c.lifetimes, self.scope), |old_scope, this| {
                     // a bare fn has no bounds, so everything
                     // contained within is scoped within its binder.
-                    this.check_lifetime_defs(&c.lifetimes);
+                    this.check_lifetime_defs(old_scope, &c.lifetimes);
                     visit::walk_ty(this, ty);
                 });
             }
@@ -167,7 +170,7 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
                 // a trait ref, which introduces a binding scope.
                 match self.def_map.borrow().get(&id) {
                     Some(&def::DefTrait(..)) => {
-                        self.with(LateScope(&Vec::new(), self.scope), |this| {
+                        self.with(LateScope(&Vec::new(), self.scope), |_, this| {
                             this.visit_path(path, id);
                         });
                     }
@@ -190,7 +193,7 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
 
     fn visit_block(&mut self, b: &ast::Block) {
         self.with(BlockScope(region::CodeExtent::from_node_id(b.id), self.scope),
-                  |this| visit::walk_block(this, b));
+                  |_, this| visit::walk_block(this, b));
     }
 
     fn visit_lifetime_ref(&mut self, lifetime_ref: &ast::Lifetime) {
@@ -232,8 +235,8 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
     fn visit_poly_trait_ref(&mut self, trait_ref: &ast::PolyTraitRef) {
         debug!("visit_poly_trait_ref trait_ref={}", trait_ref);
 
-        self.with(LateScope(&trait_ref.bound_lifetimes, self.scope), |this| {
-            this.check_lifetime_defs(&trait_ref.bound_lifetimes);
+        self.with(LateScope(&trait_ref.bound_lifetimes, self.scope), |old_scope, this| {
+            this.check_lifetime_defs(old_scope, &trait_ref.bound_lifetimes);
             for lifetime in trait_ref.bound_lifetimes.iter() {
                 this.visit_lifetime_def(lifetime);
             }
@@ -248,7 +251,7 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
 
 impl<'a> LifetimeContext<'a> {
     fn with<F>(&mut self, wrap_scope: ScopeChain, f: F) where
-        F: FnOnce(&mut LifetimeContext),
+        F: FnOnce(Scope, &mut LifetimeContext),
     {
         let LifetimeContext {sess, ref mut named_region_map, ..} = *self;
         let mut this = LifetimeContext {
@@ -258,7 +261,7 @@ impl<'a> LifetimeContext<'a> {
             def_map: self.def_map,
         };
         debug!("entering scope {}", this.scope);
-        f(&mut this);
+        f(self.scope, &mut this);
         debug!("exiting scope {}", this.scope);
     }
 
@@ -294,9 +297,9 @@ impl<'a> LifetimeContext<'a> {
         let (early, late) = generics.lifetimes.clone().partition(
             |l| referenced_idents.iter().any(|&i| i == l.lifetime.name));
 
-        self.with(EarlyScope(early_space, &early, self.scope), move |this| {
-            this.with(LateScope(&late, this.scope), move |this| {
-                this.check_lifetime_defs(&generics.lifetimes);
+        self.with(EarlyScope(early_space, &early, self.scope), move |old_scope, this| {
+            this.with(LateScope(&late, this.scope), move |_, this| {
+                this.check_lifetime_defs(old_scope, &generics.lifetimes);
                 walk(this);
             });
         });
@@ -323,7 +326,8 @@ impl<'a> LifetimeContext<'a> {
 
                 EarlyScope(space, lifetimes, s) => {
                     match search_lifetimes(lifetimes, lifetime_ref) {
-                        Some((index, decl_id)) => {
+                        Some((index, lifetime_def)) => {
+                            let decl_id = lifetime_def.id;
                             let def = DefEarlyBoundRegion(space, index, decl_id);
                             self.insert_lifetime(lifetime_ref, def);
                             return;
@@ -336,7 +340,8 @@ impl<'a> LifetimeContext<'a> {
 
                 LateScope(lifetimes, s) => {
                     match search_lifetimes(lifetimes, lifetime_ref) {
-                        Some((_index, decl_id)) => {
+                        Some((_index, lifetime_def)) => {
+                            let decl_id = lifetime_def.id;
                             let debruijn = ty::DebruijnIndex::new(late_depth + 1);
                             let def = DefLateBoundRegion(debruijn, decl_id);
                             self.insert_lifetime(lifetime_ref, def);
@@ -388,8 +393,8 @@ impl<'a> LifetimeContext<'a> {
         }
 
         match search_result {
-            Some((_depth, decl_id)) => {
-                let def = DefFreeRegion(scope_data, decl_id);
+            Some((_depth, lifetime)) => {
+                let def = DefFreeRegion(scope_data, lifetime.id);
                 self.insert_lifetime(lifetime_ref, def);
             }
 
@@ -407,7 +412,7 @@ impl<'a> LifetimeContext<'a> {
                     token::get_name(lifetime_ref.name)).as_slice());
     }
 
-    fn check_lifetime_defs(&mut self, lifetimes: &Vec<ast::LifetimeDef>) {
+    fn check_lifetime_defs(&mut self, old_scope: Scope, lifetimes: &Vec<ast::LifetimeDef>) {
         for i in range(0, lifetimes.len()) {
             let lifetime_i = &lifetimes[i];
 
@@ -422,6 +427,7 @@ impl<'a> LifetimeContext<'a> {
                 }
             }
 
+            // It is a hard error to shadow a lifetime within the same scope.
             for j in range(i + 1, lifetimes.len()) {
                 let lifetime_j = &lifetimes[j];
 
@@ -435,12 +441,54 @@ impl<'a> LifetimeContext<'a> {
                 }
             }
 
+            // It is a soft error to shadow a lifetime within a parent scope.
+            self.check_lifetime_def_for_shadowing(old_scope, &lifetime_i.lifetime);
+
             for bound in lifetime_i.bounds.iter() {
                 self.resolve_lifetime_ref(bound);
             }
         }
     }
 
+    fn check_lifetime_def_for_shadowing(&self,
+                                        mut old_scope: Scope,
+                                        lifetime: &ast::Lifetime)
+    {
+        loop {
+            match *old_scope {
+                BlockScope(_, s) => {
+                    old_scope = s;
+                }
+
+                RootScope => {
+                    return;
+                }
+
+                EarlyScope(_, lifetimes, s) |
+                LateScope(lifetimes, s) => {
+                    if let Some((_, lifetime_def)) = search_lifetimes(lifetimes, lifetime) {
+                        self.sess.span_warn(
+                            lifetime.span,
+                            format!("lifetime name `{}` shadows another \
+                                    lifetime name that is already in scope",
+                                    token::get_name(lifetime.name)).as_slice());
+                        self.sess.span_help(
+                            lifetime_def.span,
+                            format!("shadowed lifetime `{}` declared here",
+                                    token::get_name(lifetime.name)).as_slice());
+                        self.sess.span_help(
+                            lifetime.span,
+                            "shadowed lifetimes are deprecated \
+                             and will become a hard error before 1.0");
+                        return;
+                    }
+
+                    old_scope = s;
+                }
+            }
+        }
+    }
+
     fn insert_lifetime(&mut self,
                        lifetime_ref: &ast::Lifetime,
                        def: DefRegion) {
@@ -458,12 +506,12 @@ impl<'a> LifetimeContext<'a> {
     }
 }
 
-fn search_lifetimes(lifetimes: &Vec<ast::LifetimeDef>,
+fn search_lifetimes<'a>(lifetimes: &'a Vec<ast::LifetimeDef>,
                     lifetime_ref: &ast::Lifetime)
-                    -> Option<(uint, ast::NodeId)> {
+                    -> Option<(uint, &'a ast::Lifetime)> {
     for (i, lifetime_decl) in lifetimes.iter().enumerate() {
         if lifetime_decl.lifetime.name == lifetime_ref.name {
-            return Some((i, lifetime_decl.lifetime.id));
+            return Some((i, &lifetime_decl.lifetime));
         }
     }
     return None;
diff --git a/src/test/compile-fail/shadowed-lifetime.rs b/src/test/compile-fail/shadowed-lifetime.rs
new file mode 100644
index 00000000000..ff8ce7769d7
--- /dev/null
+++ b/src/test/compile-fail/shadowed-lifetime.rs
@@ -0,0 +1,43 @@
+// Copyright 2013 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 shadowed lifetimes generate an error.
+
+struct Foo<'a>(&'a int);
+
+impl<'a> Foo<'a> {
+    //~^ HELP shadowed lifetime `'a` declared here
+    fn shadow_in_method<'a>(&'a self) -> &'a int {
+        //~^ WARNING lifetime name `'a` shadows another lifetime name that is already in scope
+        //~| HELP deprecated
+        self.0
+    }
+
+    fn shadow_in_type<'b>(&'b self) -> &'b int {
+        //~^ HELP shadowed lifetime `'b` declared here
+        let x: for<'b> fn(&'b int) = panic!();
+        //~^ WARNING lifetime name `'b` shadows another lifetime name that is already in scope
+        //~| HELP deprecated
+        self.0
+    }
+
+    fn not_shadow_in_item<'b>(&'b self) {
+        struct Bar<'a, 'b>(&'a int, &'b int); // not a shadow, separate item
+        fn foo<'a, 'b>(x: &'a int, y: &'b int) { } // same
+    }
+}
+
+fn main() {
+    // intentional error that occurs after `resolve_lifetime` runs,
+    // just to ensure that this test fails to compile; when shadowed
+    // lifetimes become either an error or a proper lint, this will
+    // not be needed.
+    let x: int = 3u; //~ ERROR mismatched types
+}