Merge pull request #1672 from Manishearth/lifetime_sugg

oli-obk · web-flow · commit 7f89fb7a20d5 · 2017-04-12T10:37:57.000+02:00
Fix various false positives around needless_lifetime
diff --git a/clippy_lints/src/format.rs b/clippy_lints/src/format.rs
@@ -98,8 +98,8 @@ pub fn get_argument_fmtstr_parts<'a, 'b>(cx: &LateContext<'a, 'b>, expr: &'a Exp
 }
 
 /// Checks if the expressions matches
-/// ```rust
-/// { static __STATIC_FMTSTR: s = &["a", "b", c]; __STATIC_FMTSTR }
+/// ```rust, ignore
+/// { static __STATIC_FMTSTR: &'static[&'static str] = &["a", "b", c]; __STATIC_FMTSTR }
 /// ```
 fn check_static_str(cx: &LateContext, expr: &Expr) -> bool {
     if let Some(expr) = get_argument_fmtstr_parts(cx, expr) {
diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs
@@ -11,7 +11,6 @@
 #![feature(conservative_impl_trait)]
 
 #![allow(indexing_slicing, shadow_reuse, unknown_lints, missing_docs_in_private_items)]
-#![allow(needless_lifetimes)]
 
 extern crate syntax;
 extern crate syntax_pos;
diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs
@@ -6,6 +6,7 @@ use rustc::hir::intravisit::{Visitor, walk_ty, walk_ty_param_bound, walk_fn_decl
 use std::collections::{HashSet, HashMap};
 use syntax::codemap::Span;
 use utils::{in_external_macro, span_lint, last_path_segment};
+use syntax::symbol::keywords;
 
 /// **What it does:** Checks for lifetime annotations which can be removed by
 /// relying on lifetime elision.
@@ -58,20 +59,24 @@ impl LintPass for LifetimePass {
 
 impl<'a, 'tcx> LateLintPass<'a, 'tcx> for LifetimePass {
     fn check_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx Item) {
-        if let ItemFn(ref decl, _, _, _, ref generics, _) = item.node {
-            check_fn_inner(cx, decl, generics, item.span);
+        if let ItemFn(ref decl, _, _, _, ref generics, id) = item.node {
+            check_fn_inner(cx, decl, Some(id), generics, item.span);
         }
     }
 
     fn check_impl_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx ImplItem) {
-        if let ImplItemKind::Method(ref sig, _) = item.node {
-            check_fn_inner(cx, &sig.decl, &sig.generics, item.span);
+        if let ImplItemKind::Method(ref sig, id) = item.node {
+            check_fn_inner(cx, &sig.decl, Some(id), &sig.generics, item.span);
         }
     }
 
     fn check_trait_item(&mut self, cx: &LateContext<'a, 'tcx>, item: &'tcx TraitItem) {
-        if let TraitItemKind::Method(ref sig, _) = item.node {
-            check_fn_inner(cx, &sig.decl, &sig.generics, item.span);
+        if let TraitItemKind::Method(ref sig, ref body) = item.node {
+            let body = match *body {
+                TraitMethod::Required(_) => None,
+                TraitMethod::Provided(id) => Some(id),
+            };
+            check_fn_inner(cx, &sig.decl, body, &sig.generics, item.span);
         }
     }
 }
@@ -84,30 +89,32 @@ enum RefLt {
     Named(Name),
 }
 
-fn bound_lifetimes(bound: &TyParamBound) -> HirVec<&Lifetime> {
-    if let TraitTyParamBound(ref trait_ref, _) = *bound {
-        trait_ref.trait_ref
-            .path
-            .segments
-            .last()
-            .expect("a path must have at least one segment")
-            .parameters
-            .lifetimes()
-    } else {
-        HirVec::new()
-    }
-}
-
-fn check_fn_inner<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, decl: &'tcx FnDecl, generics: &'tcx Generics, span: Span) {
+fn check_fn_inner<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, decl: &'tcx FnDecl, body: Option<BodyId>, generics: &'tcx Generics, span: Span) {
     if in_external_macro(cx, span) || has_where_lifetimes(cx, &generics.where_clause) {
         return;
     }
 
-    let bounds_lts = generics.ty_params
-        .iter()
-        .flat_map(|typ| typ.bounds.iter().flat_map(bound_lifetimes));
-
-    if could_use_elision(cx, decl, &generics.lifetimes, bounds_lts) {
+    let mut bounds_lts = Vec::new();
+    for typ in &generics.ty_params {
+        for bound in &typ.bounds {
+            if let TraitTyParamBound(ref trait_ref, _) = *bound {
+                let bounds = trait_ref.trait_ref
+                    .path
+                    .segments
+                    .last()
+                    .expect("a path must have at least one segment")
+                    .parameters
+                    .lifetimes();
+                for bound in bounds {
+                    if bound.name != "'static" && !bound.is_elided() {
+                        return;
+                    }
+                    bounds_lts.push(bound);
+                }
+            }
+        }
+    }
+    if could_use_elision(cx, decl, body, &generics.lifetimes, bounds_lts) {
         span_lint(cx,
                   NEEDLESS_LIFETIMES,
                   span,
@@ -116,11 +123,12 @@ fn check_fn_inner<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, decl: &'tcx FnDecl, gene
     report_extra_lifetimes(cx, decl, generics);
 }
 
-fn could_use_elision<'a, 'tcx: 'a, T: Iterator<Item = &'tcx Lifetime>>(
+fn could_use_elision<'a, 'tcx: 'a>(
     cx: &LateContext<'a, 'tcx>,
     func: &'tcx FnDecl,
+    body: Option<BodyId>,
     named_lts: &'tcx [LifetimeDef],
-    bounds_lts: T
+    bounds_lts: Vec<&'tcx Lifetime>,
 ) -> bool {
     // There are two scenarios where elision works:
     // * no output references, all input references have different LT
@@ -144,8 +152,22 @@ fn could_use_elision<'a, 'tcx: 'a, T: Iterator<Item = &'tcx Lifetime>>(
         output_visitor.visit_ty(ty);
     }
 
-    let input_lts = lts_from_bounds(input_visitor.into_vec(), bounds_lts);
-    let output_lts = output_visitor.into_vec();
+    let input_lts = match input_visitor.into_vec() {
+        Some(lts) => lts_from_bounds(lts, bounds_lts.into_iter()),
+        None => return false,
+    };
+    let output_lts = match output_visitor.into_vec() {
+        Some(val) => val,
+        None => return false,
+    };
+
+    if let Some(body_id) = body {
+        let mut checker = BodyLifetimeChecker { lifetimes_used_in_body: false };
+        checker.visit_expr(&cx.tcx.hir.body(body_id).value);
+        if checker.lifetimes_used_in_body {
+            return false;
+        }
+    }
 
     // check for lifetimes from higher scopes
     for lt in input_lts.iter().chain(output_lts.iter()) {
@@ -216,13 +238,15 @@ fn unique_lifetimes(lts: &[RefLt]) -> usize {
 struct RefVisitor<'a, 'tcx: 'a> {
     cx: &'a LateContext<'a, 'tcx>,
     lts: Vec<RefLt>,
+    abort: bool,
 }
 
 impl<'v, 't> RefVisitor<'v, 't> {
     fn new(cx: &'v LateContext<'v, 't>) -> RefVisitor<'v, 't> {
         RefVisitor {
             cx: cx,
             lts: Vec::new(),
+            abort: false,
         }
     }
 
@@ -240,8 +264,12 @@ impl<'v, 't> RefVisitor<'v, 't> {
         }
     }
 
-    fn into_vec(self) -> Vec<RefLt> {
-        self.lts
+    fn into_vec(self) -> Option<Vec<RefLt>> {
+        if self.abort {
+            None
+        } else {
+            Some(self.lts)
+        }
     }
 
     fn collect_anonymous_lifetimes(&mut self, qpath: &QPath, ty: &Ty) {
@@ -292,7 +320,7 @@ impl<'a, 'tcx> Visitor<'tcx> for RefVisitor<'a, 'tcx> {
             },
             TyTraitObject(ref bounds, ref lt) => {
                 if !lt.is_elided() {
-                    self.record(&Some(*lt));
+                    self.abort = true;
                 }
                 for bound in bounds {
                     self.visit_poly_trait_ref(bound, TraitBoundModifier::None);
@@ -329,10 +357,13 @@ fn has_where_lifetimes<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, where_clause: &
                     walk_ty_param_bound(&mut visitor, bound);
                 }
                 // and check that all lifetimes are allowed
-                for lt in visitor.into_vec() {
-                    if !allowed_lts.contains(&lt) {
-                        return true;
-                    }
+                match visitor.into_vec() {
+                    None => return false,
+                    Some(lts) => for lt in lts {
+                        if !allowed_lts.contains(&lt) {
+                            return true;
+                        }
+                    },
                 }
             },
             WherePredicate::EqPredicate(ref pred) => {
@@ -384,3 +415,20 @@ fn report_extra_lifetimes<'a, 'tcx: 'a>(cx: &LateContext<'a, 'tcx>, func: &'tcx
         span_lint(cx, UNUSED_LIFETIMES, v, "this lifetime isn't used in the function definition");
     }
 }
+
+struct BodyLifetimeChecker {
+    lifetimes_used_in_body: bool,
+}
+
+impl<'tcx> Visitor<'tcx> for BodyLifetimeChecker {
+    // for lifetimes as parameters of generics
+    fn visit_lifetime(&mut self, lifetime: &'tcx Lifetime) {
+        if lifetime.name != keywords::Invalid.name() && lifetime.name != "'static" {
+            self.lifetimes_used_in_body = true;
+        }
+    }
+
+    fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> {
+        NestedVisitorMap::None
+    }
+}
diff --git a/tests/ui/lifetimes.rs b/tests/ui/lifetimes.rs
@@ -128,5 +128,29 @@ fn elided_input_named_output<'a>(_arg: &str) -> &'a str { unimplemented!() }
 fn trait_bound_ok<'a, T: WithLifetime<'static>>(_: &'a u8, _: T) { unimplemented!() }
 fn trait_bound<'a, T: WithLifetime<'a>>(_: &'a u8, _: T) { unimplemented!() }
 
+// don't warn on these, see #292
+fn trait_bound_bug<'a, T: WithLifetime<'a>>() { unimplemented!() }
+
+// #740
+struct Test {
+    vec: Vec<usize>,
+}
+
+impl Test {
+    fn iter<'a>(&'a self) -> Box<Iterator<Item = usize> + 'a> {
+        unimplemented!()
+    }
+}
+
+
+trait LintContext<'a> {}
+
+fn f<'a, T: LintContext<'a>>(_: &T) {}
+
+fn test<'a>(x: &'a [u8]) -> u8 {
+    let y: &'a u8 = &x[5];
+    *y
+}
+
 fn main() {
 }
