From 3688b33cb34f7fbee69a285cf4d76f45a227ef40 Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Tue, 11 Mar 2025 17:11:58 +0100 Subject: [PATCH 1/5] Move `adjust_derefs_manually_drop` into `clippy_utils` --- clippy_lints/src/dereference.rs | 11 +---------- clippy_utils/src/ty/mod.rs | 12 ++++++++++-- 2 files changed, 11 insertions(+), 12 deletions(-) diff --git a/clippy_lints/src/dereference.rs b/clippy_lints/src/dereference.rs index a22a2ee66d25..30f9a6374109 100644 --- a/clippy_lints/src/dereference.rs +++ b/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 { diff --git a/clippy_utils/src/ty/mod.rs b/clippy_utils/src/ty/mod.rs index c50ad17bfad1..f0402e2aff20 100644 --- a/clippy_utils/src/ty/mod.rs +++ b/clippy_utils/src/ty/mod.rs @@ -17,6 +17,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, @@ -30,7 +31,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}; @@ -1362,7 +1363,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 { match *ty.kind() { ty::Adt(def, _) if def.is_union() || def.is_struct() => { @@ -1372,3 +1372,11 @@ pub fn get_field_idx_by_name(ty: Ty<'_>, name: Symbol) -> Option { _ => 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) + }) +} From 2cfbb05d6a9293aa2ef93bca98c2ec3d373cea6d Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Mon, 10 Mar 2025 18:30:12 +0100 Subject: [PATCH 2/5] Convert `deref_addrof` lint to late lint This is necessary in order to use type analysis in later commits. --- clippy_lints/src/lib.rs | 2 +- clippy_lints/src/reference.rs | 21 +++++++-------------- 2 files changed, 8 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/lib.rs b/clippy_lints/src/lib.rs index 006145cc623c..ceb423b02ee2 100644 --- a/clippy_lints/src/lib.rs +++ b/clippy_lints/src/lib.rs @@ -717,7 +717,7 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) { 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()))); diff --git a/clippy_lints/src/reference.rs b/clippy_lints/src/reference.rs index 4bff37216eda..cb414d791191 100644 --- a/clippy_lints/src/reference.rs +++ b/clippy_lints/src/reference.rs @@ -1,8 +1,8 @@ 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 rustc_errors::Applicability; -use rustc_lint::{EarlyContext, EarlyLintPass}; +use rustc_hir::{Expr, ExprKind, Mutability, UnOp}; +use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; use rustc_span::{BytePos, Span}; @@ -37,17 +37,10 @@ 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 let ExprKind::Unary(UnOp::Deref, deref_target) = e.kind + && let ExprKind::AddrOf(_, mutability, 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(_)) @@ -79,7 +72,7 @@ impl EarlyLintPass for DerefAddrOf { }) }; - if *mutability == Mutability::Mut { + if mutability == Mutability::Mut { generate_snippet("mut") } else { generate_snippet("&") From 27478bef40046250b85d2796eb61ab185cd0d7ce Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Mon, 10 Mar 2025 18:39:52 +0100 Subject: [PATCH 3/5] Do not use `DerefMut` on `ManuallyDrop<_>` reached through unions --- clippy_lints/src/reference.rs | 32 ++++++++++++++++++++- tests/ui/deref_addrof.fixed | 54 ++++++++++++++++++++++++++++++++++- tests/ui/deref_addrof.rs | 54 ++++++++++++++++++++++++++++++++++- tests/ui/deref_addrof.stderr | 48 +++++++++++++++++++++++-------- 4 files changed, 173 insertions(+), 15 deletions(-) diff --git a/clippy_lints/src/reference.rs b/clippy_lints/src/reference.rs index cb414d791191..42d1dcaee36b 100644 --- a/clippy_lints/src/reference.rs +++ b/clippy_lints/src/reference.rs @@ -1,7 +1,8 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::{SpanRangeExt, snippet_with_applicability}; +use clippy_utils::ty::adjust_derefs_manually_drop; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, Mutability, UnOp}; +use rustc_hir::{Expr, ExprKind, Mutability, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; use rustc_span::{BytePos, Span}; @@ -44,6 +45,7 @@ impl LateLintPass<'_> for DerefAddrOf { // NOTE(tesuji): `*&` forces rustc to const-promote the array to `.rodata` section. // See #12854 for details. && !matches!(addrof_target.kind, ExprKind::Array(_)) + && !is_manually_drop_through_union(cx, addrof_target) && deref_target.span.eq_ctxt(e.span) && !addrof_target.span.from_expansion() { @@ -102,3 +104,31 @@ impl LateLintPass<'_> for DerefAddrOf { } } } + +/// Check if `expr` is part of an access to a `ManuallyDrop` entity reached through a union +fn is_manually_drop_through_union(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { + if is_reached_through_union(cx, expr) { + let typeck = cx.typeck_results(); + for (_, node) in cx.tcx.hir_parent_iter(expr.hir_id) { + if let Node::Expr(expr) = node { + if adjust_derefs_manually_drop(typeck.expr_adjustments(expr), typeck.expr_ty(expr)) { + return true; + } + } else { + break; + } + } + } + false +} + +/// 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 +} diff --git a/tests/ui/deref_addrof.fixed b/tests/ui/deref_addrof.fixed index 35dbd790e890..6b200edc38ba 100644 --- a/tests/ui/deref_addrof.fixed +++ b/tests/ui/deref_addrof.fixed @@ -1,6 +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; @@ -72,3 +77,50 @@ impl S { //~^ deref_addrof } } + +fn issue14386() { + use std::mem::ManuallyDrop; + + #[derive(Copy, Clone)] + struct Data { + num: u64, + } + + #[derive(Clone, Copy)] + struct M { + md: ManuallyDrop<[u8; 4]>, + } + + union DataWithPadding<'lt> { + data: ManuallyDrop, + prim: ManuallyDrop, + padding: [u8; size_of::()], + tup: (ManuallyDrop, ()), + indirect: M, + indirect_arr: [M; 2], + indirect_ref: &'lt mut M, + } + + let mut a = DataWithPadding { + padding: [0; size_of::()], + }; + unsafe { + a.padding = [1; size_of::()]; + //~^ deref_addrof + a.tup.1 = (); + //~^ deref_addrof + *a.prim = 0; + //~^ deref_addrof + + (*&mut a.data).num = 42; + (*&mut a.tup).0.num = 42; + (*&mut a.indirect.md)[3] = 1; + (*&mut a.indirect_arr[1].md)[3] = 1; + (*&mut a.indirect_ref.md)[3] = 1; + + // Check that raw pointers are properly considered as well + *a.prim = 0; + //~^ deref_addrof + (*&raw mut a.data).num = 42; + } +} diff --git a/tests/ui/deref_addrof.rs b/tests/ui/deref_addrof.rs index 96d1b92ef7be..abc9e819adef 100644 --- a/tests/ui/deref_addrof.rs +++ b/tests/ui/deref_addrof.rs @@ -1,6 +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; @@ -72,3 +77,50 @@ impl S { //~^ deref_addrof } } + +fn issue14386() { + use std::mem::ManuallyDrop; + + #[derive(Copy, Clone)] + struct Data { + num: u64, + } + + #[derive(Clone, Copy)] + struct M { + md: ManuallyDrop<[u8; 4]>, + } + + union DataWithPadding<'lt> { + data: ManuallyDrop, + prim: ManuallyDrop, + padding: [u8; size_of::()], + tup: (ManuallyDrop, ()), + indirect: M, + indirect_arr: [M; 2], + indirect_ref: &'lt mut M, + } + + let mut a = DataWithPadding { + padding: [0; size_of::()], + }; + unsafe { + (*&mut a.padding) = [1; size_of::()]; + //~^ deref_addrof + (*&mut a.tup).1 = (); + //~^ deref_addrof + **&mut a.prim = 0; + //~^ deref_addrof + + (*&mut a.data).num = 42; + (*&mut a.tup).0.num = 42; + (*&mut a.indirect.md)[3] = 1; + (*&mut a.indirect_arr[1].md)[3] = 1; + (*&mut a.indirect_ref.md)[3] = 1; + + // Check that raw pointers are properly considered as well + **&raw mut a.prim = 0; + //~^ deref_addrof + (*&raw mut a.data).num = 42; + } +} diff --git a/tests/ui/deref_addrof.stderr b/tests/ui/deref_addrof.stderr index 81414b625b2f..3c422a4040bb 100644 --- a/tests/ui/deref_addrof.stderr +++ b/tests/ui/deref_addrof.stderr @@ -1,5 +1,5 @@ error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:23:13 + --> tests/ui/deref_addrof.rs:28:13 | LL | let b = *&a; | ^^^ help: try: `a` @@ -8,55 +8,55 @@ LL | let b = *&a; = help: to override `-D warnings` add `#[allow(clippy::deref_addrof)]` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:26:13 + --> tests/ui/deref_addrof.rs:31:13 | LL | let b = *&get_number(); | ^^^^^^^^^^^^^^ help: try: `get_number()` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:32:13 + --> tests/ui/deref_addrof.rs:37:13 | LL | let b = *&bytes[1..2][0]; | ^^^^^^^^^^^^^^^^ help: try: `bytes[1..2][0]` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:37:13 + --> tests/ui/deref_addrof.rs:42:13 | LL | let b = *&(a); | ^^^^^ help: try: `(a)` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:40:13 + --> tests/ui/deref_addrof.rs:45:13 | LL | let b = *(&a); | ^^^^^ help: try: `a` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:44:13 + --> tests/ui/deref_addrof.rs:49:13 | LL | let b = *((&a)); | ^^^^^^^ help: try: `a` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:47:13 + --> tests/ui/deref_addrof.rs:52:13 | LL | let b = *&&a; | ^^^^ help: try: `&a` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:50:14 + --> tests/ui/deref_addrof.rs:55:14 | LL | let b = **&aref; | ^^^^^^ help: try: `aref` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:55:19 + --> tests/ui/deref_addrof.rs:60:19 | LL | let _repeat = *&[0; 64]; | ^^^^^^^^^ help: try: `[0; 64]` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:66:17 + --> tests/ui/deref_addrof.rs:71:17 | LL | inline!(*& $(@expr self)) | ^^^^^^^^^^^^^^^^ help: try: `$(@expr self)` @@ -64,12 +64,36 @@ LL | inline!(*& $(@expr self)) = note: this error originates in the macro `__inline_mac_impl` (in Nightly builds, run with -Z macro-backtrace for more info) error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:71:17 + --> tests/ui/deref_addrof.rs:76:17 | LL | inline!(*&mut $(@expr self)) | ^^^^^^^^^^^^^^^^^^^ help: try: `$(@expr self)` | = note: this error originates in the macro `__inline_mac_impl` (in Nightly builds, run with -Z macro-backtrace for more info) -error: aborting due to 11 previous errors +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:108:9 + | +LL | (*&mut a.padding) = [1; size_of::()]; + | ^^^^^^^^^^^^^^^^^ help: try: `a.padding` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:110:9 + | +LL | (*&mut a.tup).1 = (); + | ^^^^^^^^^^^^^ help: try: `a.tup` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:112:10 + | +LL | **&mut a.prim = 0; + | ^^^^^^^^^^^^ help: try: `a.prim` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:122:10 + | +LL | **&raw mut a.prim = 0; + | ^^^^^^^^^^^^^^^^ help: try: `a.prim` + +error: aborting due to 15 previous errors From ff496ad34fc823bc063293edbf48f14261c6e28b Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Wed, 7 May 2025 16:28:26 +0200 Subject: [PATCH 4/5] Lint more cases when the explicit dereference can be kept in place --- clippy_lints/src/reference.rs | 54 ++++++++++++++++++++++++++++------- tests/ui/deref_addrof.fixed | 20 +++++++++---- tests/ui/deref_addrof.rs | 10 ++++++- tests/ui/deref_addrof.stderr | 34 ++++++++++++++++++++-- 4 files changed, 99 insertions(+), 19 deletions(-) diff --git a/clippy_lints/src/reference.rs b/clippy_lints/src/reference.rs index 42d1dcaee36b..0b8993ee1e11 100644 --- a/clippy_lints/src/reference.rs +++ b/clippy_lints/src/reference.rs @@ -2,7 +2,7 @@ use clippy_utils::diagnostics::span_lint_and_sugg; use clippy_utils::source::{SpanRangeExt, snippet_with_applicability}; use clippy_utils::ty::adjust_derefs_manually_drop; use rustc_errors::Applicability; -use rustc_hir::{Expr, ExprKind, Mutability, Node, UnOp}; +use rustc_hir::{Expr, ExprKind, HirId, Mutability, Node, UnOp}; use rustc_lint::{LateContext, LateLintPass}; use rustc_session::declare_lint_pass; use rustc_span::{BytePos, Span}; @@ -45,10 +45,18 @@ impl LateLintPass<'_> for DerefAddrOf { // NOTE(tesuji): `*&` forces rustc to const-promote the array to `.rodata` section. // See #12854 for details. && !matches!(addrof_target.kind, ExprKind::Array(_)) - && !is_manually_drop_through_union(cx, addrof_target) && deref_target.span.eq_ctxt(e.span) && !addrof_target.span.from_expansion() { + // 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 keep_deref = match is_manually_drop_through_union(cx, e.hir_id, addrof_target) { + ManuallyDropThroughUnion::Directly => true, + ManuallyDropThroughUnion::Indirect => return, + ManuallyDropThroughUnion::No => false, + }; + let mut applicability = Applicability::MachineApplicable; let sugg = if e.span.from_expansion() { if let Some(macro_source) = e.span.get_source_text(cx) { @@ -97,7 +105,11 @@ impl LateLintPass<'_> for DerefAddrOf { e.span, "immediately dereferencing a reference", "try", - sugg.to_string(), + if keep_deref { + format!("(*{sugg})") + } else { + sugg.to_string() + }, applicability, ); } @@ -105,21 +117,43 @@ impl LateLintPass<'_> for DerefAddrOf { } } -/// Check if `expr` is part of an access to a `ManuallyDrop` entity reached through a union -fn is_manually_drop_through_union(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { - if is_reached_through_union(cx, expr) { +/// 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, +} + +/// 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 (_, node) in cx.tcx.hir_parent_iter(expr.hir_id) { - if let Node::Expr(expr) = node { + 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 true; + return if idx == 0 { + ManuallyDropThroughUnion::Directly + } else { + ManuallyDropThroughUnion::Indirect + }; } } else { break; } } } - false + ManuallyDropThroughUnion::No } /// Checks whether `expr` denotes an object reached through a union diff --git a/tests/ui/deref_addrof.fixed b/tests/ui/deref_addrof.fixed index 6b200edc38ba..ae8ed0dc1140 100644 --- a/tests/ui/deref_addrof.fixed +++ b/tests/ui/deref_addrof.fixed @@ -112,15 +112,23 @@ fn issue14386() { *a.prim = 0; //~^ deref_addrof - (*&mut a.data).num = 42; - (*&mut a.tup).0.num = 42; - (*&mut a.indirect.md)[3] = 1; - (*&mut a.indirect_arr[1].md)[3] = 1; - (*&mut a.indirect_ref.md)[3] = 1; + (*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 - (*&raw mut a.data).num = 42; + (*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; } } diff --git a/tests/ui/deref_addrof.rs b/tests/ui/deref_addrof.rs index abc9e819adef..4ff014059167 100644 --- a/tests/ui/deref_addrof.rs +++ b/tests/ui/deref_addrof.rs @@ -113,14 +113,22 @@ fn issue14386() { //~^ deref_addrof (*&mut a.data).num = 42; - (*&mut a.tup).0.num = 42; + //~^ deref_addrof (*&mut a.indirect.md)[3] = 1; + //~^ deref_addrof (*&mut a.indirect_arr[1].md)[3] = 1; + //~^ deref_addrof (*&mut a.indirect_ref.md)[3] = 1; + //~^ deref_addrof // Check that raw pointers are properly considered as well **&raw mut a.prim = 0; //~^ deref_addrof (*&raw mut 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; } } diff --git a/tests/ui/deref_addrof.stderr b/tests/ui/deref_addrof.stderr index 3c422a4040bb..adfa542765c0 100644 --- a/tests/ui/deref_addrof.stderr +++ b/tests/ui/deref_addrof.stderr @@ -90,10 +90,40 @@ LL | **&mut a.prim = 0; | ^^^^^^^^^^^^ help: try: `a.prim` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:122:10 + --> tests/ui/deref_addrof.rs:115:9 + | +LL | (*&mut a.data).num = 42; + | ^^^^^^^^^^^^^^ help: try: `(*a.data)` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:117:9 + | +LL | (*&mut a.indirect.md)[3] = 1; + | ^^^^^^^^^^^^^^^^^^^^^ help: try: `(*a.indirect.md)` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:119:9 + | +LL | (*&mut a.indirect_arr[1].md)[3] = 1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(*a.indirect_arr[1].md)` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:121:9 + | +LL | (*&mut a.indirect_ref.md)[3] = 1; + | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(*a.indirect_ref.md)` + +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:125:10 | LL | **&raw mut a.prim = 0; | ^^^^^^^^^^^^^^^^ help: try: `a.prim` -error: aborting due to 15 previous errors +error: immediately dereferencing a reference + --> tests/ui/deref_addrof.rs:127:9 + | +LL | (*&raw mut a.data).num = 42; + | ^^^^^^^^^^^^^^^^^^ help: try: `(*a.data)` + +error: aborting due to 20 previous errors From 7224dffc65e7082d2366aa2ca3f7620f93f5439b Mon Sep 17 00:00:00 2001 From: Samuel Tardieu Date: Wed, 7 May 2025 17:39:58 +0200 Subject: [PATCH 5/5] Do not lint data coming from macro Suggesting to remove `*&` or `*&mut` in a macro may be incorrect, as it would require tracking all possible macro usages. In some cases, it is not possible to remove the dereference or the reference. For example, in the following code, the `drmut!()` macro will be linted against while called as `drmut!(d)`, and the suggestion would be to remove the `*&mut`. That would make `drmut!(u.data).num = 1` invalid, as a `ManuallyDrop` object coming through a union cannot be implicitely dereferenced through `DerefMut`. ```rust use std::mem::ManuallyDrop; #[derive(Copy, Clone)] struct Data { num: u64, } union Union { data: ManuallyDrop, } macro_rules! drmut { ($e:expr) => { *&mut $e }; } fn f(mut u: Union, mut d: Data) { unsafe { drmut!(u.data).num = 1; } drmut!(d).num = 1; } ``` --- clippy_lints/src/reference.rs | 88 +++++++++++------------------------ tests/ui/deref_addrof.fixed | 32 ++++++------- tests/ui/deref_addrof.rs | 32 ++++++------- tests/ui/deref_addrof.stderr | 54 ++++++++------------- 4 files changed, 73 insertions(+), 133 deletions(-) diff --git a/clippy_lints/src/reference.rs b/clippy_lints/src/reference.rs index 0b8993ee1e11..3bbcad12a319 100644 --- a/clippy_lints/src/reference.rs +++ b/clippy_lints/src/reference.rs @@ -1,11 +1,11 @@ use clippy_utils::diagnostics::span_lint_and_sugg; -use clippy_utils::source::{SpanRangeExt, snippet_with_applicability}; +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_hir::{Expr, ExprKind, HirId, Mutability, Node, UnOp}; +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 @@ -40,79 +40,43 @@ declare_lint_pass!(DerefAddrOf => [DEREF_ADDROF]); impl LateLintPass<'_> for DerefAddrOf { fn check_expr(&mut self, cx: &LateContext<'_>, e: &Expr<'_>) { - if let ExprKind::Unary(UnOp::Deref, deref_target) = e.kind - && let ExprKind::AddrOf(_, mutability, addrof_target) = deref_target.kind + 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 mut sugg = || Sugg::hir_with_applicability(cx, addrof_target, "_", &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 keep_deref = match is_manually_drop_through_union(cx, e.hir_id, addrof_target) { - ManuallyDropThroughUnion::Directly => true, + let sugg = match is_manually_drop_through_union(cx, e.hir_id, addrof_target) { + ManuallyDropThroughUnion::Directly => sugg().deref(), ManuallyDropThroughUnion::Indirect => return, - ManuallyDropThroughUnion::No => false, + ManuallyDropThroughUnion::No => sugg(), }; - 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 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 mutability == Mutability::Mut { - generate_snippet("mut") - } else { - generate_snippet("&") - } - } else { - Some(snippet_with_applicability(cx, e.span, "_", &mut applicability)) - } + let sugg = if has_enclosing_paren(snippet(cx, e.span, "")) { + sugg.maybe_paren() } else { - Some(snippet_with_applicability( - cx, - addrof_target.span, - "_", - &mut applicability, - )) + sugg }; - if let Some(sugg) = sugg { - span_lint_and_sugg( - cx, - DEREF_ADDROF, - e.span, - "immediately dereferencing a reference", - "try", - if keep_deref { - format!("(*{sugg})") - } else { - sugg.to_string() - }, - applicability, - ); - } + + span_lint_and_sugg( + cx, + DEREF_ADDROF, + e.span, + "immediately dereferencing a reference", + "try", + sugg.to_string(), + applicability, + ); } } } diff --git a/tests/ui/deref_addrof.fixed b/tests/ui/deref_addrof.fixed index ae8ed0dc1140..ffe7f7d14408 100644 --- a/tests/ui/deref_addrof.fixed +++ b/tests/ui/deref_addrof.fixed @@ -1,5 +1,3 @@ -//@aux-build:proc_macros.rs - #![allow( dangerous_implicit_autorefs, clippy::explicit_auto_deref, @@ -8,9 +6,6 @@ )] #![warn(clippy::deref_addrof)] -extern crate proc_macros; -use proc_macros::inline_macros; - fn get_number() -> usize { 10 } @@ -61,21 +56,22 @@ fn main() { //~^ deref_addrof // do NOT lint for array as semantic differences with/out `*&`. let _arr = *&[0, 1, 2, 3, 4]; -} -#[derive(Copy, Clone)] -pub struct S; -#[inline_macros] -impl S { - pub fn f(&self) -> &Self { - inline!($(@expr self)) - //~^ deref_addrof - } - #[allow(unused_mut)] // mut will be unused, once the macro is fixed - pub fn f_mut(mut self) -> Self { - inline!($(@expr self)) - //~^ deref_addrof + // 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); } fn issue14386() { diff --git a/tests/ui/deref_addrof.rs b/tests/ui/deref_addrof.rs index 4ff014059167..bc253716affd 100644 --- a/tests/ui/deref_addrof.rs +++ b/tests/ui/deref_addrof.rs @@ -1,5 +1,3 @@ -//@aux-build:proc_macros.rs - #![allow( dangerous_implicit_autorefs, clippy::explicit_auto_deref, @@ -8,9 +6,6 @@ )] #![warn(clippy::deref_addrof)] -extern crate proc_macros; -use proc_macros::inline_macros; - fn get_number() -> usize { 10 } @@ -61,21 +56,22 @@ fn main() { //~^ deref_addrof // do NOT lint for array as semantic differences with/out `*&`. let _arr = *&[0, 1, 2, 3, 4]; -} -#[derive(Copy, Clone)] -pub struct S; -#[inline_macros] -impl S { - pub fn f(&self) -> &Self { - inline!(*& $(@expr self)) - //~^ deref_addrof - } - #[allow(unused_mut)] // mut will be unused, once the macro is fixed - pub fn f_mut(mut self) -> Self { - inline!(*&mut $(@expr self)) - //~^ deref_addrof + // 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); } fn issue14386() { diff --git a/tests/ui/deref_addrof.stderr b/tests/ui/deref_addrof.stderr index adfa542765c0..65dd904a8f75 100644 --- a/tests/ui/deref_addrof.stderr +++ b/tests/ui/deref_addrof.stderr @@ -1,5 +1,5 @@ error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:28:13 + --> tests/ui/deref_addrof.rs:23:13 | LL | let b = *&a; | ^^^ help: try: `a` @@ -8,122 +8,106 @@ LL | let b = *&a; = help: to override `-D warnings` add `#[allow(clippy::deref_addrof)]` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:31:13 + --> tests/ui/deref_addrof.rs:26:13 | LL | let b = *&get_number(); | ^^^^^^^^^^^^^^ help: try: `get_number()` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:37:13 + --> tests/ui/deref_addrof.rs:32:13 | LL | let b = *&bytes[1..2][0]; | ^^^^^^^^^^^^^^^^ help: try: `bytes[1..2][0]` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:42:13 + --> tests/ui/deref_addrof.rs:37:13 | LL | let b = *&(a); | ^^^^^ help: try: `(a)` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:45:13 + --> tests/ui/deref_addrof.rs:40:13 | LL | let b = *(&a); | ^^^^^ help: try: `a` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:49:13 + --> tests/ui/deref_addrof.rs:44:13 | LL | let b = *((&a)); | ^^^^^^^ help: try: `a` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:52:13 + --> tests/ui/deref_addrof.rs:47:13 | LL | let b = *&&a; | ^^^^ help: try: `&a` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:55:14 + --> tests/ui/deref_addrof.rs:50:14 | LL | let b = **&aref; | ^^^^^^ help: try: `aref` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:60:19 + --> tests/ui/deref_addrof.rs:55:19 | LL | let _repeat = *&[0; 64]; | ^^^^^^^^^ help: try: `[0; 64]` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:71:17 - | -LL | inline!(*& $(@expr self)) - | ^^^^^^^^^^^^^^^^ help: try: `$(@expr self)` - | - = note: this error originates in the macro `__inline_mac_impl` (in Nightly builds, run with -Z macro-backtrace for more info) - -error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:76:17 - | -LL | inline!(*&mut $(@expr self)) - | ^^^^^^^^^^^^^^^^^^^ help: try: `$(@expr self)` - | - = note: this error originates in the macro `__inline_mac_impl` (in Nightly builds, run with -Z macro-backtrace for more info) - -error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:108:9 + --> tests/ui/deref_addrof.rs:104:9 | LL | (*&mut a.padding) = [1; size_of::()]; | ^^^^^^^^^^^^^^^^^ help: try: `a.padding` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:110:9 + --> tests/ui/deref_addrof.rs:106:9 | LL | (*&mut a.tup).1 = (); | ^^^^^^^^^^^^^ help: try: `a.tup` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:112:10 + --> tests/ui/deref_addrof.rs:108:10 | LL | **&mut a.prim = 0; | ^^^^^^^^^^^^ help: try: `a.prim` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:115:9 + --> tests/ui/deref_addrof.rs:111:9 | LL | (*&mut a.data).num = 42; | ^^^^^^^^^^^^^^ help: try: `(*a.data)` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:117:9 + --> tests/ui/deref_addrof.rs:113:9 | LL | (*&mut a.indirect.md)[3] = 1; | ^^^^^^^^^^^^^^^^^^^^^ help: try: `(*a.indirect.md)` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:119:9 + --> tests/ui/deref_addrof.rs:115:9 | LL | (*&mut a.indirect_arr[1].md)[3] = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(*a.indirect_arr[1].md)` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:121:9 + --> tests/ui/deref_addrof.rs:117:9 | LL | (*&mut a.indirect_ref.md)[3] = 1; | ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try: `(*a.indirect_ref.md)` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:125:10 + --> tests/ui/deref_addrof.rs:121:10 | LL | **&raw mut a.prim = 0; | ^^^^^^^^^^^^^^^^ help: try: `a.prim` error: immediately dereferencing a reference - --> tests/ui/deref_addrof.rs:127:9 + --> tests/ui/deref_addrof.rs:123:9 | LL | (*&raw mut a.data).num = 42; | ^^^^^^^^^^^^^^^^^^ help: try: `(*a.data)` -error: aborting due to 20 previous errors +error: aborting due to 18 previous errors