From ca78e2428e25524831231263c579e390f7dbd1e3 Mon Sep 17 00:00:00 2001 From: Jason Newcomb Date: Mon, 30 May 2022 12:36:05 -0400 Subject: [PATCH] Add lint `swap_ptr_to_ref` --- CHANGELOG.md | 1 + clippy_lints/src/lib.register_all.rs | 1 + clippy_lints/src/lib.register_lints.rs | 1 + clippy_lints/src/lib.register_suspicious.rs | 1 + clippy_lints/src/lib.rs | 2 + clippy_lints/src/swap_ptr_to_ref.rs | 80 +++++++++++++++++++++ clippy_utils/src/paths.rs | 1 + tests/ui/swap_ptr_to_ref.fixed | 24 +++++++ tests/ui/swap_ptr_to_ref.rs | 24 +++++++ tests/ui/swap_ptr_to_ref.stderr | 28 ++++++++ tests/ui/swap_ptr_to_ref_unfixable.rs | 18 +++++ tests/ui/swap_ptr_to_ref_unfixable.stderr | 22 ++++++ 12 files changed, 203 insertions(+) create mode 100644 clippy_lints/src/swap_ptr_to_ref.rs create mode 100644 tests/ui/swap_ptr_to_ref.fixed create mode 100644 tests/ui/swap_ptr_to_ref.rs create mode 100644 tests/ui/swap_ptr_to_ref.stderr create mode 100644 tests/ui/swap_ptr_to_ref_unfixable.rs create mode 100644 tests/ui/swap_ptr_to_ref_unfixable.stderr diff --git a/CHANGELOG.md b/CHANGELOG.md index 67ca4cf708aeb..731d5cac3330b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3753,6 +3753,7 @@ Released 2018-09-13 [`suspicious_operation_groupings`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_operation_groupings [`suspicious_splitn`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_splitn [`suspicious_unary_op_formatting`]: https://rust-lang.github.io/rust-clippy/master/index.html#suspicious_unary_op_formatting +[`swap_ptr_to_ref`]: https://rust-lang.github.io/rust-clippy/master/index.html#swap_ptr_to_ref [`tabs_in_doc_comments`]: https://rust-lang.github.io/rust-clippy/master/index.html#tabs_in_doc_comments [`temporary_assignment`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_assignment [`temporary_cstring_as_ptr`]: https://rust-lang.github.io/rust-clippy/master/index.html#temporary_cstring_as_ptr diff --git a/clippy_lints/src/lib.register_all.rs b/clippy_lints/src/lib.register_all.rs index 8567b6b8a7e6a..58f8ea7b997f9 100644 --- a/clippy_lints/src/lib.register_all.rs +++ b/clippy_lints/src/lib.register_all.rs @@ -292,6 +292,7 @@ store.register_group(true, "clippy::all", Some("clippy_all"), vec![ LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), LintId::of(swap::ALMOST_SWAPPED), LintId::of(swap::MANUAL_SWAP), + LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF), 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), diff --git a/clippy_lints/src/lib.register_lints.rs b/clippy_lints/src/lib.register_lints.rs index b76eaae377e32..e04d9f4fb9027 100644 --- a/clippy_lints/src/lib.register_lints.rs +++ b/clippy_lints/src/lib.register_lints.rs @@ -497,6 +497,7 @@ store.register_lints(&[ suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL, swap::ALMOST_SWAPPED, swap::MANUAL_SWAP, + swap_ptr_to_ref::SWAP_PTR_TO_REF, tabs_in_doc_comments::TABS_IN_DOC_COMMENTS, temporary_assignment::TEMPORARY_ASSIGNMENT, to_digit_is_some::TO_DIGIT_IS_SOME, diff --git a/clippy_lints/src/lib.register_suspicious.rs b/clippy_lints/src/lib.register_suspicious.rs index 3d6d2d8951c23..43ced5070f17e 100644 --- a/clippy_lints/src/lib.register_suspicious.rs +++ b/clippy_lints/src/lib.register_suspicious.rs @@ -32,4 +32,5 @@ store.register_group(true, "clippy::suspicious", Some("clippy_suspicious"), vec! LintId::of(significant_drop_in_scrutinee::SIGNIFICANT_DROP_IN_SCRUTINEE), LintId::of(suspicious_trait_impl::SUSPICIOUS_ARITHMETIC_IMPL), LintId::of(suspicious_trait_impl::SUSPICIOUS_OP_ASSIGN_IMPL), + LintId::of(swap_ptr_to_ref::SWAP_PTR_TO_REF), ]) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 67ae2053f5ae0..81a231cbebece 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -380,6 +380,7 @@ mod strlen_on_c_strings; mod suspicious_operation_groupings; mod suspicious_trait_impl; mod swap; +mod swap_ptr_to_ref; mod tabs_in_doc_comments; mod temporary_assignment; mod to_digit_is_some; @@ -913,6 +914,7 @@ pub fn register_plugins(store: &mut rustc_lint::LintStore, sess: &Session, conf: store.register_late_pass(|| Box::new(get_first::GetFirst)); store.register_early_pass(|| Box::new(unused_rounding::UnusedRounding)); store.register_early_pass(move || Box::new(almost_complete_letter_range::AlmostCompleteLetterRange::new(msrv))); + store.register_late_pass(|| Box::new(swap_ptr_to_ref::SwapPtrToRef)); // add lints here, do not remove this comment, it's used in `new_lint` } diff --git a/clippy_lints/src/swap_ptr_to_ref.rs b/clippy_lints/src/swap_ptr_to_ref.rs new file mode 100644 index 0000000000000..75d3b040c968f --- /dev/null +++ b/clippy_lints/src/swap_ptr_to_ref.rs @@ -0,0 +1,80 @@ +use clippy_utils::diagnostics::span_lint_and_then; +use clippy_utils::source::snippet_with_context; +use clippy_utils::{match_def_path, path_def_id, paths}; +use rustc_errors::Applicability; +use rustc_hir::{BorrowKind, Expr, ExprKind, Mutability, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; +use rustc_session::{declare_lint_pass, declare_tool_lint}; +use rustc_span::{Span, SyntaxContext}; + +declare_clippy_lint! { + /// ### What it does + /// Checks for calls to `core::mem::swap` where either parameter is derived from a pointer + /// + /// ### Why is this bad? + /// When at least one parameter to `swap` is derived from a pointer it may overlap with the + /// other. This would then lead to undefined behavior. + /// + /// ### Example + /// ```rust + /// unsafe fn swap(x: &[*mut u32], y: &[*mut u32]) { + /// for (&x, &y) in x.iter().zip(y) { + /// core::mem::swap(&mut *x, &mut *y); + /// } + /// } + /// ``` + /// Use instead: + /// ```rust + /// unsafe fn swap(x: &[*mut u32], y: &[*mut u32]) { + /// for (&x, &y) in x.iter().zip(y) { + /// core::ptr::swap(x, y); + /// } + /// } + /// ``` + #[clippy::version = "1.63.0"] + pub SWAP_PTR_TO_REF, + suspicious, + "call to `mem::swap` using pointer derived references" +} +declare_lint_pass!(SwapPtrToRef => [SWAP_PTR_TO_REF]); + +impl LateLintPass<'_> for SwapPtrToRef { + fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { + if let ExprKind::Call(fn_expr, [arg1, arg2]) = e.kind + && let Some(fn_id) = path_def_id(cx, fn_expr) + && match_def_path(cx, fn_id, &paths::MEM_SWAP) + && let ctxt = e.span.ctxt() + && let (from_ptr1, arg1_span) = is_ptr_to_ref(cx, arg1, ctxt) + && let (from_ptr2, arg2_span) = is_ptr_to_ref(cx, arg2, ctxt) + && (from_ptr1 || from_ptr2) + { + span_lint_and_then( + cx, + SWAP_PTR_TO_REF, + e.span, + "call to `core::mem::swap` with a parameter derived from a raw pointer", + |diag| { + if !((from_ptr1 && arg1_span.is_none()) || (from_ptr2 && arg2_span.is_none())) { + let mut app = Applicability::MachineApplicable; + let snip1 = snippet_with_context(cx, arg1_span.unwrap_or(arg1.span), ctxt, "..", &mut app).0; + let snip2 = snippet_with_context(cx, arg2_span.unwrap_or(arg2.span), ctxt, "..", &mut app).0; + diag.span_suggestion(e.span, "use ptr::swap", format!("core::ptr::swap({}, {})", snip1, snip2), app); + } + } + ); + } + } +} + +/// Checks if the expression converts a mutable pointer to a mutable reference. If it is, also +/// returns the span of the pointer expression if it's suitable for making a suggestion. +fn is_ptr_to_ref(cx: &LateContext<'_>, e: &Expr<'_>, ctxt: SyntaxContext) -> (bool, Option) { + if let ExprKind::AddrOf(BorrowKind::Ref, Mutability::Mut, borrowed_expr) = e.kind + && let ExprKind::Unary(UnOp::Deref, derefed_expr) = borrowed_expr.kind + && cx.typeck_results().expr_ty(derefed_expr).is_unsafe_ptr() + { + (true, (borrowed_expr.span.ctxt() == ctxt || derefed_expr.span.ctxt() == ctxt).then(|| derefed_expr.span)) + } else { + (false, None) + } +} diff --git a/clippy_utils/src/paths.rs b/clippy_utils/src/paths.rs index 0064694ff929f..89789c3d85135 100644 --- a/clippy_utils/src/paths.rs +++ b/clippy_utils/src/paths.rs @@ -73,6 +73,7 @@ pub const LATE_CONTEXT: [&str; 2] = ["rustc_lint", "LateContext"]; pub const LATE_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "LateLintPass"]; #[cfg(feature = "internal")] pub const LINT: [&str; 2] = ["rustc_lint_defs", "Lint"]; +pub const MEM_SWAP: [&str; 3] = ["core", "mem", "swap"]; pub const MUTEX_GUARD: [&str; 4] = ["std", "sync", "mutex", "MutexGuard"]; pub const OPEN_OPTIONS: [&str; 3] = ["std", "fs", "OpenOptions"]; /// Preferably use the diagnostic item `sym::Option` where possible diff --git a/tests/ui/swap_ptr_to_ref.fixed b/tests/ui/swap_ptr_to_ref.fixed new file mode 100644 index 0000000000000..596b6ee919bb4 --- /dev/null +++ b/tests/ui/swap_ptr_to_ref.fixed @@ -0,0 +1,24 @@ +// run-rustfix + +#![warn(clippy::swap_ptr_to_ref)] + +use core::ptr::addr_of_mut; + +fn main() { + let mut x = 0u32; + let y: *mut _ = &mut x; + let z: *mut _ = &mut x; + + unsafe { + core::ptr::swap(y, z); + core::ptr::swap(y, &mut x); + core::ptr::swap(&mut x, y); + core::ptr::swap(addr_of_mut!(x), addr_of_mut!(x)); + } + + let y = &mut x; + let mut z = 0u32; + let z = &mut z; + + core::mem::swap(y, z); +} diff --git a/tests/ui/swap_ptr_to_ref.rs b/tests/ui/swap_ptr_to_ref.rs new file mode 100644 index 0000000000000..282f571211d95 --- /dev/null +++ b/tests/ui/swap_ptr_to_ref.rs @@ -0,0 +1,24 @@ +// run-rustfix + +#![warn(clippy::swap_ptr_to_ref)] + +use core::ptr::addr_of_mut; + +fn main() { + let mut x = 0u32; + let y: *mut _ = &mut x; + let z: *mut _ = &mut x; + + unsafe { + core::mem::swap(&mut *y, &mut *z); + core::mem::swap(&mut *y, &mut x); + core::mem::swap(&mut x, &mut *y); + core::mem::swap(&mut *addr_of_mut!(x), &mut *addr_of_mut!(x)); + } + + let y = &mut x; + let mut z = 0u32; + let z = &mut z; + + core::mem::swap(y, z); +} diff --git a/tests/ui/swap_ptr_to_ref.stderr b/tests/ui/swap_ptr_to_ref.stderr new file mode 100644 index 0000000000000..401ce070869a2 --- /dev/null +++ b/tests/ui/swap_ptr_to_ref.stderr @@ -0,0 +1,28 @@ +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref.rs:13:9 + | +LL | core::mem::swap(&mut *y, &mut *z); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use ptr::swap: `core::ptr::swap(y, z)` + | + = note: `-D clippy::swap-ptr-to-ref` implied by `-D warnings` + +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref.rs:14:9 + | +LL | core::mem::swap(&mut *y, &mut x); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use ptr::swap: `core::ptr::swap(y, &mut x)` + +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref.rs:15:9 + | +LL | core::mem::swap(&mut x, &mut *y); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use ptr::swap: `core::ptr::swap(&mut x, y)` + +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref.rs:16:9 + | +LL | core::mem::swap(&mut *addr_of_mut!(x), &mut *addr_of_mut!(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: use ptr::swap: `core::ptr::swap(addr_of_mut!(x), addr_of_mut!(x))` + +error: aborting due to 4 previous errors + diff --git a/tests/ui/swap_ptr_to_ref_unfixable.rs b/tests/ui/swap_ptr_to_ref_unfixable.rs new file mode 100644 index 0000000000000..66ea7c6529bd2 --- /dev/null +++ b/tests/ui/swap_ptr_to_ref_unfixable.rs @@ -0,0 +1,18 @@ +#![warn(clippy::swap_ptr_to_ref)] + +macro_rules! addr_of_mut_to_ref { + ($e:expr) => { + &mut *core::ptr::addr_of_mut!($e) + }; +} + +fn main() { + let mut x = 0u32; + let y: *mut _ = &mut x; + + unsafe { + core::mem::swap(addr_of_mut_to_ref!(x), &mut *y); + core::mem::swap(&mut *y, addr_of_mut_to_ref!(x)); + core::mem::swap(addr_of_mut_to_ref!(x), addr_of_mut_to_ref!(x)); + } +} diff --git a/tests/ui/swap_ptr_to_ref_unfixable.stderr b/tests/ui/swap_ptr_to_ref_unfixable.stderr new file mode 100644 index 0000000000000..c261205d556e4 --- /dev/null +++ b/tests/ui/swap_ptr_to_ref_unfixable.stderr @@ -0,0 +1,22 @@ +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref_unfixable.rs:14:9 + | +LL | core::mem::swap(addr_of_mut_to_ref!(x), &mut *y); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | + = note: `-D clippy::swap-ptr-to-ref` implied by `-D warnings` + +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref_unfixable.rs:15:9 + | +LL | core::mem::swap(&mut *y, addr_of_mut_to_ref!(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: call to `core::mem::swap` with a parameter derived from a raw pointer + --> $DIR/swap_ptr_to_ref_unfixable.rs:16:9 + | +LL | core::mem::swap(addr_of_mut_to_ref!(x), addr_of_mut_to_ref!(x)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + +error: aborting due to 3 previous errors +