- 
                Notifications
    You must be signed in to change notification settings 
- Fork 152
Fix clippy unused_unit warnings in host_bindgen macro for void return functions #658
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
Conversation
… functions Co-authored-by: jsturtevant <[email protected]>
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.
It would be nice to see a test case added to the wit file that has a function that has no returns
| quote! {} | ||
| } else { | ||
| quote! { | ||
| #[allow(clippy::unused_unit)] | 
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 don't think this will be needed since this use case is handled by return an empty quote
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.
You're right! Since emit_hl_unmarshal_result returns TokenStream::new() for void functions, the conditional logic wasn't needed. Simplified the code to use unmarshal directly. (412ab9f)
| cc 9e697e3320100630afae15901d82d23237f04e3d8090ec2c9ecb211fbf322909 # shrinks to x = 0.0 | ||
| cc 033b8487116d6b64d1c321ca2475618b943551b670ed562648c12f3a7586243a # shrinks to x = EnumA | ||
| cc 37f8b1a347c5a2a096a8904b918c0f049b729c80e329ed63dc1b501258a2973b # shrinks to x = false | ||
| cc f401ff3b24d9e0b6a5332fbaecbb5cde12d13d932d33d27a5d15ce2ea73a1125 # shrinks to x = 'A' | 
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.
Where did this come from? Don't include thes right now.
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.
Removed the proptest-regressions file that was accidentally included. (412ab9f)
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.
let's not do dependency updates in this PR and leave that for independent PRs.
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.
Reverted the Cargo.lock dependency updates to keep this PR focused only on the clippy fix. (412ab9f)
…, remove unrelated files Co-authored-by: jsturtevant <[email protected]>
| 
 Added a test case for void return function to guest.wit:  | 
| pub fn emit_hl_unmarshal_result(s: &mut State, id: Ident, rt: &etypes::Result) -> TokenStream { | ||
| match rt { | ||
| etypes::Result::Named(rs) if rs.is_empty() => quote! { () }, | ||
| etypes::Result::Named(rs) if rs.is_empty() => TokenStream::new(), | 
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 don't like this as a solution, because it changes the contract for this helper to something a lot less intuitive:
The resultant token stream will be an expression which typechecks at the Rust type ...
becomes
The resultant token stream will typecheck at the Rust type of the unnamed type of the result, or the empty stream if named results are used
which means that it can't generally be used to uniformly generate a return value. Right now, this expression is only used in the tail position of a function definition, so it works "by coincidence"---but this change makes the interface more brittle (e.g. if you wanted to refactor the macro to trace the return value, you would need special cases to not accidentally generate an empty let statement).
The
host_bindgenmacro was generating clippyunused_unitwarnings when creating bindings for WIT functions with no return value (void functions). This occurred because the macro generated explicit()return statements that clippy correctly identified as redundant.Problem
When using WIT like:
The macro generated code like:
Solution
1. Modified
emit_hl_unmarshal_resultinsrc/hyperlight_component_util/src/hl.rs:quote! { () }toTokenStream::new()for empty result types2. Updated guest code generation in
src/hyperlight_component_util/src/guest.rs:#[allow(clippy::unused_unit)]attribute conditionalResult
Now generates clean code that doesn't trigger clippy warnings:
Testing
-D clippy::unused-unit)Fixes #653.
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.