dnrops
/
rust
mirror of https://github.com/rust-lang/rust.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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
This commit is contained in:
bors 2023-06-21 23:57:02 +00:00
parent 9fa4089410 bdb2a17107
commit 6ec23880f2
4 changed files with 119 additions and 18 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

33
clippy_lints/src/methods/get_unwrap.rs
View File

@ -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,21 +22,15 @@ 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
@ -45,18 +38,24 @@ 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.
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 {

39
tests/ui/get_unwrap.fixed
View File

@ -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);
}
}

39
tests/ui/get_unwrap.rs
View File

@ -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);
}
}

26
tests/ui/get_unwrap.stderr
View File

@ -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