From 9a4c0a1c72c7a1e94c4ef80460c6fe2cb85e9f28 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 14:09:58 +0200 Subject: [PATCH 1/7] Don't lint about unused lifetimes if the lifetimes are used in the body of the function --- clippy_lints/src/lifetimes.rs | 47 +++++++++++++++++++++++++++++------ tests/ui/lifetimes.rs | 24 ++++++++++++++++++ tests/ui/lifetimes.stderr | 25 ++++++++++++++++++- 3 files changed, 87 insertions(+), 9 deletions(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 1de7e9e47f7..6bfcc475841 100644 --- 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); } } } @@ -98,7 +103,7 @@ fn bound_lifetimes(bound: &TyParamBound) -> HirVec<&Lifetime> { } } -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, generics: &'tcx Generics, span: Span) { if in_external_macro(cx, span) || has_where_lifetimes(cx, &generics.where_clause) { return; } @@ -107,7 +112,7 @@ fn check_fn_inner<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, decl: &'tcx FnDecl, gene .iter() .flat_map(|typ| typ.bounds.iter().flat_map(bound_lifetimes)); - if could_use_elision(cx, decl, &generics.lifetimes, bounds_lts) { + if could_use_elision(cx, decl, body, &generics.lifetimes, bounds_lts) { span_lint(cx, NEEDLESS_LIFETIMES, span, @@ -119,6 +124,7 @@ fn check_fn_inner<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, decl: &'tcx FnDecl, gene fn could_use_elision<'a, 'tcx: 'a, T: Iterator>( cx: &LateContext<'a, 'tcx>, func: &'tcx FnDecl, + body: Option, named_lts: &'tcx [LifetimeDef], bounds_lts: T ) -> bool { @@ -147,6 +153,14 @@ fn could_use_elision<'a, 'tcx: 'a, T: Iterator>( let input_lts = lts_from_bounds(input_visitor.into_vec(), bounds_lts); let output_lts = output_visitor.into_vec(); + 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()) { if !allowed_lts.contains(lt) { @@ -384,3 +398,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 + } +} \ No newline at end of file diff --git a/tests/ui/lifetimes.rs b/tests/ui/lifetimes.rs index f953c0ad3d2..620ceafdba2 100644 --- 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, +} + +impl Test { + fn iter<'a>(&'a self) -> Box + 'a> { + unimplemented!() + } +} + + +trait LintContext<'a> {} + +fn f<'a, T: LintContext<'a>>(cx: &T) {} + +fn test<'a>(x: &'a [u8]) -> u8 { + let y: &'a u8 = &x[5]; + *y +} + fn main() { } diff --git a/tests/ui/lifetimes.stderr b/tests/ui/lifetimes.stderr index 6d213ad45f2..aa12547bb20 100644 --- a/tests/ui/lifetimes.stderr +++ b/tests/ui/lifetimes.stderr @@ -91,5 +91,28 @@ error: explicit lifetimes given in parameter types where they could be elided 128 | fn trait_bound_ok<'a, T: WithLifetime<'static>>(_: &'a u8, _: T) { unimplemented!() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: aborting due to 14 previous errors +error: explicit lifetimes given in parameter types where they could be elided + --> $DIR/lifetimes.rs:132:1 + | +132 | fn trait_bound_bug<'a, T: WithLifetime<'a>>() { unimplemented!() } + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: explicit lifetimes given in parameter types where they could be elided + --> $DIR/lifetimes.rs:140:5 + | +140 | fn iter<'a>(&'a self) -> Box + 'a> { + | _____^ starting here... +141 | | unimplemented!() +142 | | } + | |_____^ ...ending here + +warning: unused variable: `cx` + --> $DIR/lifetimes.rs:148:30 + | +148 | fn f<'a, T: LintContext<'a>>(cx: &T) {} + | ^^ + | + = note: #[warn(unused_variables)] on by default + +error: aborting due to 16 previous errors From 8bb0a4d667a41cce80fc2d04e0ed2e47982aa2a4 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 14:10:11 +0200 Subject: [PATCH 2/7] Fix more doc issues --- clippy_lints/src/format.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clippy_lints/src/format.rs b/clippy_lints/src/format.rs index 518ce99c4e2..3a6a316226c 100644 --- 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) { From 2eae102cd1a0194c021fd86fd06b0a66e8cf41f0 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 14:29:58 +0200 Subject: [PATCH 3/7] Don't lint lifetimes after trait objects --- clippy_lints/src/lifetimes.rs | 33 ++++++++++++++++++++++++--------- tests/ui/lifetimes.stderr | 11 +---------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 6bfcc475841..88285cac037 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -150,8 +150,14 @@ fn could_use_elision<'a, 'tcx: 'a, T: Iterator>( 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), + 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 }; @@ -230,6 +236,7 @@ fn unique_lifetimes(lts: &[RefLt]) -> usize { struct RefVisitor<'a, 'tcx: 'a> { cx: &'a LateContext<'a, 'tcx>, lts: Vec, + abort: bool, } impl<'v, 't> RefVisitor<'v, 't> { @@ -237,6 +244,7 @@ impl<'v, 't> RefVisitor<'v, 't> { RefVisitor { cx: cx, lts: Vec::new(), + abort: false, } } @@ -254,8 +262,12 @@ impl<'v, 't> RefVisitor<'v, 't> { } } - fn into_vec(self) -> Vec { - self.lts + fn into_vec(self) -> Option> { + if self.abort { + None + } else { + Some(self.lts) + } } fn collect_anonymous_lifetimes(&mut self, qpath: &QPath, ty: &Ty) { @@ -306,7 +318,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); @@ -343,10 +355,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(<) { - return true; - } + match visitor.into_vec() { + None => return false, + Some(lts) => for lt in lts { + if !allowed_lts.contains(<) { + return true; + } + }, } }, WherePredicate::EqPredicate(ref pred) => { diff --git a/tests/ui/lifetimes.stderr b/tests/ui/lifetimes.stderr index aa12547bb20..0a201222910 100644 --- a/tests/ui/lifetimes.stderr +++ b/tests/ui/lifetimes.stderr @@ -97,15 +97,6 @@ error: explicit lifetimes given in parameter types where they could be elided 132 | fn trait_bound_bug<'a, T: WithLifetime<'a>>() { unimplemented!() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: explicit lifetimes given in parameter types where they could be elided - --> $DIR/lifetimes.rs:140:5 - | -140 | fn iter<'a>(&'a self) -> Box + 'a> { - | _____^ starting here... -141 | | unimplemented!() -142 | | } - | |_____^ ...ending here - warning: unused variable: `cx` --> $DIR/lifetimes.rs:148:30 | @@ -114,5 +105,5 @@ warning: unused variable: `cx` | = note: #[warn(unused_variables)] on by default -error: aborting due to 16 previous errors +error: aborting due to 15 previous errors From fae1e646ee6ae4ae4aac6d96e39ce8d33936d9a5 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 14:34:39 +0200 Subject: [PATCH 4/7] Remove useless warning --- tests/ui/lifetimes.rs | 2 +- tests/ui/lifetimes.stderr | 8 -------- 2 files changed, 1 insertion(+), 9 deletions(-) diff --git a/tests/ui/lifetimes.rs b/tests/ui/lifetimes.rs index 620ceafdba2..ea8a483924f 100644 --- a/tests/ui/lifetimes.rs +++ b/tests/ui/lifetimes.rs @@ -145,7 +145,7 @@ impl Test { trait LintContext<'a> {} -fn f<'a, T: LintContext<'a>>(cx: &T) {} +fn f<'a, T: LintContext<'a>>(_: &T) {} fn test<'a>(x: &'a [u8]) -> u8 { let y: &'a u8 = &x[5]; diff --git a/tests/ui/lifetimes.stderr b/tests/ui/lifetimes.stderr index 0a201222910..e155d55011c 100644 --- a/tests/ui/lifetimes.stderr +++ b/tests/ui/lifetimes.stderr @@ -97,13 +97,5 @@ error: explicit lifetimes given in parameter types where they could be elided 132 | fn trait_bound_bug<'a, T: WithLifetime<'a>>() { unimplemented!() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -warning: unused variable: `cx` - --> $DIR/lifetimes.rs:148:30 - | -148 | fn f<'a, T: LintContext<'a>>(cx: &T) {} - | ^^ - | - = note: #[warn(unused_variables)] on by default - error: aborting due to 15 previous errors From 856b1f1242b55c2f4fa19dc6d0f55c68e0d81426 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 14:46:54 +0200 Subject: [PATCH 5/7] Remove now useless `allow(unused_lifetimes)` from clippy --- clippy_lints/src/lib.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index f84b023c3d8..5d726ef3389 100644 --- 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; From 21d8fbd082aa66db3431debc05e485d5eb03c110 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 15:44:13 +0200 Subject: [PATCH 6/7] Don't lint for lifetime bounds required by traits --- clippy_lints/src/lifetimes.rs | 44 ++++++++++++++++++----------------- tests/ui/lifetimes.stderr | 8 +------ 2 files changed, 24 insertions(+), 28 deletions(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 88285cac037..7c095ad7cf7 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -89,29 +89,31 @@ 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, body: Option, 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)); - + 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, @@ -121,12 +123,12 @@ fn check_fn_inner<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, decl: &'tcx FnDecl, body report_extra_lifetimes(cx, decl, generics); } -fn could_use_elision<'a, 'tcx: 'a, T: Iterator>( +fn could_use_elision<'a, 'tcx: 'a>( cx: &LateContext<'a, 'tcx>, func: &'tcx FnDecl, body: Option, 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 @@ -151,7 +153,7 @@ fn could_use_elision<'a, 'tcx: 'a, T: Iterator>( } let input_lts = match input_visitor.into_vec() { - Some(lts) => lts_from_bounds(lts, bounds_lts), + Some(lts) => lts_from_bounds(lts, bounds_lts.into_iter()), None => return false, }; let output_lts = match output_visitor.into_vec() { diff --git a/tests/ui/lifetimes.stderr b/tests/ui/lifetimes.stderr index e155d55011c..6d213ad45f2 100644 --- a/tests/ui/lifetimes.stderr +++ b/tests/ui/lifetimes.stderr @@ -91,11 +91,5 @@ error: explicit lifetimes given in parameter types where they could be elided 128 | fn trait_bound_ok<'a, T: WithLifetime<'static>>(_: &'a u8, _: T) { unimplemented!() } | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -error: explicit lifetimes given in parameter types where they could be elided - --> $DIR/lifetimes.rs:132:1 - | -132 | fn trait_bound_bug<'a, T: WithLifetime<'a>>() { unimplemented!() } - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ - -error: aborting due to 15 previous errors +error: aborting due to 14 previous errors From 5f85ea8ef4e6ad9c0c0d44e427ddafe4c1dabdf4 Mon Sep 17 00:00:00 2001 From: Oliver Schneider Date: Tue, 11 Apr 2017 15:54:48 +0200 Subject: [PATCH 7/7] Add newline at end of file --- clippy_lints/src/lifetimes.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/clippy_lints/src/lifetimes.rs b/clippy_lints/src/lifetimes.rs index 7c095ad7cf7..be8e04c42df 100644 --- a/clippy_lints/src/lifetimes.rs +++ b/clippy_lints/src/lifetimes.rs @@ -431,4 +431,4 @@ impl<'tcx> Visitor<'tcx> for BodyLifetimeChecker { fn nested_visit_map<'this>(&'this mut self) -> NestedVisitorMap<'this, 'tcx> { NestedVisitorMap::None } -} \ No newline at end of file +}