Mirrors/rust-clippy
2
0
Fork 0
mirror of https://github.com/rust-lang/rust-clippy synced 2024-11-10 15:14:29 +00:00
Code Issues Projects Releases Packages Wiki Activity

Merge pull request #1312 from devonhollowood/get-unwrap

Browse source 
Implement `get_unwrap` lint
This commit is contained in:
Oliver Schneider 2016-11-09 09:49:20 +01:00 committed by GitHub
commit a260e65ead
5 changed files with 143 additions and 3 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

1
CHANGELOG.md
View file

@ -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

3
README.md
View file

@ -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

1
clippy_lints/src/lib.rs
View file

@ -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,

76
clippy_lints/src/methods.rs
View file

@ -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) {

65
tests/compile-fail/methods.rs
View file

@ -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() {