Auto merge of #10979 - y21:issue9909, r=giraffate

[`get_unwrap`]: include a borrow in the suggestion if argument is not an integer literal

Fixes #9909

I have to say, I don't really understand what the previous logic was trying to do, but this fixes the linked bug.
It was checking if the argument passed to `.get()` can be parsed as a usize (i.e. if it's an integer literal, probably?), and if not, it wouldn't include a borrow? I don't know how we came to that conclusion, but that logic doesn't work:
```rs
let slice = &[1, 2];
let _r: &i32 = slice.get({ 1 }).unwrap();
// previous suggestion: slice[{ 1 }]
// the suggestion should be: &slice[{ 1 }]
```
Here the argument passed to it isn't an integer literal, but it should still include a borrow, because it would otherwise change the type from `&i32` to `i32`.

The exception is that if the parent of the `get().unwrap()` expr is a dereference or a method call or the like, we don't need an explicit borrow because it's automatically inserted by the compiler

changelog: [`get_unwrap`]: include a borrow in the suggestion if argument is not an integer literal

Author: bors

clippy_lints/src/methods/get_unwrap.rs

@@ -3,7 +3,6 @@ use clippy_utils::diagnostics::span_lint_and_sugg;
 use clippy_utils::get_parent_expr;
 use clippy_utils::source::snippet_with_applicability;
 use clippy_utils::ty::is_type_diagnostic_item;
-use if_chain::if_chain;
 use rustc_errors::Applicability;
 use rustc_hir as hir;
 use rustc_lint::LateContext;
@@ -23,40 +22,40 @@ pub(super) fn check<'tcx>(
     let mut applicability = Applicability::MachineApplicable;
     let expr_ty = cx.typeck_results().expr_ty(recv);
     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();
         "slice"
     } else if is_type_diagnostic_item(cx, expr_ty, sym::Vec) {
-        needs_ref = get_args_str.parse::<usize>().is_ok();
         "Vec"
     } else if is_type_diagnostic_item(cx, expr_ty, sym::VecDeque) {
-        needs_ref = get_args_str.parse::<usize>().is_ok();
         "VecDeque"
     } else if !is_mut && is_type_diagnostic_item(cx, expr_ty, sym::HashMap) {
-        needs_ref = true;
         "HashMap"
     } else if !is_mut && is_type_diagnostic_item(cx, expr_ty, sym::BTreeMap) {
-        needs_ref = true;
         "BTreeMap"
     } else {
         return; // caller is not a type that we want to lint
     };
 
     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.
-    if_chain! {
-        if needs_ref;
-        if let Some(parent) = get_parent_expr(cx, expr);
-        if let hir::ExprKind::Unary(hir::UnOp::Deref, _) = parent.kind;
-        then {
-            needs_ref = false;
+    // 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
+    let needs_ref = if let Some(parent) = get_parent_expr(cx, expr)
+        && let hir::ExprKind::Unary(hir::UnOp::Deref, _)
+            | hir::ExprKind::MethodCall(..)
+            | hir::ExprKind::Field(..)
+            | hir::ExprKind::Index(..) = parent.kind
+    {
+        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;
         }
-    }
+        false
+    } else {
+        true
+    };
 
     let mut_str = if is_mut { "_mut" } else { "" };
     let borrow_str = if !needs_ref {

tests/ui/get_unwrap.fixed

@@ -70,3 +70,42 @@ fn main() {
         let _ = some_vec[0..1].to_vec();
     }
 }
+mod issue9909 {
+    #![allow(clippy::identity_op, clippy::unwrap_used, dead_code)]
+
+    fn reduced() {
+        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
+    }
+
+    // original code:
+    fn linidx(row: usize, col: usize) -> usize {
+        row * 1 + col * 3
+    }
+
+    fn main_() {
+        let mut mat = [1.0f32, 5.0, 9.0, 2.0, 6.0, 10.0, 3.0, 7.0, 11.0, 4.0, 8.0, 12.0];
+
+        for i in 0..2 {
+            for j in i + 1..3 {
+                if mat[linidx(j, 3)] > mat[linidx(i, 3)] {
+                    for k in 0..4 {
+                        let (x, rest) = mat.split_at_mut(linidx(i, k) + 1);
+                        let a = x.last_mut().unwrap();
+                        let b = &mut rest[linidx(j, k) - linidx(i, k) - 1];
+                        ::std::mem::swap(a, b);
+                    }
+                }
+            }
+        }
+        assert_eq!([9.0, 5.0, 1.0, 10.0, 6.0, 2.0, 11.0, 7.0, 3.0, 12.0, 8.0, 4.0], mat);
+    }
+}

tests/ui/get_unwrap.rs

@@ -70,3 +70,42 @@ fn main() {
         let _ = some_vec.get_mut(0..1).unwrap().to_vec();
     }
 }
+mod issue9909 {
+    #![allow(clippy::identity_op, clippy::unwrap_used, dead_code)]
+
+    fn reduced() {
+        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
+    }
+
+    // original code:
+    fn linidx(row: usize, col: usize) -> usize {
+        row * 1 + col * 3
+    }
+
+    fn main_() {
+        let mut mat = [1.0f32, 5.0, 9.0, 2.0, 6.0, 10.0, 3.0, 7.0, 11.0, 4.0, 8.0, 12.0];
+
+        for i in 0..2 {
+            for j in i + 1..3 {
+                if mat[linidx(j, 3)] > mat[linidx(i, 3)] {
+                    for k in 0..4 {
+                        let (x, rest) = mat.split_at_mut(linidx(i, k) + 1);
+                        let a = x.last_mut().unwrap();
+                        let b = rest.get_mut(linidx(j, k) - linidx(i, k) - 1).unwrap();
+                        ::std::mem::swap(a, b);
+                    }
+                }
+            }
+        }
+        assert_eq!([9.0, 5.0, 1.0, 10.0, 6.0, 2.0, 11.0, 7.0, 3.0, 12.0, 8.0, 4.0], mat);
+    }
+}

tests/ui/get_unwrap.stderr

@@ -187,5 +187,29 @@ 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:79:24
+   |
+LL |         let _x: &i32 = f.get(1 + 2).unwrap();
+   |                        ^^^^^^^^^^^^^^^^^^^^^ help: try this: `&f[1 + 2]`
+
+error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise
+  --> $DIR/get_unwrap.rs:82:18
+   |
+LL |         let _x = f.get(1 + 2).unwrap().to_string();
+   |                  ^^^^^^^^^^^^^^^^^^^^^ help: try this: `f[1 + 2]`
+
+error: called `.get().unwrap()` on a slice. Using `[]` is more clear and more concise
+  --> $DIR/get_unwrap.rs:85:18
+   |
+LL |         let _x = f.get(1 + 2).unwrap().abs();
+   |                  ^^^^^^^^^^^^^^^^^^^^^ help: try this: `f[1 + 2]`
+
+error: called `.get_mut().unwrap()` on a slice. Using `[]` is more clear and more concise
+  --> $DIR/get_unwrap.rs:103:33
+   |
+LL |                         let b = rest.get_mut(linidx(j, k) - linidx(i, k) - 1).unwrap();
+   |                                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `&mut rest[linidx(j, k) - linidx(i, k) - 1]`
+
+error: aborting due to 30 previous errors