Auto merge of rust-lang#123239 - Urgau:dangerous_implicit_autorefs, r=<try>

Implement a lint for implicit autoref of raw pointer dereference - take 2

*[t-lang nomination comment](rust-lang#123239 (comment)

This PR aims at implementing a lint for implicit autoref of raw pointer dereference, it is based on rust-lang#103735 with suggestion and improvements from rust-lang#103735 (comment).

The goal is to catch cases like this, where the user probably doesn't realise it just created a reference.

```rust
pub struct Test {
    data: [u8],
}

pub fn test_len(t: *const Test) -> usize {
    unsafe { (*t).data.len() }  // this calls <[T]>::len(&self)
}
```

Since rust-lang#103735 already went 2 times through T-lang, where they T-lang ended-up asking for a more restricted version (which is what this PR does), I would prefer this PR to be reviewed first before re-nominating it for T-lang.

----

Compared to the PR it is as based on, this PR adds 3 restrictions on the outer most expression, which must either be:
   1. A deref followed by any non-deref place projection (that intermediate deref will typically be auto-inserted)
   2. A method call annotated with `#[rustc_no_implicit_refs]`.
   3. A deref followed by a `addr_of!` or `addr_of_mut!`. See bottom of post for details.

There are several points that are not 100% clear to me when implementing the modifications:
 - ~~"4. Any number of automatically inserted deref/derefmut calls." I as never able to trigger this. Am I missing something?~~ Fixed
 - Are "index" and "field" enough?

----

cc `@JakobDegen` `@WaffleLapkin`
r? `@RalfJung`

try-job: dist-various-1
try-job: dist-various-2

Author: bors

compiler/rustc_feature/src/builtin_attrs.rs

@@ -918,6 +918,10 @@ pub static BUILTIN_ATTRIBUTES: &[BuiltinAttribute] = &[
         EncodeCrossCrate::Yes,
         "#[rustc_never_returns_null_ptr] is used to mark functions returning non-null pointers."
     ),
+    rustc_attr!(
+        rustc_no_implicit_autorefs, AttributeType::Normal, template!(Word), ErrorFollowing, EncodeCrossCrate::Yes,
+        "`#[rustc_no_implicit_autorefs]` is used to mark functions for which an autoref to the dereference of a raw pointer should not be used as an argument."
+    ),
     rustc_attr!(
         rustc_coherence_is_core, AttributeType::CrateLevel, template!(Word), ErrorFollowing, EncodeCrossCrate::No,
         "#![rustc_coherence_is_core] allows inherent methods on builtin types, only intended to be used in `core`."

compiler/rustc_lint/messages.ftl

@@ -360,6 +360,10 @@ lint_impl_trait_overcaptures = `{$self_ty}` will capture more lifetimes than pos
 lint_impl_trait_redundant_captures = all possible in-scope parameters are already captured, so `use<...>` syntax is redundant
     .suggestion = remove the `use<...>` syntax
 
+lint_implicit_unsafe_autorefs = implicit autoref creates a reference to the dereference of a raw pointer
+    .note = creating a reference requires the pointer target to be valid and imposes aliasing requirements
+    .suggestion = try using a raw pointer method instead; or if this reference is intentional, make it explicit
+
 lint_improper_ctypes = `extern` {$desc} uses type `{$ty}`, which is not FFI-safe
     .label = not FFI-safe
     .note = the type is defined here

compiler/rustc_lint/src/autorefs.rs

@@ -0,0 +1,153 @@
+use rustc_ast::{BorrowKind, UnOp};
+use rustc_hir::{Expr, ExprKind, Mutability};
+use rustc_middle::ty::adjustment::{Adjust, Adjustment, AutoBorrow, OverloadedDeref};
+use rustc_session::{declare_lint, declare_lint_pass};
+use rustc_span::sym;
+
+use crate::lints::{ImplicitUnsafeAutorefsDiag, ImplicitUnsafeAutorefsSuggestion};
+use crate::{LateContext, LateLintPass, LintContext};
+
+declare_lint! {
+    /// The `dangerous_implicit_autorefs` lint checks for implicitly taken references
+    /// to dereferences of raw pointers.
+    ///
+    /// ### Example
+    ///
+    /// ```rust
+    /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] {
+    ///     &raw 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 additional safety requirements. Unfortunately, it's possible
+    /// to take a reference to the dereference of a raw pointer implicitly, which inflicts
+    /// the usual reference requirements.
+    ///
+    /// If you are sure that you can soundly take a reference, then you can take it explicitly:
+    ///
+    /// ```rust
+    /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] {
+    ///     &raw mut (&mut *ptr)[..16]
+    /// }
+    /// ```
+    ///
+    /// Otherwise try to find an alternative way to achive your goals using only raw pointers:
+    ///
+    /// ```rust
+    /// use std::slice;
+    ///
+    /// unsafe fn fun(ptr: *mut [u8]) -> *mut [u8] {
+    ///     slice::from_raw_parts_mut(ptr.cast(), 16)
+    /// }
+    /// ```
+    pub DANGEROUS_IMPLICIT_AUTOREFS,
+    Warn,
+    "implicit reference to a dereference of a raw pointer",
+    report_in_external_macro
+}
+
+declare_lint_pass!(ImplicitAutorefs => [DANGEROUS_IMPLICIT_AUTOREFS]);
+
+impl<'tcx> LateLintPass<'tcx> for ImplicitAutorefs {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'_>) {
+        // This logic has mostly been taken from
+        // <https://github.com/rust-lang/rust/pull/103735#issuecomment-1370420305>
+
+        // 5. Either of the following:
+        //   a. A deref followed by any non-deref place projection (that intermediate
+        //      deref will typically be auto-inserted).
+        //   b. A method call annotated with `#[rustc_no_implicit_refs]`.
+        //   c. A deref followed by a `&raw const` or `&raw mut`.
+        let mut is_coming_from_deref = false;
+        let inner = match expr.kind {
+            ExprKind::AddrOf(BorrowKind::Raw, _, inner) => match inner.kind {
+                ExprKind::Unary(UnOp::Deref, inner) => {
+                    is_coming_from_deref = true;
+                    inner
+                }
+                _ => return,
+            },
+            ExprKind::Index(base, _, _) => base,
+            ExprKind::MethodCall(_, inner, _, _)
+                if let Some(def_id) = cx.typeck_results().type_dependent_def_id(expr.hir_id)
+                    && cx.tcx.has_attr(def_id, sym::rustc_no_implicit_autorefs) =>
+            {
+                inner
+            }
+            ExprKind::Field(inner, _) => inner,
+            _ => return,
+        };
+
+        let typeck = cx.typeck_results();
+        let adjustments_table = typeck.adjustments();
+
+        if let Some(adjustments) = adjustments_table.get(inner.hir_id)
+            // 4. Any number of automatically inserted deref/derefmut calls.
+            && let adjustments = peel_derefs_adjustments(&**adjustments)
+            // 3. An automatically inserted reference (might come from a deref).
+            && let [adjustment] = adjustments
+            && let Some(borrow_mutbl) = has_implicit_borrow(adjustment)
+            && let ExprKind::Unary(UnOp::Deref, dereferenced) =
+                // 2. Any number of place projections.
+                peel_place_mappers(inner).kind
+            // 1. Deref of a raw pointer.
+            && typeck.expr_ty(dereferenced).is_raw_ptr()
+        {
+            cx.emit_span_lint(
+                DANGEROUS_IMPLICIT_AUTOREFS,
+                expr.span.source_callsite(),
+                ImplicitUnsafeAutorefsDiag {
+                    suggestion: ImplicitUnsafeAutorefsSuggestion {
+                        mutbl: borrow_mutbl.ref_prefix_str(),
+                        deref: if is_coming_from_deref { "*" } else { "" },
+                        start_span: inner.span.shrink_to_lo(),
+                        end_span: inner.span.shrink_to_hi(),
+                    },
+                },
+            )
+        }
+    }
+}
+
+/// Peels expressions from `expr` that can map a place.
+fn peel_place_mappers<'tcx>(mut expr: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
+    loop {
+        match expr.kind {
+            ExprKind::Index(base, _idx, _) => expr = &base,
+            ExprKind::Field(e, _) => expr = &e,
+            _ => break expr,
+        }
+    }
+}
+
+/// Peel derefs adjustments until the last last element.
+fn peel_derefs_adjustments<'a>(mut adjs: &'a [Adjustment<'a>]) -> &'a [Adjustment<'a>] {
+    while let [Adjustment { kind: Adjust::Deref(_), .. }, end @ ..] = adjs
+        && !end.is_empty()
+    {
+        adjs = end;
+    }
+    adjs
+}
+
+/// Test if some adjustment has some implicit borrow.
+///
+/// Returns `Some(mutability)` if the argument adjustment has implicit borrow in it.
+fn has_implicit_borrow(Adjustment { kind, .. }: &Adjustment<'_>) -> Option<Mutability> {
+    match kind {
+        &Adjust::Deref(Some(OverloadedDeref { mutbl, .. })) => Some(mutbl),
+        &Adjust::Borrow(AutoBorrow::Ref(mutbl)) => Some(mutbl.into()),
+        Adjust::NeverToAny
+        | Adjust::Pointer(..)
+        | Adjust::ReborrowPin(..)
+        | Adjust::Deref(None)
+        | Adjust::Borrow(AutoBorrow::RawPtr(..)) => None,
+    }
+}

compiler/rustc_lint/src/lib.rs

@@ -36,6 +36,7 @@
 
 mod async_closures;
 mod async_fn_in_trait;
+mod autorefs;
 pub mod builtin;
 mod context;
 mod dangling;
@@ -83,6 +84,7 @@ mod utils;
 
 use async_closures::AsyncClosureUsage;
 use async_fn_in_trait::AsyncFnInTrait;
+use autorefs::*;
 use builtin::*;
 use dangling::*;
 use default_could_be_derived::DefaultCouldBeDerived;
@@ -200,6 +202,7 @@ late_lint_methods!(
             PathStatements: PathStatements,
             LetUnderscore: LetUnderscore,
             InvalidReferenceCasting: InvalidReferenceCasting,
+            ImplicitAutorefs: ImplicitAutorefs,
             // Depends on referenced function signatures in expressions
             UnusedResults: UnusedResults,
             UnitBindings: UnitBindings,

compiler/rustc_lint/src/lints.rs

@@ -55,6 +55,26 @@ pub(crate) enum ShadowedIntoIterDiagSub {
     },
 }
 
+// autorefs.rs
+#[derive(LintDiagnostic)]
+#[diag(lint_implicit_unsafe_autorefs)]
+#[note]
+pub(crate) struct ImplicitUnsafeAutorefsDiag {
+    #[subdiagnostic]
+    pub suggestion: ImplicitUnsafeAutorefsSuggestion,
+}
+
+#[derive(Subdiagnostic)]
+#[multipart_suggestion(lint_suggestion, applicability = "maybe-incorrect")]
+pub(crate) struct ImplicitUnsafeAutorefsSuggestion {
+    pub mutbl: &'static str,
+    pub deref: &'static str,
+    #[suggestion_part(code = "({mutbl}{deref}")]
+    pub start_span: Span,
+    #[suggestion_part(code = ")")]
+    pub end_span: Span,
+}
+
 // builtin.rs
 #[derive(LintDiagnostic)]
 #[diag(lint_builtin_while_true)]

compiler/rustc_passes/src/check_attr.rs

@@ -179,6 +179,9 @@ impl<'tcx> CheckAttrVisitor<'tcx> {
                         [sym::rustc_as_ptr, ..] => {
                             self.check_applied_to_fn_or_method(hir_id, attr, span, target)
                         }
+                        [sym::rustc_no_implicit_autorefs, ..] => {
+                            self.check_applied_to_fn_or_method(hir_id, attr, span, target)
+                        }
                         [sym::rustc_never_returns_null_ptr, ..] => {
                             self.check_applied_to_fn_or_method(hir_id, attr, span, target)
                         }

compiler/rustc_span/src/symbol.rs

@@ -1830,6 +1830,7 @@ symbols! {
         rustc_must_implement_one_of,
         rustc_never_returns_null_ptr,
         rustc_never_type_options,
+        rustc_no_implicit_autorefs,
         rustc_no_mir_inline,
         rustc_nonnull_optimization_guaranteed,
         rustc_nounwind,

library/alloc/src/string.rs

@@ -1832,6 +1832,7 @@ impl String {
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_stable(feature = "const_vec_string_slice", since = "1.87.0")]
     #[rustc_confusables("length", "size")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     pub const fn len(&self) -> usize {
         self.vec.len()
     }
@@ -1851,6 +1852,7 @@ impl String {
     #[must_use]
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_stable(feature = "const_vec_string_slice", since = "1.87.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     pub const fn is_empty(&self) -> bool {
         self.len() == 0
     }

library/alloc/src/vec/mod.rs

@@ -2588,7 +2588,7 @@ impl<T, A: Allocator> Vec<T, A> {
     #[inline]
     #[track_caller]
     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/core/src/ops/index.rs

@@ -67,6 +67,7 @@ pub trait Index<Idx: ?Sized> {
     ///
     /// May panic if the index is out of bounds.
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[track_caller]
     fn index(&self, index: Idx) -> &Self::Output;
 }
@@ -171,6 +172,7 @@ pub trait IndexMut<Idx: ?Sized>: Index<Idx> {
     ///
     /// May panic if the index is out of bounds.
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[track_caller]
     fn index_mut(&mut self, index: Idx) -> &mut Self::Output;
 }

library/core/src/pin.rs

@@ -676,7 +676,7 @@
 //!             let data_ptr = unpinned_src.data.as_ptr() as *const u8;
 //!             let slice_ptr = unpinned_src.slice.as_ptr() as *const u8;
 //!             let offset = slice_ptr.offset_from(data_ptr) as usize;
-//!             let len = (*unpinned_src.slice.as_ptr()).len();
+//!             let len = unpinned_src.slice.as_ptr().len();
 //!
 //!             unpinned_self.slice = NonNull::from(&mut unpinned_self.data[offset..offset+len]);
 //!         }

library/core/src/slice/mod.rs

@@ -109,6 +109,7 @@ impl<T> [T] {
     #[lang = "slice_len_fn"]
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_stable(feature = "const_slice_len", since = "1.39.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[inline]
     #[must_use]
     pub const fn len(&self) -> usize {
@@ -128,6 +129,7 @@ impl<T> [T] {
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_stable(feature = "const_slice_is_empty", since = "1.39.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[inline]
     #[must_use]
     pub const fn is_empty(&self) -> bool {
@@ -562,6 +564,7 @@ impl<T> [T] {
     /// assert_eq!(None, v.get(0..4));
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[inline]
     #[must_use]
     pub fn get<I>(&self, index: I) -> Option<&I::Output>
@@ -587,6 +590,7 @@ impl<T> [T] {
     /// assert_eq!(x, &[0, 42, 2]);
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[inline]
     #[must_use]
     pub fn get_mut<I>(&mut self, index: I) -> Option<&mut I::Output>
@@ -624,6 +628,7 @@ impl<T> [T] {
     /// }
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[inline]
     #[must_use]
     pub unsafe fn get_unchecked<I>(&self, index: I) -> &I::Output
@@ -666,6 +671,7 @@ impl<T> [T] {
     /// assert_eq!(x, &[1, 13, 4]);
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[inline]
     #[must_use]
     pub unsafe fn get_unchecked_mut<I>(&mut self, index: I) -> &mut I::Output

library/core/src/str/mod.rs

@@ -134,6 +134,7 @@ impl str {
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_stable(feature = "const_str_len", since = "1.39.0")]
     #[rustc_diagnostic_item = "str_len"]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[must_use]
     #[inline]
     pub const fn len(&self) -> usize {
@@ -153,6 +154,7 @@ impl str {
     /// ```
     #[stable(feature = "rust1", since = "1.0.0")]
     #[rustc_const_stable(feature = "const_str_is_empty", since = "1.39.0")]
+    #[cfg_attr(not(bootstrap), rustc_no_implicit_autorefs)]
     #[must_use]
     #[inline]
     pub const fn is_empty(&self) -> bool {

library/std/src/sys/pal/sgx/abi/usercalls/alloc.rs

@@ -675,7 +675,7 @@ where
 
     /// Obtain the number of elements in this user slice.
     pub fn len(&self) -> usize {
-        unsafe { (*self.0.get()).len() }
+        unsafe { self.0.get().len() }
     }
 
     /// Copies the value from user memory and appends it to `dest`.

src/tools/miri/tests/pass/dst-raw.rs

@@ -1,5 +1,7 @@
 // Test DST raw pointers
 
+#![allow(dangerous_implicit_autorefs)]
+
 trait Trait {
     fn foo(&self) -> isize;
 }

src/tools/miri/tests/pass/stacked-borrows/interior_mutability.rs

@@ -1,3 +1,5 @@
+#![allow(dangerous_implicit_autorefs)]
+
 use std::cell::{Cell, Ref, RefCell, RefMut, UnsafeCell};
 use std::mem::{self, MaybeUninit};
 

tests/ui/dynamically-sized-types/dst-raw.rs

@@ -1,6 +1,8 @@
 //@ run-pass
 // Test DST raw pointers
 
+#![allow(dangerous_implicit_autorefs)]
+
 trait Trait {
     fn foo(&self) -> isize;
 }