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

Add debug check for #[inline] attribute in public derive macro functions #1724

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mepatrick73
Copy link

@mepatrick73 mepatrick73 commented Sep 21, 2024

This commit introduces a debug-level check to ensure all public functions
generated by derive macros include the #[inline] attribute. The check
triggers an assertion if any function lacks the attribute, implemented
manually since Clippy does not currently check for missing inline lints
in proc macro-generated code. This may change if Clippy adds support
for this lint in expanded code.

Additionally, this commit adds #[inline] attributes to public functions
in the derived code.

This fixes part 2 of #7

This commit introduces a debug-level check to ensure all public functions
generated by derive macros include the #[inline] attribute.
The check triggers an assertion if any function lacks the attribute.
This is done manually because clippy doesn't check missing inline
lints in proc macro generated code.
Copy link

google-cla bot commented Sep 21, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.35%. Comparing base (04f4954) to head (d1ceff0).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1724   +/-   ##
=======================================
  Coverage   88.35%   88.35%           
=======================================
  Files          16       16           
  Lines        5889     5889           
=======================================
  Hits         5203     5203           
  Misses        686      686           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joshlf joshlf left a comment

Choose a reason for hiding this comment

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

You'll also need to update the output in zerocopy-derive/src/output_tests.rs to include the new #[inline(always)] attributes. You can see the required diff here.

Comment on lines +105 to +124
fn debug_check_pub_functions_of_trait_are_inlined(expanded_impl_blocks: proc_macro2::TokenStream) {
let file: syn::File =
syn::parse2(expanded_impl_blocks).expect("Failed to parse expanded TokenStream");

// Iterate over all impl blocks, check if functions have the inline attribute
for item in file.items {
if let syn::Item::Impl(item_impl) = item {
for impl_item in item_impl.items {
if let syn::ImplItem::Fn(method) = impl_item {
let has_inline = method.attrs.iter().any(|attr| attr.path().is_ident("inline"));
debug_assert!(
has_inline,
"Method {} does not have #[inline] attribute",
method.sig.ident
);
}
}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Can you move this to the end of the file, just above mod tests?

@joshlf
Copy link
Member

joshlf commented Sep 23, 2024

Thanks for doing this, @mepatrick73! Just one small nit and a failing test and we should be good to go!

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 this pull request may close these issues.

3 participants