Merge pull request #556 from mcarton/or_fun_call

New lint, new utility functions and nightly fix

README.md

@@ -6,7 +6,7 @@ A collection of lints to catch common mistakes and improve your Rust code.
 [Jump to usage instructions](#usage)
 
 ##Lints
-There are 92 lints included in this crate:
+There are 93 lints included in this crate:
 
 name                                                                                                           | default | meaning
 ---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@@ -63,6 +63,7 @@ name
 [option_map_unwrap_or](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or)                   | warn    | using `Option.map(f).unwrap_or(a)`, which is more succinctly expressed as `map_or(a, f)`
 [option_map_unwrap_or_else](https://github.com/Manishearth/rust-clippy/wiki#option_map_unwrap_or_else)         | warn    | using `Option.map(f).unwrap_or_else(g)`, which is more succinctly expressed as `map_or_else(g, f)`
 [option_unwrap_used](https://github.com/Manishearth/rust-clippy/wiki#option_unwrap_used)                       | allow   | using `Option.unwrap()`, which should at least get a better message using `expect()`
+[or_fun_call](https://github.com/Manishearth/rust-clippy/wiki#or_fun_call)                                     | warn    | using any `*or` method when the `*or_else` would do
 [out_of_bounds_indexing](https://github.com/Manishearth/rust-clippy/wiki#out_of_bounds_indexing)               | deny    | out of bound constant indexing
 [panic_params](https://github.com/Manishearth/rust-clippy/wiki#panic_params)                                   | warn    | missing parameters in `panic!`
 [precedence](https://github.com/Manishearth/rust-clippy/wiki#precedence)                                       | warn    | catches operations where precedence may be unclear. See the wiki for a list of cases caught

src/bit_mask.rs

@@ -278,7 +278,7 @@ fn fetch_int_literal(cx: &LateContext, lit: &Expr) -> Option<u64> {
                     _ => None,
                 }
             }
-            .and_then(|def_id| lookup_const_by_id(cx.tcx, def_id, None))
+            .and_then(|def_id| lookup_const_by_id(cx.tcx, def_id, None, None))
             .and_then(|l| fetch_int_literal(cx, l))
         }
         _ => None,

src/consts.rs

@@ -484,9 +484,9 @@ impl<'c, 'cc> ConstEvalLateContext<'c, 'cc> {
             if let Some(&PathResolution { base_def: DefConst(id), ..}) = lcx.tcx.def_map.borrow().get(&e.id) {
                 maybe_id = Some(id);
             }
-            // separate if lets to avoid doubleborrowing the defmap
+            // separate if lets to avoid double borrowing the def_map
             if let Some(id) = maybe_id {
-                if let Some(const_expr) = lookup_const_by_id(lcx.tcx, id, None) {
+                if let Some(const_expr) = lookup_const_by_id(lcx.tcx, id, None, None) {
                     let ret = self.expr(const_expr);
                     if ret.is_some() {
                         self.needed_resolution = true;

src/escape.rs

@@ -52,7 +52,7 @@ impl LintPass for EscapePass {
 impl LateLintPass for EscapePass {
     fn check_fn(&mut self, cx: &LateContext, _: visit::FnKind, decl: &FnDecl, body: &Block, _: Span, id: NodeId) {
         let param_env = ty::ParameterEnvironment::for_item(cx.tcx, id);
-        let infcx = infer::new_infer_ctxt(cx.tcx, &cx.tcx.tables, Some(param_env), false);
+        let infcx = infer::new_infer_ctxt(cx.tcx, &cx.tcx.tables, Some(param_env));
         let mut v = EscapeDelegate {
             cx: cx,
             set: NodeSet(),

src/lib.rs

@@ -194,6 +194,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
         methods::OK_EXPECT,
         methods::OPTION_MAP_UNWRAP_OR,
         methods::OPTION_MAP_UNWRAP_OR_ELSE,
+        methods::OR_FUN_CALL,
         methods::SEARCH_IS_SOME,
         methods::SHOULD_IMPLEMENT_TRAIT,
         methods::STR_TO_STRING,

src/methods.rs

@@ -4,11 +4,14 @@ use rustc::middle::ty;
 use rustc::middle::subst::{Subst, TypeSpace};
 use std::iter;
 use std::borrow::Cow;
+use syntax::ptr::P;
+use syntax::codemap::Span;
 
 use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, method_chain_args, match_trait_method,
-            walk_ptrs_ty_depth, walk_ptrs_ty};
+            walk_ptrs_ty_depth, walk_ptrs_ty, get_trait_def_id, implements_trait};
-use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
+use utils::{DEFAULT_TRAIT_PATH, OPTION_PATH, RESULT_PATH, STRING_PATH};
 use utils::MethodArgs;
+use rustc::middle::cstore::CrateStore;
 
 use self::SelfKind::*;
 use self::OutType::*;
@@ -170,6 +173,28 @@ declare_lint!(pub SEARCH_IS_SOME, Warn,
               "using an iterator search followed by `is_some()`, which is more succinctly \
                expressed as a call to `any()`");
 
+/// **What it does:** This lint checks for calls to `.or(foo(..))`, `.unwrap_or(foo(..))`, etc., and
+/// suggests to use `or_else`, `unwrap_or_else`, etc., or `unwrap_or_default` instead.
+///
+/// **Why is this bad?** The function will always be called and potentially allocate an object
+/// in expressions such as:
+/// ```rust
+/// foo.unwrap_or(String::new())
+/// ```
+/// this can instead be written:
+/// ```rust
+/// foo.unwrap_or_else(String::new)
+/// ```
+/// or
+/// ```rust
+/// foo.unwrap_or_default()
+/// ```
+///
+/// **Known problems:** If the function as side-effects, not calling it will change the semantic of
+/// the program, but you shouldn't rely on that anyway.
+declare_lint!(pub OR_FUN_CALL, Warn,
+              "using any `*or` method when the `*or_else` would do");
+
 impl LintPass for MethodsPass {
     fn get_lints(&self) -> LintArray {
         lint_array!(OPTION_UNWRAP_USED,
@@ -181,13 +206,15 @@ impl LintPass for MethodsPass {
                     WRONG_PUB_SELF_CONVENTION,
                     OK_EXPECT,
                     OPTION_MAP_UNWRAP_OR,
-                    OPTION_MAP_UNWRAP_OR_ELSE)
+                    OPTION_MAP_UNWRAP_OR_ELSE,
+                    OR_FUN_CALL)
     }
 }
 
 impl LateLintPass for MethodsPass {
     fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
-        if let ExprMethodCall(_, _, _) = expr.node {
+        if let ExprMethodCall(name, _, ref args) = expr.node {
+            // Chain calls
             if let Some(arglists) = method_chain_args(expr, &["unwrap"]) {
                 lint_unwrap(cx, expr, arglists[0]);
             } else if let Some(arglists) = method_chain_args(expr, &["to_string"]) {
@@ -207,6 +234,8 @@ impl LateLintPass for MethodsPass {
             } else if let Some(arglists) = method_chain_args(expr, &["rposition", "is_some"]) {
                 lint_search_is_some(cx, expr, "rposition", arglists[0], arglists[1]);
             }
+
+            lint_or_fun_call(cx, expr, &name.node.as_str(), &args);
         }
     }
 
@@ -258,6 +287,99 @@ impl LateLintPass for MethodsPass {
     }
 }
 
+/// Checks for the `OR_FUN_CALL` lint.
+fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P<Expr>]) {
+    /// Check for `unwrap_or(T::new())` or `unwrap_or(T::default())`.
+    fn check_unwrap_or_default(
+        cx: &LateContext,
+        name: &str,
+        fun: &Expr,
+        self_expr: &Expr,
+        arg: &Expr,
+        or_has_args: bool,
+        span: Span
+    ) -> bool {
+        if or_has_args {
+            return false;
+        }
+
+        if name == "unwrap_or" {
+            if let ExprPath(_, ref path) = fun.node {
+                let path : &str = &path.segments.last()
+                    .expect("A path must have at least one segment")
+                    .identifier.name.as_str();
+
+                if ["default", "new"].contains(&path) {
+                    let arg_ty = cx.tcx.expr_ty(arg);
+                    let default_trait_id = if let Some(default_trait_id) = get_trait_def_id(cx, &DEFAULT_TRAIT_PATH) {
+                        default_trait_id
+                    }
+                    else {
+                        return false;
+                    };
+
+                    if implements_trait(cx, arg_ty, default_trait_id) {
+                        span_lint(cx, OR_FUN_CALL, span,
+                                  &format!("use of `{}` followed by a call to `{}`", name, path))
+                            .span_suggestion(span, "try this",
+                                             format!("{}.unwrap_or_default()",
+                                                     snippet(cx, self_expr.span, "_")));
+                        return true;
+                    }
+                }
+            }
+        }
+
+        false
+    }
+
+    /// Check for `*or(foo())`.
+    fn check_general_case(
+        cx: &LateContext,
+        name: &str,
+        fun: &Expr,
+        self_expr: &Expr,
+        arg: &Expr,
+        or_has_args: bool,
+        span: Span
+    ) {
+        let self_ty = cx.tcx.expr_ty(self_expr);
+
+        let is_result = if match_type(cx, self_ty, &RESULT_PATH) {
+            true
+        }
+        else if match_type(cx, self_ty, &OPTION_PATH) {
+            false
+        }
+        else {
+            return;
+        };
+
+        let sugg = match (is_result, !or_has_args) {
+            (true, _) => format!("|_| {}", snippet(cx, arg.span, "..")),
+            (false, false) => format!("|| {}", snippet(cx, arg.span, "..")),
+            (false, true) => format!("{}", snippet(cx, fun.span, "..")),
+        };
+
+        span_lint(cx, OR_FUN_CALL, span,
+                  &format!("use of `{}` followed by a function call", name))
+            .span_suggestion(span, "try this",
+                             format!("{}.{}_else({})",
+                                     snippet(cx, self_expr.span, "_"),
+                                     name,
+                                     sugg));
+    }
+
+    if args.len() == 2 && ["map_or", "ok_or", "or", "unwrap_or"].contains(&name) {
+        if let ExprCall(ref fun, ref or_args) = args[1].node {
+            let or_has_args = !or_args.is_empty();
+            if !check_unwrap_or_default(cx, name, fun, &args[0], &args[1], or_has_args, expr.span) {
+                check_general_case(cx, name, fun, &args[0], &args[1], or_has_args, expr.span);
+            }
+        }
+    }
+}
+
 #[allow(ptr_arg)]
 // Type of MethodArgs is potentially a Vec
 /// lint use of `unwrap()` for `Option`s and `Result`s

src/utils.rs

@@ -1,20 +1,20 @@
-use rustc::lint::*;
+use consts::constant;
-use rustc_front::hir::*;
 use reexport::*;
-use syntax::codemap::{ExpnInfo, Span, ExpnFormat};
 use rustc::front::map::Node::*;
+use rustc::lint::*;
 use rustc::middle::def_id::DefId;
-use rustc::middle::ty;
+use rustc::middle::{cstore, def, infer, ty, traits};
+use rustc::session::Session;
+use rustc_front::hir::*;
 use std::borrow::Cow;
+use std::mem;
+use std::ops::{Deref, DerefMut};
+use std::str::FromStr;
 use syntax::ast::Lit_::*;
 use syntax::ast;
+use syntax::codemap::{ExpnInfo, Span, ExpnFormat};
 use syntax::errors::DiagnosticBuilder;
 use syntax::ptr::P;
-use consts::constant;
-
-use rustc::session::Session;
-use std::str::FromStr;
-use std::ops::{Deref, DerefMut};
 
 pub type MethodArgs = HirVec<P<Expr>>;
 
@@ -23,6 +23,7 @@ pub const BEGIN_UNWIND: [&'static str; 3] = ["std", "rt", "begin_unwind"];
 pub const BTREEMAP_PATH: [&'static str; 4] = ["collections", "btree", "map", "BTreeMap"];
 pub const CLONE_PATH: [&'static str; 2] = ["Clone", "clone"];
 pub const COW_PATH: [&'static str; 3] = ["collections", "borrow", "Cow"];
+pub const DEFAULT_TRAIT_PATH: [&'static str; 3] = ["core", "default", "Default"];
 pub const HASHMAP_PATH: [&'static str; 5] = ["std", "collections", "hash", "map", "HashMap"];
 pub const LL_PATH: [&'static str; 3] = ["collections", "linked_list", "LinkedList"];
 pub const MUTEX_PATH: [&'static str; 4] = ["std", "sync", "mutex", "Mutex"];
@@ -132,7 +133,7 @@ pub fn match_type(cx: &LateContext, ty: ty::Ty, path: &[&str]) -> bool {
     }
 }
 
-/// Check if the method call given in `expr` belongs to given trait.
+/// Check if the method call given in `expr` belongs to given type.
 pub fn match_impl_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
     let method_call = ty::MethodCall::expr(expr.id);
 
@@ -186,6 +187,73 @@ pub fn match_path_ast(path: &ast::Path, segments: &[&str]) -> bool {
     path.segments.iter().rev().zip(segments.iter().rev()).all(|(a, b)| a.identifier.name.as_str() == *b)
 }
 
+/// Get the definition associated to a path.
+/// TODO: investigate if there is something more efficient for that.
+pub fn path_to_def(cx: &LateContext, path: &[&str]) -> Option<cstore::DefLike> {
+    let cstore = &cx.tcx.sess.cstore;
+
+    let crates = cstore.crates();
+    let krate = crates.iter().find(|&&krate| cstore.crate_name(krate) == path[0]);
+    if let Some(krate) = krate {
+        let mut items = cstore.crate_top_level_items(*krate);
+        let mut path_it = path.iter().skip(1).peekable();
+
+        loop {
+            let segment = match path_it.next() {
+                Some(segment) => segment,
+                None => return None
+            };
+
+            for item in &mem::replace(&mut items, vec![]) {
+                if item.name.as_str() == *segment {
+                    if path_it.peek().is_none() {
+                        return Some(item.def);
+                    }
+
+                    let def_id = match item.def {
+                        cstore::DefLike::DlDef(def) => def.def_id(),
+                        cstore::DefLike::DlImpl(def_id) => def_id,
+                        _ => panic!("Unexpected {:?}", item.def),
+                    };
+
+                    items = cstore.item_children(def_id);
+                    break;
+                }
+            }
+        }
+    }
+    else {
+        None
+    }
+}
+
+/// Convenience function to get the `DefId` of a trait by path.
+pub fn get_trait_def_id(cx: &LateContext, path: &[&str]) -> Option<DefId> {
+    let def = match path_to_def(cx, path) {
+        Some(def) => def,
+        None => return None,
+    };
+
+    match def {
+        cstore::DlDef(def::DefTrait(trait_id)) => Some(trait_id),
+        _ => None,
+    }
+}
+
+/// Check whether a type implements a trait.
+/// See also `get_trait_def_id`.
+pub fn implements_trait<'a, 'tcx>(cx: &LateContext<'a, 'tcx>, ty: ty::Ty<'tcx>, trait_id: DefId) -> bool {
+    cx.tcx.populate_implementations_for_trait_if_necessary(trait_id);
+
+    let infcx = infer::new_infer_ctxt(cx.tcx, &cx.tcx.tables, None);
+    let obligation = traits::predicate_for_trait_def(cx.tcx,
+                                                     traits::ObligationCause::dummy(),
+                                                     trait_id, 0, ty,
+                                                     vec![]);
+
+    traits::SelectionContext::new(&infcx).evaluate_obligation_conservatively(&obligation)
+}
+
 /// Match an `Expr` against a chain of methods, and return the matched `Expr`s.
 ///
 /// For example, if `expr` represents the `.baz()` in `foo.bar().baz()`,
@@ -244,7 +312,7 @@ pub fn is_from_for_desugar(decl: &Decl) -> bool {
 /// snippet(cx, expr.span, "..")
 /// ```
 pub fn snippet<'a, T: LintContext>(cx: &T, span: Span, default: &'a str) -> Cow<'a, str> {
-    cx.sess().codemap().span_to_snippet(span).map(From::from).unwrap_or(Cow::Borrowed(default))
+    cx.sess().codemap().span_to_snippet(span).map(From::from).unwrap_or_else(|_| Cow::Borrowed(default))
 }
 
 /// Convert a span to a code snippet. Returns `None` if not available.

tests/compile-fail/methods.rs

@@ -175,6 +175,71 @@ fn search_is_some() {
     let _ = foo.rposition().is_some();
 }
 
+/// Checks implementation of the OR_FUN_CALL lint
+fn or_fun_call() {
+    struct Foo;
+
+    impl Foo {
+        fn new() -> Foo { Foo }
+    }
+
+    fn make<T>() -> T { unimplemented!(); }
+
+    let with_constructor = Some(vec![1]);
+    with_constructor.unwrap_or(make());
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_constructor.unwrap_or_else(make)
+
+    let with_new = Some(vec![1]);
+    with_new.unwrap_or(Vec::new());
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_new.unwrap_or_default();
+
+    let with_const_args = Some(vec![1]);
+    with_const_args.unwrap_or(Vec::with_capacity(12));
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_const_args.unwrap_or_else(|| Vec::with_capacity(12));
+
+    let with_err : Result<_, ()> = Ok(vec![1]);
+    with_err.unwrap_or(make());
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_err.unwrap_or_else(|_| make());
+
+    let with_err_args : Result<_, ()> = Ok(vec![1]);
+    with_err_args.unwrap_or(Vec::with_capacity(12));
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_err_args.unwrap_or_else(|_| Vec::with_capacity(12));
+
+    let with_default_trait = Some(1);
+    with_default_trait.unwrap_or(Default::default());
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_default_trait.unwrap_or_default();
+
+    let with_default_type = Some(1);
+    with_default_type.unwrap_or(u64::default());
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_default_type.unwrap_or_default();
+
+    let with_vec = Some(vec![1]);
+    with_vec.unwrap_or(vec![]);
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION with_vec.unwrap_or_else(|| vec![]);
+
+    let without_default = Some(Foo);
+    without_default.unwrap_or(Foo::new());
+    //~^ERROR use of `unwrap_or`
+    //~|HELP try this
+    //~|SUGGESTION without_default.unwrap_or_else(Foo::new);
+}
+
 fn main() {
     use std::io;
 

tests/compile-fail/needless_features.rs

@@ -1,5 +1,4 @@
 #![feature(plugin)]
-#![feature(convert)]
 #![plugin(clippy)]
 
 #![deny(clippy)]