diff --git a/CHANGELOG.md b/CHANGELOG.md index c48138f12b6a..047a5c4b34b3 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/README.md b/README.md index c51c175675fb..28f3b1054dcf 100644 --- a/README.md +++ b/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 diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 18e38a6dbafc..8d5edb036823 100644 --- a/clippy_lints/src/lib.rs +++ b/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, diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 79e9d2028c3e..70ae2e6a189f 100644 --- a/clippy_lints/src/methods.rs +++ b/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) { diff --git a/tests/compile-fail/methods.rs b/tests/compile-fail/methods.rs index a26a396526e9..9d430e7ac7ca 100644 --- a/tests/compile-fail/methods.rs +++ b/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 = HashMap::from_iter(vec![(1, 'a'), (2, 'b')]); + let mut some_btreemap: BTreeMap = 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() {