Auto merge of #51215 - eddyb:visit-for-a-lifetime, r=nikomatsakis

 rustc: don't visit lifetime parameters through visit_lifetime.

Ideally we'd also not use the `Lifetime` struct for parameters, but I'll leave that to @varkor (#48149).
Fixes #51185 (discovered while auditing all the `visit_lifetime` implementations).
r? @nikomatsakis

9 files changed, 122 insertions, 143 deletions

diff --git a/src/librustc/hir/intravisit.rs b/src/librustc/hir/intravisit.rs
index 5a49ee30d9c..5471568d0af 100644
--- a/src/librustc/hir/intravisit.rs
+++ b/src/librustc/hir/intravisit.rs
@@ -580,8 +580,8 @@ pub fn walk_ty<'v, V: Visitor<'v>>(visitor: &mut V, typ: &'v Ty) {
             walk_list!(visitor, visit_ty, tuple_element_types);
         }
         TyBareFn(ref function_declaration) => {
-            visitor.visit_fn_decl(&function_declaration.decl);
             walk_list!(visitor, visit_generic_param, &function_declaration.generic_params);
+            visitor.visit_fn_decl(&function_declaration.decl);
         }
         TyPath(ref qpath) => {
             visitor.visit_qpath(qpath, typ.id, typ.span);
@@ -733,7 +733,16 @@ pub fn walk_ty_param_bound<'v, V: Visitor<'v>>(visitor: &mut V, bound: &'v TyPar
 pub fn walk_generic_param<'v, V: Visitor<'v>>(visitor: &mut V, param: &'v GenericParam) {
     match *param {
         GenericParam::Lifetime(ref ld) => {
-            visitor.visit_lifetime(&ld.lifetime);
+            visitor.visit_id(ld.lifetime.id);
+            match ld.lifetime.name {
+                LifetimeName::Name(name) => {
+                    visitor.visit_name(ld.lifetime.span, name);
+                }
+                LifetimeName::Fresh(_) |
+                LifetimeName::Static |
+                LifetimeName::Implicit |
+                LifetimeName::Underscore => {}
+            }
             walk_list!(visitor, visit_lifetime, &ld.bounds);
         }
         GenericParam::Type(ref ty_param) => {
diff --git a/src/librustc/hir/lowering.rs b/src/librustc/hir/lowering.rs
index f24a803bac3..af269078b64 100644
--- a/src/librustc/hir/lowering.rs
+++ b/src/librustc/hir/lowering.rs
@@ -1214,7 +1214,13 @@ impl<'a> LoweringContext<'a> {
                 if let &hir::Ty_::TyBareFn(_) = &t.node {
                     let old_collect_elided_lifetimes = self.collect_elided_lifetimes;
                     self.collect_elided_lifetimes = false;
+
+                    // Record the "stack height" of `for<'a>` lifetime bindings
+                    // to be able to later fully undo their introduction.
+                    let old_len = self.currently_bound_lifetimes.len();
                     hir::intravisit::walk_ty(self, t);
+                    self.currently_bound_lifetimes.truncate(old_len);
+
                     self.collect_elided_lifetimes = old_collect_elided_lifetimes;
                 } else {
                     hir::intravisit::walk_ty(self, t);
@@ -1223,28 +1229,25 @@ impl<'a> LoweringContext<'a> {
 
             fn visit_poly_trait_ref(
                 &mut self,
-                polytr: &'v hir::PolyTraitRef,
-                _: hir::TraitBoundModifier,
+                trait_ref: &'v hir::PolyTraitRef,
+                modifier: hir::TraitBoundModifier,
             ) {
+                // Record the "stack height" of `for<'a>` lifetime bindings
+                // to be able to later fully undo their introduction.
                 let old_len = self.currently_bound_lifetimes.len();
+                hir::intravisit::walk_poly_trait_ref(self, trait_ref, modifier);
+                self.currently_bound_lifetimes.truncate(old_len);
+            }
 
+            fn visit_generic_param(&mut self, param: &'v hir::GenericParam) {
                 // Record the introduction of 'a in `for<'a> ...`
-                for param in &polytr.bound_generic_params {
-                    if let hir::GenericParam::Lifetime(ref lt_def) = *param {
-                        // Introduce lifetimes one at a time so that we can handle
-                        // cases like `fn foo<'d>() -> impl for<'a, 'b: 'a, 'c: 'b + 'd>`
-                        self.currently_bound_lifetimes.push(lt_def.lifetime.name);
-
-                        // Visit the lifetime bounds
-                        for lt_bound in &lt_def.bounds {
-                            self.visit_lifetime(&lt_bound);
-                        }
-                    }
+                if let hir::GenericParam::Lifetime(ref lt_def) = *param {
+                    // Introduce lifetimes one at a time so that we can handle
+                    // cases like `fn foo<'d>() -> impl for<'a, 'b: 'a, 'c: 'b + 'd>`
+                    self.currently_bound_lifetimes.push(lt_def.lifetime.name);
                 }
 
-                hir::intravisit::walk_trait_ref(self, &polytr.trait_ref);
-
-                self.currently_bound_lifetimes.truncate(old_len);
+                hir::intravisit::walk_generic_param(self, param);
             }
 
             fn visit_lifetime(&mut self, lifetime: &'v hir::Lifetime) {
diff --git a/src/librustc/hir/map/collector.rs b/src/librustc/hir/map/collector.rs
index 8f28dfab1e7..13df1ced603 100644
--- a/src/librustc/hir/map/collector.rs
+++ b/src/librustc/hir/map/collector.rs
@@ -346,12 +346,16 @@ impl<'a, 'hir> Visitor<'hir> for NodeCollector<'a, 'hir> {
         });
     }
 
-    fn visit_generics(&mut self, generics: &'hir Generics) {
-        for ty_param in generics.ty_params() {
-            self.insert(ty_param.id, NodeTyParam(ty_param));
+    fn visit_generic_param(&mut self, param: &'hir GenericParam) {
+        match *param {
+            GenericParam::Lifetime(ref ld) => {
+                self.insert(ld.lifetime.id, NodeLifetime(&ld.lifetime));
+            }
+            GenericParam::Type(ref ty_param) => {
+                self.insert(ty_param.id, NodeTyParam(ty_param));
+            }
         }
-
-        intravisit::walk_generics(self, generics);
+        intravisit::walk_generic_param(self, param);
     }
 
     fn visit_trait_item(&mut self, ti: &'hir TraitItem) {
diff --git a/src/librustc/middle/resolve_lifetime.rs b/src/librustc/middle/resolve_lifetime.rs
index 53d51d9429f..c82597813e2 100644
--- a/src/librustc/middle/resolve_lifetime.rs
+++ b/src/librustc/middle/resolve_lifetime.rs
@@ -1928,10 +1928,10 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
             }
 
             fn visit_generic_param(&mut self, param: &hir::GenericParam) {
-                if let hir::GenericParam::Lifetime(ref lifetime_def) = *param {
-                    for l in &lifetime_def.bounds {
-                        self.visit_lifetime(l);
-                    }
+                if let hir::GenericParam::Lifetime(_) = *param {
+                    // FIXME(eddyb) Do we want this? It only makes a difference
+                    // if this `for<'a>` lifetime parameter is never used.
+                    self.have_bound_regions = true;
                 }
 
                 intravisit::walk_generic_param(self, param);
@@ -2144,28 +2144,26 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
 
     fn check_lifetime_params(&mut self, old_scope: ScopeRef, params: &'tcx [hir::GenericParam]) {
         for (i, lifetime_i) in params.lifetimes().enumerate() {
-            for lifetime in params.lifetimes() {
-                match lifetime.lifetime.name {
-                    hir::LifetimeName::Static | hir::LifetimeName::Underscore => {
-                        let lifetime = lifetime.lifetime;
-                        let name = lifetime.name.name();
-                        let mut err = struct_span_err!(
-                            self.tcx.sess,
-                            lifetime.span,
-                            E0262,
-                            "invalid lifetime parameter name: `{}`",
-                            name
-                        );
-                        err.span_label(
-                            lifetime.span,
-                            format!("{} is a reserved lifetime name", name),
-                        );
-                        err.emit();
-                    }
-                    hir::LifetimeName::Fresh(_)
-                    | hir::LifetimeName::Implicit
-                    | hir::LifetimeName::Name(_) => {}
+            match lifetime_i.lifetime.name {
+                hir::LifetimeName::Static | hir::LifetimeName::Underscore => {
+                    let lifetime = lifetime_i.lifetime;
+                    let name = lifetime.name.name();
+                    let mut err = struct_span_err!(
+                        self.tcx.sess,
+                        lifetime.span,
+                        E0262,
+                        "invalid lifetime parameter name: `{}`",
+                        name
+                    );
+                    err.span_label(
+                        lifetime.span,
+                        format!("{} is a reserved lifetime name", name),
+                    );
+                    err.emit();
                 }
+                hir::LifetimeName::Fresh(_)
+                | hir::LifetimeName::Implicit
+                | hir::LifetimeName::Name(_) => {}
             }
 
             // It is a hard error to shadow a lifetime within the same scope.
@@ -2347,31 +2345,18 @@ impl<'a, 'tcx> LifetimeContext<'a, 'tcx> {
             | Region::LateBound(_, def_id, _)
             | Region::EarlyBound(_, def_id, _) => {
                 // A lifetime declared by the user.
-                let def_local_id = self.tcx.hir.as_local_node_id(def_id).unwrap();
-                if def_local_id == lifetime_ref.id {
-                    // This is weird. Because the HIR defines a
-                    // lifetime *definition* as wrapping a Lifetime,
-                    // we wind up invoking this method also for the
-                    // definitions in some cases (notably
-                    // higher-ranked types). This means that a
-                    // lifetime with one use (e.g., `for<'a> fn(&'a
-                    // u32)`) wind up being counted as two uses.  To
-                    // avoid that, we just ignore the lifetime that
-                    // corresponds to the definition.
+                let track_lifetime_uses = self.track_lifetime_uses();
+                debug!(
+                    "insert_lifetime: track_lifetime_uses={}",
+                    track_lifetime_uses
+                );
+                if track_lifetime_uses && !self.lifetime_uses.contains_key(&def_id) {
+                    debug!("insert_lifetime: first use of {:?}", def_id);
+                    self.lifetime_uses
+                        .insert(def_id, LifetimeUseSet::One(lifetime_ref));
                 } else {
-                    let track_lifetime_uses = self.track_lifetime_uses();
-                    debug!(
-                        "insert_lifetime: track_lifetime_uses={}",
-                        track_lifetime_uses
-                    );
-                    if track_lifetime_uses && !self.lifetime_uses.contains_key(&def_id) {
-                        debug!("insert_lifetime: first use of {:?}", def_id);
-                        self.lifetime_uses
-                            .insert(def_id, LifetimeUseSet::One(lifetime_ref));
-                    } else {
-                        debug!("insert_lifetime: many uses of {:?}", def_id);
-                        self.lifetime_uses.insert(def_id, LifetimeUseSet::Many);
-                    }
+                    debug!("insert_lifetime: many uses of {:?}", def_id);
+                    self.lifetime_uses.insert(def_id, LifetimeUseSet::Many);
                 }
             }
         }
@@ -2424,31 +2409,20 @@ fn insert_late_bound_lifetimes(
     let mut appears_in_where_clause = AllCollector {
         regions: FxHashSet(),
     };
+    appears_in_where_clause.visit_generics(generics);
 
     for param in &generics.params {
         match *param {
             hir::GenericParam::Lifetime(ref lifetime_def) => {
                 if !lifetime_def.bounds.is_empty() {
                     // `'a: 'b` means both `'a` and `'b` are referenced
-                    appears_in_where_clause.visit_generic_param(param);
+                    appears_in_where_clause.regions.insert(lifetime_def.lifetime.name);
                 }
             }
-            hir::GenericParam::Type(ref ty_param) => {
-                walk_list!(
-                    &mut appears_in_where_clause,
-                    visit_ty_param_bound,
-                    &ty_param.bounds
-                );
-            }
+            hir::GenericParam::Type(_) => {}
         }
     }
 
-    walk_list!(
-        &mut appears_in_where_clause,
-        visit_where_predicate,
-        &generics.where_clause.predicates
-    );
-
     debug!(
         "insert_late_bound_lifetimes: appears_in_where_clause={:?}",
         appears_in_where_clause.regions
diff --git a/src/librustc_passes/ast_validation.rs b/src/librustc_passes/ast_validation.rs
index abbd399a944..39e0a539258 100644
--- a/src/librustc_passes/ast_validation.rs
+++ b/src/librustc_passes/ast_validation.rs
@@ -441,6 +441,13 @@ impl<'a> Visitor<'a> for AstValidator<'a> {
         visit::walk_generics(self, g)
     }
 
+    fn visit_generic_param(&mut self, param: &'a GenericParam) {
+        if let GenericParam::Lifetime(ref ld) = *param {
+            self.check_lifetime(ld.lifetime.ident);
+        }
+        visit::walk_generic_param(self, param);
+    }
+
     fn visit_pat(&mut self, pat: &'a Pat) {
         match pat.node {
             PatKind::Lit(ref expr) => {
diff --git a/src/libsyntax/fold.rs b/src/libsyntax/fold.rs
index ecb4332379d..2f209b347a4 100644
--- a/src/libsyntax/fold.rs
+++ b/src/libsyntax/fold.rs
@@ -205,14 +205,6 @@ pub trait Folder : Sized {
         noop_fold_label(label, self)
     }
 
-    fn fold_lifetime(&mut self, l: Lifetime) -> Lifetime {
-        noop_fold_lifetime(l, self)
-    }
-
-    fn fold_lifetime_def(&mut self, l: LifetimeDef) -> LifetimeDef {
-        noop_fold_lifetime_def(l, self)
-    }
-
     fn fold_attribute(&mut self, at: Attribute) -> Option<Attribute> {
         noop_fold_attribute(at, self)
     }
@@ -237,14 +229,6 @@ pub trait Folder : Sized {
         noop_fold_variant_data(vdata, self)
     }
 
-    fn fold_lifetimes(&mut self, lts: Vec<Lifetime>) -> Vec<Lifetime> {
-        noop_fold_lifetimes(lts, self)
-    }
-
-    fn fold_lifetime_defs(&mut self, lts: Vec<LifetimeDef>) -> Vec<LifetimeDef> {
-        noop_fold_lifetime_defs(lts, self)
-    }
-
     fn fold_ty_param(&mut self, tp: TyParam) -> TyParam {
         noop_fold_ty_param(tp, self)
     }
@@ -273,10 +257,6 @@ pub trait Folder : Sized {
         noop_fold_interpolated(nt, self)
     }
 
-    fn fold_opt_lifetime(&mut self, o_lt: Option<Lifetime>) -> Option<Lifetime> {
-        noop_fold_opt_lifetime(o_lt, self)
-    }
-
     fn fold_opt_bounds(&mut self, b: Option<TyParamBounds>)
                        -> Option<TyParamBounds> {
         noop_fold_opt_bounds(b, self)
@@ -376,7 +356,7 @@ pub fn noop_fold_ty<T: Folder>(t: P<Ty>, fld: &mut T) -> P<Ty> {
             TyKind::Slice(ty) => TyKind::Slice(fld.fold_ty(ty)),
             TyKind::Ptr(mt) => TyKind::Ptr(fld.fold_mt(mt)),
             TyKind::Rptr(region, mt) => {
-                TyKind::Rptr(fld.fold_opt_lifetime(region), fld.fold_mt(mt))
+                TyKind::Rptr(region.map(|lt| noop_fold_lifetime(lt, fld)), fld.fold_mt(mt))
             }
             TyKind::BareFn(f) => {
                 TyKind::BareFn(f.map(|BareFnTy {generic_params, unsafety, abi, decl}| BareFnTy {
@@ -478,7 +458,7 @@ pub fn noop_fold_angle_bracketed_parameter_data<T: Folder>(data: AngleBracketedP
                                                            -> AngleBracketedParameterData
 {
     let AngleBracketedParameterData { lifetimes, types, bindings, span } = data;
-    AngleBracketedParameterData { lifetimes: fld.fold_lifetimes(lifetimes),
+    AngleBracketedParameterData { lifetimes: lifetimes.move_map(|l| noop_fold_lifetime(l, fld)),
                                   types: types.move_map(|ty| fld.fold_ty(ty)),
                                   bindings: bindings.move_map(|b| fld.fold_ty_binding(b)),
                                   span: fld.new_span(span) }
@@ -680,7 +660,7 @@ pub fn noop_fold_ty_param_bound<T>(tpb: TyParamBound, fld: &mut T)
                                    where T: Folder {
     match tpb {
         TraitTyParamBound(ty, modifier) => TraitTyParamBound(fld.fold_poly_trait_ref(ty), modifier),
-        RegionTyParamBound(lifetime) => RegionTyParamBound(fld.fold_lifetime(lifetime)),
+        RegionTyParamBound(lifetime) => RegionTyParamBound(noop_fold_lifetime(lifetime, fld)),
     }
 }
 
@@ -701,7 +681,20 @@ pub fn noop_fold_ty_param<T: Folder>(tp: TyParam, fld: &mut T) -> TyParam {
 
 pub fn noop_fold_generic_param<T: Folder>(param: GenericParam, fld: &mut T) -> GenericParam {
     match param {
-        GenericParam::Lifetime(l) => GenericParam::Lifetime(fld.fold_lifetime_def(l)),
+        GenericParam::Lifetime(l) => {
+            let attrs: Vec<_> = l.attrs.into();
+            GenericParam::Lifetime(LifetimeDef {
+                attrs: attrs.into_iter()
+                    .flat_map(|x| fld.fold_attribute(x).into_iter())
+                    .collect::<Vec<_>>()
+                    .into(),
+                lifetime: Lifetime {
+                    id: fld.new_id(l.lifetime.id),
+                    ident: fld.fold_ident(l.lifetime.ident),
+                },
+                bounds: l.bounds.move_map(|l| noop_fold_lifetime(l, fld)),
+            })
+        }
         GenericParam::Type(t) => GenericParam::Type(fld.fold_ty_param(t)),
     }
 }
@@ -719,40 +712,13 @@ pub fn noop_fold_label<T: Folder>(label: Label, fld: &mut T) -> Label {
     }
 }
 
-pub fn noop_fold_lifetime<T: Folder>(l: Lifetime, fld: &mut T) -> Lifetime {
+fn noop_fold_lifetime<T: Folder>(l: Lifetime, fld: &mut T) -> Lifetime {
     Lifetime {
         id: fld.new_id(l.id),
         ident: fld.fold_ident(l.ident),
     }
 }
 
-pub fn noop_fold_lifetime_def<T: Folder>(l: LifetimeDef, fld: &mut T)
-                                         -> LifetimeDef {
-    let attrs: Vec<_> = l.attrs.into();
-    LifetimeDef {
-        attrs: attrs.into_iter()
-            .flat_map(|x| fld.fold_attribute(x).into_iter())
-            .collect::<Vec<_>>()
-            .into(),
-        lifetime: fld.fold_lifetime(l.lifetime),
-        bounds: fld.fold_lifetimes(l.bounds),
-    }
-}
-
-pub fn noop_fold_lifetimes<T: Folder>(lts: Vec<Lifetime>, fld: &mut T) -> Vec<Lifetime> {
-    lts.move_map(|l| fld.fold_lifetime(l))
-}
-
-pub fn noop_fold_lifetime_defs<T: Folder>(lts: Vec<LifetimeDef>, fld: &mut T)
-                                          -> Vec<LifetimeDef> {
-    lts.move_map(|l| fld.fold_lifetime_def(l))
-}
-
-pub fn noop_fold_opt_lifetime<T: Folder>(o_lt: Option<Lifetime>, fld: &mut T)
-                                         -> Option<Lifetime> {
-    o_lt.map(|lt| fld.fold_lifetime(lt))
-}
-
 pub fn noop_fold_generics<T: Folder>(Generics { params, where_clause, span }: Generics,
                                      fld: &mut T) -> Generics {
     Generics {
@@ -796,8 +762,8 @@ pub fn noop_fold_where_predicate<T: Folder>(
                                                                        span}) => {
             ast::WherePredicate::RegionPredicate(ast::WhereRegionPredicate {
                 span: fld.new_span(span),
-                lifetime: fld.fold_lifetime(lifetime),
-                bounds: bounds.move_map(|bound| fld.fold_lifetime(bound))
+                lifetime: noop_fold_lifetime(lifetime, fld),
+                bounds: bounds.move_map(|bound| noop_fold_lifetime(bound, fld))
             })
         }
         ast::WherePredicate::EqPredicate(ast::WhereEqPredicate{id,
diff --git a/src/libsyntax/visit.rs b/src/libsyntax/visit.rs
index fdf8e52bbdd..dca0e2f634c 100644
--- a/src/libsyntax/visit.rs
+++ b/src/libsyntax/visit.rs
@@ -318,8 +318,8 @@ pub fn walk_ty<'a, V: Visitor<'a>>(visitor: &mut V, typ: &'a Ty) {
             walk_list!(visitor, visit_ty, tuple_element_types);
         }
         TyKind::BareFn(ref function_declaration) => {
-            walk_fn_decl(visitor, &function_declaration.decl);
             walk_list!(visitor, visit_generic_param, &function_declaration.generic_params);
+            walk_fn_decl(visitor, &function_declaration.decl);
         }
         TyKind::Path(ref maybe_qself, ref path) => {
             if let Some(ref qself) = *maybe_qself {
@@ -485,7 +485,7 @@ pub fn walk_ty_param_bound<'a, V: Visitor<'a>>(visitor: &mut V, bound: &'a TyPar
 pub fn walk_generic_param<'a, V: Visitor<'a>>(visitor: &mut V, param: &'a GenericParam) {
     match *param {
         GenericParam::Lifetime(ref l) => {
-            visitor.visit_lifetime(&l.lifetime);
+            visitor.visit_ident(l.lifetime.ident);
             walk_list!(visitor, visit_lifetime, &l.bounds);
             walk_list!(visitor, visit_attribute, &*l.attrs);
         }
diff --git a/src/test/compile-fail/underscore-lifetime-binders.rs b/src/test/compile-fail/underscore-lifetime-binders.rs
index eb00ab5f67a..6a6698957d9 100644
--- a/src/test/compile-fail/underscore-lifetime-binders.rs
+++ b/src/test/compile-fail/underscore-lifetime-binders.rs
@@ -23,7 +23,6 @@ impl<'a> Meh<'a> for u8 {}
 
 fn meh() -> Box<for<'_> Meh<'_>> //~ ERROR invalid lifetime parameter name: `'_`
 //~^ ERROR missing lifetime specifier
-//~^^ ERROR missing lifetime specifier
 {
   Box::new(5u8)
 }
diff --git a/src/test/run-pass/issue-51185.rs b/src/test/run-pass/issue-51185.rs
new file mode 100644
index 00000000000..8e286ad1419
--- /dev/null
+++ b/src/test/run-pass/issue-51185.rs
@@ -0,0 +1,17 @@
+// Copyright 2018 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.
+
+fn foo() -> impl Into<for<'a> fn(&'a ())> {
+    (|_| {}) as for<'a> fn(&'a ())
+}
+
+fn main() {
+    foo().into()(&());
+}