Skip to content
Closed
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions src/hyperlight_component_util/src/guest.rs
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,15 @@ fn emit_import_extern_decl<'a, 'b, 'c>(
ResourceItemName::Static(mn) => quote! { #mn },
},
};
// Only emit the allow attribute if unmarshal is not empty
let unmarshal_with_attr = if unmarshal.is_empty() {
quote! {}
} else {
quote! {
#[allow(clippy::unused_unit)]
Copy link
Contributor

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

Copy link
Contributor Author

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)

#unmarshal
}
};
let decl = quote! {
fn #n(&mut self, #(#param_decls),*) -> #result_decl {
let mut args = ::alloc::vec::Vec::new();
Expand All @@ -78,8 +87,7 @@ fn emit_import_extern_decl<'a, 'b, 'c>(
::hyperlight_common::flatbuffer_wrappers::function_types::ReturnType::VecBytes,
);
let ::core::result::Result::Ok(#ret) = #ret else { panic!("bad return from guest {:?}", #ret) };
#[allow(clippy::unused_unit)]
#unmarshal
#unmarshal_with_attr
}
};
match fnname {
Expand Down
2 changes: 1 addition & 1 deletion src/hyperlight_component_util/src/hl.rs
Original file line number Diff line number Diff line change
Expand Up @@ -655,7 +655,7 @@ pub fn emit_hl_unmarshal_param(s: &mut State, id: Ident, pt: &Value) -> TokenStr
/// are no names in it (i.e. a unit type)
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(),
Copy link
Member

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).

etypes::Result::Unnamed(vt) => {
let toks = emit_hl_unmarshal_value(s, id, vt);
quote! { { #toks }.0 }
Expand Down
10 changes: 10 additions & 0 deletions src/hyperlight_host/tests/wit_test.proptest-regressions
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
# Seeds for failure cases proptest has generated in the past. It is
# automatically read and these particular cases re-run before any
# novel cases are generated.
#
# It is recommended to check this file in to source control so that
# everyone who runs the test benefits from these saved cases.
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'
Copy link
Contributor

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.

Copy link
Contributor Author

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)

8 changes: 4 additions & 4 deletions src/tests/rust_guests/witguest/Cargo.lock
Copy link
Contributor

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.

Copy link
Contributor Author

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)

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading