Skip to content

rustc: don't visit lifetime parameters through visit_lifetime. #51215

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 30, 2018
Merged

rustc: don't visit lifetime parameters through visit_lifetime. #51215

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a1433f2
syntax: remove overloading of fold_lifetime{,_def}{,s}.
eddyb May 29, 2018
5c76b64
rustc: don't visit lifetime parameters through visit_lifetime.
eddyb May 30, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/librustc/hir/intravisit.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, is there no visit_lifetime_def? Can we make one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess we don't have visit_type_def. Seems a shame, but ok.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's visit_generic_param - handling just one kind of parameter is generally a bug.

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) => {
Expand Down
35 changes: 19 additions & 16 deletions src/librustc/hir/lowering.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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) {
Expand Down
14 changes: 9 additions & 5 deletions src/librustc/hir/map/collector.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down
100 changes: 37 additions & 63 deletions src/librustc/middle/resolve_lifetime.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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);
}
}
}
Expand Down Expand Up @@ -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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we had a visit_lifetime_def, we could handle this within the visit impl, which seems like it'd be nicer

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That'd be wrong, because it'd also apply on nested for's but we only want the outermost params. I almost moved this check to the loop below.

}
}
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
Expand Down
7 changes: 7 additions & 0 deletions src/librustc_passes/ast_validation.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down
Loading