Implement a lint for implicit autoref of raw pointer dereference

compiler/rustc_arena/src/lib.rs

@@ -19,6 +19,7 @@
 #![feature(pointer_byte_offsets)]
 #![feature(rustc_attrs)]
 #![cfg_attr(test, feature(test))]
+#![feature(slice_ptr_len)]
 #![feature(strict_provenance)]
 #![deny(rustc::untranslatable_diagnostic)]
 #![deny(rustc::diagnostic_outside_of_impl)]
@@ -103,7 +104,7 @@ impl<T> ArenaChunk<T> {
                 // A pointer as large as possible for zero-sized elements.
                 ptr::invalid_mut(!0)
             } else {
-                self.start().add((*self.storage.as_ptr()).len())
+                self.start().add(self.storage.as_ptr().len())
             }
         }
     }
@@ -287,7 +288,7 @@ impl<T> TypedArena<T> {
                 // If the previous chunk's len is less than HUGE_PAGE
                 // bytes, then this chunk will be least double the previous
                 // chunk's size.
-                new_cap = (*last_chunk.storage.as_ptr()).len().min(HUGE_PAGE / elem_size / 2);
+                new_cap = last_chunk.storage.as_ptr().len().min(HUGE_PAGE / elem_size / 2);
                 new_cap *= 2;
             } else {
                 new_cap = PAGE / elem_size;
@@ -395,7 +396,7 @@ impl DroplessArena {
                 // If the previous chunk's len is less than HUGE_PAGE
                 // bytes, then this chunk will be least double the previous
                 // chunk's size.
-                new_cap = (*last_chunk.storage.as_ptr()).len().min(HUGE_PAGE / 2);
+                new_cap = last_chunk.storage.as_ptr().len().min(HUGE_PAGE / 2);
                 new_cap *= 2;
             } else {
                 new_cap = PAGE;

compiler/rustc_data_structures/src/owning_ref/mod.rs

@@ -1105,14 +1105,14 @@ use std::sync::{MutexGuard, RwLockReadGuard, RwLockWriteGuard};
 impl<T: 'static> ToHandle for RefCell<T> {
     type Handle = Ref<'static, T>;
     unsafe fn to_handle(x: *const Self) -> Self::Handle {
-        (*x).borrow()
+        (&*x).borrow()
     }
 }
 
 impl<T: 'static> ToHandleMut for RefCell<T> {
     type HandleMut = RefMut<'static, T>;
     unsafe fn to_handle_mut(x: *const Self) -> Self::HandleMut {
-        (*x).borrow_mut()
+        (&*x).borrow_mut()
     }
 }
 

compiler/rustc_lint/src/implicit_unsafe_autorefs.rs

@@ -0,0 +1,159 @@
+use crate::{LateContext, LateLintPass, LintContext};
+
+use rustc_errors::Applicability;
+use rustc_hir::{self as hir, Expr, ExprKind, Mutability, UnOp};
+use rustc_middle::ty::{
+    adjustment::{Adjust, Adjustment, AutoBorrow, OverloadedDeref},
+    TyCtxt, TypeckResults,
+};
+
+declare_lint! {
+    /// The `implicit_unsafe_autorefs` lint checks for implicitly taken references to dereferences of raw pointers.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// use std::ptr::addr_of_mut;
+    ///
+    /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] {
+    ///     addr_of_mut!((*ptr)[..16])
+    ///     //                 ^^^^^^ this calls `IndexMut::index_mut(&mut ..., ..16)`,
+    ///     //                        implicitly creating a reference
+    /// }
+    /// ```
+    ///
+    /// {{produces}}
+    ///
+    /// ### Explanation
+    ///
+    /// When working with raw pointers it's usually undesirable to create references,
+    /// since they inflict a lot of safety requirement. Unfortunately, it's possible
+    /// to take a reference to a dereference of a raw pointer implicitly, which inflicts
+    /// the usual reference requirements without you even knowing that.
+    ///
+    /// If you are sure, you can soundly take a reference, then you can take it explicitly:
+    /// ```rust
+    /// # use std::ptr::addr_of_mut;
+    /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] {
+    ///     addr_of_mut!((&mut *ptr)[..16])
+    /// }
+    /// ```
+    ///
+    /// Otherwise try to find an alternative way to achive your goals that work only with
+    /// raw pointers:
+    /// ```rust
+    /// #![feature(slice_ptr_get)]
+    ///
+    /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] {
+    ///     ptr.get_unchecked_mut(..16)
+    /// }
+    /// ```
+    pub IMPLICIT_UNSAFE_AUTOREFS,
+    Warn,
+    "implicit reference to a dereference of a raw pointer"
+}
+
+declare_lint_pass!(ImplicitUnsafeAutorefs => [IMPLICIT_UNSAFE_AUTOREFS]);
+
+impl<'tcx> LateLintPass<'tcx> for ImplicitUnsafeAutorefs {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        let typeck = cx.typeck_results();
+        let adjustments_table = typeck.adjustments();
+
+        if let Some(adjustments) = adjustments_table.get(expr.hir_id)
+        && let [adjustment] = &**adjustments
+        // An auto-borrow
+        && let Some((mutbl, implicit_borrow)) = has_implicit_borrow(adjustment)
+        // ... of a place derived from a deref
+        && let ExprKind::Unary(UnOp::Deref, dereferenced) = peel_place_mappers(cx.tcx, typeck, &expr).kind
+        // ... of a raw pointer
+        && typeck.expr_ty(dereferenced).is_unsafe_ptr()
+        {
+            let mutbl = Mutability::prefix_str(&mutbl.into());
+
+            let msg = "implicit auto-ref creates a reference to a dereference of a raw pointer";
+            cx.struct_span_lint(IMPLICIT_UNSAFE_AUTOREFS, expr.span, msg, |lint| {
+                lint
+                    .note("creating a reference requires the pointer to be valid and imposes aliasing requirements");
+
+                if let Some(reason) = reason(cx.tcx, implicit_borrow, expr) {
+                    lint.note(format!("a reference is implicitly created {reason}"));
+                }
+
+                lint
+                    .multipart_suggestion(
+                        "try using a raw pointer method instead; or if this reference is intentional, make it explicit",
+                        vec![
+                            (expr.span.shrink_to_lo(), format!("(&{mutbl}")),
+                            (expr.span.shrink_to_hi(), ")".to_owned())
+                        ],
+                        Applicability::MaybeIncorrect
+                    )
+            })
+        }
+    }
+}
+
+/// Peels expressions from `expr` that can map a place.
+///
+/// For example `(*ptr).field[0]/*<-- built-in index */.field` -> `*ptr`, `f(*ptr)` -> `f(*ptr)`, etc.
+fn peel_place_mappers<'tcx>(
+    tcx: TyCtxt<'tcx>,
+    typeck: &TypeckResults<'tcx>,
+    mut expr: &'tcx Expr<'tcx>,
+) -> &'tcx Expr<'tcx> {
+    loop {
+        match expr.kind {
+            ExprKind::Index(base, idx)
+                if typeck.expr_ty(base).builtin_index() == Some(typeck.expr_ty(expr))
+                    && typeck.expr_ty(idx) == tcx.types.usize =>
+            {
+                expr = &base;
+            }
+            ExprKind::Field(e, _) => expr = &e,
+            _ => break expr,
+        }
+    }
+}
+
+/// Returns `Some(mutability)` if the argument adjustment has implicit borrow in it.
+fn has_implicit_borrow(
+    Adjustment { kind, .. }: &Adjustment<'_>,
+) -> Option<(Mutability, ImplicitBorrow)> {
+    match kind {
+        &Adjust::Deref(Some(OverloadedDeref { mutbl, .. })) => Some((mutbl, ImplicitBorrow::Deref)),
+        &Adjust::Borrow(AutoBorrow::Ref(_, mutbl)) => Some((mutbl.into(), ImplicitBorrow::Borrow)),
+        _ => None,
+    }
+}
+
+enum ImplicitBorrow {
+    Deref,
+    Borrow,
+}
+
+fn reason(tcx: TyCtxt<'_>, borrow_kind: ImplicitBorrow, expr: &Expr<'_>) -> Option<&'static str> {
+    match borrow_kind {
+        ImplicitBorrow::Deref => Some("because a deref coercion is being applied"),
+        ImplicitBorrow::Borrow => {
+            let parent = tcx.hir().get(tcx.hir().find_parent_node(expr.hir_id)?);
+
+            let hir::Node::Expr(expr) = parent else {
+                return None
+            };
+
+            let reason = match expr.kind {
+                ExprKind::MethodCall(_, _, _, _) => "to match the method receiver type",
+                ExprKind::AssignOp(_, _, _) => {
+                    "because a user-defined assignment with an operator is being used"
+                }
+                ExprKind::Index(_, _) => {
+                    "because a user-defined indexing operation is being called"
+                }
+                _ => return None,
+            };
+
+            Some(reason)
+        }
+    }
+}

compiler/rustc_lint/src/lib.rs

@@ -54,6 +54,7 @@ mod errors;
 mod expect;
 mod for_loops_over_fallibles;
 pub mod hidden_unicode_codepoints;
+mod implicit_unsafe_autorefs;
 mod internal;
 mod late;
 mod let_underscore;
@@ -89,6 +90,7 @@ use builtin::*;
 use enum_intrinsics_non_enums::EnumIntrinsicsNonEnums;
 use for_loops_over_fallibles::*;
 use hidden_unicode_codepoints::*;
+use implicit_unsafe_autorefs::*;
 use internal::*;
 use let_underscore::*;
 use methods::*;
@@ -191,6 +193,7 @@ macro_rules! late_lint_mod_passes {
             $args,
             [
                 ForLoopsOverFallibles: ForLoopsOverFallibles,
+                ImplicitUnsafeAutorefs: ImplicitUnsafeAutorefs,
                 HardwiredLints: HardwiredLints,
                 ImproperCTypesDeclarations: ImproperCTypesDeclarations,
                 ImproperCTypesDefinitions: ImproperCTypesDefinitions,

library/alloc/src/collections/btree/node.rs

@@ -544,7 +544,7 @@ impl<'a, K: 'a, V: 'a> NodeRef<marker::Mut<'a>, K, V, marker::LeafOrInternal> {
     fn set_parent_link(&mut self, parent: NonNull<InternalNode<K, V>>, parent_idx: usize) {
         let leaf = Self::as_leaf_ptr(self);
         unsafe { (*leaf).parent = Some(parent) };
-        unsafe { (*leaf).parent_idx.write(parent_idx as u16) };
+        unsafe { (*leaf).parent_idx = MaybeUninit::new(parent_idx as u16) };
     }
 }
 
@@ -1699,7 +1699,7 @@ unsafe fn slice_insert<T>(slice: &mut [MaybeUninit<T>], idx: usize, val: T) {
         if len > idx + 1 {
             ptr::copy(slice_ptr.add(idx), slice_ptr.add(idx + 1), len - idx - 1);
         }
-        (*slice_ptr.add(idx)).write(val);
+        (&mut *slice_ptr.add(idx)).write(val);
     }
 }
 
@@ -1713,7 +1713,7 @@ unsafe fn slice_remove<T>(slice: &mut [MaybeUninit<T>], idx: usize) -> T {
         let len = slice.len();
         debug_assert!(idx < len);
         let slice_ptr = slice.as_mut_ptr();
-        let ret = (*slice_ptr.add(idx)).assume_init_read();
+        let ret = (&*slice_ptr.add(idx)).assume_init_read();
         ptr::copy(slice_ptr.add(idx + 1), slice_ptr.add(idx), len - idx - 1);
         ret
     }

library/alloc/src/rc.rs

@@ -457,9 +457,9 @@ impl<T> Rc<T> {
             let inner = init_ptr.as_ptr();
             ptr::write(ptr::addr_of_mut!((*inner).value), data);
 
-            let prev_value = (*inner).strong.get();
+            let prev_value = (&(*inner).strong).get();
             debug_assert_eq!(prev_value, 0, "No prior strong references should exist");
-            (*inner).strong.set(1);
+            (&mut (*inner).strong).set(1);
 
             Rc::from_inner(init_ptr)
         };

library/alloc/src/string.rs

@@ -2909,7 +2909,7 @@ impl Drop for Drain<'_> {
         unsafe {
             // Use Vec::drain. "Reaffirm" the bounds checks to avoid
             // panic code being inserted again.
-            let self_vec = (*self.string).as_mut_vec();
+            let self_vec = (&mut *self.string).as_mut_vec();
             if self.start <= self.end && self.end <= self_vec.len() {
                 self_vec.drain(self.start..self.end);
             }

library/alloc/src/sync.rs

@@ -456,7 +456,7 @@ impl<T> Arc<T> {
             //
             // These side effects do not impact us in any way, and no other side effects are
             // possible with safe code alone.
-            let prev_value = (*inner).strong.fetch_add(1, Release);
+            let prev_value = (&(*inner).strong).fetch_add(1, Release);
             debug_assert_eq!(prev_value, 0, "No prior strong references should exist");
 
             Arc::from_inner(init_ptr)

library/alloc/src/sync/tests.rs

@@ -16,17 +16,11 @@ use std::thread;
 
 use crate::vec::Vec;
 
-struct Canary(*mut atomic::AtomicUsize);
+struct Canary<'a>(&'a atomic::AtomicUsize);
 
-impl Drop for Canary {
+impl Drop for Canary<'_> {
     fn drop(&mut self) {
-        unsafe {
-            match *self {
-                Canary(c) => {
-                    (*c).fetch_add(1, SeqCst);
-                }
-            }
-        }
+        self.0.fetch_add(1, SeqCst);
     }
 }
 
@@ -272,16 +266,16 @@ fn weak_self_cyclic() {
 
 #[test]
 fn drop_arc() {
-    let mut canary = atomic::AtomicUsize::new(0);
-    let x = Arc::new(Canary(&mut canary as *mut atomic::AtomicUsize));
+    let canary = atomic::AtomicUsize::new(0);
+    let x = Arc::new(Canary(&canary));
     drop(x);
     assert!(canary.load(Acquire) == 1);
 }
 
 #[test]
 fn drop_arc_weak() {
-    let mut canary = atomic::AtomicUsize::new(0);
-    let arc = Arc::new(Canary(&mut canary as *mut atomic::AtomicUsize));
+    let canary = atomic::AtomicUsize::new(0);
+    let arc = Arc::new(Canary(&canary));
     let arc_weak = Arc::downgrade(&arc);
     assert!(canary.load(Acquire) == 0);
     drop(arc);

library/alloc/src/tests.rs

@@ -102,8 +102,8 @@ fn raw_trait() {
     let x: Box<dyn Foo> = Box::new(Bar(17));
     let p = Box::into_raw(x);
     unsafe {
-        assert_eq!(17, (*p).get());
-        (*p).set(19);
+        assert_eq!(17, (&*p).get());
+        (&mut *p).set(19);
         let y: Box<dyn Foo> = Box::from_raw(p);
         assert_eq!(19, y.get());
     }

library/alloc/src/vec/mod.rs

@@ -1942,7 +1942,7 @@ impl<T, A: Allocator> Vec<T, A> {
     #[cfg(not(no_global_oom_handling))]
     #[inline]
     unsafe fn append_elements(&mut self, other: *const [T]) {
-        let count = unsafe { (*other).len() };
+        let count = other.len();
         self.reserve(count);
         let len = self.len();
         unsafe { ptr::copy_nonoverlapping(other as *const T, self.as_mut_ptr().add(len), count) };

library/proc_macro/src/bridge/closure.rs

@@ -19,7 +19,7 @@ struct Env;
 impl<'a, A, R, F: FnMut(A) -> R> From<&'a mut F> for Closure<'a, A, R> {
     fn from(f: &'a mut F) -> Self {
         unsafe extern "C" fn call<A, R, F: FnMut(A) -> R>(env: *mut Env, arg: A) -> R {
-            (*(env as *mut _ as *mut F))(arg)
+            (&mut *(env as *mut _ as *mut F))(arg)
         }
         Closure { call: call::<A, R, F>, env: f as *mut _ as *mut Env, _marker: PhantomData }
     }

library/std/src/sync/mpsc/mpsc_queue.rs

@@ -70,7 +70,7 @@ impl<T> Queue<T> {
         unsafe {
             let n = Node::new(Some(t));
             let prev = self.head.swap(n, Ordering::AcqRel);
-            (*prev).next.store(n, Ordering::Release);
+            (&(*prev).next).store(n, Ordering::Release);
         }
     }
 
@@ -87,13 +87,13 @@ impl<T> Queue<T> {
     pub fn pop(&self) -> PopResult<T> {
         unsafe {
             let tail = *self.tail.get();
-            let next = (*tail).next.load(Ordering::Acquire);
+            let next = (&(*tail).next).load(Ordering::Acquire);
 
             if !next.is_null() {
                 *self.tail.get() = next;
-                assert!((*tail).value.is_none());
-                assert!((*next).value.is_some());
-                let ret = (*next).value.take().unwrap();
+                assert!((&(*tail).value).is_none());
+                assert!((&(*next).value).is_some());
+                let ret = (&mut (*next).value).take().unwrap();
                 let _: Box<Node<T>> = Box::from_raw(tail);
                 return Data(ret);
             }
@@ -108,7 +108,7 @@ impl<T> Drop for Queue<T> {
         unsafe {
             let mut cur = *self.tail.get();
             while !cur.is_null() {
-                let next = (*cur).next.load(Ordering::Relaxed);
+                let next = (&(*cur).next).load(Ordering::Relaxed);
                 let _: Box<Node<T>> = Box::from_raw(cur);
                 cur = next;
             }