From de8d233b060f60a37b7fae82a36f7226892ac4e7 Mon Sep 17 00:00:00 2001 From: ms2300 Date: Wed, 12 Sep 2018 18:14:48 -0600 Subject: [PATCH 1/2] #3006 : Fixing for .get().unwrap().foo() --- clippy_lints/src/methods.rs | 2 +- tests/ui/get_unwrap.rs | 5 +++++ tests/ui/get_unwrap.stderr | 34 +++++++++++++++++++++++----------- 3 files changed, 29 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 03e3d2628e84..32e6a83e535d 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1431,7 +1431,7 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir:: ), "try this", format!( - "{}{}[{}]", + "({}{}[{}])", borrow_str, snippet(cx, get_args[0].span, "_"), snippet(cx, get_args[1].span, "_") diff --git a/tests/ui/get_unwrap.rs b/tests/ui/get_unwrap.rs index a10d4d182620..141233e0d8ad 100644 --- a/tests/ui/get_unwrap.rs +++ b/tests/ui/get_unwrap.rs @@ -43,4 +43,9 @@ fn main() { *some_btreemap.get_mut(&1).unwrap() = 'b'; *false_positive.get_mut(0).unwrap() = 1; } + + { // Test `get().unwrap().foo()` and `get_mut().unwrap().bar()` + let _ = some_vec.get(0..1).unwrap().to_vec(); + let _ = some_vec.get_mut(0..1).unwrap().to_vec(); + } } diff --git a/tests/ui/get_unwrap.stderr b/tests/ui/get_unwrap.stderr index 63f6603c821e..6b9a360f332f 100644 --- a/tests/ui/get_unwrap.stderr +++ b/tests/ui/get_unwrap.stderr @@ -2,7 +2,7 @@ error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more co --> $DIR/get_unwrap.rs:27:17 | 27 | let _ = boxed_slice.get(1).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&boxed_slice[1]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&boxed_slice[1])` | = note: `-D clippy::get-unwrap` implied by `-D warnings` @@ -10,55 +10,67 @@ error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more co --> $DIR/get_unwrap.rs:28:17 | 28 | let _ = some_slice.get(0).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_slice[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_slice[0])` error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:29:17 | 29 | let _ = some_vec.get(0).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vec[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_vec[0])` error: called `.get().unwrap()` on a VecDeque. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:30:17 | 30 | let _ = some_vecdeque.get(0).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vecdeque[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_vecdeque[0])` error: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:31:17 | 31 | let _ = some_hashmap.get(&1).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_hashmap[&1]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_hashmap[&1])` error: called `.get().unwrap()` on a BTreeMap. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:32:17 | 32 | let _ = some_btreemap.get(&1).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_btreemap[&1]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_btreemap[&1])` error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:37:10 | 37 | *boxed_slice.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut boxed_slice[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut boxed_slice[0])` error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:38:10 | 38 | *some_slice.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_slice[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_slice[0])` error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:39:10 | 39 | *some_vec.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vec[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_vec[0])` error: called `.get_mut().unwrap()` on a VecDeque. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:40:10 | 40 | *some_vecdeque.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vecdeque[0]` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_vecdeque[0])` -error: aborting due to 10 previous errors +error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise + --> $DIR/get_unwrap.rs:48:17 + | +48 | let _ = some_vec.get(0..1).unwrap().to_vec(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_vec[0..1])` + +error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise + --> $DIR/get_unwrap.rs:49:17 + | +49 | let _ = some_vec.get_mut(0..1).unwrap().to_vec(); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_vec[0..1])` + +error: aborting due to 12 previous errors From 523ba2a009c4257119870aea2204ce00c0b677ac Mon Sep 17 00:00:00 2001 From: ms2300 Date: Thu, 13 Sep 2018 02:36:13 -0600 Subject: [PATCH 2/2] Full fix of get unwrap issue --- clippy_lints/src/methods.rs | 17 ++++++++++++++--- tests/ui/get_unwrap.stderr | 24 ++++++++++++------------ 2 files changed, 26 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/methods.rs b/clippy_lints/src/methods.rs index 32e6a83e535d..6bf57383a7a1 100644 --- a/clippy_lints/src/methods.rs +++ b/clippy_lints/src/methods.rs @@ -1404,22 +1404,33 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir:: // Note: we don't want to lint `get_mut().unwrap` for HashMap or BTreeMap, // because they do not implement `IndexMut` let expr_ty = cx.tables.expr_ty(&get_args[0]); + let get_args_str = if get_args.len() > 1 { + snippet(cx, get_args[1].span, "_") + } else { + return; // not linting on a .get().unwrap() chain or variant + }; + let needs_ref; let caller_type = if derefs_to_slice(cx, &get_args[0], expr_ty).is_some() { + needs_ref = get_args_str.parse::().is_ok(); "slice" } else if match_type(cx, expr_ty, &paths::VEC) { + needs_ref = get_args_str.parse::().is_ok(); "Vec" } else if match_type(cx, expr_ty, &paths::VEC_DEQUE) { + needs_ref = get_args_str.parse::().is_ok(); "VecDeque" } else if !is_mut && match_type(cx, expr_ty, &paths::HASHMAP) { + needs_ref = true; "HashMap" } else if !is_mut && match_type(cx, expr_ty, &paths::BTREEMAP) { + needs_ref = true; "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 { "&" }; + let borrow_str = if !needs_ref { "" } else if is_mut { "&mut " } else { "&" }; span_lint_and_sugg( cx, GET_UNWRAP, @@ -1431,10 +1442,10 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir:: ), "try this", format!( - "({}{}[{}])", + "{}{}[{}]", borrow_str, snippet(cx, get_args[0].span, "_"), - snippet(cx, get_args[1].span, "_") + get_args_str ), ); } diff --git a/tests/ui/get_unwrap.stderr b/tests/ui/get_unwrap.stderr index 6b9a360f332f..669903da190c 100644 --- a/tests/ui/get_unwrap.stderr +++ b/tests/ui/get_unwrap.stderr @@ -2,7 +2,7 @@ error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more co --> $DIR/get_unwrap.rs:27:17 | 27 | let _ = boxed_slice.get(1).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&boxed_slice[1])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&boxed_slice[1]` | = note: `-D clippy::get-unwrap` implied by `-D warnings` @@ -10,67 +10,67 @@ error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more co --> $DIR/get_unwrap.rs:28:17 | 28 | let _ = some_slice.get(0).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_slice[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_slice[0]` error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:29:17 | 29 | let _ = some_vec.get(0).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_vec[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vec[0]` error: called `.get().unwrap()` on a VecDeque. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:30:17 | 30 | let _ = some_vecdeque.get(0).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_vecdeque[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vecdeque[0]` error: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:31:17 | 31 | let _ = some_hashmap.get(&1).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_hashmap[&1])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_hashmap[&1]` error: called `.get().unwrap()` on a BTreeMap. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:32:17 | 32 | let _ = some_btreemap.get(&1).unwrap(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_btreemap[&1])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_btreemap[&1]` error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:37:10 | 37 | *boxed_slice.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut boxed_slice[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut boxed_slice[0]` error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:38:10 | 38 | *some_slice.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_slice[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_slice[0]` error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:39:10 | 39 | *some_vec.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_vec[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vec[0]` error: called `.get_mut().unwrap()` on a VecDeque. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:40:10 | 40 | *some_vecdeque.get_mut(0).unwrap() = 1; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_vecdeque[0])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vecdeque[0]` error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:48:17 | 48 | let _ = some_vec.get(0..1).unwrap().to_vec(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&some_vec[0..1])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0..1]` error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise --> $DIR/get_unwrap.rs:49:17 | 49 | let _ = some_vec.get_mut(0..1).unwrap().to_vec(); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `(&mut some_vec[0..1])` + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0..1]` error: aborting due to 12 previous errors