Skip to content

Fix some false positive in needless_lifetimes lint #452

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.

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
Nov 14, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
86 changes: 62 additions & 24 deletions src/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ use reexport::*;
use rustc::lint::*;
use syntax::codemap::Span;
use rustc_front::visit::{Visitor, walk_ty, walk_ty_param_bound};
use rustc::middle::def::Def::{DefTy, DefTrait};
use std::collections::HashSet;

use utils::{in_external_macro, span_lint};
Expand Down Expand Up @@ -53,16 +54,16 @@ use self::RefLt::*;

fn check_fn_inner(cx: &LateContext, decl: &FnDecl, slf: Option<&ExplicitSelf>,
generics: &Generics, span: Span) {
if in_external_macro(cx, span) || has_where_lifetimes(&generics.where_clause) {
if in_external_macro(cx, span) || has_where_lifetimes(cx, &generics.where_clause) {
return;
}
if could_use_elision(decl, slf, &generics.lifetimes) {
if could_use_elision(cx, decl, slf, &generics.lifetimes) {
span_lint(cx, NEEDLESS_LIFETIMES, span,
"explicit lifetimes given in parameter types where they could be elided");
}
}

fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>,
fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>,
named_lts: &[LifetimeDef]) -> bool {
// There are two scenarios where elision works:
// * no output references, all input references have different LT
Expand All @@ -74,8 +75,8 @@ fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>,
let allowed_lts = allowed_lts_from(named_lts);

// these will collect all the lifetimes for references in arg/return types
let mut input_visitor = RefVisitor(Vec::new());
let mut output_visitor = RefVisitor(Vec::new());
let mut input_visitor = RefVisitor::new(cx);
let mut output_visitor = RefVisitor::new(cx);

// extract lifetime in "self" argument for methods (there is a "self" argument
// in func.inputs, but its type is TyInfer)
Expand All @@ -88,14 +89,11 @@ fn could_use_elision(func: &FnDecl, slf: Option<&ExplicitSelf>,
}
// extract lifetimes in input argument types
for arg in &func.inputs {
walk_ty(&mut input_visitor, &arg.ty);
if let TyRptr(None, _) = arg.ty.node {
input_visitor.record(&None);
}
input_visitor.visit_ty(&arg.ty);
}
// extract lifetimes in output type
if let Return(ref ty) = func.output {
walk_ty(&mut output_visitor, ty);
output_visitor.visit_ty(ty);
}

let input_lts = input_visitor.into_vec();
Expand Down Expand Up @@ -159,52 +157,92 @@ fn unique_lifetimes(lts: &[RefLt]) -> usize {
}

/// A visitor usable for rustc_front::visit::walk_ty().
struct RefVisitor(Vec<RefLt>);
struct RefVisitor<'v, 't: 'v> {
cx: &'v LateContext<'v, 't>, // context reference
lts: Vec<RefLt>
}

impl <'v, 't> RefVisitor<'v, 't> {
fn new(cx: &'v LateContext<'v, 't>) -> RefVisitor<'v, 't> {
RefVisitor { cx: cx, lts: Vec::new() }
}

impl RefVisitor {
fn record(&mut self, lifetime: &Option<Lifetime>) {
if let &Some(ref lt) = lifetime {
if lt.name.as_str() == "'static" {
self.0.push(Static);
self.lts.push(Static);
} else {
self.0.push(Named(lt.name));
self.lts.push(Named(lt.name));
}
} else {
self.0.push(Unnamed);
self.lts.push(Unnamed);
}
}

fn into_vec(self) -> Vec<RefLt> {
self.0
self.lts
}

fn collect_anonymous_lifetimes(&mut self, path: &Path, ty: &Ty) {
let last_path_segment = path.segments.last().map(|s| &s.parameters);
if let Some(&AngleBracketedParameters(ref params)) = last_path_segment {
if params.lifetimes.is_empty() {
let def = self.cx.tcx.def_map.borrow().get(&ty.id).map(|r| r.full_def());
match def {
Some(DefTy(def_id, _)) => {
if let Some(ty_def) = self.cx.tcx.adt_defs.borrow().get(&def_id) {
let scheme = ty_def.type_scheme(self.cx.tcx);
for _ in scheme.generics.regions.as_slice() {
self.record(&None);
}
}
},
Some(DefTrait(def_id)) => {
let trait_def = self.cx.tcx.trait_defs.borrow()[&def_id];
for _ in &trait_def.generics.regions {
self.record(&None);
}
},
_ => {}
}
}
}
}
}

impl<'v> Visitor<'v> for RefVisitor {
impl<'v, 't> Visitor<'v> for RefVisitor<'v, 't> {

// for lifetimes as parameters of generics
fn visit_lifetime(&mut self, lifetime: &'v Lifetime) {
self.record(&Some(*lifetime));
}

fn visit_ty(&mut self, ty: &'v Ty) {
if let TyRptr(None, _) = ty.node {
self.record(&None);
match ty.node {
TyRptr(None, _) => {
self.record(&None);
},
TyPath(_, ref path) => {
self.collect_anonymous_lifetimes(path, ty);
},
_ => {}
Copy link
Member

Choose a reason for hiding this comment

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

We should bail if there is a TyObjectSum or TyPolyTraitRef, or a TyRptr of any kind around a trait (&Trait). Can be done in followup if you want.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What is the problem with those? I've tested this on functions with trait object arguments, and I haven't noticed any problems.

Copy link
Member

Choose a reason for hiding this comment

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

It's an extra lifetime position with some strange rules around it IIRC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok. Since I don't think this PR makes the situation regarding those any worse than it is right now (or does it?), I'd rather do this separately.

}
walk_ty(self, ty);
}
}

/// Are any lifetimes mentioned in the `where` clause? If yes, we don't try to
/// reason about elision.
fn has_where_lifetimes(where_clause: &WhereClause) -> bool {
fn has_where_lifetimes(cx: &LateContext, where_clause: &WhereClause) -> bool {
for predicate in &where_clause.predicates {
match *predicate {
WherePredicate::RegionPredicate(..) => return true,
WherePredicate::BoundPredicate(ref pred) => {
// a predicate like F: Trait or F: for<'a> Trait<'a>
let mut visitor = RefVisitor(Vec::new());
let mut visitor = RefVisitor::new(cx);
// walk the type F, it may not contain LT refs
walk_ty(&mut visitor, &pred.bounded_ty);
if !visitor.0.is_empty() { return true; }
if !visitor.lts.is_empty() { return true; }
// if the bounds define new lifetimes, they are fine to occur
let allowed_lts = allowed_lts_from(&pred.bound_lifetimes);
// now walk the bounds
Expand All @@ -219,9 +257,9 @@ fn has_where_lifetimes(where_clause: &WhereClause) -> bool {
}
}
WherePredicate::EqPredicate(ref pred) => {
let mut visitor = RefVisitor(Vec::new());
let mut visitor = RefVisitor::new(cx);
walk_ty(&mut visitor, &pred.ty);
if !visitor.0.is_empty() { return true; }
if !visitor.lts.is_empty() { return true; }
}
}
}
Expand Down
21 changes: 21 additions & 0 deletions tests/compile-fail/lifetimes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,5 +85,26 @@ fn already_elided<'a>(_: &u8, _: &'a u8) -> &'a u8 {
unimplemented!()
}

fn struct_with_lt<'a>(_foo: Foo<'a>) -> &'a str { unimplemented!() } //~ERROR explicit lifetimes given

// no warning, two input lifetimes (named on the reference, anonymous on Foo)
fn struct_with_lt2<'a>(_foo: &'a Foo) -> &'a str { unimplemented!() }

// no warning, two input lifetimes (anonymous on the reference, named on Foo)
fn struct_with_lt3<'a>(_foo: &Foo<'a> ) -> &'a str { unimplemented!() }

// no warning, two input lifetimes
fn struct_with_lt4<'a, 'b>(_foo: &'a Foo<'b> ) -> &'a str { unimplemented!() }

trait WithLifetime<'a> {}
type WithLifetimeAlias<'a> = WithLifetime<'a>;

// should not warn because it won't build without the lifetime
fn trait_obj_elided<'a>(_arg: &'a WithLifetime) -> &'a str { unimplemented!() }

// this should warn because there is no lifetime on Drop, so this would be
// unambiguous if we elided the lifetime
fn trait_obj_elided2<'a>(_arg: &'a Drop) -> &'a str { unimplemented!() } //~ERROR explicit lifetimes given

fn main() {
}