OpenE2K/rust
2
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Improve OR_FUN_CALL to suggest unwrap_or_default

Browse Source
This commit is contained in:
mcarton 2016-01-18 13:11:07 +01:00
parent fb6b3bed0f
commit b5f65ec699
2 changed files with 125 additions and 34 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

105
src/methods.rs
View File

@ -5,11 +5,13 @@ 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};
use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
walk_ptrs_ty_depth, walk_ptrs_ty, get_trait_def_id, implements_trait};
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::*;
@ -172,7 +174,7 @@ declare_lint!(pub SEARCH_IS_SOME, Warn,
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., instead.
/// 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:
@ -183,10 +185,13 @@ declare_lint!(pub SEARCH_IS_SOME, Warn,
/// ```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. The will won't catch
/// `foo.unwrap_or(vec![])`.
/// 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");
@ -284,8 +289,61 @@ 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>]) {
if args.len() == 2 && ["map_or", "ok_or", "or", "unwrap_or"].contains(&name) {
let self_ty = cx.tcx.expr_ty(&args[0]);
/// 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
@ -297,20 +355,27 @@ fn lint_or_fun_call(cx: &LateContext, expr: &Expr, name: &str, args: &[P<Expr>])
return;
};
if let ExprCall(ref fun, ref or_args) = args[1].node {
let sugg = match (is_result, or_args.is_empty()) {
(true, _) => format!("|_| {}", snippet(cx, args[1].span, "..")),
(false, false) => format!("|| {}", snippet(cx, args[1].span, "..")),
(false, true) => format!("{}", snippet(cx, fun.span, "..")),
};
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, expr.span,
&format!("use of `{}` followed by a function call", name))
.span_suggestion(expr.span, "try this",
format!("{}.{}_else({})",
snippet(cx, args[0].span, "_"),
name,
sugg));
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);
}
}
}
}

54
tests/compile-fail/methods.rs
View File

@ -177,29 +177,55 @@ fn search_is_some() {
/// Checks implementation of the OR_FUN_CALL lint
fn or_fun_call() {
let foo = Some(vec![1]);
foo.unwrap_or(Vec::new());
//~^ERROR use of `unwrap_or`
//~|HELP try this
//~|SUGGESTION foo.unwrap_or_else(Vec::new);
fn make<T>() -> T { unimplemented!(); }
let bar = Some(vec![1]);
bar.unwrap_or(Vec::with_capacity(12));
let with_constructor = Some(vec![1]);
with_constructor.unwrap_or(make());
//~^ERROR use of `unwrap_or`
//~|HELP try this
//~|SUGGESTION bar.unwrap_or_else(|| Vec::with_capacity(12));
//~|SUGGESTION with_constructor.unwrap_or_else(make)
let baz : Result<_, ()> = Ok(vec![1]);
baz.unwrap_or(Vec::new());
let with_new = Some(vec![1]);
with_new.unwrap_or(Vec::new());
//~^ERROR use of `unwrap_or`
//~|HELP try this
//~|SUGGESTION baz.unwrap_or_else(|_| Vec::new());
//~|SUGGESTION with_new.unwrap_or_default();
let qux : Result<_, ()> = Ok(vec![1]);
qux.unwrap_or(Vec::with_capacity(12));
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 qux.unwrap_or_else(|_| Vec::with_capacity(12));
//~|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![]);
}
fn main() {