-
Notifications
You must be signed in to change notification settings - Fork 98
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
Implement Inline Size Assertion Annotations #1405
base: main
Are you sure you want to change the base?
Conversation
95ff00a
to
5b209e5
Compare
Implem the procedural macro `inline_assert_size_eq` to check the size of a type at compile time. Additionally, add the test file `inline_assert_size_eq_failed.rs` to ensure that the macro tirggers a compile-time error when the size is not as expected Fixes google#1329
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1405 +/- ##
=======================================
Coverage 87.71% 87.71%
=======================================
Files 15 15
Lines 5138 5138
=======================================
Hits 4507 4507
Misses 631 631 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! This will need a few changes before we can consider merging it.
@joshlf, I think this functionality would be nicely complemented by a #[assert_offset(N)]
field attribute macro (though I'd prefer us tackle that in a separate PR).
zerocopy-derive/src/lib.rs
Outdated
let name = &ast.ident; | ||
ret = quote! { | ||
#ast | ||
static_assertions::const_assert!(core::mem::size_of::<#name>() == #size); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't defer to the static_assertions
crate, since we don't want that to be a public dependency of zerocopy, nor do we want to be an explicit dependency of our consumers.
You should instead add a hygeinic helper macro to macro_util.rs
and invoke that in the proc macro.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
zerocopy-derive/src/lib.rs
Outdated
/// } | ||
/// ``` | ||
#[proc_macro_attribute] | ||
pub fn inline_assert_size_eq( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to call this assert_size_eq
, and drop the inline_
prefix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is assert_size_eq
in macro_util.rs
which check if the size are equal between two types. I think we need another name. How about assert_size_eq_val
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The items in macro_util.rs
are internal implementation details of public macros. We can rename these internal implementation details.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
To avoid confusion with the existing `const_assert` macro. I named the macro to assert the falsehood of an expression at compile time `static_const_assert` Rename `inline_assert_size_eq` to `assert_size_eq_val`
zerocopy-derive/src/lib.rs
Outdated
input: proc_macro::TokenStream, | ||
) -> proc_macro::TokenStream { | ||
let ast: Result<DeriveInput, _> = syn::parse(input.clone()); | ||
let expected_size: Result<usize, _> = args.to_string().trim().parse(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why the .to_string()
? Can't we just emit the argument directly into the size assertion?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since proc_macro::TokenStream
cannot be directly appended to quote
and can only be parsed as a string, I first parse it into a string and then convert it to a usize
.
Another way is parse it into proc_macro2::TokenStream
.
zerocopy-derive/src/lib.rs
Outdated
let ast: Result<DeriveInput, _> = syn::parse(input.clone()); | ||
let expected_size: Result<usize, _> = args.to_string().trim().parse(); | ||
let mut ret: proc_macro::TokenStream = input; | ||
if let (Ok(ast), Ok(size)) = (ast, expected_size) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The lack of an else
branch here concerns me — are there circumstances under which this would fail silently?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since parsing of args
and input
may fail, I have left the return value without using the macro. The size assertion will be performed only if these parameters are parsed correctly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we could get this to work for generic types by instead making this a helper annotation on KnownLayout
.
We would add a hidden, associated const to KnownLayout
that is checked on any operation that reflects the layout:
trait KnownLayout {
#[doc(hidden)]
const HAS_EXPECTED_LAYOUT: bool = true;
/* other items */
}
...and then this:
#[derive(KnownLayout)]
#[repr(C)]
#[zerocopy::assert(
align = core::mem::size_of::<T>(),
size = core::mem::size_of::<T>())
]
struct Foo<T> {
#[zerocopy::assert(offset = 0, size = 2 * core::mem::size_of::<T>())]
a: [T; 2],
}
...could expand to this:
impl<T> KnownLayout for Foo<T> {
const HAS_EXPECTED_LAYOUT: bool = {
assert!(::zerocopy::core_reexport::mem::size_of::<Self>() == core::mem::size_of::<T>());
assert!(::zerocopy::core_reexport::mem::align_of::<Self>() == core::mem::align_of::<T>());
assert!(::zerocopy::core_reexport::mem::offset_of!(Self, a) == 0);
assert!(::zerocopy::core_reexport::mem::size_of::<[T; 2]>() == 2 * core::mem::size_of::<T>());
true
};
}
This approach is much more powerful, but it results in compile errors at use-sites, not at definition sites. My inclination is that this tradeoff is worth it. And, perhaps for non-generic types, we can automatically emit definition-site assertions. Thoughts, @joshlf?
f3e7dd3
to
07a821c
Compare
Thanks for being so responsive to feedback! Just a heads up: It might be awhile before we're ready to merge this. We'll need to be absolutely certain about both the syntax and its interaction with generic types before we merge, and we are currently prioritizing the backlog of issues that are blocking the 0.8 release. |
Hi @jswrenn, I'm currently learning about derive macros and am trying to figure out how to make them work with generic types. I'm struggling with how to capture and parse attributes such as From the video A Practical Introduction to Derive Macros with Attributes suggests that to parse attributes, one should use the Am I on the right track here? Could you offer any advice or guidance? |
I think we'd like to avoid taking additional dependencies for this. To do it with use syn::{
Attribute,
Expr,
parse::{Parse, ParseStream},
parse_quote,
Token,
punctuated::Punctuated,
};
/// Declarations the qualities of a layout.
///
/// These function both as checked assertions, and as hints to `KnownLayout`.
struct Hints {
hints: Punctuated<Hint, Token![,]>,
}
impl Parse for Hints {
fn parse(input: ParseStream) -> syn::Result<Self> {
Ok(Self {
hints: input.parse_terminated(Hint::parse, Token![,])?,
})
}
}
/// A declaration about type/field layout qualities.
struct Hint {
kind: HintKind,
colon: Token![:],
expr: Expr,
}
impl Parse for Hint {
fn parse(input: ParseStream) -> syn::Result<Self> {
Ok(Self {
kind: input.parse()?,
colon: input.parse()?,
expr: input.parse()?,
})
}
}
mod keywords {
syn::custom_keyword!(align);
syn::custom_keyword!(offset);
syn::custom_keyword!(size);
}
/// The layout quality a hint is about.
///
/// A hint is either about alignment, offset or size.
enum HintKind {
/// The alignment of a type or field.
Align(keywords::align),
/// The offset of a field.
Offset(keywords::offset),
/// The size of a type or field.
Size(keywords::size),
}
impl Parse for HintKind {
fn parse(input: ParseStream) -> syn::Result<Self> {
let lookahead = input.lookahead1();
if lookahead.peek(keywords::align) {
input.parse().map(HintKind::Align)
} else if lookahead.peek(keywords::offset) {
input.parse().map(HintKind::Offset)
} else if lookahead.peek(keywords::size) {
input.parse().map(HintKind::Size)
} else {
Err(lookahead.error())
}
}
}
fn main() -> syn::Result<()> {
let attr: Attribute = parse_quote! {
#[hint(
align: 4,
offset: 8,
size: 0,
)]
};
if attr.path().is_ident("hint") {
let hints: Hints = attr.parse_args()?;
// do something with `hints`
}
Ok(())
} An idea I've reflected in the above code is that I think we should call these 'hints', not Well, that's the idea at least. There are a lot of particulars to figure out here. |
Then, to siphon the hints out of the AST, you'll want to leverage the use syn::{
parse,
visit::{self, Visit},
DeriveInput, Field, Generics, Ident,
};
/// Visits the AST and collects layout hints.
#[derive(Debug, Default)]
struct HintCollector<'ast> {
hints: Vec<TargetedHint<'ast>>,
errors: Vec<parse::Error>,
}
/// A hint and its target (i.e., type or field).
#[derive(Debug)]
struct TargetedHint<'ast> {
target: HintTarget<'ast>,
hints: Hints,
}
/// The target of a hint.
#[derive(Copy, Clone, Debug)]
enum HintTarget<'ast> {
Type(&'ast DeriveInput),
Field(&'ast Field),
}
impl<'ast> HintCollector<'ast> {
/// Collect the `Hints` for the given target from its given attribute list.
fn visit_attributes(&mut self, target: HintTarget<'ast>, attrs: &'ast Vec<Attribute>) {
let hints = attrs.iter().filter(|attr| attr.path().is_ident("hint"));
for attr in hints {
match attr.parse_args::<Hints>() {
Ok(hints) => {
self.hints.push(TargetedHint { target, hints });
}
Err(err) => {
self.errors.push(err);
}
}
}
}
}
impl<'ast> Visit<'ast> for HintCollector<'ast> {
fn visit_derive_input(&mut self, i: &'ast DeriveInput) {
self.visit_attributes(HintTarget::Type(i), &i.attrs);
visit::visit_derive_input(self, i);
}
fn visit_field(&mut self, i: &'ast Field) {
self.visit_attributes(HintTarget::Field(i), &i.attrs);
visit::visit_field(self, i);
}
}
fn main() -> syn::Result<()> {
use syn::DeriveInput;
let input: DeriveInput = parse_quote! {
struct Foo<T, const N: usize> {
#[hint(
align: 2,
offset: 0,
size: 2,
)]
a: u16,
#[hint(
align: core::mem::align_of::<T>(),
offset: 4,
size: core::mem::size_of::<T>() * N,
)]
b: [T; N]
}
};
let mut collector = HintCollector::default();
collector.visit_derive_input(&input);
println!("{:#?}", collector);
Ok(())
} |
Implem the procedural macro
inline_assert_size_eq
to check the size of a type at compile time. Additionally, add the test fileinline_assert_size_eq_failed.rs
to ensure that the macro tirggers a compile-time error when the size is not as expected Fixes #1329