From 5eab397e7c9840bd01f9ca165428e8eb4156cdc4 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 13 Aug 2015 19:58:11 +0530 Subject: [PATCH 1/6] Some fixes from dogfooding clippy --- src/eq_op.rs | 1 + src/lib.rs | 2 +- src/lifetimes.rs | 2 +- src/loops.rs | 4 ++-- src/utils.rs | 2 +- 5 files changed, 6 insertions(+), 5 deletions(-) diff --git a/src/eq_op.rs b/src/eq_op.rs index 495696b810cf..0b7511e7dbd9 100644 --- a/src/eq_op.rs +++ b/src/eq_op.rs @@ -1,3 +1,4 @@ +#![allow(redundant_closure)] // FIXME (#116) use rustc::lint::*; use syntax::ast::*; use syntax::ast_util as ast_util; diff --git a/src/lib.rs b/src/lib.rs index 7cb2877b2fdf..9135ecaca6c9 100755 --- a/src/lib.rs +++ b/src/lib.rs @@ -1,6 +1,6 @@ #![feature(plugin_registrar, box_syntax)] #![feature(rustc_private, collections)] -#![allow(unused_imports)] +#![allow(unused_imports, unknown_lints)] #[macro_use] extern crate syntax; diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 644537f9be17..25be88746913 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -103,7 +103,7 @@ fn could_use_elision(kind: FnKind, func: &FnDecl) -> bool { } /// Number of unique lifetimes in the given vector. -fn unique_lifetimes(lts: &Vec) -> usize { +fn unique_lifetimes(lts: &[RefLt]) -> usize { lts.iter().collect::>().len() } diff --git a/src/loops.rs b/src/loops.rs index bfb5ad6861da..937e95970ef2 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -31,7 +31,7 @@ impl LintPass for LoopsPass { walk_expr(&mut visitor, body); // linting condition: we only indexed one variable if visitor.indexed.len() == 1 { - let indexed = visitor.indexed.into_iter().next().unwrap(); + let indexed = visitor.indexed.into_iter().next().expect("Len was nonzero, but no contents found"); if visitor.nonindex { span_lint(cx, NEEDLESS_RANGE_LOOP, expr.span, &format!( "the loop variable `{}` is used to index `{}`. Consider using \ @@ -72,7 +72,7 @@ impl LintPass for LoopsPass { /// Recover the essential nodes of a desugared for loop: /// `for pat in arg { body }` becomes `(pat, arg, body)`. -fn recover_for_loop<'a>(expr: &'a Expr) -> Option<(&'a Pat, &'a Expr, &'a Expr)> { +fn recover_for_loop<'a>(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { if_let_chain! { [ let ExprMatch(ref iterexpr, ref arms, _) = expr.node, diff --git a/src/utils.rs b/src/utils.rs index 220dc6215fdb..c54a7f56f7e8 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -86,7 +86,7 @@ pub fn span_help_and_lint(cx: &Context, lint: &'static Lint, span: Span, } /// return the base type for references and raw pointers -pub fn walk_ptrs_ty<'t>(ty: ty::Ty<'t>) -> ty::Ty<'t> { +pub fn walk_ptrs_ty(ty: ty::Ty) -> ty::Ty { match ty.sty { ty::TyRef(_, ref tm) | ty::TyRawPtr(ref tm) => walk_ptrs_ty(tm.ty), _ => ty From c2bdc8571552ad74af18f71e8d026846947615e4 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 13 Aug 2015 19:59:14 +0530 Subject: [PATCH 2/6] oh the irony --- src/lifetimes.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 25be88746913..03b4de6c948e 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -130,15 +130,15 @@ impl RefVisitor { impl<'v> Visitor<'v> for RefVisitor { // for lifetimes of references - fn visit_opt_lifetime_ref(&mut self, _: Span, lifetime: &'v Option) { + fn visit_opt_lifetime_ref(&mut self, _: Span, lifetime: &Option) { self.record(lifetime); } // for lifetimes as parameters of generics - fn visit_lifetime_ref(&mut self, lifetime: &'v Lifetime) { + fn visit_lifetime_ref(&mut self, lifetime: &Lifetime) { self.record(&Some(*lifetime)); } // for lifetime bounds; the default impl calls visit_lifetime_ref - fn visit_lifetime_bound(&mut self, _: &'v Lifetime) { } + fn visit_lifetime_bound(&mut self, _: &Lifetime) { } } From 09db7f3fee25684ab0783ae80595fcc227ad0074 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 13 Aug 2015 20:11:51 +0530 Subject: [PATCH 3/6] fix --- src/collapsible_if.rs | 6 +++++- src/loops.rs | 5 ++--- src/misc.rs | 2 +- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index f0f53622c47d..be34458e0dc8 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -45,8 +45,12 @@ fn check_expr_expd(cx: &Context, e: &Expr, info: Option<&ExpnInfo>) { if in_macro(cx, info) { return; } if let ExprIf(ref check, ref then, None) = e.node { - if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), ..}) = + if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) = single_stmt_of_block(then) { + if e.span.expn_id != sp.expn_id { + return; + } + cx.sess().note(&format!("{:?} -- {:?}", e.span, sp)); span_help_and_lint(cx, COLLAPSIBLE_IF, e.span, "this if statement can be collapsed", &format!("try\nif {} && {} {}", diff --git a/src/loops.rs b/src/loops.rs index 937e95970ef2..74015bdc6bed 100644 --- a/src/loops.rs +++ b/src/loops.rs @@ -72,13 +72,12 @@ impl LintPass for LoopsPass { /// Recover the essential nodes of a desugared for loop: /// `for pat in arg { body }` becomes `(pat, arg, body)`. -fn recover_for_loop<'a>(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { +fn recover_for_loop(expr: &Expr) -> Option<(&Pat, &Expr, &Expr)> { if_let_chain! { [ let ExprMatch(ref iterexpr, ref arms, _) = expr.node, let ExprCall(_, ref iterargs) = iterexpr.node, - iterargs.len() == 1, - arms.len() == 1 && arms[0].guard.is_none(), + iterargs.len() == 1 && arms.len() == 1 && arms[0].guard.is_none(), let ExprLoop(ref block, _) = arms[0].body.node, block.stmts.is_empty(), let Some(ref loopexpr) = block.expr, diff --git a/src/misc.rs b/src/misc.rs index 6c9a7d92ce8c..7372bfed6c54 100644 --- a/src/misc.rs +++ b/src/misc.rs @@ -73,7 +73,7 @@ impl LintPass for TopLevelRefPass { } fn check_fn(&mut self, cx: &Context, _: FnKind, decl: &FnDecl, _: &Block, _: Span, _: NodeId) { - for ref arg in decl.inputs.iter() { + for ref arg in &decl.inputs { if let PatIdent(BindByRef(_), _, _) = arg.pat.node { span_lint(cx, TOPLEVEL_REF_ARG, From 49e51fe65a7762841aa7421b4c643b042c4fe4c6 Mon Sep 17 00:00:00 2001 From: Georg Brandl Date: Thu, 13 Aug 2015 17:24:47 +0200 Subject: [PATCH 4/6] lifetimes: try to fix w.r.t. lifetimes from parent scopes (fixes #162) --- src/lifetimes.rs | 60 +++++++++++++++++++++++++++------ tests/compile-fail/lifetimes.rs | 7 ++-- 2 files changed, 55 insertions(+), 12 deletions(-) diff --git a/src/lifetimes.rs b/src/lifetimes.rs index 03b4de6c948e..c333f04ab449 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -18,14 +18,23 @@ impl LintPass for LifetimePass { lint_array!(NEEDLESS_LIFETIMES) } - fn check_fn(&mut self, cx: &Context, kind: FnKind, decl: &FnDecl, - _: &Block, span: Span, _: NodeId) { - if in_external_macro(cx, span) { - return; + fn check_item(&mut self, cx: &Context, item: &Item) { + if let ItemFn(ref decl, _, _, _, ref generics, _) = item.node { + check_fn_inner(cx, decl, None, &generics.lifetimes, item.span); } - if could_use_elision(kind, decl) { - span_lint(cx, NEEDLESS_LIFETIMES, span, - "explicit lifetimes given in parameter types where they could be elided"); + } + + fn check_impl_item(&mut self, cx: &Context, item: &ImplItem) { + if let MethodImplItem(ref sig, _) = item.node { + check_fn_inner(cx, &*sig.decl, Some(&sig.explicit_self), + &sig.generics.lifetimes, item.span); + } + } + + fn check_trait_item(&mut self, cx: &Context, item: &TraitItem) { + if let MethodTraitItem(ref sig, _) = item.node { + check_fn_inner(cx, &*sig.decl, Some(&sig.explicit_self), + &sig.generics.lifetimes, item.span); } } } @@ -39,10 +48,34 @@ enum RefLt { } use self::RefLt::*; -fn could_use_elision(kind: FnKind, func: &FnDecl) -> bool { +fn check_fn_inner(cx: &Context, decl: &FnDecl, slf: Option<&ExplicitSelf>, + named_lts: &[LifetimeDef], span: Span) { + if in_external_macro(cx, span) { + return; + } + if could_use_elision(decl, slf, named_lts) { + 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>, + named_lts: &[LifetimeDef]) -> bool { // There are two scenarios where elision works: // * no output references, all input references have different LT // * output references, exactly one input reference with same LT + // All lifetimes must be unnamed, 'static or defined without bounds on the + // level of the current item. + + // check named LTs + let mut allowed_lts = HashSet::new(); + for lt in named_lts { + if lt.bounds.is_empty() { + allowed_lts.insert(Named(lt.lifetime.name)); + } + } + allowed_lts.insert(Unnamed); + allowed_lts.insert(Static); // these will collect all the lifetimes for references in arg/return types let mut input_visitor = RefVisitor(Vec::new()); @@ -50,8 +83,8 @@ fn could_use_elision(kind: FnKind, func: &FnDecl) -> bool { // extract lifetime in "self" argument for methods (there is a "self" argument // in func.inputs, but its type is TyInfer) - if let FnKind::FkMethod(_, sig, _) = kind { - match sig.explicit_self.node { + if let Some(slf) = slf { + match slf.node { SelfRegion(ref opt_lt, _, _) => input_visitor.record(opt_lt), SelfExplicit(ref ty, _) => walk_ty(&mut input_visitor, ty), _ => { } @@ -69,6 +102,13 @@ fn could_use_elision(kind: FnKind, func: &FnDecl) -> bool { let input_lts = input_visitor.into_vec(); let output_lts = output_visitor.into_vec(); + // check for lifetimes from higher scopes + for lt in input_lts.iter().chain(output_lts.iter()) { + if !allowed_lts.contains(lt) { + return false; + } + } + // no input lifetimes? easy case! if input_lts.is_empty() { return false; diff --git a/tests/compile-fail/lifetimes.rs b/tests/compile-fail/lifetimes.rs index 36daa69fb317..7f463ec70b42 100755 --- a/tests/compile-fail/lifetimes.rs +++ b/tests/compile-fail/lifetimes.rs @@ -31,11 +31,13 @@ fn deep_reference_3<'a>(x: &'a u8, _y: u8) -> Result<&'a u8, ()> { Ok(x) } type Ref<'r> = &'r u8; -fn lifetime_param_1<'a>(_x: Ref<'a>, _y: &'a u8) { } +fn lifetime_param_1<'a>(_x: Ref<'a>, _y: &'a u8) { } // no error, same lifetime on two params -fn lifetime_param_2<'a, 'b: 'a>(_x: Ref<'a>, _y: &'b u8) { } +fn lifetime_param_2<'a, 'b>(_x: Ref<'a>, _y: &'b u8) { } //~^ERROR explicit lifetimes given +fn lifetime_param_3<'a, 'b: 'a>(_x: Ref<'a>, _y: &'b u8) { } // no error, bounded lifetime + struct X { x: u8, } @@ -68,6 +70,7 @@ fn main() { let _ = deep_reference_3(&1, 2); lifetime_param_1(&1, &2); lifetime_param_2(&1, &2); + lifetime_param_3(&1, &2); let foo = X { x: 1 }; foo.self_and_out(); From 485960a00cba610dd0d00d6cd1a104f0797759b7 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 13 Aug 2015 21:07:19 +0530 Subject: [PATCH 5/6] Add dogfood script --- util/dogfood.sh | 1 + 1 file changed, 1 insertion(+) create mode 100644 util/dogfood.sh diff --git a/util/dogfood.sh b/util/dogfood.sh new file mode 100644 index 000000000000..1fa091c9f395 --- /dev/null +++ b/util/dogfood.sh @@ -0,0 +1 @@ +rm -rf target* && cargo build --lib && cp -R target target_recur && cargo rustc -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy \ No newline at end of file From 3cf5c36296d3f6b2203109c6b1757c83a55f09f0 Mon Sep 17 00:00:00 2001 From: Manish Goregaokar Date: Thu, 13 Aug 2015 21:10:45 +0530 Subject: [PATCH 6/6] Address review comments, move to travis --- .travis.yml | 1 + src/lifetimes.rs | 6 +++--- tests/compile-fail/lifetimes.rs | 28 +++++++--------------------- util/dogfood.sh | 5 ++++- 4 files changed, 15 insertions(+), 25 deletions(-) diff --git a/.travis.yml b/.travis.yml index bd997a0c22ef..7eaa61c55721 100644 --- a/.travis.yml +++ b/.travis.yml @@ -5,3 +5,4 @@ sudo: false script: - python util/update_lints.py -c - cargo test + - bash util/dogfood.sh diff --git a/src/lifetimes.rs b/src/lifetimes.rs index c333f04ab449..c3c915ea777e 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -170,15 +170,15 @@ impl RefVisitor { impl<'v> Visitor<'v> for RefVisitor { // for lifetimes of references - fn visit_opt_lifetime_ref(&mut self, _: Span, lifetime: &Option) { + fn visit_opt_lifetime_ref(&mut self, _: Span, lifetime: &'v Option) { self.record(lifetime); } // for lifetimes as parameters of generics - fn visit_lifetime_ref(&mut self, lifetime: &Lifetime) { + fn visit_lifetime_ref(&mut self, lifetime: &'v Lifetime) { self.record(&Some(*lifetime)); } // for lifetime bounds; the default impl calls visit_lifetime_ref - fn visit_lifetime_bound(&mut self, _: &Lifetime) { } + fn visit_lifetime_bound(&mut self, _: &'v Lifetime) { } } diff --git a/tests/compile-fail/lifetimes.rs b/tests/compile-fail/lifetimes.rs index 7f463ec70b42..287a8199d2c1 100755 --- a/tests/compile-fail/lifetimes.rs +++ b/tests/compile-fail/lifetimes.rs @@ -2,7 +2,7 @@ #![plugin(clippy)] #![deny(needless_lifetimes)] - +#![allow(dead_code)] fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) { } //~^ERROR explicit lifetimes given @@ -54,27 +54,13 @@ impl X { fn self_and_same_in<'s>(&'s self, _x: &'s u8) { } // no error, same lifetimes on two params } +struct Foo<'a>(&'a u8); + +impl<'a> Foo<'a> { + fn self_shared_lifetime(&self, _: &'a u8) {} // no error, lifetime 'a not defined in method + fn self_bound_lifetime<'b: 'a>(&self, _: &'b u8) {} // no error, bounds exist +} static STATIC: u8 = 1; fn main() { - distinct_lifetimes(&1, &2, 3); - distinct_and_static(&1, &2, &STATIC); - same_lifetime_on_input(&1, &2); - only_static_on_input(&1, &2, &STATIC); - in_and_out(&1, 2); - multiple_in_and_out_1(&1, &2); - multiple_in_and_out_2(&1, &2); - in_static_and_out(&1, &STATIC); - let _ = deep_reference_1(&1, &2); - let _ = deep_reference_2(Ok(&1)); - let _ = deep_reference_3(&1, 2); - lifetime_param_1(&1, &2); - lifetime_param_2(&1, &2); - lifetime_param_3(&1, &2); - - let foo = X { x: 1 }; - foo.self_and_out(); - foo.self_and_in_out(&1); - foo.distinct_self_and_in(&1); - foo.self_and_same_in(&1); } diff --git a/util/dogfood.sh b/util/dogfood.sh index 1fa091c9f395..51dd465a25d7 100644 --- a/util/dogfood.sh +++ b/util/dogfood.sh @@ -1 +1,4 @@ -rm -rf target* && cargo build --lib && cp -R target target_recur && cargo rustc -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy \ No newline at end of file +rm -rf target*/*so +cargo build --lib && cp -R target target_recur && cargo rustc -- -Zextra-plugins=clippy -Ltarget_recur/debug -Dclippy || exit 1 +rm -rf target_recur +