fix: host_bindgen! returns Result instead of panicking on guest errors

[jsturtevant]

fixes: #1316

host_bindgen! export wrappers no longer panic when Callable::call() returns Err (guest abort/trap/timeout). Host callers now receive the HyperlightError.

Breaking change

Host-side guest export traits are now fallible and use a *Exports suffix. Host functions imported by the guest keep the original WIT signatures.

Example WIT:

world test {
  import roundtrip;
  export roundtrip;
}

interface roundtrip {
  echo: func(x: string) -> string;
}

Generated/used Rust shape:

// Guest -> host import implementation: unchanged.
impl test::wit::Roundtrip for Host {
    fn echo(&mut self, x: String) -> String { x }
}

// Host -> guest export call: renamed trait and fallible result.
use test::wit::{RoundtripExports, TestExports};

let value: Result<String, HyperlightError> = sandbox.roundtrip().echo("hi".into());

Signed-off-by: James Sturtevant jsturtevant@gmail.com

[ludfjig]

I think I am fine with this, but I suspect this is intentional. I think we generally want to avoid Result types as much as possible where it makes sense, but since this matches the current guest calling convention maybe let's defer that discussion to later

[jsturtevant]

I think I am fine with this, but I suspect this is intentional. I think we generally want to avoid Result types as much as possible where it makes sense, but since this matches the current guest calling convention maybe let's defer that discussion to later

Open to other ways of addressing this I guess but a panic in the guest shouldn't panic this host. It seemed practical to match existing behavoir

[syntactically]

I agree with @ludfjig that, generally speaking, the semantic gap of lifting everything from the interface into Option isn't particularly attractive. However, I do see the need to deal with errors that fundamentally can happen during execution on the host side.

Is there a reason that this is also applied to the host functions imported by the guest? I think that generally speaking it is the ability to export a function implementing an interface, but actually implement a different interface (i.e. one with more optionality) that is the most semantically damaging---so the implicit Results are particularly unattractive on guest->host function calls (and to some extent in the implementation of guest functions, although the ability to trap/panic/etc is perhaps equivalent). It seems like splitting this (so that host->guest function calls can return errors, but host functions can't be implemented by returning errors) is what is common in the prior art e.g. the wasmtime host bindgen. Is it just about continuing to be able to use the same trait for the import and export versions of an interface? Maybe a type parameter to track whether this is an imported or exported usage would make sense?

Let me know what you think about the usability of that. As I think about this more, from the wasm perspective especially, I do think that having the Results on host function calls is slightly unattractive, but it's not quite as bad as I would have guessed, since from wasm's perspective, a (wasm) import that's called always does in fact have the option of causing the entire world to be torn town via purposefully causing a trap, and this is perhaps in some ways similar behaviour.

[jsturtevant]

Is there a reason that this is also applied to the host functions imported by the guest?

This was to match the current non-wit implementation. We had improved the situation with errors in #868.

I think that generally speaking it is the ability to export a function implementing an interface, but actually implement a different interface (i.e. one with more optionality) that is the most semantically damaging---so the implicit Results are particularly unattractive on guest->host function calls (and to some extent in the implementation of guest functions, although the ability to trap/panic/etc is perhaps equivalent). It seems like splitting this (so that host->guest function calls can return errors, but host functions can't be implemented by returning errors) is what is common in the prior art e.g. the wasmtime host bindgen.

Agree with the general sentiment that's its not ideal. I am not so sure we don't want host function to return errors, it seems useful to me to be able to say something went wrong across the boundry. Be interesting in what others think

[jsturtevant]

Agree with the general sentiment that's its not ideal. I am not so sure we don't want host function to return errors, it seems useful to me to be able to say something went wrong across the boundry. Be interesting in what others think

actually, was thinking on this some more and WIT does have the ability capture "result" types so we probalby don't need this on the guest side (I don't see the equivalent ::std::result::Result::Ok(#ret) = #ret else { panic!("bad return from guest {:?}", #ret) }; on the Guest->Host calls. Maybe there is another way to handle that too... I am open to ideas just really think we shouldn't panic if guest traps.

[syntactically]

just really think we shouldn't panic if guest traps

I'm with you on that one :)

This was to match the current non-wit implementation. We had improved the situation with errors in #868.

I am not so sure we don't want host function to return errors, it seems useful to me to be able to say something went wrong across the boundry.

I'm still not totally convinced on this, although I can see the arguments. Can you elaborate a little more on the use cases you see? If we did allow this, I think it's important that it not be semantically visible in the guest, which shouldn't know whether its imported component is implemented across a hyperlight-host boundary or by any other component, so we would need to propagate the error return up, and I would think that it would probably make sense to poison the sandbox (probably by panicking the guest?). If there's not a super strong use case, having the optionality just on the host->guest calls and not the hostcall returns seems like it is a starting point that gets rid of the main issue and is minimally invasive?

actually, was thinking on this some more and WIT does have the ability capture "result" types so we probalby don't need this on the guest side (I don't see the equivalent ::std::result::Result::Ok(#ret) = #ret else { panic!("bad return from guest {:?}", #ret) }; on the Guest->Host calls. Maybe there is another way to handle that too...

There is an unwrap on the host call result here.

Precisely the thing that I want to be careful about here is conflating WIT-level semantically-there-in-the-API optionality with this unconditionally-added-everywhere Hyperlight-level optionality. As I said in the paragraph above, even if we did add this for host function return types, I would not want to see it meaningfully exposed to the guest code.

[jsturtevant]

I am not so sure we don't want host function to return errors, it seems useful to me to be able to say something went wrong across the boundry.

I'm still not totally convinced on this, although I can see the arguments. Can you elaborate a little more on the use cases you see? If we did allow this, I think it's important that it not be semantically visible in the guest, which shouldn't know whether its imported component is implemented across a hyperlight-host boundary or by any other component, so we would need to propagate the error return up, and I would think that it would probably make sense to poison the sandbox (probably by panicking the guest?). If there's not a super strong use case, having the optionality just on the host->guest calls and not the hostcall returns seems like it is a starting point that gets rid of the main issue and is minimally invasive?

Yes I am in agree that we shouldn't do this now. I'm going to revisit this and see if I can come up with something different for consideration.

[ludfjig]

@jsturtevant should we close this one?

[jsturtevant]

We need a version of this, let me see if I can fix it up this week

[Copilot]

Pull request overview

This PR updates the host_bindgen!-generated host-side export wrappers so that guest call failures (e.g., abort/trap/timeout) are surfaced to callers as Result<_, HyperlightError> instead of crashing the host via panic!. This aligns exported guest calls with other Hyperlight call paths that already return errors.

Changes:

• Change host export wrapper generation to propagate Callable::call() failures via ? and return Ok(...) on success.
• Update generated host-side export trait method signatures to return Result<T, HyperlightError> (while keeping constructors unaffected).
• Update tests to unwrap successful guest calls and add a synthetic error-propagation test; also add some codegen deduplication state to avoid duplicate emitted types.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/hyperlight_host/tests/wit_test.rs	Updates callers for new Result-returning export methods and adds a test ensuring errors are returned instead of panicking.
src/hyperlight_component_util/src/rtypes.rs	Changes generated host-side export trait method return types to Result<_, HyperlightError> and adjusts export-instance trait naming.
src/hyperlight_component_util/src/resource.rs	Makes generated resource-table new() constructor pub(crate) (needed by new/updated usage patterns).
src/hyperlight_component_util/src/host.rs	Removes panic! on guest call errors in generated wrappers; returns Result and propagates failures with ?.
src/hyperlight_component_util/src/emit.rs	Adds module-level tracking (emitted_type_names) to prevent duplicate type emissions during codegen.

[jsturtevant]

I refreshed this. I wasn't able to find a way to not do this on host side but it is minimally invasive I believe. On the guest side we can use wit interface as discussed above.

[danbugs]

Mostly LGTM. Just minor nits.

[syntactically]

Just to make sure I understand, is the point of adding the Exports onto the end of the trait name on the host side just to make sure that if the same interface is used as an import and an export, the import and export don't collide?

Is there any barrier to centralizing the logic about the function result types in emit_func_result, whose job it usually is to decide the function result types?

[syntactically]

I don't follow what this is being used for?

[jsturtevant]

I've added a comment to address this and re-worked it. When the import and export where sharing resources it ended up with duplicates. This is tracking that so they are shared with out duplication

emit_instance  walks  roundtrip 's decls twice and writing them both:

line  20:            pub struct Testrecord {     <- from the import walk
line  78:            pub struct Testrecord {     <- from the export walk

Intended to hit "comment" rather than "approve"

[jsturtevant]

Just to make sure I understand, is the point of adding the Exports onto the end of the trait name on the host side just to make sure that if the same interface is used as an import and an export, the import and export don't collide?

yes, now that they are not the same shape, I needed a way to make those distinct.

[syntactically]

I think that the actual panic-related changes look pretty good to me now.

When the import and export where sharing resources it ended up with duplicates. This is tracking that so they are shared with out duplication

Just to clarify, is this something new brought on by the changes in this PR, or pre-existing behaviour that you happened upon and wanted to fix at the same time? I was assuming the former & therefore confused, but if the latter it makes sense, and i did manage to reproduce it locally before the change, so I'm guessing it's that?

I am still not convinced by this implementation, which feels a bit invasive/global to me.

Among other things, If I am reading it correctly, I think that, if you had an import and an export, or two imports or two exports, with the same wit-style package/etc names but different contents, it would sort of smash them together and make something not usable as either.

I would expect at a first glance that this is only an issue in one place: the instance case of rtypes::emit_extern_decl. The reasoning for that is:

• For all other kinds of externs that we support in the bindgen, they are emitted "in place" as members of the type/trait for the parent instance/component with a name based on the externname, which validation already ensures does not collide.
• Only in the Instance case do we rename the type over to a namespace based on the wit package parsing we do of its name, which I think is where it becomes possible for that to happen twice, which is the fundamental problem here?

Did you run into it anywhere else? If not, even when mitigating it at this layer, I think there should be a more contained approach, where in the emit::State we keep track of which wit-names we have already generated an instance for, and check/maintain that list only in the one place in rtypes::emit_extern_decl. At least, the test you added passes with the attached patch.

0001-hyperlight_component_util-Do-not-emit-duplicate-inst.patch

It feels a little unsatisfying to mitigate it at the codegen level here at all, because in some sense we are dealing with some duplication that we should be aware of in a phase where we have a rich semantic structure, but instead are dealing with it at a layer where everything is very syntactic and ugly and the modifications we can make and the sanity-checking we can do is very limited. For example, there is no easy, sensible, semantic way at this level to make sure that the instances we do not emit are "equal" to those that we do.

Another approach would be to go up a level and have a sanity check at the etypes level when we have the types in a nice form. There we could

• scan through the import/export tree to see if there are any instances which have the same wit-style package/namespace name; and
• if there are, use the functions from the subtype module to check them for equality; however, this will not be totally trivial, because:
  • The subtyping functions will need to be extended to include the definition of instance subtyping from the spec pdf; and
  • There is not a natural single typing context in which we can check both of them, so we may have to synthesize one. There is some complexity here due to the way that the deftypes for the instances will be the same QualifiedInstance, but the things we actually have in the imports/exports have had their universal quantifiers stripped off them to make other typechecking easier. This is a bit of a problem, actually, which I am not sure how to resolve within a few minutes of thinking.

[syntactically]

It might make sense to merge just the panic changes from this and then make a new PR for the other changes where we can deliberate a little bit more the best way to solve them?

[jsturtevant]

Sounds good. I did look at doing something higher up but was a much more evasive set of changes and felt much more challenging to review. Splitting it out as a separate issues/pr makes sense to me.

[jsturtevant]

Just to clarify, is this something new brought on by the changes in this PR, or pre-existing behaviour that you happened upon and wanted to fix at the same time? I was assuming the former & therefore confused, but if the latter it makes sense, and i did manage to reproduce it locally before the change, so I'm guessing it's that?

ah, So I just tried removing the deduplication code and found that it is needed. We are confused because both of these are true 😓

The inline test I added fails on main with no changes and once I move the interface from Roundtrip -> RoundtripExports (because they are two different shapes now) it starts failing with these changes.

It might make sense to merge just the panic changes from this and then make a new PR for the other changes where we can deliberate a little bit more the best way to solve them?

So I don't think we can do this. We could either:

• make the "hack" I did here at the code gen level and open an issue to fix more wholistically
• make the more generic changes to up a layer, making these changes a bit more extensive