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

Merge pull request #579 from oli-obk/single_match_else

Browse Source 
Single match else
This commit is contained in:
Manish Goregaokar 2016-02-01 16:30:03 +05:30
commit fd906c043f
7 changed files with 130 additions and 85 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
README.md
View File

@ -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 104 lints included in this crate:
There are 105 lints included in this crate:
name | default | meaning
---------------------------------------------------------------------------------------------------------------|---------|------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
@ -89,6 +89,7 @@ name
[shadow_unrelated](https://github.com/Manishearth/rust-clippy/wiki#shadow_unrelated) | allow | The name is re-bound without even using the original value
[should_implement_trait](https://github.com/Manishearth/rust-clippy/wiki#should_implement_trait) | warn | defining a method that should be implementing a std trait
[single_match](https://github.com/Manishearth/rust-clippy/wiki#single_match) | warn | a match statement with a single nontrivial arm (i.e, where the other arm is `_ => {}`) is used; recommends `if let` instead
[single_match_else](https://github.com/Manishearth/rust-clippy/wiki#single_match_else) | allow | a match statement with a two arms where the second arm's pattern is a wildcard; recommends `if let` instead
[str_to_string](https://github.com/Manishearth/rust-clippy/wiki#str_to_string) | warn | using `to_string()` on a str, which should be `to_owned()`
[string_add](https://github.com/Manishearth/rust-clippy/wiki#string_add) | allow | using `x + ..` where x is a `String`; suggests using `push_str()` instead
[string_add_assign](https://github.com/Manishearth/rust-clippy/wiki#string_add_assign) | allow | using `x = x + ..` where x is a `String`; suggests using `push_str()` instead

65
src/collapsible_if.rs
View File

@ -54,43 +54,38 @@ impl LateLintPass for CollapsibleIf {
fn check_if(cx: &LateContext, e: &Expr) {
if let ExprIf(ref check, ref then, ref else_) = e.node {
match *else_ {
Some(ref else_) => {
if_let_chain! {[
let ExprBlock(ref block) = else_.node,
block.stmts.is_empty(),
block.rules == BlockCheckMode::DefaultBlock,
let Some(ref else_) = block.expr,
let ExprIf(_, _, _) = else_.node
], {
span_lint_and_then(cx,
COLLAPSIBLE_IF,
block.span,
"this `else { if .. }` block can be collapsed", |db| {
db.span_suggestion(block.span, "try",
format!("else {}",
snippet_block(cx, else_.span, "..")));
});
}}
}
None => {
if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) =
if let Some(ref else_) = *else_ {
if_let_chain! {[
let ExprBlock(ref block) = else_.node,
block.stmts.is_empty(),
block.rules == BlockCheckMode::DefaultBlock,
let Some(ref else_) = block.expr,
let ExprIf(_, _, _) = else_.node
], {
span_lint_and_then(cx,
COLLAPSIBLE_IF,
block.span,
"this `else { if .. }` block can be collapsed", |db| {
db.span_suggestion(block.span, "try",
format!("else {}",
snippet_block(cx, else_.span, "..")));
});
}}
} else if let Some(&Expr{ node: ExprIf(ref check_inner, ref content, None), span: sp, ..}) =
single_stmt_of_block(then) {
if e.span.expn_id != sp.expn_id {
return;
}
span_lint_and_then(cx,
COLLAPSIBLE_IF,
e.span,
"this if statement can be collapsed", |db| {
db.span_suggestion(e.span, "try",
format!("if {} && {} {}",
check_to_string(cx, check),
check_to_string(cx, check_inner),
snippet_block(cx, content.span, "..")));
});
}
if e.span.expn_id != sp.expn_id {
return;
}
span_lint_and_then(cx,
COLLAPSIBLE_IF,
e.span,
"this if statement can be collapsed", |db| {
db.span_suggestion(e.span, "try",
format!("if {} && {} {}",
check_to_string(cx, check),
check_to_string(cx, check_inner),
snippet_block(cx, content.span, "..")));
});
}
}
}

1
src/lib.rs
View File

@ -149,6 +149,7 @@ pub fn plugin_registrar(reg: &mut Registry) {
reg.register_lint_group("clippy_pedantic", vec![
matches::SINGLE_MATCH_ELSE,
methods::OPTION_UNWRAP_USED,
methods::RESULT_UNWRAP_USED,
methods::WRONG_PUB_SELF_CONVENTION,

76
src/matches.rs
View File

@ -28,6 +28,23 @@ declare_lint!(pub SINGLE_MATCH, Warn,
"a match statement with a single nontrivial arm (i.e, where the other arm \
is `_ => {}`) is used; recommends `if let` instead");
/// **What it does:** This lint checks for matches with a two arms where an `if let` will usually suffice. It is `Allow` by default.
///
/// **Why is this bad?** Just readability – `if let` nests less than a `match`.
///
/// **Known problems:** Personal style preferences may differ
///
/// **Example:**
/// ```
/// match x {
/// Some(ref foo) -> bar(foo),
/// _ => bar(other_ref),
/// }
/// ```
declare_lint!(pub SINGLE_MATCH_ELSE, Allow,
"a match statement with a two arms where the second arm's pattern is a wildcard; \
recommends `if let` instead");
/// **What it does:** This lint checks for matches where all arms match a reference, suggesting to remove the reference and deref the matched expression instead. It also checks for `if let &foo = bar` blocks. It is `Warn` by default.
///
/// **Why is this bad?** It just makes the code less readable. That reference destructuring adds nothing to the code.
@ -89,7 +106,7 @@ pub struct MatchPass;
impl LintPass for MatchPass {
fn get_lints(&self) -> LintArray {
lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL)
lint_array!(SINGLE_MATCH, MATCH_REF_PATS, MATCH_BOOL, SINGLE_MATCH_ELSE)
}
}
@ -112,34 +129,49 @@ impl LateLintPass for MatchPass {
fn check_single_match(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) {
if arms.len() == 2 &&
arms[0].pats.len() == 1 && arms[0].guard.is_none() &&
arms[1].pats.len() == 1 && arms[1].guard.is_none() &&
is_unit_expr(&arms[1].body) {
arms[1].pats.len() == 1 && arms[1].guard.is_none() {
let els = if is_unit_expr(&arms[1].body) {
None
} else if let ExprBlock(_) = arms[1].body.node {
// matches with blocks that contain statements are prettier as `if let + else`
Some(&*arms[1].body)
} else {
// allow match arms with just expressions
return;
};
let ty = cx.tcx.expr_ty(ex);
if ty.sty != ty::TyBool || cx.current_level(MATCH_BOOL) == Allow {
check_single_match_single_pattern(cx, ex, arms, expr);
check_single_match_opt_like(cx, ex, arms, expr, ty);
check_single_match_single_pattern(cx, ex, arms, expr, els);
check_single_match_opt_like(cx, ex, arms, expr, ty, els);
}
}
}
fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr) {
fn check_single_match_single_pattern(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, els: Option<&Expr>) {
if arms[1].pats[0].node == PatWild {
let lint = if els.is_some() {
SINGLE_MATCH_ELSE
} else {
SINGLE_MATCH
};
let els_str = els.map_or(String::new(), |els| format!(" else {}", expr_block(cx, els, None, "..")));
span_lint_and_then(cx,
SINGLE_MATCH,
lint,
expr.span,
"you seem to be trying to use match for destructuring a single pattern. \
Consider using `if let`", |db| {
db.span_suggestion(expr.span, "try this",
format!("if let {} = {} {}",
format!("if let {} = {} {}{}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, ex.span, ".."),
expr_block(cx, &arms[0].body, None, "..")));
expr_block(cx, &arms[0].body, None, ".."),
els_str));
});
}
}
fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty) {
// list of candidate Enums we know will never get any more membre
fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr: &Expr, ty: ty::Ty, els: Option<&Expr>) {
// list of candidate Enums we know will never get any more members
let candidates = &[
(&COW_PATH, "Borrowed"),
(&COW_PATH, "Cow::Borrowed"),
@ -151,23 +183,37 @@ fn check_single_match_opt_like(cx: &LateContext, ex: &Expr, arms: &[Arm], expr:
];
let path = match arms[1].pats[0].node {
PatEnum(ref path, _) => path.to_string(),
PatEnum(ref path, Some(ref inner)) => {
// contains any non wildcard patterns? e.g. Err(err)
if inner.iter().any(|pat| if let PatWild = pat.node { false } else { true }) {
return;
}
path.to_string()
},
PatEnum(ref path, None) => path.to_string(),
PatIdent(BindByValue(MutImmutable), ident, None) => ident.node.to_string(),
_ => return
};
for &(ty_path, pat_path) in candidates {
if &path == pat_path && match_type(cx, ty, ty_path) {
let lint = if els.is_some() {
SINGLE_MATCH_ELSE
} else {
SINGLE_MATCH
};
let els_str = els.map_or(String::new(), |els| format!(" else {}", expr_block(cx, els, None, "..")));
span_lint_and_then(cx,
SINGLE_MATCH,
lint,
expr.span,
"you seem to be trying to use match for destructuring a single pattern. \
Consider using `if let`", |db| {
db.span_suggestion(expr.span, "try this",
format!("if let {} = {} {}",
format!("if let {} = {} {}{}",
snippet(cx, arms[0].pats[0].span, ".."),
snippet(cx, ex.span, ".."),
expr_block(cx, &arms[0].body, None, "..")));
expr_block(cx, &arms[0].body, None, ".."),
els_str));
});
}
}

28
src/mut_reference.rs
View File

@ -33,27 +33,19 @@ impl LateLintPass for UnnecessaryMutPassed {
let borrowed_table = cx.tcx.tables.borrow();
match e.node {
ExprCall(ref fn_expr, ref arguments) => {
match borrowed_table.node_types.get(&fn_expr.id) {
Some(function_type) => {
if let ExprPath(_, ref path) = fn_expr.node {
check_arguments(cx, &arguments, function_type, &format!("{}", path));
}
}
None => unreachable!(), // A function with unknown type is called.
// If this happened the compiler would have aborted the
// compilation long ago.
};
let function_type = borrowed_table.node_types
.get(&fn_expr.id)
.expect("A function with an unknown type is called. \
If this happened, the compiler would have \
aborted the compilation long ago");
if let ExprPath(_, ref path) = fn_expr.node {
check_arguments(cx, &arguments, function_type, &format!("{}", path));
}
}
ExprMethodCall(ref name, _, ref arguments) => {
let method_call = MethodCall::expr(e.id);
match borrowed_table.method_map.get(&method_call) {
Some(method_type) => {
check_arguments(cx, &arguments, method_type.ty, &format!("{}", name.node.as_str()))
}
None => unreachable!(), // Just like above, this should never happen.
};
let method_type = borrowed_table.method_map.get(&method_call).expect("This should never happen.");
check_arguments(cx, &arguments, method_type.ty, &format!("{}", name.node.as_str()))
}
_ => {}
}

20
src/utils.rs
View File

@ -657,20 +657,10 @@ pub fn is_expn_of(cx: &LateContext, mut span: Span, name: &str) -> Option<Span>
(ei.callee.name(), ei.call_site)
})
});
return match span_name_span {
Some((mac_name, new_span)) => {
if mac_name.as_str() == name {
Some(new_span)
}
else {
span = new_span;
continue;
}
}
None => {
None
}
};
match span_name_span {
Some((mac_name, new_span)) if mac_name.as_str() == name => return Some(new_span),
None => return None,
Some((_, new_span)) => span = new_span,
}
}
}

22
tests/compile-fail/matches.rs
View File

@ -3,12 +3,32 @@
#![plugin(clippy)]
#![deny(clippy)]
#![allow(unused)]
#![deny(single_match_else)]
use std::borrow::Cow;
enum Foo { Bar, Baz(u8) }
use Foo::*;
enum ExprNode {
ExprAddrOf,
Butterflies,
Unicorns,
}
static NODE: ExprNode = ExprNode::Unicorns;
fn unwrap_addr() -> Option<&'static ExprNode> {
match ExprNode::Butterflies { //~ ERROR you seem to be trying to use match
//~^ HELP try
ExprNode::ExprAddrOf => Some(&NODE),
_ => {
let x = 5;
None
},
}
}
fn single_match(){
let x = Some(1u8);
@ -33,7 +53,7 @@ fn single_match(){
_ => ()
}
// Not linted (content in the else)
// Not linted (no block with statements in the single arm)
match z {
(2...3, 7...9) => println!("{:?}", z),
_ => println!("nope"),