Auto merge of rust-lang#8916 - Jarcho:swap_ptr_to_ref, r=Manishearth

New lint `swap_ptr_to_ref`

fixes: rust-lang#7381

changelog: New lint `swap_ptr_to_ref`

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

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),

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,

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),
 ])

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`
 }
 

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<Span>) {
+    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)
+    }
+}

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

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

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

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
+

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

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
+