Skip to content

Remove Copy supertrait from VaList #160221

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
rust-bors merged 1 commit into from
Jul 31, 2026
Merged

Remove Copy supertrait from VaList #160221

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion library/core/src/ffi/va_list.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -313,7 +313,7 @@ const impl<'f> Drop for VaList<'f> {
// meantime.
#[lang = "va_arg_safe"]
#[stable(feature = "c_variadic", since = "CURRENT_RUSTC_VERSION")]
pub impl(self) unsafe trait VaArgSafe: Copy {}
pub impl(self) unsafe trait VaArgSafe {}

crate::cfg_select! {
any(target_arch = "avr", target_arch = "msp430") => {
Expand Down Expand Up @@ -458,6 +458,10 @@ impl<'f> VaList<'f> {
/// - The actual type of the argument `U` is compatible with `T` (as defined below).
/// - If `U` and `T` are both integer types, then the value passed by the caller must be
/// representable in both types.
/// - If `T` is not [`Copy`], then it must not have already been read using `next_arg`
/// on a [`clone`][VaList::clone]d copy of this `VaList`.
/// (Currently, all types implementing [`VaArgSafe`] also implement [`Copy`],
/// but this may change in the future.)
Comment on lines +461 to +464

@tgross35 tgross35 Jul 30, 2026 •
edited by rustbot
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Miri has been enforcing the other conditions, does it already catch this?

Cc @rust-lang/miri

View changes since the review

@Jules-Bertholet Jules-Bertholet Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wouldn't expect Miri to directly enforce this, as it is library UB and not language UB. Similar to ptr::reading a valid instance of an arbitrary type

@tgross35 tgross35 Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, the UB would probably manifest upon drop and be caught.

@folkertdev folkertdev Jul 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This cannot be triggered because VaArgSafe is only implemented for Copy types currently.

I personally have little appetite for opening that up much further. There are some obscure numeric types that we will likely add in time (f16, f128, x87 f80, ppc f128, Complex<T>, bf16), but portability for some of those is already not great (e.g. clang and gcc diverge), and for anything beyond that I just don't think it is needed.

(I'm fine with this change though, if we believe this is a better way to handle it)

@Jules-Bertholet Jules-Bertholet Jul 30, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My personal preference would be the exact opposite, have impl<T> VaArgSafe for T {} and rely on lints to flag incorrect usage instead. But that is a discussion for another day

@nia-e nia-e Jul 30, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think cutting the user-visible behaviour is good, so i concur fully with removing the copy bound esp if it's a change we can undo if needed in the future & makes the stabilised api surface smaller. i would certainly raise more eyebrows at a blanket impl of the likes suggested but that's offtopic ^^

///
/// Types `T` and `U` are compatible when:
///
Expand Down
Loading