include a ref if argument is not just a numeric literal

Author: y21

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::<usize>().is_ok();
+        needs_ref = true;
         "slice"
     } else if is_type_diagnostic_item(cx, expr_ty, sym::Vec) {
-        needs_ref = get_args_str.parse::<usize>().is_ok();
+        needs_ref = true;
         "Vec"
     } else if is_type_diagnostic_item(cx, expr_ty, sym::VecDeque) {
-        needs_ref = get_args_str.parse::<usize>().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;
         }
     }
 

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
 }

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
 }

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