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

Make the inner type wrapped in Unalign public #836

Open
ChayimFriedman2 opened this issue Feb 3, 2024 · 3 comments
Open

Make the inner type wrapped in Unalign public #836

ChayimFriedman2 opened this issue Feb 3, 2024 · 3 comments

Comments

@ChayimFriedman2
Copy link

This is not exactly a customer request since I'm not using this crate currently, but I was looking on the docs and I wondered why won't you make the inner field of Unalign public?

Usually when prefer to make newtype's field private, but in this case making it public has a big advantage: we cannot make a reference to it and so we cannot expose it in a non-copying way, but if it is public the compiler allows reads/writes to fields of it as long as we never take a reference and only read/write directly. This can make working with this type more comfortable.

jswrenn added a commit that referenced this issue Mar 27, 2024
This allows for directly writing to the field safely. Since Rust
ensures that unsafe uses of this field require `unsafe`, we should
additionally consider deprecating many of the methods of `Unalign`.

Fixes #836
jswrenn added a commit that referenced this issue Mar 27, 2024
This allows for directly writing to the field safely. Since Rust
ensures that unsafe uses of this field require `unsafe`, we should
additionally consider deprecating many of the methods of `Unalign`.

Fixes #836
@joshlf
Copy link
Member

joshlf commented Mar 27, 2024

I'm concerned that this will back us into a corner. In particular, there's a chance that we may want to support unsized Unalign types in the future (i.e., Unalign<T: ?Sized>) (#209). We probably can't do that without some heavy type machinery that might require changing the fields of Unalign to "manually reconstruct" the layout we're trying to build. (This comes from various limitations regarding repr(packed) with unsized types.)

I agree that there's a minor ergonomic benefit with making the inner field public, but are there any patterns that can't be expressed with our existing API and methods? If it's just an ergonomic thing, I'm inclined to not do this right now, but if it's an expressibility issue, then that's a different story.

@joshlf
Copy link
Member

joshlf commented Mar 27, 2024

Note that #1076 implements this. I've closed it, but we can re-open if we decide to move forward with this.

@joshlf
Copy link
Member

joshlf commented Nov 25, 2024

Merging #1860 into this

Original text by @kupiakos:


This enables full field reference projection for fields that are sufficiently aligned and field read/write access for all fields and subfields. This must be done through direct struct field access for the compiler to accept it.

Sample use case

Say the input buffer you receive has bytes in the format of BigStruct, and our function reads specific fields out of those bytes based on a struct layout defined by bindgen. There are too many unread fields to reasonably copy it. However, the input buffer may be unaligned. So, you first access the prefix as a &Unalign<BigStruct>.

In order to access specific unaligned fields safely, you need one of the following:

  • Field projection
  • To copy the whole struct
  • To use unsafe
  • Have field access to the contents of the Unalign.

Currently, the workaround is to declare your own repr(packed) newtype that has an accessible inner value. But honestly, I should be able to use the built-in Unalign newtype for this. Rust will guard against an accidental reference to an unaligned field.


It might be worth gating the inner pub field based on Rust version since the lint producing a hard error for referencing an underaligned field was introduced in 1.69.

Additionally, if we add T: ?Sized to Unalign, then only in 1.76 was this lint fixed for unsized fields.

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

Successfully merging a pull request may close this issue.

2 participants