Merge pull request #1312 from devonhollowood/get-unwrap

Implement `get_unwrap` lint

Author: oli-obk

CHANGELOG.md

@@ -266,6 +266,7 @@ All notable changes to this project will be documented in this file.
 [`for_kv_map`]: https://github.com/Manishearth/rust-clippy/wiki#for_kv_map
 [`for_loop_over_option`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option
 [`for_loop_over_result`]: https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result
+[`get_unwrap`]: https://github.com/Manishearth/rust-clippy/wiki#get_unwrap
 [`identity_op`]: https://github.com/Manishearth/rust-clippy/wiki#identity_op
 [`if_let_redundant_pattern_matching`]: https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching
 [`if_let_some_result`]: https://github.com/Manishearth/rust-clippy/wiki#if_let_some_result

README.md

@@ -182,7 +182,7 @@ You can check out this great service at [clippy.bashy.io](https://clippy.bashy.i
 
 ## Lints
 
-There are 176 lints included in this crate:
+There are 177 lints included in this crate:
 
 name                                                                                                                   | default | triggers on
 -----------------------------------------------------------------------------------------------------------------------|---------|----------------------------------------------------------------------------------------------------------------------------------
@@ -238,6 +238,7 @@ name
 [for_kv_map](https://github.com/Manishearth/rust-clippy/wiki#for_kv_map)                                               | warn    | looping on a map using `iter` when `keys` or `values` would do
 [for_loop_over_option](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_option)                           | warn    | for-looping over an `Option`, which is more clearly expressed as an `if let`
 [for_loop_over_result](https://github.com/Manishearth/rust-clippy/wiki#for_loop_over_result)                           | warn    | for-looping over a `Result`, which is more clearly expressed as an `if let`
+[get_unwrap](https://github.com/Manishearth/rust-clippy/wiki#get_unwrap)                                               | warn    | using `.get().unwrap()` or `.get_mut().unwrap()` when using `[]` would work instead
 [identity_op](https://github.com/Manishearth/rust-clippy/wiki#identity_op)                                             | warn    | using identity operations, e.g. `x + 0` or `y / 1`
 [if_let_redundant_pattern_matching](https://github.com/Manishearth/rust-clippy/wiki#if_let_redundant_pattern_matching) | warn    | use the proper utility function avoiding an `if let`
 [if_let_some_result](https://github.com/Manishearth/rust-clippy/wiki#if_let_some_result)                               | warn    | usage of `ok()` in `if let Some(pat)` statements is unnecessary, match on `Ok(pat)` instead

clippy_lints/src/lib.rs

@@ -383,6 +383,7 @@ pub fn register_plugins(reg: &mut rustc_plugin::Registry) {
         methods::CLONE_ON_COPY,
         methods::EXTEND_FROM_SLICE,
         methods::FILTER_NEXT,
+        methods::GET_UNWRAP,
         methods::ITER_NTH,
         methods::ITER_SKIP_NEXT,
         methods::NEW_RET_NO_SELF,

clippy_lints/src/methods.rs

@@ -464,6 +464,32 @@ declare_lint! {
     "using `.skip(x).next()` on an iterator"
 }
 
+/// **What it does:** Checks for use of `.get().unwrap()` (or
+/// `.get_mut().unwrap`) on a standard library type which implements `Index`
+///
+/// **Why is this bad?** Using the Index trait (`[]`) is more clear and more
+/// concise.
+///
+/// **Known problems:** None.
+///
+/// **Example:**
+/// ```rust
+/// let some_vec = vec![0, 1, 2, 3];
+/// let last = some_vec.get(3).unwrap();
+/// *some_vec.get_mut(0).unwrap() = 1;
+/// ```
+/// The correct use would be:
+/// ```rust
+/// let some_vec = vec![0, 1, 2, 3];
+/// let last = some_vec[3];
+/// some_vec[0] = 1;
+/// ```
+declare_lint! {
+    pub GET_UNWRAP,
+    Warn,
+    "using `.get().unwrap()` or `.get_mut().unwrap()` when using `[]` would work instead"
+}
+
 
 impl LintPass for Pass {
     fn get_lints(&self) -> LintArray {
@@ -487,11 +513,15 @@ impl LintPass for Pass {
                     FILTER_NEXT,
                     FILTER_MAP,
                     ITER_NTH,
-                    ITER_SKIP_NEXT)
+                    ITER_SKIP_NEXT,
+                    GET_UNWRAP)
     }
 }
 
 impl LateLintPass for Pass {
+    #[allow(unused_attributes)]
+    // ^ required because `cyclomatic_complexity` attribute shows up as unused
+    #[cyclomatic_complexity = "30"]
     fn check_expr(&mut self, cx: &LateContext, expr: &hir::Expr) {
         if in_macro(cx, expr.span) {
             return;
@@ -500,7 +530,12 @@ impl LateLintPass for Pass {
         match expr.node {
             hir::ExprMethodCall(name, _, ref args) => {
                 // Chain calls
-                if let Some(arglists) = method_chain_args(expr, &["unwrap"]) {
+                // GET_UNWRAP needs to be checked before general `UNWRAP` lints
+                if let Some(arglists) = method_chain_args(expr, &["get", "unwrap"]) {
+                    lint_get_unwrap(cx, expr, arglists[0], false);
+                } else if let Some(arglists) = method_chain_args(expr, &["get_mut", "unwrap"]) {
+                    lint_get_unwrap(cx, expr, arglists[0], true);
+                } else if let Some(arglists) = method_chain_args(expr, &["unwrap"]) {
                     lint_unwrap(cx, expr, arglists[0]);
                 } else if let Some(arglists) = method_chain_args(expr, &["ok", "expect"]) {
                     lint_ok_expect(cx, expr, arglists[0]);
@@ -818,6 +853,43 @@ fn lint_iter_nth(cx: &LateContext, expr: &hir::Expr, iter_args: &MethodArgs, is_
     );
 }
 
+fn lint_get_unwrap(cx: &LateContext, expr: &hir::Expr, get_args: &MethodArgs, is_mut: bool) {
+    // Note: we don't want to lint `get_mut().unwrap` for HashMap or BTreeMap,
+    // because they do not implement `IndexMut`
+    let expr_ty = cx.tcx.expr_ty(&get_args[0]);
+    let caller_type = if derefs_to_slice(cx, &get_args[0], expr_ty).is_some() {
+        "slice"
+    } else if match_type(cx, expr_ty, &paths::VEC) {
+        "Vec"
+    } else if match_type(cx, expr_ty, &paths::VEC_DEQUE) {
+        "VecDeque"
+    } else if !is_mut && match_type(cx, expr_ty, &paths::HASHMAP) {
+        "HashMap"
+    } else if !is_mut && match_type(cx, expr_ty, &paths::BTREEMAP) {
+        "BTreeMap"
+    } else {
+        return; // caller is not a type that we want to lint
+    };
+
+    let mut_str = if is_mut { "_mut" } else { "" };
+    let borrow_str = if is_mut { "&mut " } else { "&" };
+    span_lint_and_then(
+        cx,
+        GET_UNWRAP,
+        expr.span,
+        &format!("called `.get{0}().unwrap()` on a {1}. Using `[]` is more clear and more concise",
+                 mut_str, caller_type),
+        |db| {
+            db.span_suggestion(
+                expr.span,
+                "try this",
+                format!("{}{}[{}]", borrow_str, snippet(cx, get_args[0].span, "_"),
+                        snippet(cx, get_args[1].span, "_"))
+            );
+        }
+    );
+}
+
 fn lint_iter_skip_next(cx: &LateContext, expr: &hir::Expr){
     // lint if caller of skip is an Iterator
     if match_trait_method(cx, expr, &paths::ITERATOR) {

tests/compile-fail/methods.rs

@@ -10,6 +10,7 @@ use std::collections::HashMap;
 use std::collections::HashSet;
 use std::collections::VecDeque;
 use std::ops::Mul;
+use std::iter::FromIterator;
 
 struct T;
 
@@ -388,6 +389,70 @@ fn iter_skip_next() {
     let _ = foo.filter().skip(42).next();
 }
 
+struct GetFalsePositive {
+    arr: [u32; 3],
+}
+
+impl GetFalsePositive {
+    fn get(&self, pos: usize) -> Option<&u32> { self.arr.get(pos) }
+    fn get_mut(&mut self, pos: usize) -> Option<&mut u32> { self.arr.get_mut(pos) }
+}
+
+/// Checks implementation of `GET_UNWRAP` lint
+fn get_unwrap() {
+    let mut some_slice = &mut [0, 1, 2, 3];
+    let mut some_vec = vec![0, 1, 2, 3];
+    let mut some_vecdeque: VecDeque<_> = some_vec.iter().cloned().collect();
+    let mut some_hashmap: HashMap<u8, char> = HashMap::from_iter(vec![(1, 'a'), (2, 'b')]);
+    let mut some_btreemap: BTreeMap<u8, char> = BTreeMap::from_iter(vec![(1, 'a'), (2, 'b')]);
+    let mut false_positive = GetFalsePositive { arr: [0, 1, 2] };
+
+    { // Test `get().unwrap()`
+        let _ = some_slice.get(0).unwrap();
+        //~^ERROR called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION some_slice[0]
+        let _ = some_vec.get(0).unwrap();
+        //~^ERROR called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION some_vec[0]
+        let _ = some_vecdeque.get(0).unwrap();
+        //~^ERROR called `.get().unwrap()` on a VecDeque. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION some_vecdeque[0]
+        let _ = some_hashmap.get(&1).unwrap();
+        //~^ERROR called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION some_hashmap[&1]
+        let _ = some_btreemap.get(&1).unwrap();
+        //~^ERROR called `.get().unwrap()` on a BTreeMap. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION some_btreemap[&1]
+
+        let _ = false_positive.get(0).unwrap();
+    }
+
+    { // Test `get_mut().unwrap()`
+        *some_slice.get_mut(0).unwrap() = 1;
+        //~^ERROR called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION &mut some_slice[0]
+        *some_vec.get_mut(0).unwrap() = 1;
+        //~^ERROR called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION &mut some_vec[0]
+        *some_vecdeque.get_mut(0).unwrap() = 1;
+        //~^ERROR called `.get_mut().unwrap()` on a VecDeque. Using `[]` is more clear and more concise
+        //~|HELP try this
+        //~|SUGGESTION &mut some_vecdeque[0]
+
+        // Check false positives
+        *some_hashmap.get_mut(&1).unwrap() = 'b';
+        *some_btreemap.get_mut(&1).unwrap() = 'b';
+        *false_positive.get_mut(0).unwrap() = 1;
+    }
+}
+
 
 #[allow(similar_names)]
 fn main() {