Inline trait methods in derive-generated code

[joshlf]

Status

• [x] #341
• [ ] Deny clippy::missing_inline_in_public_items in derive-generated code

zerocopy-derive

We don't care about inline attributes for zerocopy-derive itself, but we do care for code emitted by zerocopy-derive. We need to figure out a way to either modify zerocopy-derive's output or modify our zerocopy-derive tests so that, when running zerocopy-derive tests, missing #[inline] attributes generate warnings or errors. It might be possible to use the clippy::missing_inline_in_public_items lint for this, but I'm not sure.

zerocopy

Many zerocopy trait methods contain very little logic or no logic at all, but are currently not marked with any inline attribute, and so cannot be inlined across a crate boundary. We should change this.

#[inline] attributes are now enforced by Clippy as of #341.

Mentoring instructions

Interested in contributing? See our contributing guide.

Figure out how we can ensure that the clippy::missing_inline_in_public_items lint is enforced (at least during testing) in code emitted by zerocopy-derive.

[zoo868e]

Hello @joshlf, I'm interested in resolving this issue. Could you please guide me on whether I should inline each function or if there are specific rules to determine which functions need to be inline?

[joshlf]

Sure! I'd start by reading this article, which gives a good overview of inlining in Rust.

Specifically the ones I'm interested in are:

• All methods on FromZeroes, FromBytes, and AsBytes; despite being generic, these are implemented for many concrete types in our crate, which means that, e.g., <usize as FromZeroes>::zero has a concrete impl in our crate.
• I'm not sure whether the functions/methods on types in the byteorder module are ever concrete. On the one hand, they all operate on a generic O type parameter, but on the other hand, we provide type aliases that specify concrete types for O. If I had to guess, I'd guess that our crate will not contain concrete instances, but I'm not sure, and it's also not a bad idea to add #[inline] as a normal inlining hint (rather than as a way of working around the cross-crate inlining issue) since all of those functions/methods are tiny.
• All methods emitted in generated code by zerocopy-derive.

[joshlf]

Hey @zoo868e, sorry for aping this, but I discovered that Clippy has a missing_inline_in_public_items lint which just solves the problem for us mindlessly, so I enabled that in #341. This only applies to zerocopy, though. I'm not yet sure how to prevent ourselves from backsliding on code emitted by zerocopy-derive.

[zoo868e]

Hi @joshlf, so I should prioritize focusing on the inline aspect within the zerocopy-derive, correct?

[joshlf]

Yep, that'd be great! Would you like me to assign this issue to you?

I've also recently added a bunch more https://github.com/google/zerocopy/labels/help%20wanted issues, so you're welcome to skim those and pick something else instead that looks interesting.

[zoo868e]

Sure, assign this issue to me. I'm delving into the source code, and I'll also review other issues.

[mepatrick73]

I've been working on this for a while and have found that manually expanding the derive proc macro and then checking if the functions inside the impl blocks have the #[inline] attribute is a good approach. This requires using syn with the full feature set enabled.

Here’s a link to a partial solution that I’ve implemented to check for an Enum deriving the KnownLayout trait.

Could you please review this approach and let me know if it seems sufficient? I’d appreciate any feedback on whether I should proceed with this solution or if there are improvements to consider.

[joshlf]

Oh this is awesome!

I'll let @jswrenn chime in before I give you specific advice - he might have a different take - but my gut says that the best way forward here is to use the code you have here to test the output of derives at runtime if debug_assertions are enabled. That way we don't have to write separate tests that exercise this behavior, but instead we get testing for free whenever anyone runs this with debug_assertions, which of course includes our entire test suite. Given that this is an extremely coarse-grained property (ie, whether or not we emit an #[inline] annotation is not going to change on the basis of what input you provide to the derive unless), that should be sufficient to give us the confidence we need.

[mepatrick73]

I’ve implemented an inline attribute check using debug_assertions, and it’s working well. It actually helped me identify a public function in zerocopy-derive/src/enum.rs that was missing the #[inline] attribute — something I hadn’t noticed just by reviewing the source code.

You can see the implementation here: commit link.

Let me know if you like this implementation!

[jswrenn]

@joshlf The risk of your proposal is that we might fail to compile customer code on what, really, is just a compiler hint. The risk of denial of service, I think, outstrips the benefits.

[joshlf]

Okay, @jswrenn and I talked offline. Summarizing our discussion here.

@jswrenn:

IMO, we shouldn't even be testing for this. It's a compiler hint. UI tests of our derive expansion are sufficient assurance. Let's say that we discover a case where the attribute in counter-productive. Do we then have to overhaul our whole testing framework for that one function? Testing friction is real.

@joshlf:

I disagree for a few reasons.

First, I think that the risk of forgetting these attributes is real, as we've seen both in zerocopy (when we enabled the Clippy lint) and with the commit that @mepatrick73 demo'd. UI tests of the derive expansion don't cut it because that assumes that reviewers will notice the absence of #[inline] when we're adding tests for a new function. In other words, the whole point of doing this as a "forall" test rather than a test that we have to enable on each function separately is exactly to catch cases where we forget since it's never top of mind.

Second, I think the risk of breakage is low. This test fails when an attribute is missing, and that has nothing to do with user input. There are not going to be cases where we sometimes emit #[inline] and other times don't depending on how the derive is invoked. So if we break someone, that implies that we just completely failed to exercise a particular derive in our tests.

Third, if we discover that an attribute is counter-productive, and it turns out that modifying the framework to exempt that case is difficult, we can just rip the entire testing framework out at that point if we deem that it's not worth the hassle. There's no backwards-compatibility requirement here.

We also agreed that it'd be ideal to use Clippy if possible, but we're not sure whether it's possible to get that to work.

---

TL;DR: @mepatrick73 Can you try to see if it's possible to emit a #[deny(clippy::missing_inline_in_public_items)] attribute on the unsafe impl block emitted by fn impl_block in zerocopy-derive/src/lib.rs and have ./cargo.sh +nightly clippy --tests -p zerocopy-derive catch it?

If that turns out not to be possible, then we're happy to go with the solution you've proposed. I'll have some suggested changes, but the rough shape looks good! Thanks for your work on this!

[mepatrick73]

I did some testing and examined Clippy's source code. It turns out that the missing_inline lint specifically checks whether the code originates from a procedural macro before applying the lint. If the code comes from a proc macro, the lint early returns and doesn't bother checking the lint. Here's the relevant snippet from Clippy's source:

impl<'tcx> LateLintPass<'tcx> for MissingInline {
    fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx hir::Item<'_>) {
        if rustc_middle::lint::in_external_macro(cx.sess(), it.span) || is_executable_or_proc_macro(cx) {
            return;
        }

• Source Link

To verify this behavior, I modified Clippy by adding an early panic inside the check:

impl<'tcx> LateLintPass<'tcx> for MissingInline {
    fn check_item(&mut self, cx: &LateContext<'tcx>, it: &'tcx hir::Item<'_>) {
        if rustc_middle::lint::in_external_macro(cx.sess(), it.span) || is_executable_or_proc_macro(cx) {
            panic!("Found the issue!");
            return;
        }

I ran clippy on local crate with the following main.rs

#[deny(clippy::missing_inline_in_public_items)]
#[derive(zerocopy_derive::KnownLayout, Debug)]
enum Foo {
    A,
}

fn main() {
    println!("Hello, world!");
    let stub = Foo::A;
    println!("{:?}", stub);
}

which led to the follow stacktrace :

warning: `clippy_lints` (lib) generated 1 warning
   Compiling clippy v0.1.83 (/home/patrick/git/rust-clippy)
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 11.03s
    Checking test-for-lint v0.1.0 (/home/patrick/personal/test-for-lint/test-for-lint)
thread 'rustc' panicked at clippy_lints/src/missing_inline.rs:91:13:
Found the issue!

Additionally, after reviewing some Clippy documentation, I found this guide: Handling Macro Expansions in Clippy, which outlines the challenge of linting code generated by macro expansions:

The general rule of thumb is that we should ignore code with macro expansions when working with Clippy because the code can be dynamic in ways that are difficult or impossible for us to foresee.

I'm not to sure what the right way forward is, perhaps filling an issue for Clippy to add a feature flag to check missing_inline in proc macros could be worth considering.

[joshlf]

Wow, great detective work! Given the complexity here, I think two avenues are fine:

• Land the code you already wrote
• (Optionally, in parallel) submit an issue to Clippy and see if we can get them to add a feature or attribute as you proposed

Thanks again for digging in!

[joshlf]

Also I hope you don't mind me assigning this issue to you since you're off to the races 🙂 Feel free to un-assign if you'd prefer.

[mepatrick73]

All good! I'll get to working on it tomorrow when I have some free time.