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

refactor: improve ergonomics of fn replace #250

Conversation

vados-cosmonic
Copy link
Contributor

@vados-cosmonic vados-cosmonic commented Oct 14, 2023

After trialing replace_(imported|exported)_func in WASI-Virt, it's clear that the ergonomics around the builder function need to be improved. FunctionBuilder (particularly FunctionBuilder::new()) is difficult to use without a mutable borrow of the module itself.

This commit refactors replace_(imported|exported)_func in order to pass through the mutable borrow which makes it easier to use FunctionBuilders.

@vados-cosmonic vados-cosmonic force-pushed the refactor/improve-ergonomics-of-fn-replace branch from 8115634 to 91cc935 Compare October 14, 2023 23:55
@guybedford
Copy link
Collaborator

Thanks for putting some more thought to this. I was wondering - could we just pass an already-created InstrSeqBuilder here so that the first argument to the function is body where body is the builder for the function builder? Just like a |then| works today - https://docs.rs/walrus/0.20.1/walrus/struct.InstrSeqBuilder.html#example-4.

@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 17, 2023

Thanks for the pointer, that's a great idea -- I'll make the change!

  • Use impl IntrSeqBuilder modification fn similar to if_else on FunctionBuilder

@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 17, 2023

Hey @guybedford Ran into an issue -- with the changes, I run into an issue where I can't get the LocalFunction to be returned from the closure.

The way the other functions work, they don't generate the LocalFunction, they just let you manipulate the InstrSeqBuilder, but in this case we want the user to be able to pass the arguments right? So that means they need to actually build the local function.

Here's a snapshot of the problem:

        // Replace the existing function with a new one with a reversed const value
        let new_fn_id = module
            .replace_exported_func(original_fn_id, |mut body| {
                body.i32_const(4321).drop();
                body.local_func(vec![]) // TODO(FIX): Can't move LocalFunction produced here out of deref to body!
            })
            .expect("function replacement worked");

We want the user to have control over that local_func call to actually generate the function with the arguments they want (ex. maybe you want to always give the same arguments to some fn), but we get this error:

error[E0507]: cannot move out of dereference of `InstrSeqBuilder<'_>`
   --> src/module/functions/mod.rs:706:17
    |
706 |                 body.local_func(vec![]) // TODO(FIX): Can't move LocalFunction produced here out of deref to body!
    |                 ^^^^^^^^^^^^^^^^^^^^^^^ move occurs because value has type `FunctionBuilder`, which does not implement the `Copy` trait

This also is an issue if we pass in the full FunctionBuilder (rather than the seq builder), rather than create it inside.

@guybedford
Copy link
Collaborator

in this case we want the user to be able to pass the arguments right?

Could we just use the same localIds from the original function, passing that list as one of the arguments?

@vados-cosmonic
Copy link
Contributor Author

Ah in that case, we could call body.local_func(..) from outside all-together, that would work, I think. I'll give it a shot

@guybedford
Copy link
Collaborator

Unless there are other drawbacks we need to be aware of it seems like that might be the idiomatic pattern to me? The only case I think where it might be restrictive is if you want to pass an already constructed function?

@vados-cosmonic vados-cosmonic force-pushed the refactor/improve-ergonomics-of-fn-replace branch from 01e2356 to 41e1c29 Compare October 18, 2023 00:30
@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 18, 2023

Yeah I think that's right -- IMO being able to pass an already pre-created function is pretty useful, but that's a bridge that we can cross when we get there. I don't know exactly how but , but there's probably a clean way to graft the implementation of an existing Function to another one using append_instruction and the instruction_mapping in there, so you could graft all the instructions over fairly easily in the closure.

Going to take one final look and this should be ready for review! Squashed & ready for review now, let me know if you find any more improvements to make!

@vados-cosmonic vados-cosmonic force-pushed the refactor/improve-ergonomics-of-fn-replace branch 3 times, most recently from b0a24cb to 76ee5c5 Compare October 18, 2023 03:50
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

This seems good to me, happy to go ahead with the change!

@vados-cosmonic vados-cosmonic force-pushed the refactor/improve-ergonomics-of-fn-replace branch from 76ee5c5 to fcac3ea Compare October 18, 2023 17:04
@vados-cosmonic
Copy link
Contributor Author

Sorry a small formatting issue slipped through, squashed & it's ready when tests pass & you're good to merge 🙇

Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

More clearly marking the request for changes per the query in comment #250 (comment).

src/module/functions/mod.rs Show resolved Hide resolved
src/module/functions/mod.rs Outdated Show resolved Hide resolved
@vados-cosmonic vados-cosmonic force-pushed the refactor/improve-ergonomics-of-fn-replace branch from db07266 to af96691 Compare October 18, 2023 18:41
Copy link
Collaborator

@guybedford guybedford left a comment

Choose a reason for hiding this comment

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

Yeah this looks right to me now! Let's land once the tests are passing.

After trialing `replace_(imported|exported)_func` in WASI-Virt, it's clear
that the ergonomics around the builder function need to be
improved. `FunctionBuilder` (particularly `FunctionBuilder::new()` is
difficult to use without a mutable borrow of the module itself.

This commit refactors `replace_(imported|exported)_func` in order to
pass through the mutable borrow which makes it easier to use
`FunctionBuilder`s.

Signed-off-by: Victor Adossi <[email protected]>
@vados-cosmonic vados-cosmonic force-pushed the refactor/improve-ergonomics-of-fn-replace branch from af96691 to f74b185 Compare October 18, 2023 20:36
@vados-cosmonic
Copy link
Contributor Author

vados-cosmonic commented Oct 18, 2023

Ahhh apologies I forgot to convert some more spots to the new API -- tests have been fixed

@guybedford guybedford merged commit 45ca488 into rustwasm:main Oct 18, 2023
8 checks passed
@vados-cosmonic vados-cosmonic deleted the refactor/improve-ergonomics-of-fn-replace branch October 18, 2023 21:10
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 this pull request may close these issues.

2 participants