Skip to content

Bring back the previous behavior of call_guest_function_by_name #761

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

Merged
merged 4 commits into from
Aug 8, 2025

Conversation

jprendes
Copy link
Contributor

@jprendes jprendes commented Aug 4, 2025

This PR brings back the previous behaviour of call_guest_function_by_name, and exposes the new behavioud behind a different name: run call.

Originally I wanted to call the new behaviour call, but that method exists as an alias to call_guest_function_by_name throught Callable trait.
The risk of using the call name is that anyone using the Callable trait would get an (almost) silent change in behaviour (almost because there would be a lint about Callable being unused).
Decided that there's no big risk of anyone using the Callable trait directly other than through the WIT macros, so using call should be fine.

Let me know if anyone prefers a different name.

@jprendes jprendes added the kind/bugfix For PRs that fix bugs label Aug 4, 2025
@jprendes jprendes force-pushed the snapshot2 branch 2 times, most recently from ee49c52 to d99d24b Compare August 4, 2025 14:51
dblnz
dblnz previously approved these changes Aug 4, 2025
Copy link
Contributor

@dblnz dblnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

ludfjig
ludfjig previously approved these changes Aug 4, 2025
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

jsturtevant
jsturtevant previously approved these changes Aug 4, 2025
@jprendes jprendes dismissed stale reviews from jsturtevant and ludfjig via fc3eb8c August 5, 2025 08:51
@jprendes jprendes force-pushed the snapshot2 branch 2 times, most recently from fc3eb8c to 4026423 Compare August 5, 2025 09:12
dblnz
dblnz previously approved these changes Aug 5, 2025
ludfjig
ludfjig previously approved these changes Aug 5, 2025
Copy link
Contributor

@ludfjig ludfjig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I guess we decided we don't care about the Callable trait?

@jsturtevant
Copy link
Contributor

LGTM. I guess we decided we don't care about the Callable trait?

It looks like we need to provide a way for this to choose the behavior, or we need to break it/keep current behavior

@jprendes jprendes dismissed stale reviews from ludfjig and dblnz via 01d7a69 August 5, 2025 22:08
@jprendes jprendes force-pushed the snapshot2 branch 4 times, most recently from c8f4d66 to 02338d4 Compare August 7, 2025 16:52
Copy link
Member

@syntactically syntactically left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am slightly confused by the division of labour between 65ff6ff and 1d74a97. It seems like the former has code that uses the snaphsot member that is perhaps not correct without the changes from the latter?

Is the intention with 796c6c9 to make all the tests call through Callable?

@jprendes
Copy link
Contributor Author

jprendes commented Aug 7, 2025

Is the intention with 796c6c9 to make all the tests call through Callable?

No, I renamed the method from run to call. Callable is not being used in the tests, other than through the macro.

@jprendes
Copy link
Contributor Author

jprendes commented Aug 7, 2025

Let me update the PR description

@syntactically
Copy link
Member

syntactically commented Aug 7, 2025

No, I renamed the method from run to call. Callable is not being used in the tests, other than through the macro.

How is the name conflict with Callable::call managed?

Do you mind squashing the run rename with the places where each bit of code was introduced as well? It seems a bit silly to have one commit renaming things from call_guest_by_name to run and another doing it back tocall!

@jprendes
Copy link
Contributor Author

jprendes commented Aug 7, 2025

How is the name conflict with Callable::call managed?

If the trait is not on scope, there's no conflict.
If the trait is on scope, doing sox.call, the new method wins over the trait.
If doing Callable::call(&sbox, ...) again there's no conflict, it uses the trait method.
If doing <sbox as Callable>::call, i ha ent tried, but I guess the trait is used.

@jprendes jprendes force-pushed the snapshot2 branch 2 times, most recently from 5ea43c3 to 04f8b3d Compare August 8, 2025 15:39
@jprendes jprendes merged commit 0f88a66 into hyperlight-dev:main Aug 8, 2025
33 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bugfix For PRs that fix bugs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants