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

As of Rust v1.62, enums can now use #[derive(Default)] #333

Closed
mimullin-bbry opened this issue Jan 24, 2023 · 5 comments · Fixed by #334
Closed

As of Rust v1.62, enums can now use #[derive(Default)] #333

mimullin-bbry opened this issue Jan 24, 2023 · 5 comments · Fixed by #334
Assignees

Comments

@mimullin-bbry
Copy link
Contributor

Rust version 1.62 stabilized using derive(Default) for enums by marking the #[default] value to be used

See: rust-lang/rust#94457

Rust version 1.69 (unsure about 1.68) will start throwing a clippy warning for using manual impl Default when the derive(Default) can be used. These warnings will apply to the generated skeletons created by libbpf-cargo, causing issues to users.

    = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#derivable_impls
    = note: `#[warn(clippy::derivable_impls)]` on by default
    = help: remove the manual implementation...
help: ...and instead derive it...

libbpf-cargo should either update the minimum version to 1.62 and use the derive(Default) in this area of the code (https://github.com/libbpf/libbpf-rs/blob/master/libbpf-cargo/src/btf/btf.rs#L989). Or a #![allow(clippy::derivable_impls)] can be added to this area of the code (https://github.com/libbpf/libbpf-rs/blob/master/libbpf-cargo/src/gen.rs#L684), and the MRV can stay at 1.58.0

@danielocfb
Copy link
Collaborator

Thanks for pointing that out. It does not seem worth it to bump our minimum supported Rust version by four for this minor improvement, to be honest, so I am tempted to just suppress the warning, and include a TODO to fix this up once we require this version anyway. Are you interested in creating a pull request by any chance, @mimullin-bbry ?

@mimullin-bbry
Copy link
Contributor Author

Are you interested in creating a pull request by any chance, @mimullin-bbry ?

I am, I just was interested in guidance about which path the project would prefer. The "stay with 1.58" is a much less invasive and easier to implement fix (just plop in the allow and be done with it), however the derive(Default) method makes the generated skeletons slightly more readable (though not by much tbh). Also, the clippy warning isn't specific to just enums; suppressing the warning means the project will miss out on methods to make the generated skeletons "cleaner" in the future.

@danielocfb
Copy link
Collaborator

Sounds good. Yeah, I am not saying we should never go with #[default] on variants, just to postpone doing so until we bump to 1.62 for other reasons. But admittedly, we have no hard requirements for staying on 1.58, so if you feel strongly about going all the way, be my guest.

suppressing the warning means the project will miss out on methods to make the generated skeletons "cleaner" in the future

Fair enough. I am not too worried about that, though. For one, we should try and tag the smallest entity in question (the very enum definition or Default impl); in which case I'd expect we still see a warning for other types in the future. But it's also unlikely that we won't notice it through other avenues (e.g., by following Rust changelog announcements).

@mimullin-bbry
Copy link
Contributor Author

I'll go with the "stick to 1.58".

FYI: Beta just updated to 1.68, and the clippy warning is occurring. Thus, this issue might start impacting people's work flows. I'll try and get an MR ready this evening after work.

@mimullin-bbry
Copy link
Contributor Author

#334

@danielocfb danielocfb linked a pull request Jan 26, 2023 that will close this issue
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