diff --git a/clippy_lints/src/methods/get_unwrap.rs b/clippy_lints/src/methods/get_unwrap.rs index ffc3a4d780e..209d42ebabd 100644 --- a/clippy_lints/src/methods/get_unwrap.rs +++ b/clippy_lints/src/methods/get_unwrap.rs @@ -25,13 +25,13 @@ pub(super) fn check<'tcx>( let get_args_str = snippet_with_applicability(cx, get_arg.span, "..", &mut applicability); let mut needs_ref; let caller_type = if derefs_to_slice(cx, recv, expr_ty).is_some() { - needs_ref = get_args_str.parse::().is_ok(); + needs_ref = true; "slice" } else if is_type_diagnostic_item(cx, expr_ty, sym::Vec) { - needs_ref = get_args_str.parse::().is_ok(); + needs_ref = true; "Vec" } else if is_type_diagnostic_item(cx, expr_ty, sym::VecDeque) { - needs_ref = get_args_str.parse::().is_ok(); + needs_ref = true; "VecDeque" } else if !is_mut && is_type_diagnostic_item(cx, expr_ty, sym::HashMap) { needs_ref = true; @@ -45,16 +45,23 @@ pub(super) fn check<'tcx>( let mut span = expr.span; - // Handle the case where the result is immediately dereferenced - // by not requiring ref and pulling the dereference into the - // suggestion. + // Handle the case where the result is immediately dereferenced, + // either directly be the user, or as a result of a method call or the like + // by not requiring an explicit reference if_chain! { if needs_ref; if let Some(parent) = get_parent_expr(cx, expr); - if let hir::ExprKind::Unary(hir::UnOp::Deref, _) = parent.kind; + if let hir::ExprKind::Unary(hir::UnOp::Deref, _) + | hir::ExprKind::MethodCall(..) + | hir::ExprKind::Field(..) + | hir::ExprKind::Index(..) = parent.kind; then { + if let hir::ExprKind::Unary(hir::UnOp::Deref, _) = parent.kind { + // if the user explicitly dereferences the result, we can adjust + // the span to also include the deref part + span = parent.span; + } needs_ref = false; - span = parent.span; } } diff --git a/tests/ui/get_unwrap.fixed b/tests/ui/get_unwrap.fixed index 9061c9aa404..3e4c2b499e4 100644 --- a/tests/ui/get_unwrap.fixed +++ b/tests/ui/get_unwrap.fixed @@ -69,4 +69,17 @@ fn main() { let _ = some_vec[0..1].to_vec(); let _ = some_vec[0..1].to_vec(); } + issue9909(); +} +fn issue9909() { + let f = &[1, 2, 3]; + + let _x: &i32 = &f[1 + 2]; + // ^ include a borrow in the suggestion, even if the argument is not just a numeric literal + + let _x = f[1 + 2].to_string(); + // ^ don't include a borrow here + + let _x = f[1 + 2].abs(); + // ^ don't include a borrow here } diff --git a/tests/ui/get_unwrap.rs b/tests/ui/get_unwrap.rs index b5e4e472037..a967971a7dc 100644 --- a/tests/ui/get_unwrap.rs +++ b/tests/ui/get_unwrap.rs @@ -69,4 +69,17 @@ fn main() { let _ = some_vec.get(0..1).unwrap().to_vec(); let _ = some_vec.get_mut(0..1).unwrap().to_vec(); } + issue9909(); +} +fn issue9909() { + let f = &[1, 2, 3]; + + let _x: &i32 = f.get(1 + 2).unwrap(); + // ^ include a borrow in the suggestion, even if the argument is not just a numeric literal + + let _x = f.get(1 + 2).unwrap().to_string(); + // ^ don't include a borrow here + + let _x = f.get(1 + 2).unwrap().abs(); + // ^ don't include a borrow here } diff --git a/tests/ui/get_unwrap.stderr b/tests/ui/get_unwrap.stderr index a6a34620e92..6221c236830 100644 --- a/tests/ui/get_unwrap.stderr +++ b/tests/ui/get_unwrap.stderr @@ -187,5 +187,47 @@ LL | let _ = some_vec.get_mut(0..1).unwrap().to_vec(); | = help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message -error: aborting due to 26 previous errors +error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise + --> $DIR/get_unwrap.rs:77:20 + | +LL | let _x: &i32 = f.get(1 + 2).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^ help: try this: `&f[1 + 2]` + +error: used `unwrap()` on an `Option` value + --> $DIR/get_unwrap.rs:77:20 + | +LL | let _x: &i32 = f.get(1 + 2).unwrap(); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message + +error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise + --> $DIR/get_unwrap.rs:80:14 + | +LL | let _x = f.get(1 + 2).unwrap().to_string(); + | ^^^^^^^^^^^^^^^^^^^^^ help: try this: `f[1 + 2]` + +error: used `unwrap()` on an `Option` value + --> $DIR/get_unwrap.rs:80:14 + | +LL | let _x = f.get(1 + 2).unwrap().to_string(); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message + +error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise + --> $DIR/get_unwrap.rs:83:14 + | +LL | let _x = f.get(1 + 2).unwrap().abs(); + | ^^^^^^^^^^^^^^^^^^^^^ help: try this: `f[1 + 2]` + +error: used `unwrap()` on an `Option` value + --> $DIR/get_unwrap.rs:83:14 + | +LL | let _x = f.get(1 + 2).unwrap().abs(); + | ^^^^^^^^^^^^^^^^^^^^^ + | + = help: if you don't want to handle the `None` case gracefully, consider using `expect()` to provide a better panic message + +error: aborting due to 32 previous errors