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 FooBits type generated by #[derive(CheckedBitPattern)] should implement NoUninit if Self is NoUninit #215

Open
nerditation opened this issue Dec 12, 2023 · 3 comments

Comments

@nerditation
Copy link

summary

currently, the #[derive(CheckedBitPattern)] on a struct Foo expands to code like this:

#[repr(C)]
#[derive(Clone, Copy, ::bytemuck::AnyBitPattern)]
#[cfg_attr(not(target_arch = "spirv"), derive(Debug))]
pub struct FooBits {
	field: <Field as ::bytemuck::CheckedBitPattern>::Bits,
}
unsafe impl ::bytemuck::CheckedBitPattern for Foo {
	type Bits = FooBits;
	#[inline]
	#[allow(clippy::double_comparisons)]
	fn is_valid_bit_pattern(bits: &FooBits) -> bool {
		<Field as ::bytemuck::CheckedBitPattern>::is_valid_bit_pattern(&{ bits.field})
			&& true
	}
}

however, the generated FooBits type cannot be used as a buffer which will be later read into, because bytes_of_mut requires the type to be NoUninit + AnyBitPattern, while FooBits doesn't implement NoUninit, even if Foo is NoUninit.

I suggest the derive macro for CheckedBitPattern to add an implmentation of NoUninit for the generated "Bits" type, something like:

unsafe impl NoUninit for FooBits where Foo: NoUninit {}

context

I was using CheckedBitPattern as a validator for very simple binary file format. I could write something like this:

	let mut file = File::open("/path/to/file")?;
	let mut buffer = [0u8; mem::size_of::<Header>()];
	file.read_exact(&mut buffer))?;
	let header: Header = try_cast(buffer)?;

but I would like to use the HeaderBits as buffer, like this:

	let mut file = File::open("/path/to/file")?;
	let mut header = HeaderBits::zeroed();
	file.read_exact(bytes_of_mut(&mut header))?;
	let header: Header = try_cast(header)?;

but the code won't compile, unless I manually add:

unsafe impl NoUninit for HeaderBits {}

my Header type looks like this:

#[derive(Debug, Clone, Copy, NoUninit)]
#[repr(transparent)]
pub struct Magic([u8; 4]);
unsafe impl CheckedBitPattern for Magic {
	type Bits = [u8; 4];
	fn is_valid_bit_pattern(bits: &Self::Bits) -> bool {
		bits == b"\x27smp"
	}
}
#[derive(Debug, Clone, Copy, NoUninit)]
#[repr(C, packed)]
pub struct Version {
	pub major: u8,
	pub minor: u8,
}
unsafe impl CheckedBitPattern for Version {
	type Bits = [u8; 2];
	fn is_valid_bit_pattern(bits: &Self::Bits) -> bool {
		let &[major, minor] = bits;
		major == 1 || (major == 2 && minor <= 2)
	}
}
#[derive(Debug, Clone, Copy, NoUninit, CheckedBitPattern)]
#[repr(C, packed)]
pub struct Header {
	pub magic: Magic,
	pub version: Version,
	pub flags: u16,
}

btw, it would also be convenient if the CheckedBitPattern trait could have a conversion function, e.g. try_from_bits(), something like:

pub unsafe trait CheckedBitPattern: Copy {
	type Bits: AnyBitPattern;
	// Required method
	fn is_valid_bit_pattern(bits: &Self::Bits) -> bool;
	// Provided method
	fn try_from_bits(bits: Self::Bits) -> Result<Self, CheckedCastError> {
		try_cast(bits)
	}
}

or, alternatively, we can add an try_into_checked() method for the generated "Bits" type.

@Lokathor
Copy link
Owner

@fu5ha this seems like something for you since you're the proc-macro guardian

@fu5ha
Copy link
Collaborator

fu5ha commented Dec 12, 2023

👍 agree with the analysis, feel free to PR and I will review or ping me in a couple weeks and I can hopefully implement it

@nerditation
Copy link
Author

I tried to take a look at the code. I find it more complicated then expected. the main issue is, derive macros don't receive the derive attribute itself. for example, given the definition:

#[derive(Foo, Bar)]
struct MyStruct {}

the procedural macro of Foo cannot see Bar is present; same for Bar.

in this case, CheckedBitPattern cannot determine whether the type derives NoUninit or not. I can think of two possible solutions here:

  1. let the user annotate with a helper attribute for the CheckedBitPattern derive macro, something like:

    #[derive(Clone, Copy, NoUninit, CheckedBitPattern)]
    #[checked_bits_type(additional_traits(NoUninit))]
    struct MyStruct {}
    • pros: can derive other traits for the generated Bits type, not limited to NoUninit;
    • cons: traits like NoUninit need to be repeated
  2. use the unstable #![allow_trivial_constraints] feature and put the implementation behind a feature gate.

    the implementation is just as simple as:

    unsafe impl NoUninit for FooBits where Foo: NoUninit {}
    • pros: easy to implement, also covers the case when the type doesn't #derive but manually unsafe impl NoUninit.
    • cons: only works on nightly, and rfc2056 isn't likely to be stablized in the near future.

there's also another "non-solution": in document, ask the user to write multiple #[derive()] separately, and in specific order. for example, from my testing, in the following code:

#[derive(Foo)]
#[derive(Bar)]
struct MyStruct {}

the input token stream of Foo contains the attribute #[derive(Bar)], but not the other way around. so if we write #[derive(NoUninit)] after #[derive(CheckedBitPattern)], the derive macro of CheckedBitPattern will see the NoUninit derive attribute.

but this is a "non-solution", not only because it is delicate and feels "hacky", but most importantly, rustfmt will merge separate #[derive()] attribtes into one single attribute, so it doesn't actually work at all.

at this point, I feel like it's probably just not worth it to add this feature.

anyway, I'm not an expert on procedural macros, maybe I'm missing something obvious. @fu5ha any opinions or suggestions? thanks.

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

3 participants