Skip to content

Commit 81fa266

Browse files
borsbors
committed
Auto merge of #3638 - mikerite:fix-3625, r=oli-obk
Improve `get_unwrap` suggestion Handle case where a reference is immediately dereferenced. Fixes #3625
2 parents c63b634 + d2ea635 commit 81fa266

File tree

4 files changed

+114
-19
lines changed

4 files changed

+114
-19
lines changed

‎clippy_lints/src/methods/mod.rs

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1603,7 +1603,7 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir::
16031603
} else {
16041604
return; // not linting on a .get().unwrap() chain or variant
16051605
};
1606-
let needs_ref;
1606+
let mut needs_ref;
16071607
let caller_type = if derefs_to_slice(cx, &get_args[0], expr_ty).is_some() {
16081608
needs_ref = get_args_str.parse::<usize>().is_ok();
16091609
"slice"
@@ -1623,6 +1623,21 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir::
16231623
return; // caller is not a type that we want to lint
16241624
};
16251625

1626+
let mut span = expr.span;
1627+
1628+
// Handle the case where the result is immedately dereferenced
1629+
// by not requiring ref and pulling the dereference into the
1630+
// suggestion.
1631+
if_chain! {
1632+
if needs_ref;
1633+
if let Some(parent) = get_parent_expr(cx, expr);
1634+
if let hir::ExprKind::Unary(hir::UnOp::UnDeref, _) = parent.node;
1635+
then {
1636+
needs_ref = false;
1637+
span = parent.span;
1638+
}
1639+
}
1640+
16261641
let mut_str = if is_mut { "_mut" } else { "" };
16271642
let borrow_str = if !needs_ref {
16281643
""
@@ -1631,10 +1646,11 @@ fn lint_get_unwrap(cx: &LateContext<'_, '_>, expr: &hir::Expr, get_args: &[hir::
16311646
} else {
16321647
"&"
16331648
};
1649+
16341650
span_lint_and_sugg(
16351651
cx,
16361652
GET_UNWRAP,
1637-
expr.span,
1653+
span,
16381654
&format!(
16391655
"called `.get{0}().unwrap()` on a {1}. Using `[]` is more clear and more concise",
16401656
mut_str, caller_type

‎tests/ui/get_unwrap.fixed

Lines changed: 70 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
// Copyright 2014-2018 The Rust Project Developers. See the COPYRIGHT
2+
// file at the top-level directory of this distribution.
3+
//
4+
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
5+
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
6+
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
7+
// option. This file may not be copied, modified, or distributed
8+
// except according to those terms.
9+
10+
// run-rustfix
11+
#![allow(unused_mut)]
12+
13+
use std::collections::BTreeMap;
14+
use std::collections::HashMap;
15+
use std::collections::VecDeque;
16+
use std::iter::FromIterator;
17+
18+
struct GetFalsePositive {
19+
arr: [u32; 3],
20+
}
21+
22+
impl GetFalsePositive {
23+
fn get(&self, pos: usize) -> Option<&u32> {
24+
self.arr.get(pos)
25+
}
26+
fn get_mut(&mut self, pos: usize) -> Option<&mut u32> {
27+
self.arr.get_mut(pos)
28+
}
29+
}
30+
31+
fn main() {
32+
let mut boxed_slice: Box<[u8]> = Box::new([0, 1, 2, 3]);
33+
let mut some_slice = &mut [0, 1, 2, 3];
34+
let mut some_vec = vec![0, 1, 2, 3];
35+
let mut some_vecdeque: VecDeque<_> = some_vec.iter().cloned().collect();
36+
let mut some_hashmap: HashMap<u8, char> = HashMap::from_iter(vec![(1, 'a'), (2, 'b')]);
37+
let mut some_btreemap: BTreeMap<u8, char> = BTreeMap::from_iter(vec![(1, 'a'), (2, 'b')]);
38+
let mut false_positive = GetFalsePositive { arr: [0, 1, 2] };
39+
40+
{
41+
// Test `get().unwrap()`
42+
let _ = &boxed_slice[1];
43+
let _ = &some_slice[0];
44+
let _ = &some_vec[0];
45+
let _ = &some_vecdeque[0];
46+
let _ = &some_hashmap[&1];
47+
let _ = &some_btreemap[&1];
48+
let _ = false_positive.get(0).unwrap();
49+
// Test with deref
50+
let _: u8 = boxed_slice[1];
51+
}
52+
53+
{
54+
// Test `get_mut().unwrap()`
55+
boxed_slice[0] = 1;
56+
some_slice[0] = 1;
57+
some_vec[0] = 1;
58+
some_vecdeque[0] = 1;
59+
// Check false positives
60+
*some_hashmap.get_mut(&1).unwrap() = 'b';
61+
*some_btreemap.get_mut(&1).unwrap() = 'b';
62+
*false_positive.get_mut(0).unwrap() = 1;
63+
}
64+
65+
{
66+
// Test `get().unwrap().foo()` and `get_mut().unwrap().bar()`
67+
let _ = some_vec[0..1].to_vec();
68+
let _ = some_vec[0..1].to_vec();
69+
}
70+
}

‎tests/ui/get_unwrap.rs

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
// option. This file may not be copied, modified, or distributed
88
// except according to those terms.
99

10+
// run-rustfix
1011
#![allow(unused_mut)]
1112

1213
use std::collections::BTreeMap;
@@ -45,6 +46,8 @@ fn main() {
4546
let _ = some_hashmap.get(&1).unwrap();
4647
let _ = some_btreemap.get(&1).unwrap();
4748
let _ = false_positive.get(0).unwrap();
49+
// Test with deref
50+
let _: u8 = *boxed_slice.get(1).unwrap();
4851
}
4952

5053
{

‎tests/ui/get_unwrap.stderr

Lines changed: 23 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,76 +1,82 @@
11
error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise
2-
--> $DIR/get_unwrap.rs:41:17
2+
--> $DIR/get_unwrap.rs:42:17
33
|
44
LL | let _ = boxed_slice.get(1).unwrap();
55
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&boxed_slice[1]`
66
|
77
= note: `-D clippy::get-unwrap` implied by `-D warnings`
88

99
error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise
10-
--> $DIR/get_unwrap.rs:42:17
10+
--> $DIR/get_unwrap.rs:43:17
1111
|
1212
LL | let _ = some_slice.get(0).unwrap();
1313
| ^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_slice[0]`
1414

1515
error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise
16-
--> $DIR/get_unwrap.rs:43:17
16+
--> $DIR/get_unwrap.rs:44:17
1717
|
1818
LL | let _ = some_vec.get(0).unwrap();
1919
| ^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vec[0]`
2020

2121
error: called `.get().unwrap()` on a VecDeque. Using `[]` is more clear and more concise
22-
--> $DIR/get_unwrap.rs:44:17
22+
--> $DIR/get_unwrap.rs:45:17
2323
|
2424
LL | let _ = some_vecdeque.get(0).unwrap();
2525
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_vecdeque[0]`
2626

2727
error: called `.get().unwrap()` on a HashMap. Using `[]` is more clear and more concise
28-
--> $DIR/get_unwrap.rs:45:17
28+
--> $DIR/get_unwrap.rs:46:17
2929
|
3030
LL | let _ = some_hashmap.get(&1).unwrap();
3131
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_hashmap[&1]`
3232

3333
error: called `.get().unwrap()` on a BTreeMap. Using `[]` is more clear and more concise
34-
--> $DIR/get_unwrap.rs:46:17
34+
--> $DIR/get_unwrap.rs:47:17
3535
|
3636
LL | let _ = some_btreemap.get(&1).unwrap();
3737
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&some_btreemap[&1]`
3838

39+
error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise
40+
--> $DIR/get_unwrap.rs:50:21
41+
|
42+
LL | let _: u8 = *boxed_slice.get(1).unwrap();
43+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `boxed_slice[1]`
44+
3945
error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise
40-
--> $DIR/get_unwrap.rs:52:10
46+
--> $DIR/get_unwrap.rs:55:9
4147
|
4248
LL | *boxed_slice.get_mut(0).unwrap() = 1;
43-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut boxed_slice[0]`
49+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `boxed_slice[0]`
4450

4551
error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise
46-
--> $DIR/get_unwrap.rs:53:10
52+
--> $DIR/get_unwrap.rs:56:9
4753
|
4854
LL | *some_slice.get_mut(0).unwrap() = 1;
49-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_slice[0]`
55+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_slice[0]`
5056

5157
error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise
52-
--> $DIR/get_unwrap.rs:54:10
58+
--> $DIR/get_unwrap.rs:57:9
5359
|
5460
LL | *some_vec.get_mut(0).unwrap() = 1;
55-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vec[0]`
61+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0]`
5662

5763
error: called `.get_mut().unwrap()` on a VecDeque. Using `[]` is more clear and more concise
58-
--> $DIR/get_unwrap.rs:55:10
64+
--> $DIR/get_unwrap.rs:58:9
5965
|
6066
LL | *some_vecdeque.get_mut(0).unwrap() = 1;
61-
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut some_vecdeque[0]`
67+
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vecdeque[0]`
6268

6369
error: called `.get().unwrap()` on a Vec. Using `[]` is more clear and more concise
64-
--> $DIR/get_unwrap.rs:64:17
70+
--> $DIR/get_unwrap.rs:67:17
6571
|
6672
LL | let _ = some_vec.get(0..1).unwrap().to_vec();
6773
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0..1]`
6874

6975
error: called `.get_mut().unwrap()` on a Vec. Using `[]` is more clear and more concise
70-
--> $DIR/get_unwrap.rs:65:17
76+
--> $DIR/get_unwrap.rs:68:17
7177
|
7278
LL | let _ = some_vec.get_mut(0..1).unwrap().to_vec();
7379
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `some_vec[0..1]`
7480

75-
error: aborting due to 12 previous errors
81+
error: aborting due to 13 previous errors
7682

0 commit comments

Comments
 (0)