Skip to content

Extend NumBuffer API to allow casting - #160514

Open
GuillaumeGomez wants to merge 2 commits into
rust-lang:mainfrom
GuillaumeGomez:numbuffer-casts
Open

Extend NumBuffer API to allow casting#160514
GuillaumeGomez wants to merge 2 commits into
rust-lang:mainfrom
GuillaumeGomez:numbuffer-casts

Conversation

@GuillaumeGomez

@GuillaumeGomez GuillaumeGomez commented Aug 4, 2026

Copy link
Copy Markdown
Member

Fixes #159341.

Follow-up of #138215.

New approach to extend NumBuffer API as discussed here. First attempt was #143636.

With this approach, NumBuffer now has a second generic argument. Not sure if it's ok considering it's stable. But it wouldn't break existing code as the second generic argument has a default value. I also split the API in two parts so we can have some conversions done at compile-time.

I added a ui test too to show what the compiler error looks like for the const conversion.

r? @Amanieu

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 4, 2026
@rust-log-analyzer

This comment has been minimized.

@Amanieu Amanieu added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Aug 4, 2026
Comment thread library/core/src/fmt/num_buffer.rs Outdated
#[stable(feature = "int_format_into", since = "1.98.0")]
pub struct NumBuffer<T: NumBufferTrait> {
pub(crate) buf: T::Buf,
pub struct NumBuffer<T, B = <T as NumBufferTrait>::Buf> {

@hanna-kruppe hanna-kruppe Aug 4, 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.

If this needs a new parameter, it must be feature-gated so it's not insta-stable, like for example Vec's allocator parameters:

pub struct Vec<T, #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator = Global> {

But the extra parameter makes no sense to me.

  1. Without changing the format_into methods as well, you can't even use NumBuffer<T, B> for B != T::Buf.
  2. It's extremely awkward extra API surface, both there being an extra parameter at all, and specifically it being a random array type.
  3. I don't see a reason why it's needed. It should be possible to cast &mut NumBuffer<T> -> &mut NumBuffer<U> as long as the buffer for T is at least as large as the one for U.

Please tell me what I'm missing.

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What would the type buf field look like without an extra generic? Maybe I missed something obvious so if you have a better idea which doesn't require adding a new generic, I'm VERY interested.

@hanna-kruppe hanna-kruppe Aug 4, 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.

The field would be the same as it is today. If you have a &mut NumBuffer<i16> pointing to a 6-byte buffer, and cast that pointer to &mut NumBuffer<u8> that points to a 3-byte buffer, that should just work, it's just not using parts of the buffer it doesn't need.

@GuillaumeGomez GuillaumeGomez Aug 4, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, but from there, how do you upcast to u16 or i16? The buffer is big enough, but how do you know that?

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.

Why would you need to cast in that direction? If you want to remember the total length of the buffer, then don't forget the original length yet. You can instead shorten the buffer directly to the respective integer type at every format_into call site. In particular that seems like it's exactly what the use case in #159341 needs.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm really unhappy with the feature we came up with. Casting every time we want to use format_into on a different integer is just super weird. Making format_into accept any integer as long as the buffer can hold its string representation seemed like a much better approach.

So anyway, no bidirectional cast, that simplifies things quite a lot (yes, a transmute works perfectly fine in this case).

@rust-log-analyzer

This comment has been minimized.

);
}
// SAFETY: The target `NumBuffer` buffer is not bigger so this conversion is ok.
unsafe { core::mem::transmute::<&mut NumBuffer<T>, &mut NumBuffer<U>>(self) }

@programmerjake programmerjake Aug 4, 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.

for the transmute to be valid, NumBuffer needs to be repr(transparent) or some other well-defined repr, repr(Rust) isn't really. Also, NumBufferTrait should be unsafe and require Buf to be an array of MaybeUninit<u8>.

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good point! Gonna add the repr(transparent).

It's a conversion between NumBuffer, not part of NumBufferTrait, so not sure there is any benefit in enforcing Buf to be an array of MaybeUninit<u8>.

@programmerjake programmerjake Aug 4, 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.

the transmute's safety depends on Buf being a transmutable type that behaves like an array of bytes, so unless the trait is unsafe, third party code can do:

impl NumBufferTrait for MyType1 {
    type Buf = [MaybeUninit<u8>; 8];
}

impl NumBufTrait for MyType2 {
    type Buf = Box<[u8; 1]>;
}

and then use that transmute to convert unsoundly from one to the other which allows reading/writing to nearly arbitrary addresses from safe code.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

NumBufferTrait is not implementable outside of core for now, but that's a good concern in case we allow it in the future (which would be nice, pattern type incoming!).

How would you make the trait work with the Buf type being different (although still an array) for all integers (and eventually floats at some point?). The size of the array is different for each integer after all.

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.

oh, I was mostly just saying that we'd require the implementer via. unsafe to have Buf be [MaybeUninit<u8>; N] for some N.

in the future we could migrate to using type-level constants (previously spelled type const but I think that's been changed):

pub trait NumBufferTrait {
    type const BUF_SIZE: usize;
}

#[repr(transparent)]
pub struct NumBuffer<T: NumBufferTrait> {
    buf: [MaybeUninit<u8>; T::BUF_SIZE],
    _phantom: PhantomData<fn(T)>,
}

@rust-log-analyzer

This comment has been minimized.

Comment on lines +113 to +116
assert!(
core::mem::size_of::<T::Buf>() >= core::mem::size_of::<U::Buf>(),
"target `NumBuffer` size must smaller or equal to source `NumBuffer` size"
);

@qaijuang qaijuang Aug 5, 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.

Do you mean?

Suggested change
assert!(
core::mem::size_of::<T::Buf>() >= core::mem::size_of::<U::Buf>(),
"target `NumBuffer` size must smaller or equal to source `NumBuffer` size"
);
assert!(
core::mem::size_of::<T::Buf>() >= core::mem::size_of::<U::Buf>(),
"target `NumBuffer` size must be smaller or equal to source `NumBuffer` size"
);

View changes since the review

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Yes, good catch!

@rust-log-analyzer

Copy link
Copy Markdown
Collaborator

The job pr-check-2 failed! Check out the build log: (web) (plain enhanced) (plain)

Click to see the possible cause of the failure (guessed by this bot)
---- library/core/src/fmt/num_buffer.rs - fmt::num_buffer::NumBuffer<T>::cast_into (line 87) stdout ----
error[E0658]: use of unstable library feature `fmt_internals`
  --> library/core/src/fmt/num_buffer.rs:93:34
   |
93 | assert_eq!(16u16.format_into(buf.cast_into::<u16>()), "16");
   |                                  ^^^^^^^^^
   |
   = help: add `#![feature(fmt_internals)]` to the crate attributes to enable
   = note: this compiler was built on 2026-08-05; consider upgrading it if it is out of date

error[E0658]: use of unstable library feature `fmt_internals`
  --> library/core/src/fmt/num_buffer.rs:94:37
   |
94 | assert_eq!(u16::MAX.format_into(buf.cast_into::<u16>()), u16::MAX.to_string());
   |                                     ^^^^^^^^^
   |
   = help: add `#![feature(fmt_internals)]` to the crate attributes to enable
   = note: this compiler was built on 2026-08-05; consider upgrading it if it is out of date

error[E0658]: use of unstable library feature `fmt_internals`
  --> library/core/src/fmt/num_buffer.rs:96:35
   |
96 | assert_eq!(-16i16.format_into(buf.cast_into::<i16>()), "-16");
   |                                   ^^^^^^^^^
   |
   = help: add `#![feature(fmt_internals)]` to the crate attributes to enable
   = note: this compiler was built on 2026-08-05; consider upgrading it if it is out of date

error[E0600]: cannot apply unary operator `-` to type `&str`
  --> library/core/src/fmt/num_buffer.rs:96:12
   |
96 | assert_eq!(-16i16.format_into(buf.cast_into::<i16>()), "-16");
   |            ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ cannot apply unary operator `-`

error[E0658]: use of unstable library feature `fmt_internals`
  --> library/core/src/fmt/num_buffer.rs:97:37
   |
97 | assert_eq!(i16::MIN.format_into(buf.cast_into::<i16>()), i16::MIN.to_string());
   |                                     ^^^^^^^^^
   |
   = help: add `#![feature(fmt_internals)]` to the crate attributes to enable
   = note: this compiler was built on 2026-08-05; consider upgrading it if it is out of date

@rosymati

rosymati commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Not super happy that we ended up with just a hidden transmute. I don't feel like cast_into is better or adds more value than the earlier approach where format_into accepts any integer whose representation fits the buffer; if anything it seems more confusing, and we lose the possibility of upcasting.

It's a bit disappointing to see this gated on basically "what's the use case?", because std doesn't know these use cases, and if it has to expose a limited API then I'm not sure what std is buying users over an external crate.

"just don't forget the original length" is very handwavy, I'm afraid. What happens if someone eventually needs upcasting logic? The same "nobody will need this" argument was made in the original implementation, and it's why we're here.

I'm happy to have my use case unblocked, but it feels weird that we're refusing to provide the version that boils down to a const assertion, and instead providing a nicely wrapped transmute.

@rosymati

rosymati commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

To further add to my previous comment, I've been eyeing just vendoring the itoa/#143636 version, which would make this irrelevant for this use case. Obviously std is free to proceed how it believes, but if my options are "vendor a simple reusable buffer" vs "write a bunch of hidden transmutes" just because std wasn't happy with the API, then I will choose the former.

Of course my use case proves that there's reason to have this, and I'd be happy if we could instead reopen a discussion for the previous implementation, which feels really clean to me and I fear was dismissed on the basis of "we don't need this".

I hope a discussion can happen, but as the one who opened #159341 and provided the use case, I want to be clear that I think this solution is worse than just not doing anything and leaving me with a custom/vendored implementation, or implementing the previous PR

@GuillaumeGomez

Copy link
Copy Markdown
Member Author

Just to be clear: I'm also unhappy with the cast approach, and even further with the no upcast limitations that was asked. I also think that making format_into generic would be a much better approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

I-libs-api-nominated Nominated for discussion during a libs-api team meeting. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

NumBuffer<T> ties a buffer to one integer type; no reusable buffer for mixed-integer formatting

8 participants