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

samueltardieu · 2025-03-10T18:19:58Z

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

rustbot · 2025-03-10T18:20:03Z

rustbot has assigned @Alexendoo.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Alexendoo · 2025-03-11T14:24:42Z

tests/ui/deref_addrof.rs

+        // Lint, as `Copy` fields in unions are ok.
+        (*(&raw mut a.with_padding)) = [1; size_of::<DataWithPadding>()];
+        //~^ deref_addrof
+        *(*(&raw mut a.x)) = 0;
+        //~^ deref_addrof
+        (*(&raw mut a.y)).0 = 0;
+        //~^ deref_addrof


The error looks to be specific to ManuallyDrop rather than Copy, e.g. this is an error even though everything is Copy

#[derive(Copy, Clone)] struct Data { num: u64, } union U { data: ManuallyDrop<Data>, } fn x(mut a: U) { unsafe { a.data.num = 3; } }

*a.x = 0; doesn't hit it because it doesn't use DerefMut

https://github.com/rust-lang/rust/blob/705421b52239d7393b4738764b192179d5c139c2/compiler/rustc_hir_typeck/src/place_op.rs#L311-L323

The error looks to be specific to ManuallyDrop rather than Copy, e.g. this is an error even though everything is Copy

Indeed. And it requires going through the parent exprs to check if a ManuallyDrop is mutably dereferenced. I have reused some code from reference.rs and shared it in clippy_utils::ty.

samueltardieu · 2025-03-31T16:52:16Z

@Alexendoo Anything missing on my side?

Alexendoo · 2025-04-20T18:27:12Z

clippy_lints/src/reference.rs

+/// Check if `expr` is an access to an union field which contains a dereferenced
+/// `ManuallyDrop<_>` entity.
+fn is_manually_drop_union_field(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    let typeck = cx.typeck_results();
+    if let ExprKind::Field(parent, _) = expr.kind
+        && typeck.expr_ty_adjusted(parent).is_union()
+    {
+        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
+}


Currently this lints

use std::mem::ManuallyDrop; #[derive(Clone, Copy)] struct B { md: ManuallyDrop<[u8; 4]> } union U { b: B, } fn x(mut u: U) { unsafe { (*&mut u.b.md)[3] = 1; // ~~~~~~ `expr` } }

Currently we only check if u.b is a union, but we need to keep going while we still see field/index expressions since it doesn't have to directly in the union

The parent iter I believe can be replaced with is_manually_drop([type of expr])

I'll check this, thanks for the review. @rustbot author

Done, the cases you mention, and some similar ones, are no longer linted.

However, I am not sure that

The parent iter I believe can be replaced with is_manually_drop([type of expr])

holds, as not all ManuallyDrop are equal here: **&mut a.prim = 0 is linted and fixed as *a.prim = 0, as there is an explicit deref left. However, (*&mut a.data).num = 42 is currently no longer linted, as a.data.num = 42 would be an implicit deref, but we could suggest to keep the explicit dereference instead, as in (*a.data).num = 42.

I'll think more about this.

This is not as simple as keeping the reference and removing the dereference in some case where the implicit dereference happens later in the expression:

(*&mut a.tup).0.num = 42;

should be rewritten as

(*a.tup.0).num = 42;

if we want to remove the &num. That would require moving the explicit dereference to the point where the implicit DerefMut would take place, so this case should not be linted.

I've added a new commit which reinstates those cases and suggests the proper fix. I may squash the two commits after the review.

One thing I have not checked is if it is really wise to keep linting in macros, as the lint is currently doing. If a macro is used with a ManuallyDrop field reached through a union, the lint may incorrectly suggest fixing the macro in cases where it works fine, which would generate an error in other cases. I would be in favor of disabling the lint in macros, even if we have false negatives, this would be better than false positives. Thoughts?

@rustbot ready

cc @RalfJung - is this a bug in the https://rust-lang.github.io/rfcs/2514-union-initialization-and-drop.html#potential-pitfalls-around-derefmut error?

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=cefd3d31f4c0b4163595f212d9b3ad98

Similar example adapted from the RFC but without any *

https://play.rust-lang.org/?version=stable&mode=debug&edition=2024&gist=6d1a15c1ec5f2eb6f60bb54ae31ecf05

It seems odd that this would error:

union U<T> { x:(), f: ManuallyDrop<T> } fn main() { let mut u : U<(Vec<i32>,)> = U { x: () }; let r = u; unsafe { r.f.0 = Vec::new() }; // uninitialized `Vec` being dropped }

and the 2nd example you gave would not.

But I don't think I had any involvement with that error, and from a soundness perspective everything is fine here -- there's an unsafe block and you wrote your code wrong, so you got UB. So I'm the wrong person to ask here, sorry; auto-(de)ref is far outside my realm.

I'd suggest to open a rustc issue clearly describing the problem; your example above isn't very self-explaining and it took me a bit of experimentation to even figure out what you were likely talking about.

Thanks for taking a look, I pinged since I saw you authored rust-lang/rust#75584 but I didn't catch the date being that long ago

I'll write up an issue for it

Ah, I guess I did write that code then, and entirely forgot about it since then.^^ But I was never very comfortable in rustc_typeck.

samueltardieu · 2025-05-06T22:09:25Z

Rebased

rustbot · 2025-05-06T22:10:39Z

Reminder, once the PR becomes ready for a review, use @rustbot ready.

This is necessary in order to use type analysis in later commits.

samueltardieu · 2025-05-16T15:20:32Z

Rebased

samueltardieu · 2025-06-18T04:31:07Z

Ping @Alexendoo

samueltardieu · 2025-07-28T12:26:01Z

This has been ready for months now.

r? clippy

blyxyas · 2025-07-31T12:07:05Z

I'll make reviewing this a priority. A review should be ready soon.

blyxyas

What an interesting bug, I have some questions and some refactorings! Btw, if the original lint handled macros correctly, is there a hard reason to aovid them now, or are you just safeguarding the lint for future bugs?

blyxyas · 2025-07-31T12:53:06Z

clippy_lints/src/reference.rs

+    let typeck = cx.typeck_results();
+    if is_reached_through_union(cx, addrof_target) {


We shouldn't query typeck results if is_reached_through_union can return false.

blyxyas · 2025-07-31T12:55:43Z

clippy_lints/src/reference.rs

+        for (idx, id) in std::iter::once(expr_id)
+            .chain(cx.tcx.hir_parent_id_iter(expr_id))


Looking at hir_parent_id_iter, I think that it already includes the current expr_id in the traversal, doesn't it?

I don't think so: the .next() ends with

self.current_id = parent_id; Some(parent_id)

So it never yields current_id (which is the initialization value).

blyxyas · 2025-07-31T12:57:41Z

clippy_lints/src/reference.rs

+            .chain(cx.tcx.hir_parent_id_iter(expr_id))
+            .enumerate()
+        {
+            if let Node::Expr(expr) = cx.tcx.hir_node(id) {


Instead of getting the node, we can use hir_parent_iter directly and avoid this step. As .map is lazy and won't compute not-traversed nodes, it won't have be a performance impact.

~~I'll make the change, but I'll still have to use hir_node() for the initial node.~~

In fact, hir_parent_iter() will only complicate the code, as I need to call hir_node() for the initial node, and hir_parent_iter() is not more efficient as it calls hir_node() on any obtained HirId.

blyxyas · 2025-07-31T13:10:50Z

clippy_lints/src/reference.rs

+            let 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,
+            };


hir_with_applicability is pretty costly, this can be refactored to avoid computing the suggestion in the return case.

What about something like this?

Suggested change

let 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,

};

// 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 manually_drop_through_union = is_manually_drop_through_union(cx, e.hir_id, addrof_target);

let sugg = if manually_drop_through_union == ManuallyDropThoughUnion::Indirect {

return;

} else {

let raw_sugg = Sugg::hir_with_applicability(cx, addrof_target, "_", &mut applicability);

if manually_drop_through_union == ManuallyDropThroughUnion::Directly {

raw_sugg.deref()

}

raw_sugg

}

I'll use a closure to avoid early evaluation, it allows me to keep the match which I find clear.

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<Data>, } 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; } ```

samueltardieu · 2025-07-31T13:56:13Z

What an interesting bug, I have some questions and some refactorings! Btw, if the original lint handled macros correctly, is there a hard reason to aovid them now, or are you just safeguarding the lint for future bugs?

I don't think the original lint handled macros correctly in presence of ManuallyDrop reached through unions (as it didn't handle any code correctly in such a case, macro or not), and it is hard to make it correct with macros once it is fixed for non-macro code. There is a description in the last commit for why we don't lint macros, I'll copy it here:

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.

use std::mem::ManuallyDrop;

#[derive(Copy, Clone)]
struct Data {
    num: u64,
}

union Union {
    data: ManuallyDrop<Data>,
}

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;
}

rustbot assigned Alexendoo Mar 10, 2025

rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Mar 10, 2025

samueltardieu force-pushed the push-xtnpwsvsssyx branch from cd1337d to 263a25e Compare March 10, 2025 18:29

Alexendoo reviewed Mar 11, 2025

View reviewed changes

samueltardieu force-pushed the push-xtnpwsvsssyx branch from 263a25e to 18d0707 Compare March 11, 2025 18:08

samueltardieu changed the title ~~Do not suggest to use implicit DerefMut on non-Copy union fields~~ Do not suggest to use implicit DerefMut on ManuallyDrop union fields Mar 11, 2025

Alexendoo reviewed Apr 20, 2025

View reviewed changes

This comment has been minimized.

Sign in to view

samueltardieu force-pushed the push-xtnpwsvsssyx branch from 18d0707 to 89124a0 Compare May 6, 2025 22:08

rustbot removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label May 6, 2025

rustbot added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label May 6, 2025

samueltardieu force-pushed the push-xtnpwsvsssyx branch from 89124a0 to dde75ef Compare May 7, 2025 14:18

samueltardieu changed the title ~~Do not suggest to use implicit DerefMut on ManuallyDrop union fields~~ Do not suggest to use implicit DerefMut on ManuallyDrop reached through unions May 7, 2025

samueltardieu force-pushed the push-xtnpwsvsssyx branch from dde75ef to ca9e429 Compare May 7, 2025 14:50

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels May 7, 2025

samueltardieu force-pushed the push-xtnpwsvsssyx branch from e5aab3a to 24dd8a0 Compare May 7, 2025 21:15

This comment has been minimized.

Sign in to view

samueltardieu added 2 commits May 16, 2025 17:20

Move adjust_derefs_manually_drop into clippy_utils

3688b33

Convert deref_addrof lint to late lint

2cfbb05

This is necessary in order to use type analysis in later commits.

samueltardieu force-pushed the push-xtnpwsvsssyx branch from 24dd8a0 to d2786bf Compare May 16, 2025 15:20

samueltardieu requested a review from Alexendoo May 26, 2025 08:22

Alexendoo mentioned this pull request May 26, 2025

DerefMut auto-deref error for union fields sometimes doesn't trigger rust-lang/rust#141621

Open

rustbot assigned blyxyas and unassigned Alexendoo Jul 28, 2025

blyxyas requested changes Jul 31, 2025

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Jul 31, 2025

samueltardieu added 3 commits July 31, 2025 15:53

Do not use DerefMut on ManuallyDrop<_> reached through unions

27478be

Lint more cases when the explicit dereference can be kept in place

ff496ad

samueltardieu force-pushed the push-xtnpwsvsssyx branch from d2786bf to 7224dff Compare July 31, 2025 13:53

samueltardieu requested a review from blyxyas July 31, 2025 14:00

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) labels Jul 31, 2025

		let typeck = cx.typeck_results();
		if is_reached_through_union(cx, addrof_target) {

		for (idx, id) in std::iter::once(expr_id)
		.chain(cx.tcx.hir_parent_id_iter(expr_id))

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

Are you sure you want to change the base?

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

Conversation

samueltardieu commented Mar 10, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Mar 31, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RalfJung May 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

samueltardieu commented May 6, 2025

Uh oh!

rustbot commented May 6, 2025

Uh oh!

This comment has been minimized.

samueltardieu commented May 16, 2025

Uh oh!

samueltardieu commented Jun 18, 2025

Uh oh!

samueltardieu commented Jul 28, 2025

Uh oh!

blyxyas commented Jul 31, 2025

Uh oh!

blyxyas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu commented Jul 31, 2025

Uh oh!

Uh oh!

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

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

samueltardieu commented Mar 10, 2025 •

edited by rustbot

Loading

RalfJung May 26, 2025 •

edited

Loading

samueltardieu Jul 31, 2025 •

edited

Loading