Do not suggest to use implicit DerefMut on ManuallyDrop reached through unions (#14387)

This requires making the `deref_addrof` lint a late lint instead of an
early lint to check for types.

changelog: [`deref_addrof`]: do not suggest implicit `DerefMut` on
`ManuallyDrop` union fields

Fix #14386

Author: blyxyas

clippy_lints/src/dereference.rs

@@ -1,12 +1,11 @@
 use clippy_utils::diagnostics::{span_lint_and_sugg, span_lint_hir_and_then};
 use clippy_utils::source::{snippet_with_applicability, snippet_with_context};
 use clippy_utils::sugg::has_enclosing_paren;
-use clippy_utils::ty::{implements_trait, is_manually_drop};
+use clippy_utils::ty::{adjust_derefs_manually_drop, implements_trait, is_manually_drop};
 use clippy_utils::{
     DefinedTy, ExprUseNode, expr_use_ctxt, get_parent_expr, is_block_like, is_lint_allowed, path_to_local,
     peel_middle_ty_refs,
 };
-use core::mem;
 use rustc_ast::util::parser::ExprPrecedence;
 use rustc_data_structures::fx::FxIndexMap;
 use rustc_errors::Applicability;
@@ -707,14 +706,6 @@ fn try_parse_ref_op<'tcx>(
     ))
 }
 
-// Checks if the adjustments contains a deref of `ManuallyDrop<_>`
-fn adjust_derefs_manually_drop<'tcx>(adjustments: &'tcx [Adjustment<'tcx>], mut ty: Ty<'tcx>) -> bool {
-    adjustments.iter().any(|a| {
-        let ty = mem::replace(&mut ty, a.target);
-        matches!(a.kind, Adjust::Deref(Some(ref op)) if op.mutbl == Mutability::Mut) && is_manually_drop(ty)
-    })
-}
-
 // Checks whether the type for a deref call actually changed the type, not just the mutability of
 // the reference.
 fn deref_method_same_type<'tcx>(result_ty: Ty<'tcx>, arg_ty: Ty<'tcx>) -> bool {

clippy_lints/src/lib.rs

@@ -600,7 +600,7 @@ pub fn register_lint_passes(store: &mut rustc_lint::LintStore, conf: &'static Co
     store.register_late_pass(move |_| Box::new(trait_bounds::TraitBounds::new(conf)));
     store.register_late_pass(|_| Box::new(comparison_chain::ComparisonChain));
     store.register_late_pass(move |tcx| Box::new(mut_key::MutableKeyType::new(tcx, conf)));
-    store.register_early_pass(|| Box::new(reference::DerefAddrOf));
+    store.register_late_pass(|_| Box::new(reference::DerefAddrOf));
     store.register_early_pass(|| Box::new(double_parens::DoubleParens));
     let format_args = format_args_storage.clone();
     store.register_late_pass(move |_| Box::new(format_impl::FormatImpl::new(format_args.clone())));

clippy_lints/src/reference.rs

@@ -1,10 +1,11 @@
 use clippy_utils::diagnostics::span_lint_and_sugg;
-use clippy_utils::source::{SpanRangeExt, snippet_with_applicability};
-use rustc_ast::ast::{Expr, ExprKind, Mutability, UnOp};
+use clippy_utils::source::snippet;
+use clippy_utils::sugg::{Sugg, has_enclosing_paren};
+use clippy_utils::ty::adjust_derefs_manually_drop;
 use rustc_errors::Applicability;
-use rustc_lint::{EarlyContext, EarlyLintPass};
+use rustc_hir::{Expr, ExprKind, HirId, Node, UnOp};
+use rustc_lint::{LateContext, LateLintPass};
 use rustc_session::declare_lint_pass;
-use rustc_span::{BytePos, Span};
 
 declare_clippy_lint! {
     /// ### What it does
@@ -37,75 +38,95 @@ declare_clippy_lint! {
 
 declare_lint_pass!(DerefAddrOf => [DEREF_ADDROF]);
 
-fn without_parens(mut e: &Expr) -> &Expr {
-    while let ExprKind::Paren(ref child_e) = e.kind {
-        e = child_e;
-    }
-    e
-}
-
-impl EarlyLintPass for DerefAddrOf {
-    fn check_expr(&mut self, cx: &EarlyContext<'_>, e: &Expr) {
-        if let ExprKind::Unary(UnOp::Deref, ref deref_target) = e.kind
-            && let ExprKind::AddrOf(_, ref mutability, ref addrof_target) = without_parens(deref_target).kind
+impl LateLintPass<'_> for DerefAddrOf {
+    fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) {
+        if !e.span.from_expansion()
+            && let ExprKind::Unary(UnOp::Deref, deref_target) = e.kind
+            && !deref_target.span.from_expansion()
+            && let ExprKind::AddrOf(_, _, addrof_target) = deref_target.kind
             // NOTE(tesuji): `*&` forces rustc to const-promote the array to `.rodata` section.
             // See #12854 for details.
             && !matches!(addrof_target.kind, ExprKind::Array(_))
             && deref_target.span.eq_ctxt(e.span)
             && !addrof_target.span.from_expansion()
         {
             let mut applicability = Applicability::MachineApplicable;
-            let sugg = if e.span.from_expansion() {
-                if let Some(macro_source) = e.span.get_source_text(cx) {
-                    // Remove leading whitespace from the given span
-                    // e.g: ` $visitor` turns into `$visitor`
-                    let trim_leading_whitespaces = |span: Span| {
-                        span.get_source_text(cx)
-                            .and_then(|snip| {
-                                #[expect(clippy::cast_possible_truncation)]
-                                snip.find(|c: char| !c.is_whitespace())
-                                    .map(|pos| span.lo() + BytePos(pos as u32))
-                            })
-                            .map_or(span, |start_no_whitespace| e.span.with_lo(start_no_whitespace))
-                    };
+            let mut sugg = || Sugg::hir_with_applicability(cx, addrof_target, "_", &mut applicability);
 
-                    let mut generate_snippet = |pattern: &str| {
-                        #[expect(clippy::cast_possible_truncation)]
-                        macro_source.rfind(pattern).map(|pattern_pos| {
-                            let rpos = pattern_pos + pattern.len();
-                            let span_after_ref = e.span.with_lo(BytePos(e.span.lo().0 + rpos as u32));
-                            let span = trim_leading_whitespaces(span_after_ref);
-                            snippet_with_applicability(cx, span, "_", &mut applicability)
-                        })
-                    };
+            // If this expression is an explicit `DerefMut` of a `ManuallyDrop` reached through a
+            // union, we may remove the reference if we are at the point where the implicit
+            // dereference would take place. Otherwise, we should not lint.
+            let sugg = match is_manually_drop_through_union(cx, e.hir_id, addrof_target) {
+                ManuallyDropThroughUnion::Directly => sugg().deref(),
+                ManuallyDropThroughUnion::Indirect => return,
+                ManuallyDropThroughUnion::No => sugg(),
+            };
+
+            let sugg = if has_enclosing_paren(snippet(cx, e.span, "")) {
+                sugg.maybe_paren()
+            } else {
+                sugg
+            };
+
+            span_lint_and_sugg(
+                cx,
+                DEREF_ADDROF,
+                e.span,
+                "immediately dereferencing a reference",
+                "try",
+                sugg.to_string(),
+                applicability,
+            );
+        }
+    }
+}
+
+/// Is this a `ManuallyDrop` reached through a union, and when is `DerefMut` called on it?
+enum ManuallyDropThroughUnion {
+    /// `ManuallyDrop` reached through a union and immediately explicitely dereferenced
+    Directly,
+    /// `ManuallyDrop` reached through a union, and dereferenced later on
+    Indirect,
+    /// Any other situation
+    No,
+}
 
-                    if *mutability == Mutability::Mut {
-                        generate_snippet("mut")
+/// Check if `addrof_target` is part of an access to a `ManuallyDrop` entity reached through a
+/// union, and when it is dereferenced using `DerefMut` starting from `expr_id` and going up.
+fn is_manually_drop_through_union(
+    cx: &LateContext<'_>,
+    expr_id: HirId,
+    addrof_target: &Expr<'_>,
+) -> ManuallyDropThroughUnion {
+    if is_reached_through_union(cx, addrof_target) {
+        let typeck = cx.typeck_results();
+        for (idx, id) in std::iter::once(expr_id)
+            .chain(cx.tcx.hir_parent_id_iter(expr_id))
+            .enumerate()
+        {
+            if let Node::Expr(expr) = cx.tcx.hir_node(id) {
+                if adjust_derefs_manually_drop(typeck.expr_adjustments(expr), typeck.expr_ty(expr)) {
+                    return if idx == 0 {
+                        ManuallyDropThroughUnion::Directly
                     } else {
-                        generate_snippet("&")
-                    }
-                } else {
-                    Some(snippet_with_applicability(cx, e.span, "_", &mut applicability))
+                        ManuallyDropThroughUnion::Indirect
+                    };
                 }
             } else {
-                Some(snippet_with_applicability(
-                    cx,
-                    addrof_target.span,
-                    "_",
-                    &mut applicability,
-                ))
-            };
-            if let Some(sugg) = sugg {
-                span_lint_and_sugg(
-                    cx,
-                    DEREF_ADDROF,
-                    e.span,
-                    "immediately dereferencing a reference",
-                    "try",
-                    sugg.to_string(),
-                    applicability,
-                );
+                break;
             }
         }
     }
+    ManuallyDropThroughUnion::No
+}
+
+/// Checks whether `expr` denotes an object reached through a union
+fn is_reached_through_union(cx: &LateContext<'_>, mut expr: &Expr<'_>) -> bool {
+    while let ExprKind::Field(parent, _) | ExprKind::Index(parent, _, _) = expr.kind {
+        if cx.typeck_results().expr_ty_adjusted(parent).is_union() {
+            return true;
+        }
+        expr = parent;
+    }
+    false
 }

clippy_utils/src/ty/mod.rs

@@ -18,6 +18,7 @@ use rustc_lint::LateContext;
 use rustc_middle::mir::ConstValue;
 use rustc_middle::mir::interpret::Scalar;
 use rustc_middle::traits::EvaluationResult;
+use rustc_middle::ty::adjustment::{Adjust, Adjustment};
 use rustc_middle::ty::layout::ValidityRequirement;
 use rustc_middle::ty::{
     self, AdtDef, AliasTy, AssocItem, AssocTag, Binder, BoundRegion, FnSig, GenericArg, GenericArgKind, GenericArgsRef,
@@ -31,7 +32,7 @@ use rustc_trait_selection::traits::query::normalize::QueryNormalizeExt;
 use rustc_trait_selection::traits::{Obligation, ObligationCause};
 use std::assert_matches::debug_assert_matches;
 use std::collections::hash_map::Entry;
-use std::iter;
+use std::{iter, mem};
 
 use crate::path_res;
 use crate::paths::{PathNS, lookup_path_str};
@@ -1382,7 +1383,6 @@ pub fn is_slice_like<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
         || matches!(ty.kind(), ty::Adt(adt_def, _) if cx.tcx.is_diagnostic_item(sym::Vec, adt_def.did()))
 }
 
-/// Gets the index of a field by name.
 pub fn get_field_idx_by_name(ty: Ty<'_>, name: Symbol) -> Option<usize> {
     match *ty.kind() {
         ty::Adt(def, _) if def.is_union() || def.is_struct() => {
@@ -1392,3 +1392,11 @@ pub fn get_field_idx_by_name(ty: Ty<'_>, name: Symbol) -> Option<usize> {
         _ => None,
     }
 }
+
+/// Checks if the adjustments contain a mutable dereference of a `ManuallyDrop<_>`.
+pub fn adjust_derefs_manually_drop<'tcx>(adjustments: &'tcx [Adjustment<'tcx>], mut ty: Ty<'tcx>) -> bool {
+    adjustments.iter().any(|a| {
+        let ty = mem::replace(&mut ty, a.target);
+        matches!(a.kind, Adjust::Deref(Some(op)) if op.mutbl == Mutability::Mut) && is_manually_drop(ty)
+    })
+}

tests/ui/deref_addrof.fixed

@@ -1,11 +1,11 @@
-//@aux-build:proc_macros.rs
-
-#![allow(clippy::return_self_not_must_use, clippy::useless_vec)]
+#![allow(
+    dangerous_implicit_autorefs,
+    clippy::explicit_auto_deref,
+    clippy::return_self_not_must_use,
+    clippy::useless_vec
+)]
 #![warn(clippy::deref_addrof)]
 
-extern crate proc_macros;
-use proc_macros::inline_macros;
-
 fn get_number() -> usize {
     10
 }
@@ -56,19 +56,75 @@ fn main() {
     //~^ deref_addrof
     // do NOT lint for array as semantic differences with/out `*&`.
     let _arr = *&[0, 1, 2, 3, 4];
+
+    // Do not lint when text comes from macro
+    macro_rules! mac {
+        (dr) => {
+            *&0
+        };
+        (dr $e:expr) => {
+            *&$e
+        };
+        (r $e:expr) => {
+            &$e
+        };
+    }
+    let b = mac!(dr);
+    let b = mac!(dr a);
+    let b = *mac!(r a);
 }
 
-#[derive(Copy, Clone)]
-pub struct S;
-#[inline_macros]
-impl S {
-    pub fn f(&self) -> &Self {
-        inline!($(@expr self))
-        //~^ deref_addrof
+fn issue14386() {
+    use std::mem::ManuallyDrop;
+
+    #[derive(Copy, Clone)]
+    struct Data {
+        num: u64,
     }
-    #[allow(unused_mut)] // mut will be unused, once the macro is fixed
-    pub fn f_mut(mut self) -> Self {
-        inline!($(@expr self))
+
+    #[derive(Clone, Copy)]
+    struct M {
+        md: ManuallyDrop<[u8; 4]>,
+    }
+
+    union DataWithPadding<'lt> {
+        data: ManuallyDrop<Data>,
+        prim: ManuallyDrop<u64>,
+        padding: [u8; size_of::<Data>()],
+        tup: (ManuallyDrop<Data>, ()),
+        indirect: M,
+        indirect_arr: [M; 2],
+        indirect_ref: &'lt mut M,
+    }
+
+    let mut a = DataWithPadding {
+        padding: [0; size_of::<DataWithPadding>()],
+    };
+    unsafe {
+        a.padding = [1; size_of::<DataWithPadding>()];
         //~^ deref_addrof
+        a.tup.1 = ();
+        //~^ deref_addrof
+        *a.prim = 0;
+        //~^ deref_addrof
+
+        (*a.data).num = 42;
+        //~^ deref_addrof
+        (*a.indirect.md)[3] = 1;
+        //~^ deref_addrof
+        (*a.indirect_arr[1].md)[3] = 1;
+        //~^ deref_addrof
+        (*a.indirect_ref.md)[3] = 1;
+        //~^ deref_addrof
+
+        // Check that raw pointers are properly considered as well
+        *a.prim = 0;
+        //~^ deref_addrof
+        (*a.data).num = 42;
+        //~^ deref_addrof
+
+        // Do not lint, as the dereference happens later, we cannot
+        // just remove `&mut`
+        (*&mut a.tup).0.num = 42;
     }
 }