Implement `volatile_composites` lint #15686

jsgf · 2025-09-16T15:03:48Z

Volatile reads and writes to non-primitive types are not well-defined, and can cause problems.

changelog: [volatile_composites]: Lint when read/write_volatile is used on composite types
(structs, arrays, etc) as their semantics are not well defined.

rustbot · 2025-09-16T15:03:52Z

r? @samueltardieu

rustbot has assigned @samueltardieu.
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

github-actions · 2025-09-16T15:03:55Z

No changes for 1d7c1af

samueltardieu

Thanks for the contribution. It is in a good shape already, but I have some questions.

Do not hesitate to add extra tests, especially if my comments were wrong, in order to distinguish between the cases you thought of when the code was more complete than what I suggest.

Also, I would like to have more doc comments on functions, so that it is easier to read.

View changes since this review

samueltardieu · 2025-09-18T10:49:27Z

clippy_lints/src/volatile_composites.rs

+    ///     }
+    /// }
+    /// ```
+    #[clippy::version = "1.91.0"]


Suggested change

#[clippy::version = "1.91.0"]

#[clippy::version = "1.92.0"]

samueltardieu · 2025-09-18T10:51:50Z

clippy_lints/src/volatile_composites.rs

+// NonNull::{read_volatile,write_volatile}
+
+// primitive type:
+// unit, [iu]{8,16,32,64,128?}, f{32,64}, thin pointer, usize, isize, bool, char


What about f16 and f128?

What are those comments anyway? Were those notes you took during development that need to be removed?

Yes, I'll clean these up. In practice this set of types is whatever Ty::is_primitive returns true for, which includes f16 and f128.

samueltardieu · 2025-09-18T10:52:32Z

clippy_lints/src/volatile_composites.rs

+// Zero-sized types are intrinsically safe to use volatile on since they won't
+// actually generate *any* loads or stores. But this is also used to skip zero
+// fields of #[repr(transparent)] structures.


Can you use doc comments to comment out functions? This makes it easier to navigate when later editing the code, as IDEs will render them properly.

samueltardieu · 2025-09-18T10:54:39Z

clippy_lints/src/volatile_composites.rs

+// actually generate *any* loads or stores. But this is also used to skip zero
+// fields of #[repr(transparent)] structures.
+fn is_zero_sized_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    if let Ok(ty) = cx.tcx.try_normalize_erasing_regions(cx.typing_env(), ty)


Doesn't layout_of() already normalize the type? Are there cases that would not be covered by the following?

cx.tcx .layout_of(cx.typing_env().as_query_input(ty)) .is_ok_and(|layout| layout.layout.is_zst())

If there are, it would be great to add them as tests.

samueltardieu · 2025-09-18T11:04:49Z

clippy_lints/src/volatile_composites.rs

+    if let ty::RawPtr(_inner, _) = ty.kind() {
+        ty.pointee_metadata_ty_or_projection(cx.tcx).is_unit()
+    } else {
+        false
+    }


Are there cases of real () metadata? Won't the following be equivalent?

matches!(ty.kind(), ty::RawPtr(inner, _) if inner.has_trivial_sizedness(cx.tcx, SizedTraitKind::Sized))

samueltardieu · 2025-09-18T11:06:50Z

clippy_lints/src/volatile_composites.rs

+    if let ty::Adt(adt_def, _args) = ty.kind()
+        && adt_def.is_enum()
+        && adt_def.repr().inhibit_struct_field_reordering()
+    {
+        adt_def.is_payloadfree()
+    } else {
+        false
+    }


Nit: I prefer the shorter equivalent form but it is a matter of taste.

Suggested change

if let ty::Adt(adt_def, _args) = ty.kind()

&& adt_def.is_enum()

&& adt_def.repr().inhibit_struct_field_reordering()

{

adt_def.is_payloadfree()

} else {

false

}

ty.ty_adt_def().is_some_and(|adt_def| {

adt_def.is_enum() && adt_def.repr().inhibit_struct_field_reordering() && adt_def.is_payloadfree()

})

samueltardieu · 2025-09-18T11:15:54Z

clippy_lints/src/volatile_composites.rs

+
+// We can't know about a generic type, so just let it pass to avoid noise
+fn is_generic<'tcx>(_cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    ty.flags().intersects(ty::TypeFlags::HAS_PARAM)


ty.flags() will be removed at some point. Can't you use ty.has_param()? Or ty.has_non_region_param() if a generic lifetime wouldn't change the lint (along with a test with raw pointer on a type with a lifetime)?

Also, this function is so short that you could probably remove it and replace its call by its content.

samueltardieu · 2025-09-18T11:21:29Z

clippy_lints/src/volatile_composites.rs

+                    // Raw pointers
+                    ty::RawPtr(innerty, _) => report_volatile_safe(cx, expr, *innerty),
+                    // std::ptr::NonNull
+                    ty::Adt(adt_def, args) if cx.tcx.is_diagnostic_item(sym::NonNull, adt_def.did()) => {


Nit: we have a utility for this check in clippy_utils::ty:

Suggested change

ty::Adt(adt_def, args) if cx.tcx.is_diagnostic_item(sym::NonNull, adt_def.did()) => {

_ if is_type_diagnostic_item(cx, self_ty, sym::NonNull) => {

samueltardieu · 2025-09-18T11:31:19Z

clippy_lints/src/volatile_composites.rs

+                        cx.tcx.get_diagnostic_name(def_id),
+                        Some(sym::ptr_read_volatile | sym::ptr_write_volatile)
+                    )
+                    && let ty::RawPtr(ptrty, _) = cx.typeck_results().expr_ty(arg_ptr).kind()


Shouldn't you use expr_ty_adjusted() here? The lint currently doesn't catch:

let mut s = String::from("foo"); unsafe { std::ptr::write_volatile(&mut s, String::from("bar")); //~^ volatile_composites }

while it probably should.

Yes, good catch. I wasn't sure what an "adjustment" was for types.

jsgf · 2025-09-20T20:50:57Z

@samueltardieu Thanks very much for your feedback. I'll address your comments shortly. This is the first time I've delved into clippy's internals so I was drawing on existing examples and guesswork for a lot of this change, so your pointers are much appreciated.

clippy_lints/src/volatile_composites.rs

jsgf · 2025-09-20T21:52:58Z

I addressed review comments in a separate commit, but I can fold them if it all looks OK.

samueltardieu

This is starting to look good.

I'd like to see two more tests, they may be covered by other paths but it would be more explicit to me:

struct Empty;

        (&mut Empty as *mut Empty).write_volatile(Empty); // OK
        (0xdead as *mut Wrapper<Empty>).write_volatile(Wrapper((), Empty, ())); // OK

Also, regarding primitive types, it looks like this lint should cover non-atomic volatile operations on non-atomic primitive types. Recommending in the description to use primitive types might be misleading, and maybe the name of the lint should be something like non_atomic_volatile_operations or something like that.

View changes since this review

samueltardieu · 2025-09-21T06:18:16Z

clippy_lints/src/volatile_composites.rs

+    cx.tcx
+        .layout_of(cx.typing_env().as_query_input(ty))
+        .is_ok_and(|layout| layout.is_zst())


You can probably further simplify this:

Suggested change

cx.tcx

.layout_of(cx.typing_env().as_query_input(ty))

.is_ok_and(|layout| layout.is_zst())

cx.layout_of(ty).is_ok_and(|layout| layout.is_zst())

samueltardieu · 2025-09-21T06:48:53Z

clippy_lints/src/volatile_composites.rs

+
+/// Top-level predicate for whether a type is volatile-safe or not.
+fn is_volatile_safe_ty<'tcx>(cx: &LateContext<'tcx>, ty: Ty<'tcx>) -> bool {
+    ty.is_primitive()


Are all primitive types volatile safe? I would think that there is no chance that 32-bit architectures support atomic read and write to u128 data for example.

I would avoid using the term "atomic" with respect to volatile - volatile does not make any atomic guarantees (certainly not in the std::sync::atomic sense). Volatile is a bridge to memory outside the Rust abstract machine memory model, so at the Rust language level it has pretty weak and machine-specific guarantees. So there's no guarantee that a volatile operation on, say, a u32 will be a single memory load/store, except as a machine-specific implementation detail (though it would be very surprising if it weren't true on any >=32 bit cpu). So the fact that u128 probably won't turn into a single load/store is fine.

The specific problem this lint is intended to highlight is that there's no consensus about what this means for a composite type like a struct. Some people believe that write_volatile should treat it as a single entity, and so the compiler is OK to write multiple fields in a single store if that makes sense. Others believe that it should treat each field as a separate entity which is stored as an independent object. The documentation is ambiguous about this, and makes no guarantees either way, and can change from compiler (or llvm) version to version. So code which depends on one or the other interpretation may work today but stop tomorrow.

Even as it is, if the target address has normal memory semantics (eg a memory region shared with a different OS process or CPU complex), it may not make a difference one way or the other so this lint wouldn't apply regardless of what type is used.

We could consider a separate lint which flags volatile read/writes for non-composite types which are not accessed with a single load/store. But that would be highly target-specific and ultimately depends on the backend's (ie typically llvm, but could be cranelift or gcc) precise implementation, which could change from version to version, and also depend on opt-level and other flags. So that sounds like it would be hard to get right.

Ultimately volatile read/write are already unsafe so the user already bears a high burden of proof to make it safe. This is just one aspect of it, and this lint is intended to help them get it right, but obviously can't be aware of all the factors in play.

jsgf · 2025-09-21T23:43:07Z

I simplified that ZST predicate and added a test with Empty.

Also, regarding primitive types, it looks like this lint should cover non-atomic volatile operations on non-atomic primitive types. Recommending in the description to use primitive types might be misleading, and maybe the name of the lint should be something like non_atomic_volatile_operations or something like that.

See my reply to your other comment, though the TL;DR is that I think we should avoid the use of "atomic" in this context, and whether a primitive volatile read/write results in a single load/store is a separate (though related) issue, which is much harder to check for.

jsgf · 2025-09-21T23:48:48Z

(Also remove use of "atomic" in comment in exactly the sense I say we shouldn't be using it.)

jsgf · 2025-09-26T20:53:38Z

@samueltardieu ping?

samueltardieu

Can you please squash the changes?

I hadn't noticed that they were ready to be reviewed because the S-waiting-on-author label was set. You can comment with @rustbot ready or request a review using GitHub's interface to set the S-waiting-on-review label instead.

View changes since this review

samueltardieu · 2025-09-27T11:43:59Z

I've opened a FCP thread on Zulip.

@rustbot label S-final-comment-period -Swaiting-on-author

Volatile reads and writes to non-primitive types are not well-defined, and can cause problems. See rust-lang#15529 for more details.

rustbot · 2025-09-27T23:22:13Z

This PR was rebased onto a different master commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

jsgf · 2025-09-27T23:22:19Z

Squashed and rebased to current master.

rustbot assigned samueltardieu Sep 16, 2025

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

rustbot added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work A-lint Area: New lints and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties labels Sep 16, 2025

This comment has been minimized.

Sign in to view

jsgf force-pushed the volatile_composites branch from 7b7b0ee to 14ae176 Compare September 16, 2025 15:09

This comment has been minimized.

Sign in to view

jsgf force-pushed the volatile_composites branch 3 times, most recently from ec5febf to fc17293 Compare September 16, 2025 15:42

samueltardieu added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties and removed S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work labels Sep 17, 2025

samueltardieu requested changes Sep 18, 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 Sep 18, 2025

jsgf commented Sep 20, 2025

View reviewed changes

clippy_lints/src/volatile_composites.rs Show resolved Hide resolved

jsgf force-pushed the volatile_composites branch from 340ae34 to ec9f7c0 Compare September 20, 2025 23:47

samueltardieu requested changes Sep 21, 2025

View reviewed changes

jsgf force-pushed the volatile_composites branch 2 times, most recently from 139eeda to 9e95933 Compare September 21, 2025 23:20

jsgf force-pushed the volatile_composites branch from 9e95933 to 5a1391d Compare September 21, 2025 23:48

samueltardieu approved these changes Sep 27, 2025

View reviewed changes

rustbot added the S-final-comment-period Status: final comment period it will be merged unless new objections are raised (~1 week) label Sep 27, 2025

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

Implement volatile_composites lint

1d7c1af

Volatile reads and writes to non-primitive types are not well-defined, and can cause problems. See rust-lang#15529 for more details.

jsgf force-pushed the volatile_composites branch from 5a1391d to 1d7c1af Compare September 27, 2025 23:22

samueltardieu added this pull request to the merge queue Oct 6, 2025

Merged via the queue into rust-lang:master with commit 2c71638 Oct 6, 2025
11 checks passed

	ty::Adt(adt_def, args) if cx.tcx.is_diagnostic_item(sym::NonNull, adt_def.did()) => {
	_ if is_type_diagnostic_item(cx, self_ty, sym::NonNull) => {

Implement volatile_composites lint #15686

Implement volatile_composites lint #15686

Uh oh!

Conversation

jsgf commented Sep 16, 2025 • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Sep 16, 2025

Uh oh!

github-actions bot commented Sep 16, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

samueltardieu left a comment • edited by rustbot 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

samueltardieu Sep 18, 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 Sep 18, 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!

Choose a reason for hiding this comment

Uh oh!

jsgf commented Sep 20, 2025

Uh oh!

Uh oh!

jsgf commented Sep 20, 2025

Uh oh!

samueltardieu left a comment • edited by rustbot 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!

jsgf Sep 21, 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!

jsgf commented Sep 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsgf commented Sep 21, 2025

Uh oh!

jsgf commented Sep 26, 2025

Uh oh!

samueltardieu left a comment • edited by rustbot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

rustbot commented Sep 27, 2025

Uh oh!

jsgf commented Sep 27, 2025

Uh oh!

Uh oh!

Implement `volatile_composites` lint #15686

Implement `volatile_composites` lint #15686

jsgf commented Sep 16, 2025 •

edited by rustbot

Loading

github-actions bot commented Sep 16, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu Sep 18, 2025 •

edited

Loading

samueltardieu Sep 18, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

jsgf Sep 21, 2025 •

edited

Loading

jsgf commented Sep 21, 2025 •

edited

Loading

samueltardieu left a comment •

edited by rustbot

Loading

samueltardieu commented Sep 27, 2025 •

edited

Loading