From 919601bc511608f1b66fe7c51d4b879e2abdec40 Mon Sep 17 00:00:00 2001 From: Wilco Kusee Date: Tue, 19 Dec 2017 23:22:16 +0100 Subject: [PATCH 1/2] Lint for matching option as ref --- clippy_lints/src/lib.rs | 1 + clippy_lints/src/matches.rs | 75 +++++++++++++++++++++++++++++++++++-- tests/ui/matches.rs | 15 ++++++++ tests/ui/matches.stderr | 22 +++++++++++ 4 files changed, 110 insertions(+), 3 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 6ceb6176b..9b598df4f 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -499,6 +499,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) { loops::WHILE_LET_LOOP, loops::WHILE_LET_ON_ITERATOR, map_clone::MAP_CLONE, + matches::MATCH_AS_REF, matches::MATCH_BOOL, matches::MATCH_OVERLAPPING_ARM, matches::MATCH_REF_PATS, diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index dd3b2f00b..326ce83a8 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -11,8 +11,8 @@ use syntax::ast::LitKind; use syntax::ast::NodeId; use syntax::codemap::Span; use utils::paths; -use utils::{expr_block, in_external_macro, is_allowed, is_expn_of, match_type, remove_blocks, snippet, - span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty}; +use utils::{expr_block, in_external_macro, is_allowed, is_expn_of, match_qpath, match_type, remove_blocks, + snippet, span_lint_and_sugg, span_lint_and_then, span_note_and_lint, walk_ptrs_ty}; use utils::sugg::Sugg; /// **What it does:** Checks for matches with a single arm where an `if let` @@ -145,6 +145,27 @@ declare_lint! { "a match with `Err(_)` arm and take drastic actions" } +/// **What it does:** Checks for match which is used to add a reference to an +/// `Option` value. +/// +/// **Why is this bad?** Using `as_ref()` instead is shorter. +/// +/// **Known problems:** None. +/// +/// **Example:** +/// ```rust +/// let x: Option<()> = None; +/// let r: Option<&()> = match x { +/// None => None, +/// Some(ref v) => Some(v), +/// }; +/// ``` +declare_lint! { + pub MATCH_AS_REF, + Warn, + "a match on an Option value instead of using `as_ref()`" +} + #[allow(missing_copy_implementations)] pub struct MatchPass; @@ -156,7 +177,8 @@ impl LintPass for MatchPass { MATCH_BOOL, SINGLE_MATCH_ELSE, MATCH_OVERLAPPING_ARM, - MATCH_WILD_ERR_ARM + MATCH_WILD_ERR_ARM, + MATCH_AS_REF ) } } @@ -171,6 +193,7 @@ impl<'a, 'tcx> LateLintPass<'a, 'tcx> for MatchPass { check_match_bool(cx, ex, arms, expr); check_overlapping_arms(cx, ex, arms); check_wild_err_arm(cx, ex, arms); + check_match_as_ref(cx, ex, arms, expr); } if let ExprMatch(ref ex, ref arms, source) = expr.node { check_match_ref_pats(cx, ex, arms, source, expr); @@ -411,6 +434,24 @@ fn check_match_ref_pats(cx: &LateContext, ex: &Expr, arms: &[Arm], source: Match } } +fn check_match_as_ref(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() { + if (is_ref_some_arm(&arms[0]) && is_none_arm(&arms[1])) || + (is_ref_some_arm(&arms[1]) && is_none_arm(&arms[0])) { + span_lint_and_sugg( + cx, + MATCH_AS_REF, + expr.span, + "use as_ref() instead", + "try this", + format!("{}.as_ref()", snippet(cx, ex.span, "_")) + ) + } + } +} + /// Get all arms that are unbounded `PatRange`s. fn all_ranges<'a, 'tcx>( cx: &LateContext<'a, 'tcx>, @@ -524,6 +565,34 @@ fn is_unit_expr(expr: &Expr) -> bool { } } +// Checks if arm has the form `None => None` +fn is_none_arm(arm: &Arm) -> bool { + match arm.pats[0].node { + PatKind::Path(ref path) if match_qpath(path, &paths::OPTION_NONE) => true, + _ => false, + } +} + +// Checks if arm has the form `Some(ref v) => Some(v)` (checks for `ref` and `ref mut`) +fn is_ref_some_arm(arm: &Arm) -> bool { + if_chain! { + if let PatKind::TupleStruct(ref path, ref pats, _) = arm.pats[0].node; + if pats.len() == 1 && match_qpath(path, &paths::OPTION_SOME); + if let PatKind::Binding(rb, _, ref ident, _) = pats[0].node; + if rb == BindingAnnotation::Ref || rb == BindingAnnotation::RefMut; + if let ExprCall(ref e, ref args) = remove_blocks(&arm.body).node; + if let ExprPath(ref some_path) = e.node; + if match_qpath(some_path, &paths::OPTION_SOME) && args.len() == 1; + if let ExprPath(ref qpath) = args[0].node; + if let &QPath::Resolved(_, ref path2) = qpath; + if path2.segments.len() == 1; + then { + return ident.node == path2.segments[0].name + } + } + false +} + fn has_only_ref_pats(arms: &[Arm]) -> bool { let mapped = arms.iter() .flat_map(|a| &a.pats) diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index 8130436d4..72a36338a 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -315,5 +315,20 @@ fn match_wild_err_arm() { } } +fn match_as_ref() { + let owned : Option<()> = None; + let borrowed = match owned { + None => None, + Some(ref v) => Some(v), + }; + + let mut mut_owned : Option<()> = None; + let mut mut_borrowed = match mut_owned { + None => None, + Some(ref mut v) => Some(v), + }; + +} + fn main() { } diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index 8c0ec49e6..3d7b8f784 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -426,3 +426,25 @@ note: consider refactoring into `Ok(3) | Ok(_)` | ^^^^^^^^^^^^^^ = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) +error: use as_ref() instead + --> $DIR/matches.rs:320:20 + | +320 | let borrowed = match owned { + | ____________________^ +321 | | None => None, +322 | | Some(ref v) => Some(v), +323 | | }; + | |_____^ help: try this: `owned.as_ref()` + | + = note: `-D match-as-ref` implied by `-D warnings` + +error: use as_ref() instead + --> $DIR/matches.rs:326:28 + | +326 | let mut mut_borrowed = match mut_owned { + | ____________________________^ +327 | | None => None, +328 | | Some(ref mut v) => Some(v), +329 | | }; + | |_____^ help: try this: `mut_owned.as_ref()` + From a6ccc6fe3d67f4802d58a9a76a4f1d308a8a96f3 Mon Sep 17 00:00:00 2001 From: Wilco Kusee Date: Wed, 20 Dec 2017 10:39:48 +0100 Subject: [PATCH 2/2] Also suggest as_mut for match_as_ref --- clippy_lints/src/matches.rs | 27 +++++++++++++++++---------- tests/ui/matches.rs | 8 ++++---- tests/ui/matches.stderr | 16 ++++++++-------- 3 files changed, 29 insertions(+), 22 deletions(-) diff --git a/clippy_lints/src/matches.rs b/clippy_lints/src/matches.rs index 326ce83a8..2183c6e4f 100644 --- a/clippy_lints/src/matches.rs +++ b/clippy_lints/src/matches.rs @@ -148,7 +148,7 @@ declare_lint! { /// **What it does:** Checks for match which is used to add a reference to an /// `Option` value. /// -/// **Why is this bad?** Using `as_ref()` instead is shorter. +/// **Why is this bad?** Using `as_ref()` or `as_mut()` instead is shorter. /// /// **Known problems:** None. /// @@ -163,7 +163,7 @@ declare_lint! { declare_lint! { pub MATCH_AS_REF, Warn, - "a match on an Option value instead of using `as_ref()`" + "a match on an Option value instead of using `as_ref()` or `as_mut`" } #[allow(missing_copy_implementations)] @@ -438,15 +438,22 @@ fn check_match_as_ref(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() { - if (is_ref_some_arm(&arms[0]) && is_none_arm(&arms[1])) || - (is_ref_some_arm(&arms[1]) && is_none_arm(&arms[0])) { + let arm_ref: Option = if is_none_arm(&arms[0]) { + is_ref_some_arm(&arms[1]) + } else if is_none_arm(&arms[1]) { + is_ref_some_arm(&arms[0]) + } else { + None + }; + if let Some(rb) = arm_ref { + let suggestion = if rb == BindingAnnotation::Ref { "as_ref" } else { "as_mut" }; span_lint_and_sugg( cx, MATCH_AS_REF, expr.span, - "use as_ref() instead", + &format!("use {}() instead", suggestion), "try this", - format!("{}.as_ref()", snippet(cx, ex.span, "_")) + format!("{}.{}()", snippet(cx, ex.span, "_"), suggestion) ) } } @@ -574,7 +581,7 @@ fn is_none_arm(arm: &Arm) -> bool { } // Checks if arm has the form `Some(ref v) => Some(v)` (checks for `ref` and `ref mut`) -fn is_ref_some_arm(arm: &Arm) -> bool { +fn is_ref_some_arm(arm: &Arm) -> Option { if_chain! { if let PatKind::TupleStruct(ref path, ref pats, _) = arm.pats[0].node; if pats.len() == 1 && match_qpath(path, &paths::OPTION_SOME); @@ -585,12 +592,12 @@ fn is_ref_some_arm(arm: &Arm) -> bool { if match_qpath(some_path, &paths::OPTION_SOME) && args.len() == 1; if let ExprPath(ref qpath) = args[0].node; if let &QPath::Resolved(_, ref path2) = qpath; - if path2.segments.len() == 1; + if path2.segments.len() == 1 && ident.node == path2.segments[0].name; then { - return ident.node == path2.segments[0].name + return Some(rb) } } - false + None } fn has_only_ref_pats(arms: &[Arm]) -> bool { diff --git a/tests/ui/matches.rs b/tests/ui/matches.rs index 72a36338a..67a901f65 100644 --- a/tests/ui/matches.rs +++ b/tests/ui/matches.rs @@ -316,14 +316,14 @@ fn match_wild_err_arm() { } fn match_as_ref() { - let owned : Option<()> = None; - let borrowed = match owned { + let owned: Option<()> = None; + let borrowed: Option<&()> = match owned { None => None, Some(ref v) => Some(v), }; - let mut mut_owned : Option<()> = None; - let mut mut_borrowed = match mut_owned { + let mut mut_owned: Option<()> = None; + let borrow_mut: Option<&mut ()> = match mut_owned { None => None, Some(ref mut v) => Some(v), }; diff --git a/tests/ui/matches.stderr b/tests/ui/matches.stderr index 3d7b8f784..62c77c778 100644 --- a/tests/ui/matches.stderr +++ b/tests/ui/matches.stderr @@ -427,10 +427,10 @@ note: consider refactoring into `Ok(3) | Ok(_)` = note: this error originates in a macro outside of the current crate (in Nightly builds, run with -Z external-macro-backtrace for more info) error: use as_ref() instead - --> $DIR/matches.rs:320:20 + --> $DIR/matches.rs:320:33 | -320 | let borrowed = match owned { - | ____________________^ +320 | let borrowed: Option<&()> = match owned { + | _________________________________^ 321 | | None => None, 322 | | Some(ref v) => Some(v), 323 | | }; @@ -438,13 +438,13 @@ error: use as_ref() instead | = note: `-D match-as-ref` implied by `-D warnings` -error: use as_ref() instead - --> $DIR/matches.rs:326:28 +error: use as_mut() instead + --> $DIR/matches.rs:326:39 | -326 | let mut mut_borrowed = match mut_owned { - | ____________________________^ +326 | let borrow_mut: Option<&mut ()> = match mut_owned { + | _______________________________________^ 327 | | None => None, 328 | | Some(ref mut v) => Some(v), 329 | | }; - | |_____^ help: try this: `mut_owned.as_ref()` + | |_____^ help: try this: `mut_owned.as_mut()`