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

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) => {

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) {

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) {

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

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) => {