Auto merge of #24162 - pnkfelix:fsk-detect-duplicate-loop-labels, r=n…

…ikomatsakis

Check for duplicate loop labels in function bodies.

See also: http://internals.rust-lang.org/t/psa-rejecting-duplicate-loop-labels/1833

The change, which we are putting in as future-proofing in preparation for future potential additions to the language (namely labeling arbitrary blocks and using those labels in borrow expressions), means that code like this will start emitting warnings:

```rust
fn main() {
    { 'a: loop { break; } }
    { 'a: loop { break; } }
}
```

To make the above code compile without warnings, write this instead:

```rust
fn main() {
    { 'a: loop { break; } }
    { 'b: loop { break; } }
}
```

Since this change is only introducing a new warnings, this change is non-breaking.

Fix #21633

src/librustc/middle/resolve_lifetime.rs

@@ -24,6 +24,7 @@ use middle::region;
 use middle::subst;
 use middle::ty;
 use std::fmt;
+use std::mem::replace;
 use syntax::ast;
 use syntax::codemap::Span;
 use syntax::parse::token::special_idents;
@@ -70,6 +71,9 @@ struct LifetimeContext<'a> {
 
     // I'm sorry.
     trait_ref_hack: bool,
+
+    // List of labels in the function/method currently under analysis.
+    labels_in_fn: Vec<(ast::Ident, Span)>,
 }
 
 enum ScopeChain<'a> {
@@ -97,13 +101,18 @@ pub fn krate(sess: &Session, krate: &ast::Crate, def_map: &DefMap) -> NamedRegio
         scope: &ROOT_SCOPE,
         def_map: def_map,
         trait_ref_hack: false,
+        labels_in_fn: vec![],
     }, krate);
     sess.abort_if_errors();
     named_region_map
 }
 
 impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
     fn visit_item(&mut self, item: &ast::Item) {
+        // Items save/restore the set of labels. This way innner items
+        // can freely reuse names, be they loop labels or lifetimes.
+        let saved = replace(&mut self.labels_in_fn, vec![]);
+
         // Items always introduce a new root scope
         self.with(RootScope, |_, this| {
             match item.node {
@@ -137,23 +146,26 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
                 }
             }
         });
+
+        // Done traversing the item; restore saved set of labels.
+        replace(&mut self.labels_in_fn, saved);
     }
 
     fn visit_fn(&mut self, fk: visit::FnKind<'v>, fd: &'v ast::FnDecl,
                 b: &'v ast::Block, s: Span, _: ast::NodeId) {
         match fk {
             visit::FkItemFn(_, generics, _, _, _) => {
                 self.visit_early_late(subst::FnSpace, generics, |this| {
-                    visit::walk_fn(this, fk, fd, b, s)
+                    this.walk_fn(fk, fd, b, s)
                 })
             }
             visit::FkMethod(_, sig, _) => {
                 self.visit_early_late(subst::FnSpace, &sig.generics, |this| {
-                    visit::walk_fn(this, fk, fd, b, s)
+                    this.walk_fn(fk, fd, b, s)
                 })
             }
             visit::FkFnBlock(..) => {
-                visit::walk_fn(self, fk, fd, b, s)
+                self.walk_fn(fk, fd, b, s)
             }
         }
     }
@@ -190,13 +202,19 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
     }
 
     fn visit_trait_item(&mut self, trait_item: &ast::TraitItem) {
+        // We reset the labels on every trait item, so that different
+        // methods in an impl can reuse label names.
+        let saved = replace(&mut self.labels_in_fn, vec![]);
+
         if let ast::MethodTraitItem(ref sig, None) = trait_item.node {
             self.visit_early_late(
                 subst::FnSpace, &sig.generics,
                 |this| visit::walk_trait_item(this, trait_item))
         } else {
             visit::walk_trait_item(self, trait_item);
         }
+
+        replace(&mut self.labels_in_fn, saved);
     }
 
     fn visit_block(&mut self, b: &ast::Block) {
@@ -286,7 +304,170 @@ impl<'a, 'v> Visitor<'v> for LifetimeContext<'a> {
     }
 }
 
+#[derive(Copy, Clone, PartialEq)]
+enum ShadowKind { Label, Lifetime }
+struct Original { kind: ShadowKind, span: Span }
+struct Shadower { kind: ShadowKind, span: Span }
+
+fn original_label(span: Span) -> Original {
+    Original { kind: ShadowKind::Label, span: span }
+}
+fn shadower_label(span: Span) -> Shadower {
+    Shadower { kind: ShadowKind::Label, span: span }
+}
+fn original_lifetime(l: &ast::Lifetime) -> Original {
+    Original { kind: ShadowKind::Lifetime, span: l.span }
+}
+fn shadower_lifetime(l: &ast::Lifetime) -> Shadower {
+    Shadower { kind: ShadowKind::Lifetime, span: l.span }
+}
+
+impl ShadowKind {
+    fn desc(&self) -> &'static str {
+        match *self {
+            ShadowKind::Label => "label",
+            ShadowKind::Lifetime => "lifetime",
+        }
+    }
+}
+
+fn signal_shadowing_problem(
+    sess: &Session, name: ast::Name, orig: Original, shadower: Shadower) {
+    if let (ShadowKind::Lifetime, ShadowKind::Lifetime) = (orig.kind, shadower.kind) {
+        // lifetime/lifetime shadowing is an error
+        sess.span_err(shadower.span,
+                      &format!("{} name `{}` shadows a \
+                                {} name that is already in scope",
+                               shadower.kind.desc(), name, orig.kind.desc()));
+    } else {
+        // shadowing involving a label is only a warning, due to issues with
+        // labels and lifetimes not being macro-hygienic.
+        sess.span_warn(shadower.span,
+                      &format!("{} name `{}` shadows a \
+                                {} name that is already in scope",
+                               shadower.kind.desc(), name, orig.kind.desc()));
+    }
+    sess.span_note(orig.span,
+                   &format!("shadowed {} `{}` declared here",
+                            orig.kind.desc(), name));
+}
+
+// Adds all labels in `b` to `ctxt.labels_in_fn`, signalling a warning
+// if one of the label shadows a lifetime or another label.
+fn extract_labels<'v, 'a>(ctxt: &mut LifetimeContext<'a>, b: &'v ast::Block) {
+
+    struct GatherLabels<'a> {
+        sess: &'a Session,
+        scope: Scope<'a>,
+        labels_in_fn: &'a mut Vec<(ast::Ident, Span)>,
+    }
+
+    let mut gather = GatherLabels {
+        sess: ctxt.sess,
+        scope: ctxt.scope,
+        labels_in_fn: &mut ctxt.labels_in_fn,
+    };
+    gather.visit_block(b);
+    return;
+
+    impl<'v, 'a> Visitor<'v> for GatherLabels<'a> {
+        fn visit_expr(&mut self, ex: &'v ast::Expr) {
+            if let Some(label) = expression_label(ex) {
+                for &(prior, prior_span) in &self.labels_in_fn[..] {
+                    // FIXME (#24278): non-hygienic comparision
+                    if label.name == prior.name {
+                        signal_shadowing_problem(self.sess,
+                                                 label.name,
+                                                 original_label(prior_span),
+                                                 shadower_label(ex.span));
+                    }
+                }
+
+                check_if_label_shadows_lifetime(self.sess,
+                                                self.scope,
+                                                label,
+                                                ex.span);
+
+                self.labels_in_fn.push((label, ex.span));
+            }
+            visit::walk_expr(self, ex)
+        }
+
+        fn visit_item(&mut self, _: &ast::Item) {
+            // do not recurse into items defined in the block
+        }
+    }
+
+    fn expression_label(ex: &ast::Expr) -> Option<ast::Ident> {
+        match ex.node {
+            ast::ExprWhile(_, _, Some(label))       |
+            ast::ExprWhileLet(_, _, _, Some(label)) |
+            ast::ExprForLoop(_, _, _, Some(label))  |
+            ast::ExprLoop(_, Some(label))          => Some(label),
+            _ => None,
+        }
+    }
+
+    fn check_if_label_shadows_lifetime<'a>(sess: &'a Session,
+                                           mut scope: Scope<'a>,
+                                           label: ast::Ident,
+                                           label_span: Span) {
+        loop {
+            match *scope {
+                BlockScope(_, s) => { scope = s; }
+                RootScope => { return; }
+
+                EarlyScope(_, lifetimes, s) |
+                LateScope(lifetimes, s) => {
+                    for lifetime_def in lifetimes {
+                        // FIXME (#24278): non-hygienic comparision
+                        if label.name == lifetime_def.lifetime.name {
+                            signal_shadowing_problem(
+                                sess,
+                                label.name,
+                                original_lifetime(&lifetime_def.lifetime),
+                                shadower_label(label_span));
+                            return;
+                        }
+                    }
+                    scope = s;
+                }
+            }
+        }
+    }
+}
+
 impl<'a> LifetimeContext<'a> {
+    // This is just like visit::walk_fn, except that it extracts the
+    // labels of the function body and swaps them in before visiting
+    // the function body itself.
+    fn walk_fn<'b>(&mut self,
+                   fk: visit::FnKind,
+                   fd: &ast::FnDecl,
+                   fb: &'b ast::Block,
+                   _span: Span) {
+        match fk {
+            visit::FkItemFn(_, generics, _, _, _) => {
+                visit::walk_fn_decl(self, fd);
+                self.visit_generics(generics);
+            }
+            visit::FkMethod(_, sig, _) => {
+                visit::walk_fn_decl(self, fd);
+                self.visit_generics(&sig.generics);
+                self.visit_explicit_self(&sig.explicit_self);
+            }
+            visit::FkFnBlock(..) => {
+                visit::walk_fn_decl(self, fd);
+            }
+        }
+
+        // After inpsecting the decl, add all labels from the body to
+        // `self.labels_in_fn`.
+        extract_labels(self, fb);
+
+        self.visit_block(fb);
+    }
+
     fn with<F>(&mut self, wrap_scope: ScopeChain, f: F) where
         F: FnOnce(Scope, &mut LifetimeContext),
     {
@@ -297,6 +478,7 @@ impl<'a> LifetimeContext<'a> {
             scope: &wrap_scope,
             def_map: self.def_map,
             trait_ref_hack: self.trait_ref_hack,
+            labels_in_fn: self.labels_in_fn.clone(),
         };
         debug!("entering scope {:?}", this.scope);
         f(self.scope, &mut this);
@@ -494,6 +676,17 @@ impl<'a> LifetimeContext<'a> {
                                         mut old_scope: Scope,
                                         lifetime: &ast::Lifetime)
     {
+        for &(label, label_span) in &self.labels_in_fn {
+            // FIXME (#24278): non-hygienic comparision
+            if lifetime.name == label.name {
+                signal_shadowing_problem(self.sess,
+                                         lifetime.name,
+                                         original_label(label_span),
+                                         shadower_lifetime(&lifetime));
+                return;
+            }
+        }
+
         loop {
             match *old_scope {
                 BlockScope(_, s) => {
@@ -507,15 +700,11 @@ impl<'a> LifetimeContext<'a> {
                 EarlyScope(_, lifetimes, s) |
                 LateScope(lifetimes, s) => {
                     if let Some((_, lifetime_def)) = search_lifetimes(lifetimes, lifetime) {
-                        self.sess.span_err(
-                            lifetime.span,
-                            &format!("lifetime name `{}` shadows another \
-                                     lifetime name that is already in scope",
-                                     token::get_name(lifetime.name)));
-                        self.sess.span_note(
-                            lifetime_def.span,
-                            &format!("shadowed lifetime `{}` declared here",
-                                     token::get_name(lifetime.name)));
+                        signal_shadowing_problem(
+                            self.sess,
+                            lifetime.name,
+                            original_lifetime(&lifetime_def),
+                            shadower_lifetime(&lifetime));
                         return;
                     }
 

src/test/compile-fail/loop-labeled-break-value.rs

@@ -16,6 +16,6 @@ fn main() {
         let _: i32 = 'inner: loop { break 'inner }; //~ ERROR mismatched types
     }
     loop {
-        let _: i32 = 'inner: loop { loop { break 'inner } }; //~ ERROR mismatched types
+        let _: i32 = 'inner2: loop { loop { break 'inner2 } }; //~ ERROR mismatched types
     }
 }

src/test/compile-fail/loops-reject-duplicate-labels-2.rs

@@ -0,0 +1,51 @@
+// Copyright 2015 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.
+
+#![feature(rustc_attrs)]
+
+// ignore-tidy-linelength
+
+// Issue #21633: reject duplicate loop labels in function bodies.
+//
+// This is testing the generalization (to the whole function body)
+// discussed here:
+// http://internals.rust-lang.org/t/psa-rejecting-duplicate-loop-labels/1833
+
+pub fn foo() {
+    { 'fl: for _ in 0..10 { break; } }   //~ NOTE shadowed label `'fl` declared here
+    { 'fl: loop { break; } }             //~ WARN label name `'fl` shadows a label name that is already in scope
+
+    { 'lf: loop { break; } }             //~ NOTE shadowed label `'lf` declared here
+    { 'lf: for _ in 0..10 { break; } }   //~ WARN label name `'lf` shadows a label name that is already in scope
+
+    { 'wl: while 2 > 1 { break; } }      //~ NOTE shadowed label `'wl` declared here
+    { 'wl: loop { break; } }             //~ WARN label name `'wl` shadows a label name that is already in scope
+
+    { 'lw: loop { break; } }             //~ NOTE shadowed label `'lw` declared here
+    { 'lw: while 2 > 1 { break; } }      //~ WARN label name `'lw` shadows a label name that is already in scope
+
+    { 'fw: for _ in 0..10 { break; } }   //~ NOTE shadowed label `'fw` declared here
+    { 'fw: while 2 > 1 { break; } }      //~ WARN label name `'fw` shadows a label name that is already in scope
+
+    { 'wf: while 2 > 1 { break; } }      //~ NOTE shadowed label `'wf` declared here
+    { 'wf: for _ in 0..10 { break; } }   //~ WARN label name `'wf` shadows a label name that is already in scope
+
+    { 'tl: while let Some(_) = None::<i32> { break; } } //~ NOTE shadowed label `'tl` declared here
+    { 'tl: loop { break; } }             //~ WARN label name `'tl` shadows a label name that is already in scope
+
+    { 'lt: loop { break; } }             //~ NOTE shadowed label `'lt` declared here
+    { 'lt: while let Some(_) = None::<i32> { break; } }
+                                        //~^ WARN label name `'lt` shadows a label name that is already in scope
+}
+
+#[rustc_error]
+pub fn main() { //~ ERROR compilation successful
+    foo();
+}