Skip to content
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.

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

The upcoming behavior of reinterpret for isbits types is dubious and not explicitly documented #52135

Closed
nsajko opened this issue Nov 12, 2023 · 2 comments

Comments

@nsajko
Copy link
Contributor

nsajko commented Nov 12, 2023

The upcoming behavior of reinterpret for "plain data" types, introduced in #47116, is to reinterpret only the non-padding bytes of an isbits value. This generalizes the functionality compared to the latest release, v1.9, where reintepreting non-primitive isbits types was not allowed. Personally I'd like for this to get reverted, so I'm listing some issues with it, but at the very least consider this as a doc issue.

  1. Why expand the reinterpret API, which is already unsafe and IMO ugly (e.g., the reinterpret-for-isbits and reinterpret-for-arrays could really have been two different functions)?

  2. The behavior depends on the host endianness, and encourages user code whose correctness depends on the host-endianness. While little-endian is currently completely dominant, this expansion of the reinterpret API seems like an unecessary and subtle footgun for the Julia ecosystem, as it becomes a trap when some big-endian architecture becomes relevant again. Encouraging host-endianness-dependent behavior is especially weird given the "correctness bugs" scrutiny that Julia is subject to.

  3. The padding-handling for isbits-reinterpret types is inconsistent with the array-reinterpret behavior. So, this works for a tuple, but fails for an array of such tuples:

julia> reinterpret(Tuple{UInt16,UInt8}, (0x1, 0x0203))
(0x0301, 0x02)

julia> reinterpret(Tuple{UInt16,UInt8}, [(0x1, 0x0203)])
1-element reinterpret(Tuple{UInt16, UInt8}, ::Vector{Tuple{UInt8, UInt16}}):
Error showing value of type Base.ReinterpretArray{Tuple{UInt16, UInt8}, 1, Tuple{UInt8, UInt16}, Vector{Tuple{UInt8, UInt16}}, false}:
ERROR: Padding of type Tuple{UInt16, UInt8} is not compatible with type Tuple{UInt8, UInt16}.
  1. The new behavior is not documented, except implicitly, through a usage example. While Round-trip reintrepretation of all bits types #47116 tried to document the new behavior, the subsequent Merge new reinterpret with essentials.jl reinterpret #50367 then removed that doc string.
@BioTurboNick
Copy link
Contributor

BioTurboNick commented Nov 13, 2023

  1. Assuming they should be separated, that would have to wait until 2.0, wouldn't it? "Unsafe" (though it's not unsafe in the same way that pointer manipulation or out of bounds array accesses are) can also be very useful. It would be helpful if you could provide a concrete alternative rather than just seeking to revert it.
  2. Could you please clarify where you see a change in behavior in the API based on endianness? Or are you strictly talking about how someone could use it in a way that introduces a dependence in their code on endianness? (And is this not already an issue with array reinterpretation? So it would not be introducing a new problem to be aware of when using it.)
  3. The array behavior could be loosened, and then it would be consistent.
  4. The docstring could be restored.

@BioTurboNick
Copy link
Contributor

BioTurboNick commented Nov 13, 2023

Correction on 3 - I see that ReinterpretArray's version would be hard to make consistent, as it stacks padding in-place. Both concepts IMO make sense for their own types. But perhaps in 2.0 they should be separated, and for now the documentation could be clarified on the differences.

vtjnash pushed a commit that referenced this issue Feb 13, 2024
As suggested in #52135, I've added some to the `reinterpret` docs.
@nsajko nsajko closed this as not planned Won't fix, can't repro, duplicate, stale Nov 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants