From 1b9fbd8801019b9235c08e09acb92d21ac0c8c74 Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 29 Jan 2016 22:18:14 +0100 Subject: [PATCH 1/5] Fix false positive in NEEDLESS_LIFETIMES --- src/lifetimes.rs | 60 ++++++++++++++++++++++++--------- tests/compile-fail/lifetimes.rs | 5 +++ 2 files changed, 50 insertions(+), 15 deletions(-) diff --git a/src/lifetimes.rs b/src/lifetimes.rs index b83ac390edf..1441015eb0e 100644 --- a/src/lifetimes.rs +++ b/src/lifetimes.rs @@ -65,13 +65,30 @@ enum RefLt { Static, Named(Name), } -use self::RefLt::*; + +fn bound_lifetimes(bound: &TyParamBound) -> Option> { + if let TraitTyParamBound(ref trait_ref, _) = *bound { + let lt = trait_ref.trait_ref.path.segments + .last().expect("a path must have at least one segment") + .parameters.lifetimes(); + + Some(lt) + } else { + None + } +} fn check_fn_inner(cx: &LateContext, decl: &FnDecl, slf: Option<&ExplicitSelf>, generics: &Generics, span: Span) { if in_external_macro(cx, span) || has_where_lifetimes(cx, &generics.where_clause) { return; } - if could_use_elision(cx, decl, slf, &generics.lifetimes) { + + let bounds_lts = + generics.ty_params + .iter() + .flat_map(|ref typ| typ.bounds.iter().filter_map(bound_lifetimes).flat_map(|lts| lts)); + + if could_use_elision(cx, decl, slf, &generics.lifetimes, bounds_lts) { span_lint(cx, NEEDLESS_LIFETIMES, span, @@ -80,7 +97,10 @@ fn check_fn_inner(cx: &LateContext, decl: &FnDecl, slf: Option<&ExplicitSelf>, g report_extra_lifetimes(cx, decl, &generics, slf); } -fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>, named_lts: &[LifetimeDef]) -> bool { +fn could_use_elision<'a, T: Iterator>( + cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf>, + named_lts: &[LifetimeDef], bounds_lts: T +) -> 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 @@ -112,7 +132,7 @@ fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf> output_visitor.visit_ty(ty); } - let input_lts = input_visitor.into_vec(); + let input_lts = lts_from_bounds(input_visitor.into_vec(), bounds_lts); let output_lts = output_visitor.into_vec(); // check for lifetimes from higher scopes @@ -129,7 +149,7 @@ fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf> // no output lifetimes, check distinctness of input lifetimes // only unnamed and static, ok - if input_lts.iter().all(|lt| *lt == Unnamed || *lt == Static) { + if input_lts.iter().all(|lt| *lt == RefLt::Unnamed || *lt == RefLt::Static) { return false; } // we have no output reference, so we only need all distinct lifetimes @@ -142,8 +162,8 @@ fn could_use_elision(cx: &LateContext, func: &FnDecl, slf: Option<&ExplicitSelf> } if input_lts.len() == 1 { match (&input_lts[0], &output_lts[0]) { - (&Named(n1), &Named(n2)) if n1 == n2 => true, - (&Named(_), &Unnamed) => true, + (&RefLt::Named(n1), &RefLt::Named(n2)) if n1 == n2 => true, + (&RefLt::Named(_), &RefLt::Unnamed) => true, _ => false, // already elided, different named lifetimes // or something static going on } @@ -157,22 +177,32 @@ fn allowed_lts_from(named_lts: &[LifetimeDef]) -> HashSet { 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(RefLt::Named(lt.lifetime.name)); } } - allowed_lts.insert(Unnamed); - allowed_lts.insert(Static); + allowed_lts.insert(RefLt::Unnamed); + allowed_lts.insert(RefLt::Static); allowed_lts } +fn lts_from_bounds<'a, T: Iterator>(mut vec: Vec, bounds_lts: T) -> Vec { + for lt in bounds_lts { + if lt.name.as_str() != "'static" { + vec.push(RefLt::Named(lt.name)); + } + } + + vec +} + /// Number of unique lifetimes in the given vector. fn unique_lifetimes(lts: &[RefLt]) -> usize { lts.iter().collect::>().len() } -/// A visitor usable for rustc_front::visit::walk_ty(). +/// A visitor usable for `rustc_front::visit::walk_ty()`. struct RefVisitor<'v, 't: 'v> { - cx: &'v LateContext<'v, 't>, // context reference + cx: &'v LateContext<'v, 't>, lts: Vec, } @@ -187,12 +217,12 @@ impl<'v, 't> RefVisitor<'v, 't> { fn record(&mut self, lifetime: &Option) { if let Some(ref lt) = *lifetime { if lt.name.as_str() == "'static" { - self.lts.push(Static); + self.lts.push(RefLt::Static); } else { - self.lts.push(Named(lt.name)); + self.lts.push(RefLt::Named(lt.name)); } } else { - self.lts.push(Unnamed); + self.lts.push(RefLt::Unnamed); } } diff --git a/tests/compile-fail/lifetimes.rs b/tests/compile-fail/lifetimes.rs index eb161af9dc3..408b6762df6 100644 --- a/tests/compile-fail/lifetimes.rs +++ b/tests/compile-fail/lifetimes.rs @@ -3,6 +3,7 @@ #![deny(needless_lifetimes, unused_lifetimes)] #![allow(dead_code)] + fn distinct_lifetimes<'a, 'b>(_x: &'a u8, _y: &'b u8, _z: u8) { } //~^ERROR explicit lifetimes given @@ -97,6 +98,7 @@ fn struct_with_lt3<'a>(_foo: &Foo<'a> ) -> &'a str { unimplemented!() } 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 @@ -123,5 +125,8 @@ fn named_input_elided_output<'a>(_arg: &'a str) -> &str { unimplemented!() } //~ fn elided_input_named_output<'a>(_arg: &str) -> &'a str { unimplemented!() } +fn trait_bound_ok<'a, T: WithLifetime<'static>>(_: &'a u8, _: T) { unimplemented!() } //~ERROR explicit lifetimes given +fn trait_bound<'a, T: WithLifetime<'a>>(_: &'a u8, _: T) { unimplemented!() } + fn main() { } From 3a39bbaf741f74342c694e59a3e0b1888279131c Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 29 Jan 2016 22:19:14 +0100 Subject: [PATCH 2/5] Small cleanup --- src/entry.rs | 6 +++--- src/matches.rs | 4 ++-- src/utils.rs | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/entry.rs b/src/entry.rs index d9fb7269be6..64d6fa7be38 100644 --- a/src/entry.rs +++ b/src/entry.rs @@ -93,19 +93,19 @@ fn check_for_insert(cx: &LateContext, span: Span, map: &Expr, key: &Expr, expr: ], { let help = if sole_expr { format!("{}.entry({}).or_insert({})", - snippet(cx, map.span, ".."), + snippet(cx, map.span, "map"), snippet(cx, params[1].span, ".."), snippet(cx, params[2].span, "..")) } else { format!("{}.entry({})", - snippet(cx, map.span, ".."), + snippet(cx, map.span, "map"), snippet(cx, params[1].span, "..")) }; span_lint_and_then(cx, MAP_ENTRY, span, &format!("usage of `contains_key` followed by `insert` on `{}`", kind), |db| { - db.span_suggestion(span, "Consider using", help.clone()); + db.span_suggestion(span, "Consider using", help); }); } } diff --git a/src/matches.rs b/src/matches.rs index 4cb4df19bf8..0c18d35fa12 100644 --- a/src/matches.rs +++ b/src/matches.rs @@ -223,8 +223,8 @@ fn check_match_bool(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) { expr.span, "you seem to be trying to match on a boolean expression. Consider using \ an if..else block:", move |db| { - if let Some(ref sugg) = sugg { - db.span_suggestion(expr.span, "try this", sugg.clone()); + if let Some(sugg) = sugg { + db.span_suggestion(expr.span, "try this", sugg); } }); } diff --git a/src/utils.rs b/src/utils.rs index a6dbcfd9e2f..b5355919496 100644 --- a/src/utils.rs +++ b/src/utils.rs @@ -649,7 +649,7 @@ fn is_cast_ty_equal(left: &Ty, right: &Ty) -> bool { } } -/// Return the pre-expansion span is this comes from a expansion of the macro `name`. +/// Return the pre-expansion span if is this comes from an expansion of the macro `name`. pub fn is_expn_of(cx: &LateContext, mut span: Span, name: &str) -> Option { loop { let span_name_span = cx.tcx.sess.codemap().with_expn_info(span.expn_id, |expn| { From 997a565aeb92f01342d4e35075be609b292befcd Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 29 Jan 2016 22:24:08 +0100 Subject: [PATCH 3/5] Make the python scripts py3 and pep8 compatible --- util/update_lints.py | 4 +++- util/update_wiki.py | 37 ++++++++++++++++++++++++++----------- 2 files changed, 29 insertions(+), 12 deletions(-) diff --git a/util/update_lints.py b/util/update_lints.py index 6a59abcdc15..9f105a2699c 100755 --- a/util/update_lints.py +++ b/util/update_lints.py @@ -1,7 +1,8 @@ #!/usr/bin/env python # Generate a Markdown table of all lints, and put it in README.md. # With -n option, only print the new table to stdout. -# With -c option, print a warning and set exit status to 1 if a file would be changed. +# With -c option, print a warning and set exit status to 1 if a file would be +# changed. import os import re @@ -18,6 +19,7 @@ nl_escape_re = re.compile(r'\\\n\s*') wiki_link = 'https://github.com/Manishearth/rust-clippy/wiki' + def collect(lints, fn): """Collect all lints from a file. diff --git a/util/update_wiki.py b/util/update_wiki.py index 96333c1e4b3..842f0ed6d8e 100755 --- a/util/update_wiki.py +++ b/util/update_wiki.py @@ -1,8 +1,12 @@ #!/usr/bin/env python # Generate the wiki Home.md page from the contained doc comments # requires the checked out wiki in ../rust-clippy.wiki/ -# with -c option, print a warning and set exit status 1 if the file would be changed. -import os, re, sys +# with -c option, print a warning and set exit status 1 if the file would be +# changed. +import os +import re +import sys + def parse_path(p="src"): d = {} @@ -14,10 +18,10 @@ def parse_path(p="src"): START = 0 LINT = 1 + def parse_file(d, f): last_comment = [] comment = True - lint = None with open(f) as rs: for line in rs: @@ -35,27 +39,33 @@ def parse_file(d, f): l = line.strip() m = re.search(r"pub\s+([A-Z_]+)", l) if m: - print "found %s in %s" % (m.group(1).lower(), f) + print("found %s in %s" % (m.group(1).lower(), f)) d[m.group(1).lower()] = last_comment last_comment = [] comment = True if "}" in l: - print "Warning: Missing Lint-Name in", f + print("Warning: Missing Lint-Name in", f) comment = True PREFIX = """Welcome to the rust-clippy wiki! -Here we aim to collect further explanations on the lints clippy provides. So without further ado: +Here we aim to collect further explanations on the lints clippy provides. So \ +without further ado: """ WARNING = """ # A word of warning -Clippy works as a *plugin* to the compiler, which means using an unstable internal API. We have gotten quite good at keeping pace with the API evolution, but the consequence is that clippy absolutely needs to be compiled with the version of `rustc` it will run on, otherwise you will get strange errors of missing symbols.""" +Clippy works as a *plugin* to the compiler, which means using an unstable \ +internal API. We have gotten quite good at keeping pace with the API \ +evolution, but the consequence is that clippy absolutely needs to be compiled \ +with the version of `rustc` it will run on, otherwise you will get strange \ +errors of missing symbols.""" + def write_wiki_page(d, f): - keys = d.keys() + keys = list(d.keys()) keys.sort() with open(f, "w") as w: w.write(PREFIX) @@ -65,6 +75,7 @@ def write_wiki_page(d, f): for k in keys: w.write("\n# `%s`\n\n%s" % (k, "".join(d[k]))) + def check_wiki_page(d, f): errors = [] with open(f) as w: @@ -74,17 +85,21 @@ def check_wiki_page(d, f): v = d.pop(m.group(1), "()") if v == "()": errors.append("Missing wiki entry: " + m.group(1)) - keys = d.keys() + keys = list(d.keys()) keys.sort() for k in keys: errors.append("Spurious wiki entry: " + k) if errors: - print "\n".join(errors) + print("\n".join(errors)) sys.exit(1) -if __name__ == "__main__": + +def main(): d = parse_path() if "-c" in sys.argv: check_wiki_page(d, "../rust-clippy.wiki/Home.md") else: write_wiki_page(d, "../rust-clippy.wiki/Home.md") + +if __name__ == "__main__": + main() From 95599c6a620efcb39f130be5a77c46900a71ad0e Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 29 Jan 2016 22:42:19 +0100 Subject: [PATCH 4/5] Synchronise comments with wiki Wiki commits bfa439b and 9b8ced8. --- src/collapsible_if.rs | 2 +- src/derive.rs | 2 ++ src/items_after_statements.rs | 8 ++++++-- 3 files changed, 9 insertions(+), 3 deletions(-) diff --git a/src/collapsible_if.rs b/src/collapsible_if.rs index fb1d7f696d1..dd89b22a40f 100644 --- a/src/collapsible_if.rs +++ b/src/collapsible_if.rs @@ -19,7 +19,7 @@ use syntax::codemap::Spanned; use utils::{in_macro, snippet, snippet_block, span_lint_and_then}; /// **What it does:** This lint checks for nested `if`-statements which can be collapsed by -/// `&&`-combining their conditions and for `else { if .. } expressions that can be collapsed to +/// `&&`-combining their conditions and for `else { if .. }` expressions that can be collapsed to /// `else if ..`. It is `Warn` by default. /// /// **Why is this bad?** Each `if`-statement adds one level of nesting, which makes code look more complex than it really is. diff --git a/src/derive.rs b/src/derive.rs index b1c0bde40a2..ca7649f75b3 100644 --- a/src/derive.rs +++ b/src/derive.rs @@ -29,6 +29,7 @@ use rustc::middle::ty::TypeVariants; /// impl PartialEq for Foo { /// .. /// } +/// ``` declare_lint! { pub DERIVE_HASH_NOT_EQ, Warn, @@ -52,6 +53,7 @@ declare_lint! { /// impl Clone for Foo { /// .. /// } +/// ``` declare_lint! { pub EXPL_IMPL_CLONE_ON_COPY, Warn, diff --git a/src/items_after_statements.rs b/src/items_after_statements.rs index 5f109dac058..8eb3364b2bd 100644 --- a/src/items_after_statements.rs +++ b/src/items_after_statements.rs @@ -5,9 +5,12 @@ use syntax::attr::*; use syntax::ast::*; use utils::in_macro; -/// **What it does:** It `Warn`s on blocks where there are items that are declared in the middle of or after the statements +/// **What it does:** It `Warn`s on blocks where there are items that are declared in the middle of +/// or after the statements /// -/// **Why is this bad?** Items live for the entire scope they are declared in. But statements are processed in order. This might cause confusion as it's hard to figure out which item is meant in a statement. +/// **Why is this bad?** Items live for the entire scope they are declared in. But statements are +/// processed in order. This might cause confusion as it's hard to figure out which item is meant +/// in a statement. /// /// **Known problems:** None /// @@ -23,6 +26,7 @@ use utils::in_macro; /// } /// foo(); // prints "foo" /// } +/// ``` declare_lint! { pub ITEMS_AFTER_STATEMENTS, Warn, "finds blocks where an item comes after a statement" } pub struct ItemsAfterStatemets; From f7bab322f65c3cbc5a312ea02fb6f4781c167e1e Mon Sep 17 00:00:00 2001 From: mcarton Date: Fri, 29 Jan 2016 22:49:48 +0100 Subject: [PATCH 5/5] Fix formatting on wiki --- src/vec.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/vec.rs b/src/vec.rs index b46795a3cdd..41a477c34ff 100644 --- a/src/vec.rs +++ b/src/vec.rs @@ -14,7 +14,7 @@ use utils::{is_expn_of, match_path, snippet, span_lint_and_then}; /// **Known problems:** None. /// /// **Example:** -/// ```rust, ignore +/// ```rust,ignore /// foo(&vec![1, 2]) /// ``` declare_lint! {