Merge pull request #599 from mcarton/lt

Fix false positive with NEEDLESS_LIFETIMES and some cleanup

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.

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,

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);
             });
         }
     }

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;

src/lifetimes.rs

@@ -65,13 +65,30 @@ enum RefLt {
     Static,
     Named(Name),
 }
-use self::RefLt::*;
+
+fn bound_lifetimes(bound: &TyParamBound) -> Option<HirVec<&Lifetime>> {
+    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<Item=&'a Lifetime>>(
+    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<RefLt> {
     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<Item=&'a Lifetime>>(mut vec: Vec<RefLt>, bounds_lts: T) -> Vec<RefLt> {
+    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::<HashSet<_>>().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<RefLt>,
 }
 
@@ -187,12 +217,12 @@ impl<'v, 't> RefVisitor<'v, 't> {
     fn record(&mut self, lifetime: &Option<Lifetime>) {
         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);
         }
     }
 

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);
             }
         });
     }

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<Span> {
     loop {
         let span_name_span = cx.tcx.sess.codemap().with_expn_info(span.expn_id, |expn| {

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! {

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() {
 }

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.
 

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()