mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

Refactor check_expr() impl for MethodsPass

Browse Source
This commit is contained in:
Devon Hollowood 2015-12-27 01:22:53 -08:00
parent dbf1cdf34a
commit f1aac931bd
2 changed files with 188 additions and 103 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

269
src/methods.rs
View File

@ -5,8 +5,8 @@
use std::iter;
use std::borrow::Cow;
use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, walk_ptrs_ty_depth,
walk_ptrs_ty};
use utils::{snippet, span_lint, span_note_and_lint, match_path, match_type, match_method_chain,
walk_ptrs_ty_depth, walk_ptrs_ty};
use utils::{OPTION_PATH, RESULT_PATH, STRING_PATH};
use self::SelfKind::*;
@ -157,107 +157,21 @@ fn get_lints(&self) -> LintArray {
impl LateLintPass for MethodsPass {
fn check_expr(&mut self, cx: &LateContext, expr: &Expr) {
if let ExprMethodCall(ref name, _, ref args) = expr.node {
let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0]));
match &*name.node.as_str() {
"unwrap" if match_type(cx, obj_ty, &OPTION_PATH) => {
span_lint(cx, OPTION_UNWRAP_USED, expr.span,
"used unwrap() on an Option value. If you don't want \
to handle the None case gracefully, consider using \
expect() to provide a better panic message");
},
"unwrap" if match_type(cx, obj_ty, &RESULT_PATH) => {
span_lint(cx, RESULT_UNWRAP_USED, expr.span,
"used unwrap() on a Result value. Graceful handling \
of Err values is preferred");
},
"to_string" if obj_ty.sty == ty::TyStr => {
let mut arg_str = snippet(cx, args[0].span, "_");
if ptr_depth > 1 {
arg_str = Cow::Owned(format!(
"({}{})",
iter::repeat('*').take(ptr_depth - 1).collect::<String>(),
arg_str));
}
span_lint(cx, STR_TO_STRING, expr.span, &format!(
"`{}.to_owned()` is faster", arg_str));
},
"to_string" if match_type(cx, obj_ty, &STRING_PATH) => {
span_lint(cx, STRING_TO_STRING, expr.span, "`String.to_string()` is a no-op; use \
`clone()` to make a copy");
},
"expect" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
if inner_name.node.as_str() == "ok"
&& match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &RESULT_PATH) {
let result_type = cx.tcx.expr_ty(&inner_args[0]);
if let Some(error_type) = get_error_type(cx, result_type) {
if has_debug_impl(error_type, cx) {
span_lint(cx, OK_EXPECT, expr.span,
"called `ok().expect()` on a Result \
value. You can call `expect` directly \
on the `Result`");
}
}
}
},
// check Option.map(_).unwrap_or(_)
"unwrap_or" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
if inner_name.node.as_str() == "map"
&& match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) {
// lint message
let msg =
"called `map(f).unwrap_or(a)` on an Option value. This can be done \
more directly by calling `map_or(a, f)` instead";
// get args to map() and unwrap_or()
let map_arg = snippet(cx, inner_args[1].span, "..");
let unwrap_arg = snippet(cx, args[1].span, "..");
// lint, with note if neither arg is > 1 line and both map() and
// unwrap_or() have the same span
let multiline = map_arg.lines().count() > 1
|| unwrap_arg.lines().count() > 1;
let same_span = inner_args[1].span.expn_id == args[1].span.expn_id;
if same_span && !multiline {
span_note_and_lint(
cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span,
&format!("replace this with map_or({1}, {0})",
map_arg, unwrap_arg)
);
}
else if same_span && multiline {
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
};
}
},
// check Option.map(_).unwrap_or_else(_)
"unwrap_or_else" => if let ExprMethodCall(ref inner_name, _, ref inner_args) = args[0].node {
if inner_name.node.as_str() == "map"
&& match_type(cx, cx.tcx.expr_ty(&inner_args[0]), &OPTION_PATH) {
// lint message
let msg =
"called `map(f).unwrap_or_else(g)` on an Option value. This can be \
done more directly by calling `map_or_else(g, f)` instead";
// get args to map() and unwrap_or_else()
let map_arg = snippet(cx, inner_args[1].span, "..");
let unwrap_arg = snippet(cx, args[1].span, "..");
// lint, with note if neither arg is > 1 line and both map() and
// unwrap_or_else() have the same span
let multiline = map_arg.lines().count() > 1
|| unwrap_arg.lines().count() > 1;
let same_span = inner_args[1].span.expn_id == args[1].span.expn_id;
if same_span && !multiline {
span_note_and_lint(
cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span,
&format!("replace this with map_or_else({1}, {0})",
map_arg, unwrap_arg)
);
}
else if same_span && multiline {
span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg);
};
}
},
_ => {},
if let ExprMethodCall(_, _, _) = expr.node {
if match_method_chain(expr, &["unwrap"]) {
lint_unwrap(cx, expr);
}
else if match_method_chain(expr, &["to_string"]) {
lint_to_string(cx, expr);
}
else if match_method_chain(expr, &["ok", "expect"]) {
lint_ok_expect(cx, expr);
}
else if match_method_chain(expr, &["map", "unwrap_or"]) {
lint_map_unwrap_or(cx, expr);
}
else if match_method_chain(expr, &["map", "unwrap_or_else"]) {
lint_map_unwrap_or_else(cx, expr);
}
}
}
@ -304,6 +218,155 @@ fn check_item(&mut self, cx: &LateContext, item: &Item) {
}
}
/// lint use of `unwrap()` for `Option`s and `Result`s
fn lint_unwrap(cx: &LateContext, expr: &Expr) {
let args = match expr.node {
ExprMethodCall(_, _, ref args) => args,
_ => panic!("clippy methods.rs: should not have called `lint_unwrap()` on a non-matching \
expression!"),
};
let (obj_ty, _) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0]));
if match_type(cx, obj_ty, &OPTION_PATH) {
span_lint(cx, OPTION_UNWRAP_USED, expr.span,
"used unwrap() on an Option value. If you don't want to handle the None case \
gracefully, consider using expect() to provide a better panic message");
}
else if match_type(cx, obj_ty, &RESULT_PATH) {
span_lint(cx, RESULT_UNWRAP_USED, expr.span,
"used unwrap() on a Result value. Graceful handling of Err values is preferred");
}
}
/// lint use of `to_string()` for `&str`s and `String`s
fn lint_to_string(cx: &LateContext, expr: &Expr) {
let args = match expr.node {
ExprMethodCall(_, _, ref args) => args,
_ => panic!("clippy methods.rs: should not have called `lint_to_string()` on a \
non-matching expression!"),
};
let (obj_ty, ptr_depth) = walk_ptrs_ty_depth(cx.tcx.expr_ty(&args[0]));
if obj_ty.sty == ty::TyStr {
let mut arg_str = snippet(cx, args[0].span, "_");
if ptr_depth > 1 {
arg_str = Cow::Owned(format!(
"({}{})",
iter::repeat('*').take(ptr_depth - 1).collect::<String>(),
arg_str));
}
span_lint(cx, STR_TO_STRING, expr.span,
&format!("`{}.to_owned()` is faster", arg_str));
}
else if match_type(cx, obj_ty, &STRING_PATH) {
span_lint(cx, STRING_TO_STRING, expr.span,
"`String.to_string()` is a no-op; use `clone()` to make a copy");
}
}
/// lint use of `ok().expect()` for `Result`s
fn lint_ok_expect(cx: &LateContext, expr: &Expr) {
let expect_args = match expr.node {
ExprMethodCall(_, _, ref expect_args) => expect_args,
_ => panic!("clippy methods.rs: Should not have called `lint_ok_expect()` on a \
non-matching expression!")
};
let ok_args = match expect_args[0].node {
ExprMethodCall(_, _, ref ok_args) => ok_args,
_ => panic!("clippy methods.rs: Should not have called `lint_ok_expect()` on a \
non-matching expression!")
};
// lint if the caller of `ok()` is a `Result`
if match_type(cx, cx.tcx.expr_ty(&ok_args[0]), &RESULT_PATH) {
let result_type = cx.tcx.expr_ty(&ok_args[0]);
if let Some(error_type) = get_error_type(cx, result_type) {
if has_debug_impl(error_type, cx) {
span_lint(cx, OK_EXPECT, expr.span,
"called `ok().expect()` on a Result value. You can call `expect` \
directly on the `Result`");
}
}
}
}
/// lint use of `map().unwrap_or()` for `Option`s
fn lint_map_unwrap_or(cx: &LateContext, expr: &Expr) {
let unwrap_args = match expr.node {
ExprMethodCall(_, _, ref unwrap_args) => unwrap_args,
_ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or()` on a \
non-matching expression!")
};
let map_args = match unwrap_args[0].node {
ExprMethodCall(_, _, ref map_args) => map_args,
_ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or()` on a \
non-matching expression!")
};
// lint if the caller of `map()` is an `Option`
if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) {
// lint message
let msg = "called `map(f).unwrap_or(a)` on an Option value. This can be done more \
directly by calling `map_or(a, f)` instead";
// get snippets for args to map() and unwrap_or()
let map_snippet = snippet(cx, map_args[1].span, "..");
let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
// lint, with note if neither arg is > 1 line and both map() and
// unwrap_or() have the same span
let multiline = map_snippet.lines().count() > 1
|| unwrap_snippet.lines().count() > 1;
let same_span = map_args[1].span.expn_id == unwrap_args[1].span.expn_id;
if same_span && !multiline {
span_note_and_lint(
cx, OPTION_MAP_UNWRAP_OR, expr.span, msg, expr.span,
&format!("replace this with map_or({1}, {0})", map_snippet, unwrap_snippet)
);
}
else if same_span && multiline {
span_lint(cx, OPTION_MAP_UNWRAP_OR, expr.span, msg);
};
}
}
/// lint use of `map().unwrap_or_else()` for `Option`s
fn lint_map_unwrap_or_else(cx: &LateContext, expr: &Expr) {
let unwrap_args = match expr.node {
ExprMethodCall(_, _, ref unwrap_args) => unwrap_args,
_ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or_else()` on a \
non-matching expression!")
};
let map_args = match unwrap_args[0].node {
ExprMethodCall(_, _, ref map_args) => map_args,
_ => panic!("clippy methods.rs: Should not have called `lint_map_unwrap_or_else()` on a \
non-matching expression!")
};
if match_type(cx, cx.tcx.expr_ty(&map_args[0]), &OPTION_PATH) {
// lint message
let msg = "called `map(f).unwrap_or_else(g)` on an Option value. This can be done more \
directly by calling `map_or_else(g, f)` instead";
// get snippets for args to map() and unwrap_or_else()
let map_snippet = snippet(cx, map_args[1].span, "..");
let unwrap_snippet = snippet(cx, unwrap_args[1].span, "..");
// lint, with note if neither arg is > 1 line and both map() and
// unwrap_or_else() have the same span
let multiline = map_snippet.lines().count() > 1
|| unwrap_snippet.lines().count() > 1;
let same_span = map_args[1].span.expn_id == unwrap_args[1].span.expn_id;
if same_span && !multiline {
span_note_and_lint(
cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg, expr.span,
&format!("replace this with map_or_else({1}, {0})", map_snippet, unwrap_snippet)
);
}
else if same_span && multiline {
span_lint(cx, OPTION_MAP_UNWRAP_OR_ELSE, expr.span, msg);
};
}
}
// Given a `Result<T, E>` type, return its error type (`E`)
fn get_error_type<'a>(cx: &LateContext, ty: ty::Ty<'a>) -> Option<ty::Ty<'a>> {
if !match_type(cx, ty, &RESULT_PATH) {

22
src/utils.rs
View File

@ -136,6 +136,7 @@ pub fn match_impl_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
false
}
}
/// check if method call given in "expr" belongs to given trait
pub fn match_trait_method(cx: &LateContext, expr: &Expr, path: &[&str]) -> bool {
let method_call = ty::MethodCall::expr(expr.id);
@ -163,6 +164,27 @@ pub fn match_path_ast(path: &ast::Path, segments: &[&str]) -> bool {
|(a, b)| a.identifier.name.as_str() == *b)
}
/// match an Expr against a chain of methods. For example, if `expr` represents the `.baz()` in
/// `foo.bar().baz()`, `matched_method_chain(expr, &["bar", "baz"])` will return true.
pub fn match_method_chain(expr: &Expr, methods: &[&str]) -> bool {
let mut current = &expr.node ;
for method_name in methods.iter().rev() { // method chains are stored last -> first
if let ExprMethodCall(ref name, _, ref args) = *current {
if name.node.as_str() == *method_name {
current = &args[0].node
}
else {
return false;
}
}
else {
return false;
}
}
true
}
/// get the name of the item the expression is in, if available
pub fn get_item_name(cx: &LateContext, expr: &Expr) -> Option<Name> {
let parent_id = cx.tcx.map.get_parent(expr.id);