mikros/rust
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

merge XOR_SWAP with MANUAL_SWAP

Browse Source
This commit is contained in:
hamidreza kalbasi 2021-07-29 16:38:17 +04:30
parent dbb10b87f8
commit 6d36d1a835
7 changed files with 178 additions and 240 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

1
CHANGELOG.md
View File

@ -2900,7 +2900,6 @@ Released 2018-09-13
[`wrong_pub_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_pub_self_convention
[`wrong_self_convention`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_self_convention
[`wrong_transmute`]: https://rust-lang.github.io/rust-clippy/master/index.html#wrong_transmute
[`xor_swap`]: https://rust-lang.github.io/rust-clippy/master/index.html#xor_swap
[`zero_divided_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_divided_by_zero
[`zero_prefixed_literal`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_prefixed_literal
[`zero_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_ptr

3
clippy_lints/src/lib.rs
View File

@ -922,7 +922,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL,
swap::ALMOST_SWAPPED,
swap::MANUAL_SWAP,
swap::XOR_SWAP,
tabs_in_doc_comments::TABS_IN_DOC_COMMENTS,
temporary_assignment::TEMPORARY_ASSIGNMENT,
to_digit_is_some::TO_DIGIT_IS_SOME,
@ -1420,7 +1419,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL),
LintId::of(swap::ALMOST_SWAPPED),
LintId::of(swap::MANUAL_SWAP),
LintId::of(swap::XOR_SWAP),
LintId::of(tabs_in_doc_comments::TABS_IN_DOC_COMMENTS),
LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT),
LintId::of(to_digit_is_some::TO_DIGIT_IS_SOME),
@ -1649,7 +1647,6 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf:
LintId::of(strings::STRING_FROM_UTF8_AS_BYTES),
LintId::of(strlen_on_c_strings::STRLEN_ON_C_STRINGS),
LintId::of(swap::MANUAL_SWAP),
LintId::of(swap::XOR_SWAP),
LintId::of(temporary_assignment::TEMPORARY_ASSIGNMENT),
LintId::of(transmute::CROSSPOINTER_TRANSMUTE),
LintId::of(transmute::TRANSMUTES_EXPRESSIBLE_AS_PTR_CASTS),

259
clippy_lints/src/swap.rs
View File

@ -2,7 +2,7 @@ use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_and_then};
use clippy_utils::source::snippet_with_applicability;
use clippy_utils::sugg::Sugg;
use clippy_utils::ty::is_type_diagnostic_item;
use clippy_utils::{differing_macro_contexts, eq_expr_value};
use clippy_utils::{can_mut_borrow_both, differing_macro_contexts, eq_expr_value};
use if_chain::if_chain;
use rustc_errors::Applicability;
use rustc_hir::{BinOpKind, Block, Expr, ExprKind, PatKind, QPath, Stmt, StmtKind};
@ -10,7 +10,7 @@ use rustc_lint::{LateContext, LateLintPass};
use rustc_middle::ty;
use rustc_session::{declare_lint_pass, declare_tool_lint};
use rustc_span::source_map::Spanned;
use rustc_span::sym;
use rustc_span::{sym, Span};
declare_clippy_lint! {
/// ### What it does
@ -65,37 +65,7 @@ declare_clippy_lint! {
"`foo = bar; bar = foo` sequence"
}
declare_clippy_lint! {
/// **What it does:** Checks for uses of xor-based swaps.
///
/// **Why is this bad?** The `std::mem::swap` function exposes the intent better
/// without deinitializing or copying either variable.
///
/// **Known problems:** None.
///
/// **Example:**
///
/// ```rust
/// let mut a = 1;
/// let mut b = 2;
///
/// a ^= b;
/// b ^= a;
/// a ^= b;
/// ```
///
/// Use std::mem::swap() instead:
/// ```rust
/// let mut a = 1;
/// let mut b = 2;
/// std::mem::swap(&mut a, &mut b);
/// ```
pub XOR_SWAP,
complexity,
"xor-based swap of two variables"
}
declare_lint_pass!(Swap => [MANUAL_SWAP, ALMOST_SWAPPED, XOR_SWAP]);
declare_lint_pass!(Swap => [MANUAL_SWAP, ALMOST_SWAPPED]);
impl<'tcx> LateLintPass<'tcx> for Swap {
fn check_block(&mut self, cx: &LateContext<'tcx>, block: &'tcx Block<'_>) {
@ -105,6 +75,63 @@ impl<'tcx> LateLintPass<'tcx> for Swap {
}
}
fn generate_swap_warning(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>, span: Span, is_xor_based: bool) {
let mut applicability = Applicability::MachineApplicable;
if !can_mut_borrow_both(cx, e1, e2) {
if let ExprKind::Index(lhs1, idx1) = e1.kind {
if let ExprKind::Index(lhs2, idx2) = e2.kind {
if eq_expr_value(cx, lhs1, lhs2) {
let ty = cx.typeck_results().expr_ty(lhs1).peel_refs();
if matches!(ty.kind(), ty::Slice(_))
|| matches!(ty.kind(), ty::Array(_, _))
|| is_type_diagnostic_item(cx, ty, sym::vec_type)
|| is_type_diagnostic_item(cx, ty, sym::vecdeque_type)
{
let slice = Sugg::hir_with_applicability(cx, lhs1, "<slice>", &mut applicability);
span_lint_and_sugg(
cx,
MANUAL_SWAP,
span,
&format!("this looks like you are swapping elements of `{}` manually", slice),
"try",
format!(
"{}.swap({}, {})",
slice.maybe_par(),
snippet_with_applicability(cx, idx1.span, "..", &mut applicability),
snippet_with_applicability(cx, idx2.span, "..", &mut applicability),
),
applicability,
);
}
}
}
}
return;
}
let first = Sugg::hir_with_applicability(cx, e1, "..", &mut applicability);
let second = Sugg::hir_with_applicability(cx, e2, "..", &mut applicability);
span_lint_and_then(
cx,
MANUAL_SWAP,
span,
&format!("this looks like you are swapping `{}` and `{}` manually", first, second),
|diag| {
diag.span_suggestion(
span,
"try",
format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()),
applicability,
);
if !is_xor_based {
diag.note("or maybe you should use `std::mem::replace`?");
}
},
);
}
/// Implementation of the `MANUAL_SWAP` lint.
fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) {
for w in block.stmts.windows(3) {
@ -128,123 +155,13 @@ fn check_manual_swap(cx: &LateContext<'_>, block: &Block<'_>) {
if eq_expr_value(cx, tmp_init, lhs1);
if eq_expr_value(cx, rhs1, lhs2);
then {
if let ExprKind::Field(lhs1, _) = lhs1.kind {
if let ExprKind::Field(lhs2, _) = lhs2.kind {
if lhs1.hir_id.owner == lhs2.hir_id.owner {
return;
}
}
}
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_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()),
)
} 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),
|diag| {
if !sugg.is_empty() {
diag.span_suggestion(
span,
"try",
sugg,
applicability,
);
if replace {
diag.note("or maybe you should use `std::mem::replace`?");
}
}
}
);
generate_swap_warning(cx, lhs1, lhs2, span, false);
}
}
}
}
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>, &'a Expr<'a>, &'a Expr<'a>),
/// 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(lhs1, idx1) = lhs1.kind {
if let ExprKind::Index(lhs2, idx2) = lhs2.kind {
if eq_expr_value(cx, lhs1, lhs2) {
let ty = cx.typeck_results().expr_ty(lhs1).peel_refs();
if matches!(ty.kind(), ty::Slice(_))
|| matches!(ty.kind(), ty::Array(_, _))
|| is_type_diagnostic_item(cx, ty, sym::vec_type)
|| is_type_diagnostic_item(cx, ty, sym::vecdeque_type)
{
return Slice::Swappable(lhs1, idx1, idx2);
}
} else {
return Slice::NotSwappable;
}
}
}
Slice::None
}
/// Implementation of the `ALMOST_SWAPPED` lint.
fn check_suspicious_swap(cx: &LateContext<'_>, block: &Block<'_>) {
for w in block.stmts.windows(2) {
@ -295,62 +212,20 @@ fn check_suspicious_swap(cx: &LateContext<'_>, block: &Block<'_>) {
}
}
/// Implementation of the `XOR_SWAP` lint.
/// Implementation of the xor case for `MANUAL_SWAP` lint.
fn check_xor_swap(cx: &LateContext<'_>, block: &Block<'_>) {
for window in block.stmts.windows(3) {
if_chain! {
if let Some((lhs0, rhs0)) = extract_sides_of_xor_assign(&window[0]);
if let Some((lhs1, rhs1)) = extract_sides_of_xor_assign(&window[1]);
if let Some((lhs2, rhs2)) = extract_sides_of_xor_assign(&window[2]);
if eq_expr_value(cx, lhs0, rhs1)
&& eq_expr_value(cx, rhs1, lhs2)
&& eq_expr_value(cx, rhs0, lhs1)
&& eq_expr_value(cx, lhs1, rhs2);
if eq_expr_value(cx, lhs0, rhs1);
if eq_expr_value(cx, lhs2, rhs1);
if eq_expr_value(cx, lhs1, rhs0);
if eq_expr_value(cx, lhs1, rhs2);
then {
let span = window[0].span.to(window[2].span);
let mut applicability = Applicability::MachineApplicable;
match check_for_slice(cx, lhs0, rhs0) {
Slice::Swappable(slice, idx0, idx1) => {
if let Some(slice) = Sugg::hir_opt(cx, slice) {
span_lint_and_sugg(
cx,
XOR_SWAP,
span,
&format!(
"this xor-based swap of the elements of `{}` can be \
more clearly expressed using `.swap`",
slice
),
"try",
format!(
"{}.swap({}, {})",
slice.maybe_par(),
snippet_with_applicability(cx, idx0.span, "..", &mut applicability),
snippet_with_applicability(cx, idx1.span, "..", &mut applicability)
),
applicability
)
}
}
Slice::None => {
if let (Some(first), Some(second)) = (Sugg::hir_opt(cx, lhs0), Sugg::hir_opt(cx, rhs0)) {
span_lint_and_sugg(
cx,
XOR_SWAP,
span,
&format!(
"this xor-based swap of `{}` and `{}` can be \
more clearly expressed using `std::mem::swap`",
first, second
),
"try",
format!("std::mem::swap({}, {})", first.mut_addr(), second.mut_addr()),
applicability,
);
}
}
Slice::NotSwappable => {}
}
generate_swap_warning(cx, lhs0, rhs0, span, true);
}
};
}

48
clippy_utils/src/lib.rs
View File

@ -558,6 +558,54 @@ pub fn trait_ref_of_method<'tcx>(cx: &LateContext<'tcx>, hir_id: HirId) -> Optio
None
}
/// This method will return tuple of projection stack and root of the expression,
/// used in `can_mut_borrow_both`.
///
/// For example, if `e` represents the `v[0].a.b[x]`
/// this method will return a tuple, composed of a `Vec`
/// containing the `Expr`s for `v[0], v[0].a, v[0].a.b, v[0].a.b[x]`
/// and a `Expr` for root of them, `v`
fn projection_stack<'a, 'hir>(mut e: &'a Expr<'hir>) -> (Vec<&'a Expr<'hir>>, &'a Expr<'hir>) {
let mut result = vec![];
let root = loop {
match e.kind {
ExprKind::Index(ep, _) | ExprKind::Field(ep, _) => {
result.push(e);
e = ep;
},
_ => break e,
};
};
result.reverse();
(result, root)
}
/// Checks if two expressions can be mutably borrowed simultaneously
/// and they aren't dependent on borrowing same thing twice
pub fn can_mut_borrow_both(cx: &LateContext<'_>, e1: &Expr<'_>, e2: &Expr<'_>) -> bool {
let (s1, r1) = projection_stack(e1);
let (s2, r2) = projection_stack(e2);
if !eq_expr_value(cx, r1, r2) {
return true;
}
for (x1, x2) in s1.iter().zip(s2.iter()) {
match (&x1.kind, &x2.kind) {
(ExprKind::Field(_, i1), ExprKind::Field(_, i2)) => {
if i1 != i2 {
return true;
}
},
(ExprKind::Index(_, i1), ExprKind::Index(_, i2)) => {
if !eq_expr_value(cx, i1, i2) {
return false;
}
},
_ => return false,
}
}
false
}
/// Checks if the top level expression can be moved into a closure as is.
pub fn can_move_expr_to_closure_no_visit(cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>, jump_targets: &[HirId]) -> bool {
match expr.kind {

25
tests/ui/swap.fixed
View File

@ -6,6 +6,7 @@
clippy::no_effect,
clippy::redundant_clone,
redundant_semicolons,
dead_code,
unused_assignments
)]
@ -20,9 +21,7 @@ struct Bar {
fn field() {
let mut bar = Bar { a: 1, b: 2 };
let temp = bar.a;
bar.a = bar.b;
bar.b = temp;
std::mem::swap(&mut bar.a, &mut bar.b);
let mut baz = vec![bar.clone(), bar.clone()];
let temp = baz[0].a;
@ -51,6 +50,7 @@ fn unswappable_slice() {
foo[1][0] = temp;
// swap(foo[0][1], foo[1][0]) would fail
// this could use split_at_mut and mem::swap, but that is not much simpler.
}
fn vec() {
@ -95,20 +95,19 @@ fn xor_unswappable_slice() {
foo[0][1] ^= foo[1][0];
foo[1][0] ^= foo[0][0];
foo[0][1] ^= foo[1][0];
// swap(foo[0][1], foo[1][0]) would fail
// this could use split_at_mut and mem::swap, but that is not much simpler.
}
fn distinct_slice() {
let foo = &mut [vec![1, 2], vec![3, 4]];
let bar = &mut [vec![1, 2], vec![3, 4]];
std::mem::swap(&mut foo[0][1], &mut bar[1][0]);
}
#[rustfmt::skip]
fn main() {
field();
array();
slice();
unswappable_slice();
vec();
xor_swap_locals();
xor_field_swap();
xor_slice_swap();
xor_no_swap();
xor_unswappable_slice();
let mut a = 42;
let mut b = 1337;

23
tests/ui/swap.rs
View File

@ -6,6 +6,7 @@
clippy::no_effect,
clippy::redundant_clone,
redundant_semicolons,
dead_code,
unused_assignments
)]
@ -55,6 +56,7 @@ fn unswappable_slice() {
foo[1][0] = temp;
// swap(foo[0][1], foo[1][0]) would fail
// this could use split_at_mut and mem::swap, but that is not much simpler.
}
fn vec() {
@ -107,20 +109,21 @@ fn xor_unswappable_slice() {
foo[0][1] ^= foo[1][0];
foo[1][0] ^= foo[0][0];
foo[0][1] ^= foo[1][0];
// swap(foo[0][1], foo[1][0]) would fail
// this could use split_at_mut and mem::swap, but that is not much simpler.
}
fn distinct_slice() {
let foo = &mut [vec![1, 2], vec![3, 4]];
let bar = &mut [vec![1, 2], vec![3, 4]];
let temp = foo[0][1];
foo[0][1] = bar[1][0];
bar[1][0] = temp;
}
#[rustfmt::skip]
fn main() {
field();
array();
slice();
unswappable_slice();
vec();
xor_swap_locals();
xor_field_swap();
xor_slice_swap();
xor_no_swap();
xor_unswappable_slice();
let mut a = 42;
let mut b = 1337;

59
tests/ui/swap.stderr
View File

@ -1,15 +1,16 @@
error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:35:5
error: this looks like you are swapping `bar.a` and `bar.b` manually
--> $DIR/swap.rs:24:5
|
LL | / let temp = foo[0];
LL | | foo[0] = foo[1];
LL | | foo[1] = temp;
| |_________________^ help: try: `foo.swap(0, 1)`
LL | / let temp = bar.a;
LL | | bar.a = bar.b;
LL | | bar.b = temp;
| |________________^ help: try: `std::mem::swap(&mut bar.a, &mut bar.b)`
|
= note: `-D clippy::manual-swap` implied by `-D warnings`
= note: or maybe you should use `std::mem::replace`?
error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:44:5
--> $DIR/swap.rs:36:5
|
LL | / let temp = foo[0];
LL | | foo[0] = foo[1];
@ -17,41 +18,57 @@ LL | | foo[1] = temp;
| |_________________^ help: try: `foo.swap(0, 1)`
error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:62:5
--> $DIR/swap.rs:45:5
|
LL | / let temp = foo[0];
LL | | foo[0] = foo[1];
LL | | foo[1] = temp;
| |_________________^ help: try: `foo.swap(0, 1)`
error: this xor-based swap of `a` and `b` can be more clearly expressed using `std::mem::swap`
--> $DIR/swap.rs:73:5
error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:64: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:75:5
|
LL | / a ^= b;
LL | | b ^= a;
LL | | a ^= b;
| |___________^ help: try: `std::mem::swap(&mut a, &mut b)`
|
= note: `-D clippy::xor-swap` implied by `-D warnings`
error: this xor-based swap of `bar.a` and `bar.b` can be more clearly expressed using `std::mem::swap`
--> $DIR/swap.rs:81:5
error: this looks like you are swapping `bar.a` and `bar.b` manually
--> $DIR/swap.rs:83:5
|
LL | / bar.a ^= bar.b;
LL | | bar.b ^= bar.a;
LL | | bar.a ^= bar.b;
| |___________________^ help: try: `std::mem::swap(&mut bar.a, &mut bar.b)`
error: this xor-based swap of the elements of `foo` can be more clearly expressed using `.swap`
--> $DIR/swap.rs:89:5
error: this looks like you are swapping elements of `foo` manually
--> $DIR/swap.rs:91:5
|
LL | / foo[0] ^= foo[1];
LL | | foo[1] ^= foo[0];
LL | | foo[0] ^= foo[1];
| |_____________________^ help: try: `foo.swap(0, 1)`
error: this looks like you are swapping `foo[0][1]` and `bar[1][0]` manually
--> $DIR/swap.rs:120:5
|
LL | / let temp = foo[0][1];
LL | | foo[0][1] = bar[1][0];
LL | | bar[1][0] = temp;
| |____________________^ help: try: `std::mem::swap(&mut foo[0][1], &mut bar[1][0])`
|
= note: or maybe you should use `std::mem::replace`?
error: this looks like you are swapping `a` and `b` manually
--> $DIR/swap.rs:131:7
--> $DIR/swap.rs:134:7
|
LL | ; let t = a;
| _______^
@ -62,7 +79,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:140:7
--> $DIR/swap.rs:143:7
|
LL | ; let t = c.0;
| _______^
@ -73,7 +90,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:128:5
--> $DIR/swap.rs:131:5
|
LL | / a = b;
LL | | b = a;
@ -83,7 +100,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:137:5
--> $DIR/swap.rs:140:5
|
LL | / c.0 = a;
LL | | a = c.0;
@ -91,5 +108,5 @@ LL | | a = c.0;
|
= note: or maybe you should use `std::mem::replace`?
error: aborting due to 10 previous errors
error: aborting due to 12 previous errors