Merge pull request #2281 from detrumi/match-as-ref

Lint for matching option as ref

Author: oli-obk

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,

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()` or `as_mut()` 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()` or `as_mut`"
+}
+
 #[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,31 @@ 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() {
+        let arm_ref: Option<BindingAnnotation> = 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,
+                &format!("use {}() instead", suggestion),
+                "try this",
+                format!("{}.{}()", snippet(cx, ex.span, "_"), suggestion)
+            )
+        }
+    }
+}
+
 /// Get all arms that are unbounded `PatRange`s.
 fn all_ranges<'a, 'tcx>(
     cx: &LateContext<'a, 'tcx>,
@@ -524,6 +572,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) -> Option<BindingAnnotation> {
+    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 && ident.node == path2.segments[0].name;
+        then {
+            return Some(rb)
+        }
+    }
+    None
+}
+
 fn has_only_ref_pats(arms: &[Arm]) -> bool {
     let mapped = arms.iter()
         .flat_map(|a| &a.pats)

tests/ui/matches.rs

@@ -315,5 +315,20 @@ fn match_wild_err_arm() {
     }
 }
 
+fn match_as_ref() {
+    let owned: Option<()> = None;
+    let borrowed: Option<&()> = match owned {
+        None => None,
+        Some(ref v) => Some(v),
+    };
+
+    let mut mut_owned: Option<()> = None;
+    let borrow_mut: Option<&mut ()> = match mut_owned {
+        None => None,
+        Some(ref mut v) => Some(v),
+    };
+
+}
+
 fn main() {
 }

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:33
+    |
+320 |       let borrowed: Option<&()> = 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_mut() instead
+   --> $DIR/matches.rs:326:39
+    |
+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_mut()`
+