Consider deref'ed argument as non-temporary (#15172)
If there are more than one dereference (there is one corresponding matched with a borrow in any case), consider that the argument might point to a place expression, which is the safest choice. Also, use an appropriate number of dereferences in suggestions involving arguments using themselves multiple dereferences. Fixes rust-lang/rust-clippy#15166 changelog: [`swap_with_temporary`]: fix false positive leading to different semantics being suggested, and use the right number of dereferences in suggestion r? y21 <!-- TRIAGEBOT_START --> <!-- TRIAGEBOT_SUMMARY_START --> ### Summary Notes - [beta-nomination](https://github.com/rust-lang/rust-clippy/pull/15172#issuecomment-3016752569) by [samueltardieu](https://github.com/samueltardieu) *Managed by `@rustbot`—see [help](https://forge.rust-lang.org/triagebot/note.html) for details* <!-- TRIAGEBOT_SUMMARY_END --> <!-- TRIAGEBOT_END -->
This commit is contained in:
Timo
GitHub
commit
936baca0cd
4 changed files with 166 additions and 16 deletions
|
|
@ -4,6 +4,7 @@ use rustc_ast::BorrowKind;
|
|||
use rustc_errors::{Applicability, Diag};
|
||||
use rustc_hir::{Expr, ExprKind, Node, QPath};
|
||||
use rustc_lint::LateContext;
|
||||
use rustc_middle::ty::adjustment::Adjust;
|
||||
use rustc_span::sym;
|
||||
|
||||
use super::SWAP_WITH_TEMPORARY;
|
||||
|
|
@ -11,12 +12,12 @@ use super::SWAP_WITH_TEMPORARY;
|
|||
const MSG_TEMPORARY: &str = "this expression returns a temporary value";
|
||||
const MSG_TEMPORARY_REFMUT: &str = "this is a mutable reference to a temporary value";
|
||||
|
||||
pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args: &[Expr<'_>]) {
|
||||
pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &Expr<'_>, func: &Expr<'_>, args: &'tcx [Expr<'_>]) {
|
||||
if let ExprKind::Path(QPath::Resolved(_, func_path)) = func.kind
|
||||
&& let Some(func_def_id) = func_path.res.opt_def_id()
|
||||
&& cx.tcx.is_diagnostic_item(sym::mem_swap, func_def_id)
|
||||
{
|
||||
match (ArgKind::new(&args[0]), ArgKind::new(&args[1])) {
|
||||
match (ArgKind::new(cx, &args[0]), ArgKind::new(cx, &args[1])) {
|
||||
(ArgKind::RefMutToTemp(left_temp), ArgKind::RefMutToTemp(right_temp)) => {
|
||||
emit_lint_useless(cx, expr, &args[0], &args[1], left_temp, right_temp);
|
||||
},
|
||||
|
|
@ -28,10 +29,10 @@ pub(super) fn check(cx: &LateContext<'_>, expr: &Expr<'_>, func: &Expr<'_>, args
|
|||
}
|
||||
|
||||
enum ArgKind<'tcx> {
|
||||
// Mutable reference to a place, coming from a macro
|
||||
RefMutToPlaceAsMacro(&'tcx Expr<'tcx>),
|
||||
// Place behind a mutable reference
|
||||
RefMutToPlace(&'tcx Expr<'tcx>),
|
||||
// Mutable reference to a place, coming from a macro, and number of dereferences to use
|
||||
RefMutToPlaceAsMacro(&'tcx Expr<'tcx>, usize),
|
||||
// Place behind a mutable reference, and number of dereferences to use
|
||||
RefMutToPlace(&'tcx Expr<'tcx>, usize),
|
||||
// Temporary value behind a mutable reference
|
||||
RefMutToTemp(&'tcx Expr<'tcx>),
|
||||
// Any other case
|
||||
|
|
@ -39,13 +40,29 @@ enum ArgKind<'tcx> {
|
|||
}
|
||||
|
||||
impl<'tcx> ArgKind<'tcx> {
|
||||
fn new(arg: &'tcx Expr<'tcx>) -> Self {
|
||||
if let ExprKind::AddrOf(BorrowKind::Ref, _, target) = arg.kind {
|
||||
if target.is_syntactic_place_expr() {
|
||||
/// Build a new `ArgKind` from `arg`. There must be no false positive when returning a
|
||||
/// `ArgKind::RefMutToTemp` variant, as this may cause a spurious lint to be emitted.
|
||||
fn new(cx: &LateContext<'tcx>, arg: &'tcx Expr<'tcx>) -> Self {
|
||||
if let ExprKind::AddrOf(BorrowKind::Ref, _, target) = arg.kind
|
||||
&& let adjustments = cx.typeck_results().expr_adjustments(arg)
|
||||
&& adjustments
|
||||
.first()
|
||||
.is_some_and(|adj| matches!(adj.kind, Adjust::Deref(None)))
|
||||
&& adjustments
|
||||
.last()
|
||||
.is_some_and(|adj| matches!(adj.kind, Adjust::Borrow(_)))
|
||||
{
|
||||
let extra_derefs = adjustments[1..adjustments.len() - 1]
|
||||
.iter()
|
||||
.filter(|adj| matches!(adj.kind, Adjust::Deref(_)))
|
||||
.count();
|
||||
// If a deref is used, `arg` might be a place expression. For example, a mutex guard
|
||||
// would dereference into the mutex content which is probably not temporary.
|
||||
if target.is_syntactic_place_expr() || extra_derefs > 0 {
|
||||
if arg.span.from_expansion() {
|
||||
ArgKind::RefMutToPlaceAsMacro(arg)
|
||||
ArgKind::RefMutToPlaceAsMacro(arg, extra_derefs)
|
||||
} else {
|
||||
ArgKind::RefMutToPlace(target)
|
||||
ArgKind::RefMutToPlace(target, extra_derefs)
|
||||
}
|
||||
} else {
|
||||
ArgKind::RefMutToTemp(target)
|
||||
|
|
@ -106,10 +123,15 @@ fn emit_lint_assign(cx: &LateContext<'_>, expr: &Expr<'_>, target: &ArgKind<'_>,
|
|||
let mut applicability = Applicability::MachineApplicable;
|
||||
let ctxt = expr.span.ctxt();
|
||||
let assign_target = match target {
|
||||
ArgKind::Expr(target) | ArgKind::RefMutToPlaceAsMacro(target) => {
|
||||
Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability).deref()
|
||||
},
|
||||
ArgKind::RefMutToPlace(target) => Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability),
|
||||
ArgKind::Expr(target) => Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability).deref(),
|
||||
ArgKind::RefMutToPlaceAsMacro(arg, derefs) => (0..*derefs).fold(
|
||||
Sugg::hir_with_context(cx, arg, ctxt, "_", &mut applicability).deref(),
|
||||
|sugg, _| sugg.deref(),
|
||||
),
|
||||
ArgKind::RefMutToPlace(target, derefs) => (0..*derefs).fold(
|
||||
Sugg::hir_with_context(cx, target, ctxt, "_", &mut applicability),
|
||||
|sugg, _| sugg.deref(),
|
||||
),
|
||||
ArgKind::RefMutToTemp(_) => unreachable!(),
|
||||
};
|
||||
let assign_source = Sugg::hir_with_context(cx, temp, ctxt, "_", &mut applicability);
|
||||
|
|
|
|||
|
|
@ -72,3 +72,49 @@ fn dont_lint_those(s: &mut S, v: &mut [String], w: Option<&mut String>) {
|
|||
swap(&mut s.t, v.get_mut(0).unwrap());
|
||||
swap(w.unwrap(), &mut s.t);
|
||||
}
|
||||
|
||||
fn issue15166() {
|
||||
use std::sync::Mutex;
|
||||
|
||||
struct A {
|
||||
thing: Mutex<Vec<u8>>,
|
||||
}
|
||||
|
||||
impl A {
|
||||
fn a(&self) {
|
||||
let mut new_vec = vec![42];
|
||||
// Do not lint here, as neither `new_vec` nor the result of `.lock().unwrap()` are temporaries
|
||||
swap(&mut new_vec, &mut self.thing.lock().unwrap());
|
||||
for v in new_vec {
|
||||
// Do something with v
|
||||
}
|
||||
// Here `vec![42]` is temporary though, and a proper dereference will have to be used in the fix
|
||||
*self.thing.lock().unwrap() = vec![42];
|
||||
//~^ ERROR: swapping with a temporary value is inefficient
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn multiple_deref() {
|
||||
let mut v1 = &mut &mut &mut vec![42];
|
||||
***v1 = vec![];
|
||||
//~^ ERROR: swapping with a temporary value is inefficient
|
||||
|
||||
struct Wrapper<T: ?Sized>(T);
|
||||
impl<T: ?Sized> std::ops::Deref for Wrapper<T> {
|
||||
type Target = T;
|
||||
fn deref(&self) -> &Self::Target {
|
||||
&self.0
|
||||
}
|
||||
}
|
||||
impl<T: ?Sized> std::ops::DerefMut for Wrapper<T> {
|
||||
fn deref_mut(&mut self) -> &mut Self::Target {
|
||||
&mut self.0
|
||||
}
|
||||
}
|
||||
|
||||
use std::sync::Mutex;
|
||||
let mut v1 = Mutex::new(Wrapper(Wrapper(vec![42])));
|
||||
***v1.lock().unwrap() = vec![];
|
||||
//~^ ERROR: swapping with a temporary value is inefficient
|
||||
}
|
||||
|
|
|
|||
|
|
@ -72,3 +72,49 @@ fn dont_lint_those(s: &mut S, v: &mut [String], w: Option<&mut String>) {
|
|||
swap(&mut s.t, v.get_mut(0).unwrap());
|
||||
swap(w.unwrap(), &mut s.t);
|
||||
}
|
||||
|
||||
fn issue15166() {
|
||||
use std::sync::Mutex;
|
||||
|
||||
struct A {
|
||||
thing: Mutex<Vec<u8>>,
|
||||
}
|
||||
|
||||
impl A {
|
||||
fn a(&self) {
|
||||
let mut new_vec = vec![42];
|
||||
// Do not lint here, as neither `new_vec` nor the result of `.lock().unwrap()` are temporaries
|
||||
swap(&mut new_vec, &mut self.thing.lock().unwrap());
|
||||
for v in new_vec {
|
||||
// Do something with v
|
||||
}
|
||||
// Here `vec![42]` is temporary though, and a proper dereference will have to be used in the fix
|
||||
swap(&mut vec![42], &mut self.thing.lock().unwrap());
|
||||
//~^ ERROR: swapping with a temporary value is inefficient
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
fn multiple_deref() {
|
||||
let mut v1 = &mut &mut &mut vec![42];
|
||||
swap(&mut ***v1, &mut vec![]);
|
||||
//~^ ERROR: swapping with a temporary value is inefficient
|
||||
|
||||
struct Wrapper<T: ?Sized>(T);
|
||||
impl<T: ?Sized> std::ops::Deref for Wrapper<T> {
|
||||
type Target = T;
|
||||
fn deref(&self) -> &Self::Target {
|
||||
&self.0
|
||||
}
|
||||
}
|
||||
impl<T: ?Sized> std::ops::DerefMut for Wrapper<T> {
|
||||
fn deref_mut(&mut self) -> &mut Self::Target {
|
||||
&mut self.0
|
||||
}
|
||||
}
|
||||
|
||||
use std::sync::Mutex;
|
||||
let mut v1 = Mutex::new(Wrapper(Wrapper(vec![42])));
|
||||
swap(&mut vec![], &mut v1.lock().unwrap());
|
||||
//~^ ERROR: swapping with a temporary value is inefficient
|
||||
}
|
||||
|
|
|
|||
|
|
@ -96,5 +96,41 @@ note: this expression returns a temporary value
|
|||
LL | swap(mac!(refmut y), &mut func());
|
||||
| ^^^^^^
|
||||
|
||||
error: aborting due to 8 previous errors
|
||||
error: swapping with a temporary value is inefficient
|
||||
--> tests/ui/swap_with_temporary.rs:92:13
|
||||
|
|
||||
LL | swap(&mut vec![42], &mut self.thing.lock().unwrap());
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use assignment instead: `*self.thing.lock().unwrap() = vec![42]`
|
||||
|
|
||||
note: this expression returns a temporary value
|
||||
--> tests/ui/swap_with_temporary.rs:92:23
|
||||
|
|
||||
LL | swap(&mut vec![42], &mut self.thing.lock().unwrap());
|
||||
| ^^^^^^^^
|
||||
|
||||
error: swapping with a temporary value is inefficient
|
||||
--> tests/ui/swap_with_temporary.rs:100:5
|
||||
|
|
||||
LL | swap(&mut ***v1, &mut vec![]);
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use assignment instead: `***v1 = vec![]`
|
||||
|
|
||||
note: this expression returns a temporary value
|
||||
--> tests/ui/swap_with_temporary.rs:100:27
|
||||
|
|
||||
LL | swap(&mut ***v1, &mut vec![]);
|
||||
| ^^^^^^
|
||||
|
||||
error: swapping with a temporary value is inefficient
|
||||
--> tests/ui/swap_with_temporary.rs:118:5
|
||||
|
|
||||
LL | swap(&mut vec![], &mut v1.lock().unwrap());
|
||||
| ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use assignment instead: `***v1.lock().unwrap() = vec![]`
|
||||
|
|
||||
note: this expression returns a temporary value
|
||||
--> tests/ui/swap_with_temporary.rs:118:15
|
||||
|
|
||||
LL | swap(&mut vec![], &mut v1.lock().unwrap());
|
||||
| ^^^^^^
|
||||
|
||||
error: aborting due to 11 previous errors
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue