Skip to content

Commit

Permalink
Auto merge of #4877 - flip1995:manual_swap_4853, r=llogiq
Browse files Browse the repository at this point in the history 
Fix FP in manual_swap lint with slice-like types

Fixes #4853

changelog: Fix FP in [`manual_swap`] lint with slice-like types and make it auto applicable
  • Loading branch information
@bors
bors committed Dec 3, 2019
2 parents f0fc18a + aa2381d commit c66cca4
Show file tree
Hide file tree
Showing 4 changed files with 196 additions and 58 deletions.
145 changes: 94 additions & 51 deletions clippy_lints/src/swap.rs
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use crate::utils::sugg::Sugg;
use crate::utils::{
differing_macro_contexts, is_type_diagnostic_item, match_type, paths, snippet, span_lint_and_then, walk_ptrs_ty,
SpanlessEq,
differing_macro_contexts, is_type_diagnostic_item, match_type, paths, snippet_with_applicability,
span_lint_and_then, walk_ptrs_ty, SpanlessEq,
};
use if_chain::if_chain;
use matches::matches;
Expand Down Expand Up @@ -97,29 +97,6 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) {
if SpanlessEq::new(cx).ignore_fn().eq_expr(tmp_init, lhs1);
if SpanlessEq::new(cx).ignore_fn().eq_expr(rhs1, lhs2);
then {
fn check_for_slice<'a>(
cx: &LateContext<'_, '_>,
lhs1: &'a Expr,
lhs2: &'a Expr,
) -> Option<(&'a Expr, &'a Expr, &'a Expr)> {
if let ExprKind::Index(ref lhs1, ref idx1) = lhs1.kind {
if let ExprKind::Index(ref lhs2, ref idx2) = lhs2.kind {
if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, lhs2) {
let ty = walk_ptrs_ty(cx.tables.expr_ty(lhs1));

if matches!(ty.kind, ty::Slice(_)) ||
matches!(ty.kind, ty::Array(_, _)) ||
is_type_diagnostic_item(cx, ty, Symbol::intern("vec_type")) ||
match_type(cx, ty, &paths::VEC_DEQUE) {
return Some((lhs1, idx1, idx2));
}
}
}
}

None
}

if let ExprKind::Field(ref lhs1, _) = lhs1.kind {
if let ExprKind::Field(ref lhs2, _) = lhs2.kind {
if lhs1.hir_id.owner_def_id() == lhs2.hir_id.owner_def_id() {
Expand All @@ -128,47 +105,113 @@ fn check_manual_swap(cx: &LateContext<'_, '_>, block: &Block) {
}
}

let (replace, what, sugg) = if let Some((slice, idx1, idx2)) = check_for_slice(cx, lhs1, lhs2) {
let mut applicability = Applicability::MachineApplicable;

let slice = check_for_slice(cx, lhs1, lhs2);
let (replace, what, sugg) = if let Slice::NotSwappable = slice {
return;
} else if let Slice::Swappable(slice, idx1, idx2) = slice {
if let Some(slice) = Sugg::hir_opt(cx, slice) {
(false,
format!(" elements of `{}`", slice),
format!("{}.swap({}, {})",
slice.maybe_par(),
snippet(cx, idx1.span, ".."),
snippet(cx, idx2.span, "..")))
(
false,
format!(" elements of `{}`", slice),
format!(
"{}.swap({}, {})",
slice.maybe_par(),
snippet_with_applicability(cx, idx1.span, "..", &mut applicability),
snippet_with_applicability(cx, idx2.span, "..", &mut applicability),
),
)
} else {
(false, String::new(), String::new())
}
} else if let (Some(first), Some(second)) = (Sugg::hir_opt(cx, lhs1), Sugg::hir_opt(cx, rhs1)) {
(true, format!(" `{}` and `{}`", first, second),
format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()))
(
true,
format!(" `{}` and `{}`", first, second),
format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()),
)
} else {
(true, String::new(), String::new())
};

let span = w[0].span.to(second.span);

span_lint_and_then(cx,
MANUAL_SWAP,
span,
&format!("this looks like you are swapping{} manually", what),
|db| {
if !sugg.is_empty() {
db.span_suggestion(
span,
"try",
sugg,
Applicability::Unspecified,
);
span_lint_and_then(
cx,
MANUAL_SWAP,
span,
&format!("this looks like you are swapping{} manually", what),
|db| {
if !sugg.is_empty() {
db.span_suggestion(
span,
"try",
sugg,
applicability,
);

if replace {
db.note("or maybe you should use `std::mem::replace`?");
}
}
});
if replace {
db.note("or maybe you should use `std::mem::replace`?");
}
}
}
);
}
}
}
}

enum Slice<'a> {
/// `slice.swap(idx1, idx2)` can be used
///
/// ## Example
///
/// ```rust
/// # let mut a = vec![0, 1];
/// let t = a[1];
/// a[1] = a[0];
/// a[0] = t;
/// // can be written as
/// a.swap(0, 1);
/// ```
Swappable(&'a Expr, &'a Expr, &'a Expr),
/// The `swap` function cannot be used.
///
/// ## Example
///
/// ```rust
/// # let mut a = [vec![1, 2], vec![3, 4]];
/// let t = a[0][1];
/// a[0][1] = a[1][0];
/// a[1][0] = t;
/// ```
NotSwappable,
/// Not a slice
None,
}

/// Checks if both expressions are index operations into "slice-like" types.
fn check_for_slice<'a>(cx: &LateContext<'_, '_>, lhs1: &'a Expr, lhs2: &'a Expr) -> Slice<'a> {
if let ExprKind::Index(ref lhs1, ref idx1) = lhs1.kind {
if let ExprKind::Index(ref lhs2, ref idx2) = lhs2.kind {
if SpanlessEq::new(cx).ignore_fn().eq_expr(lhs1, lhs2) {
let ty = walk_ptrs_ty(cx.tables.expr_ty(lhs1));

if matches!(ty.kind, ty::Slice(_))
|| matches!(ty.kind, ty::Array(_, _))
|| is_type_diagnostic_item(cx, ty, Symbol::intern("vec_type"))
|| match_type(cx, ty, &paths::VEC_DEQUE)
{
return Slice::Swappable(lhs1, idx1, idx2);
}
} else {
return Slice::NotSwappable;
}
}
}

Slice::None
}

/// Implementation of the `ALMOST_SWAPPED` lint.
Expand Down
83 changes: 83 additions & 0 deletions tests/ui/swap.fixed
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,83 @@
// run-rustfix

#![warn(clippy::all)]
#![allow(
clippy::blacklisted_name,
clippy::no_effect,
clippy::redundant_clone,
redundant_semicolon,
unused_assignments
)]

struct Foo(u32);

#[derive(Clone)]
struct Bar {
a: u32,
b: u32,
}

fn field() {
let mut bar = Bar { a: 1, b: 2 };

let temp = bar.a;
bar.a = bar.b;
bar.b = temp;

let mut baz = vec![bar.clone(), bar.clone()];
let temp = baz[0].a;
baz[0].a = baz[1].a;
baz[1].a = temp;
}

fn array() {
let mut foo = [1, 2];
foo.swap(0, 1);

foo.swap(0, 1);
}

fn slice() {
let foo = &mut [1, 2];
foo.swap(0, 1);

foo.swap(0, 1);
}

fn unswappable_slice() {
let foo = &mut [vec![1, 2], vec![3, 4]];
let temp = foo[0][1];
foo[0][1] = foo[1][0];
foo[1][0] = temp;

// swap(foo[0][1], foo[1][0]) would fail
}

fn vec() {
let mut foo = vec![1, 2];
foo.swap(0, 1);

foo.swap(0, 1);
}

#[rustfmt::skip]
fn main() {
field();
array();
slice();
unswappable_slice();
vec();

let mut a = 42;
let mut b = 1337;

std::mem::swap(&mut a, &mut b);

; std::mem::swap(&mut a, &mut b);

let mut c = Foo(42);

std::mem::swap(&mut c.0, &mut a);

; std::mem::swap(&mut c.0, &mut a);
}
12 changes: 12 additions & 0 deletions tests/ui/swap.rs
View file
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
// run-rustfix

#![warn(clippy::all)]
#![allow(
clippy::blacklisted_name,
Expand Down Expand Up @@ -46,6 +48,15 @@ fn slice() {
foo.swap(0, 1);
}

fn unswappable_slice() {
let foo = &mut [vec![1, 2], vec![3, 4]];
let temp = foo[0][1];
foo[0][1] = foo[1][0];
foo[1][0] = temp;

// swap(foo[0][1], foo[1][0]) would fail
}

fn vec() {
let mut foo = vec![1, 2];
let temp = foo[0];
Expand All @@ -60,6 +71,7 @@ fn main() {
field();
array();
slice();
unswappable_slice();
vec();

let mut a = 42;
Expand Down
14 changes: 7 additions & 7 deletions tests/ui/swap.stderr
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:33:5
--> $DIR/swap.rs:35:5
|
LL | / let temp = foo[0];
LL | | foo[0] = foo[1];
Expand All @@ -9,23 +9,23 @@ LL | | foo[1] = temp;
= note: `-D clippy::manual-swap` implied by `-D warnings`

error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:42:5
--> $DIR/swap.rs:44:5
|
LL | / let temp = foo[0];
LL | | foo[0] = foo[1];
LL | | foo[1] = temp;
| |_________________^ help: try: `foo.swap(0, 1)`

error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:51:5
--> $DIR/swap.rs:62:5
|
LL | / let temp = foo[0];
LL | | foo[0] = foo[1];
LL | | foo[1] = temp;
| |_________________^ help: try: `foo.swap(0, 1)`

error: this looks like you are swapping `a` and `b` manually
--> $DIR/swap.rs:71:7
--> $DIR/swap.rs:83:7
|
LL | ; let t = a;
| _______^
Expand All @@ -36,7 +36,7 @@ LL | | b = t;
= note: or maybe you should use `std::mem::replace`?

error: this looks like you are swapping `c.0` and `a` manually
--> $DIR/swap.rs:80:7
--> $DIR/swap.rs:92:7
|
LL | ; let t = c.0;
| _______^
Expand All @@ -47,7 +47,7 @@ LL | | a = t;
= note: or maybe you should use `std::mem::replace`?

error: this looks like you are trying to swap `a` and `b`
--> $DIR/swap.rs:68:5
--> $DIR/swap.rs:80:5
|
LL | / a = b;
LL | | b = a;
Expand All @@ -57,7 +57,7 @@ LL | | b = a;
= note: or maybe you should use `std::mem::replace`?

error: this looks like you are trying to swap `c.0` and `a`
--> $DIR/swap.rs:77:5
--> $DIR/swap.rs:89:5
|
LL | / c.0 = a;
LL | | a = c.0;
Expand Down

0 comments on commit c66cca4

Please sign in to comment.