Initial impl of raw_assign_to_drop

Author: lukaslueg

CHANGELOG.md

@@ -6533,6 +6533,7 @@ Released 2018-09-13
 [`range_plus_one`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_plus_one
 [`range_step_by_zero`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_step_by_zero
 [`range_zip_with_len`]: https://rust-lang.github.io/rust-clippy/master/index.html#range_zip_with_len
+[`raw_assign_to_drop`]: https://rust-lang.github.io/rust-clippy/master/index.html#raw_assign_to_drop
 [`rc_buffer`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_buffer
 [`rc_clone_in_vec_init`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_clone_in_vec_init
 [`rc_mutex`]: https://rust-lang.github.io/rust-clippy/master/index.html#rc_mutex

clippy_lints/src/declared_lints.rs

@@ -599,6 +599,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::operators::MODULO_ONE_INFO,
     crate::operators::NEEDLESS_BITWISE_BOOL_INFO,
     crate::operators::OP_REF_INFO,
+    crate::operators::RAW_ASSIGN_TO_DROP_INFO,
     crate::operators::REDUNDANT_COMPARISONS_INFO,
     crate::operators::SELF_ASSIGNMENT_INFO,
     crate::operators::VERBOSE_BIT_MASK_INFO,

clippy_lints/src/operators/mod.rs

@@ -19,6 +19,7 @@ mod modulo_one;
 mod needless_bitwise_bool;
 mod numeric_arithmetic;
 mod op_ref;
+mod raw_assign_to_drop;
 mod self_assignment;
 mod verbose_bit_mask;
 
@@ -807,6 +808,67 @@ declare_clippy_lint! {
     "explicit self-assignment"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    ///
+    /// Checks for assignments via raw pointers that involve types with destructors.
+    ///
+    /// ### Why restrict this?
+    ///
+    /// Assignments of the form `*ptr = new_value;` assume that `*ptr` contains an initialized
+    /// value, and unconditionally execute the `std::ops::Drop`-implementation if such
+    /// implementation is defined on the type of `*ptr`. If the old value is in fact
+    /// uninitialized or otherwise invalid, the execution of `std::ops::Drop::drop(&mut self)`
+    /// is always Undefined Behavior.
+    ///
+    /// The unsafe-block required to assign via the raw-pointer should include a SAFETY-comment
+    /// that explains why it is always safe to execute the destructor.
+    ///
+    /// If the code can not guarantee that the old value can be safely dropped, use
+    /// `std::ptr::write()` to overwrite the (possibly nonexisting) value without executing
+    /// the destructor; this may leak resources if the old value did in fact exist.
+    ///
+    /// Use `std::ptr::drop_in_place()` to (selectively) execute the destructor, having
+    /// established that it is safe to do so.
+    ///
+    /// ### Example
+    /// ```no_run
+    /// unsafe fn foo(oldvalue: *mut String) {
+    ///     // Direct assignment always executes `String`'s destructor on `oldvalue`.
+    ///     // However, we can't guarantee that executing the destructor is safe.
+    ///     unsafe { *oldvalue = "New Value".to_owned(); }
+    /// }
+    /// ```
+    /// Use instead:
+    /// ```no_run
+    /// #[allow(clippy::raw_assign_to_drop)]
+    /// unsafe fn foo(oldvalue: *mut String, oldvalue_is_initialized: bool) {
+    ///     let newvalue = "New Value".to_owned();
+    ///
+    ///     // In this example, `oldvalue_is_initialized` is an oracle for what
+    ///     // the programmer might be able to establish statically.
+    ///     if oldvalue_is_initialized {
+    ///         // Having established that `oldvalue` points to a valid value, it is safe to
+    ///         // execute the destructor and assign a new value.
+    ///
+    ///         // SAFETY: oldvalue is properly aligned, valid for writes, and initialized
+    ///         unsafe {
+    ///             *oldvalue = newvalue;
+    ///         }
+    ///     } else {
+    ///         // Overwrite the old value without running the destructor.
+    ///
+    ///         // SAFETY: oldvalue is properly aligned and valid for writes
+    ///         unsafe { oldvalue.write(newvalue); }
+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.91.0"]
+    pub RAW_ASSIGN_TO_DROP,
+    restriction,
+    "assignment via raw pointer that involves destructors"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for manual implementation of `midpoint`.
@@ -904,6 +966,7 @@ impl_lint_pass!(Operators => [
     MODULO_ARITHMETIC,
     NEEDLESS_BITWISE_BOOL,
     SELF_ASSIGNMENT,
+    RAW_ASSIGN_TO_DROP,
     MANUAL_MIDPOINT,
     MANUAL_IS_MULTIPLE_OF,
 ]);
@@ -954,6 +1017,7 @@ impl<'tcx> LateLintPass<'tcx> for Operators {
             ExprKind::Assign(lhs, rhs, _) => {
                 assign_op_pattern::check(cx, e, lhs, rhs, self.msrv);
                 self_assignment::check(cx, e, lhs, rhs);
+                raw_assign_to_drop::check(cx, lhs);
             },
             ExprKind::Unary(op, arg) => {
                 if op == UnOp::Neg {

clippy_lints/src/operators/raw_assign_to_drop.rs

@@ -0,0 +1,43 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use rustc_hir::{Expr, ExprKind, UnOp};
+use rustc_lint::LateContext;
+
+use super::RAW_ASSIGN_TO_DROP;
+
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, lhs: &'tcx Expr<'_>) {
+    if let ExprKind::Unary(UnOp::Deref, expr) = lhs.kind
+        && let ty = cx.typeck_results().expr_ty(expr)
+        && ty.is_raw_ptr()
+        && let Some(deref_ty) = ty.builtin_deref(true)
+        && deref_ty.needs_drop(cx.tcx, cx.typing_env())
+    {
+        if let ExprKind::MethodCall(path, self_arg, [], ..) = expr.kind
+            && let rustc_middle::ty::Adt(ty_def, ..) = cx.typeck_results().expr_ty(self_arg).kind()
+            && ty_def.is_unsafe_cell()
+            && path.ident.as_str() == "get"
+        {
+            // Don't lint if the raw pointer was directly retrieved from UnsafeCell::get()
+            // We assume those to be safely managed
+            return;
+        }
+        span_lint_and_then(
+            cx,
+            RAW_ASSIGN_TO_DROP,
+            expr.span,
+            "assignment via raw pointer always executes destructor",
+            |diag| {
+                diag.note(format!(
+                    "the destructor defined by `{deref_ty}` is executed during assignment of the new value"
+                ));
+                diag.span_label(
+                    expr.span,
+                    "the old value may be uninitialized, causing Undefined Behavior when the destructor executes",
+                );
+                diag.help("use `std::ptr::write()` to overwrite a possibly uninitialized place");
+                diag.help(
+                    "use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists",
+                );
+            },
+        );
+    }
+}

tests/ui/raw_assign_to_drop.rs

@@ -0,0 +1,42 @@
+#![warn(clippy::raw_assign_to_drop)]
+#![allow(clippy::missing_safety_doc)]
+
+use std::cell::UnsafeCell;
+
+pub unsafe fn foo(r: *mut String, i: *mut i32) {
+    unsafe {
+        *r = "foo".to_owned();
+        //~^ raw_assign_to_drop
+
+        // no lint on {integer}
+        *i = 47;
+
+        (*r, *r) = ("foo".to_owned(), "bar".to_owned());
+        //~^ raw_assign_to_drop
+        //~^^ raw_assign_to_drop
+
+        (*r, *i) = ("foo".to_owned(), 47);
+        //~^ raw_assign_to_drop
+
+        let mut x: String = Default::default();
+        *(&mut x as *mut _) = "Foo".to_owned();
+        //~^ raw_assign_to_drop
+
+        // no lint on `u8`
+        *x.as_mut_ptr() = b'a';
+
+        let mut v: Vec<String> = vec![];
+        *v.as_mut_ptr() = Default::default();
+        //~^ raw_assign_to_drop
+    }
+}
+
+pub unsafe fn unsafecell() {
+    // No lint
+    let c = UnsafeCell::new(String::new());
+    unsafe {
+        *c.get() = String::new();
+    }
+}
+
+fn main() {}

tests/ui/raw_assign_to_drop.stderr

@@ -0,0 +1,64 @@
+error: assignment via raw pointer always executes destructor
+  --> tests/ui/raw_assign_to_drop.rs:8:10
+   |
+LL |         *r = "foo".to_owned();
+   |          ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes
+   |
+   = note: the destructor defined by `std::string::String` is executed during assignment of the new value
+   = help: use `std::ptr::write()` to overwrite a possibly uninitialized place
+   = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists
+   = note: `-D clippy::raw-assign-to-drop` implied by `-D warnings`
+   = help: to override `-D warnings` add `#[allow(clippy::raw_assign_to_drop)]`
+
+error: assignment via raw pointer always executes destructor
+  --> tests/ui/raw_assign_to_drop.rs:14:11
+   |
+LL |         (*r, *r) = ("foo".to_owned(), "bar".to_owned());
+   |           ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes
+   |
+   = note: the destructor defined by `std::string::String` is executed during assignment of the new value
+   = help: use `std::ptr::write()` to overwrite a possibly uninitialized place
+   = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists
+
+error: assignment via raw pointer always executes destructor
+  --> tests/ui/raw_assign_to_drop.rs:14:15
+   |
+LL |         (*r, *r) = ("foo".to_owned(), "bar".to_owned());
+   |               ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes
+   |
+   = note: the destructor defined by `std::string::String` is executed during assignment of the new value
+   = help: use `std::ptr::write()` to overwrite a possibly uninitialized place
+   = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists
+
+error: assignment via raw pointer always executes destructor
+  --> tests/ui/raw_assign_to_drop.rs:18:11
+   |
+LL |         (*r, *i) = ("foo".to_owned(), 47);
+   |           ^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes
+   |
+   = note: the destructor defined by `std::string::String` is executed during assignment of the new value
+   = help: use `std::ptr::write()` to overwrite a possibly uninitialized place
+   = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists
+
+error: assignment via raw pointer always executes destructor
+  --> tests/ui/raw_assign_to_drop.rs:22:10
+   |
+LL |         *(&mut x as *mut _) = "Foo".to_owned();
+   |          ^^^^^^^^^^^^^^^^^^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes
+   |
+   = note: the destructor defined by `std::string::String` is executed during assignment of the new value
+   = help: use `std::ptr::write()` to overwrite a possibly uninitialized place
+   = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists
+
+error: assignment via raw pointer always executes destructor
+  --> tests/ui/raw_assign_to_drop.rs:29:10
+   |
+LL |         *v.as_mut_ptr() = Default::default();
+   |          ^^^^^^^^^^^^^^ the old value may be uninitialized, causing Undefined Behavior when the destructor executes
+   |
+   = note: the destructor defined by `std::string::String` is executed during assignment of the new value
+   = help: use `std::ptr::write()` to overwrite a possibly uninitialized place
+   = help: use `std::ptr::drop_in_place()` to drop the previous value, having established such value exists
+
+error: aborting due to 6 previous errors
+