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.

See #15529 for more details.

Thank you for making Clippy better!

We're collecting our changelog from pull request descriptions.
If your PR only includes internal changes, you can just write
changelog: none. Otherwise, please write a short comment
explaining your change.

It's also helpful for us that the lint name is put within backticks (` `),
and then encapsulated by square brackets ([]), for example:

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

If your PR fixes an issue, you can add fixes #issue_number into this
PR description. This way the issue will be automatically closed when
your PR is merged.

If you added a new lint, here's a checklist for things that will be
checked during review or continuous integration.

Followed lint naming conventions
Added passing UI tests (including committed .stderr file)
cargo test passes locally
Executed cargo dev update_lints
Added lint documentation
Run cargo dev fmt

Note that you can skip the above if you are just opening a WIP PR in
order to get feedback.

Delete this line and everything above before opening your PR.

Note that we are currently not taking in new PRs that add new lints. We are in a
feature freeze. Check out the book for more information. If you open a
feature-adding pull request, its review will be delayed.

Please write a short comment explaining your change (or "none" for internal only changes)

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

Summary Notes

Feature-freeze by github-actions[bot]

Managed by @rustbot—see help for details

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

Lintcheck changes for ec9f7c0

Lint	Added	Removed	Changed
`clippy::missing_trait_methods`	0	0	165

This comment will be updated if you push new changes

rustbot · 2025-09-16T15:09:55Z

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.

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

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.

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

clippy_lints/src/volatile_composites.rs

+            // Look for method calls to `write_volatile`/`read_volatile`, which
+            // apply to both raw pointers and std::ptr::NonNull.
+            ExprKind::MethodCall(name, self_arg, _, _)
+                if matches!(name.ident.name, sym::read_volatile | sym::write_volatile) =>


@samueltardieu By the way, I don't really understand how (or if) this works - I have a test to validate it and it seems to work as expected. Does this really only match the real read/write_volatile functions or could it match anything named read_volatile?

Likewise below for ExprKind::Call to the std/core::ptr::read/write_volatile functions.

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

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

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

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

Implement volatile_composites lint

fc17293

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

Address review comments

ec9f7c0

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

	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

Are you sure you want to change the base?

Implement volatile_composites lint #15686

Conversation

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

Uh oh!

Summary Notes

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.

rustbot commented Sep 16, 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!

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!

Choose a reason for hiding this comment

Uh oh!

jsgf commented Sep 20, 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