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

Feature/using component #678

Merged
merged 9 commits into from
Dec 18, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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
44 changes: 44 additions & 0 deletions .vscode/tasks.json
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,50 @@
"kind": "build",
"isDefault": true
}
},
{
"label": "rust clippy",
"type": "process",
"command": "cargo",
"args": [
"clippy",
"--all-targets",
"--all-features",
"--",
"-D",
"warnings",
"-A",
"clippy::extra_unused_lifetimes"
],
"options": {
"cwd": "${workspaceRoot}/native/wasmex"
},
"problemMatcher": [
"$mixCompileWarning",
"$mixCompileError"
],
"group": {
"kind": "build",
"isDefault": true
}
},
{
"label": "rust format",
"type": "process",
"command": "cargo",
"args": ["fmt"],
"options": {
"cwd": "${workspaceRoot}/native/wasmex"
},
"problemMatcher": [
"$mixCompileWarning",
"$mixCompileError"
],
"group": {
"kind": "build",
"isDefault": true
}
}

]
}
36 changes: 36 additions & 0 deletions lib/wasmex/components/component.ex
Original file line number Diff line number Diff line change
Expand Up @@ -31,4 +31,40 @@ defmodule Wasmex.Components.Component do
resource -> {:ok, __wrap_resource__(resource)}
end
end

defmacro __using__(opts) do
genserver_stuff =
superchris marked this conversation as resolved.
Show resolved Hide resolved
quote do
use GenServer

def start_link(opts) do
Wasmex.Components.start_link(opts)
end

def handle_call(request, from, state) do
Wasmex.Components.handle_call(request, from, state)
end
end

functions =
if wit_path = Keyword.get(opts, :wit) do
wit_contents = File.read!(wit_path)
Copy link
Owner

Choose a reason for hiding this comment

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

thinking aloud: should we maybe not hand over a file path but rather the wit definition (file content) itself?
maybe the wit content doesn't come from a file but from - i don't know - a network request or was generated somehow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hmm.. as it turns out rust code annoyingly needs a path passed to it, but it doesn't have to actually be a path that exists. I think it uses it to produce errors perhaps. In any event, if we change it to pass the wit contents as a string we have to make up a path somehow. Any thoughts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm thinking I will just have the wit option just mean a string containing the wit content, and add a wit_path or wit_file option to specify the file(s). In the future I'd also like to add component or component_bytes options to pass in the binary, which I think I can also parse with a little more code

exported_functions = Wasmex.Native.wit_exported_functions(wit_path, wit_contents)

for {function, arity} <- exported_functions do
Copy link
Owner

Choose a reason for hiding this comment

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

can't wait for imported function support! :D

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, definitely high on my list of next things.

arglist = Macro.generate_arguments(arity, __MODULE__)
function_atom = function |> String.replace("-", "_") |> String.to_atom()
tessi marked this conversation as resolved.
Show resolved Hide resolved

quote do
def unquote(function_atom)(pid, unquote_splicing(arglist)) do
Wasmex.Components.call_function(pid, unquote(function), [unquote_splicing(arglist)])
end
end
end
else
[]
end

[genserver_stuff, functions]
end
end
2 changes: 2 additions & 0 deletions lib/wasmex/native.ex
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@

def component_call_function(_store, _instance, _function_name, _params), do: error()

def wit_exported_functions(path, wit), do: error()

Check warning on line 96 in lib/wasmex/native.ex

View workflow job for this annotation

GitHub Actions / OTP 25.2 / Elixir 1.15.8

variable "path" is unused (if the variable is not meant to be used, prefix it with an underscore)

Check warning on line 96 in lib/wasmex/native.ex

View workflow job for this annotation

GitHub Actions / OTP 25.2 / Elixir 1.15.8

variable "wit" is unused (if the variable is not meant to be used, prefix it with an underscore)

Check warning on line 96 in lib/wasmex/native.ex

View workflow job for this annotation

GitHub Actions / OTP 26.2 / Elixir 1.15.8

variable "path" is unused (if the variable is not meant to be used, prefix it with an underscore)

Check warning on line 96 in lib/wasmex/native.ex

View workflow job for this annotation

GitHub Actions / OTP 26.2 / Elixir 1.15.8

variable "wit" is unused (if the variable is not meant to be used, prefix it with an underscore)
superchris marked this conversation as resolved.
Show resolved Hide resolved

# When the NIF is loaded, it will override functions in this module.
# Calling error is handles the case when the nif could not be loaded.
defp error, do: :erlang.nif_error(:nif_not_loaded)
Expand Down
50 changes: 48 additions & 2 deletions native/wasmex/Cargo.lock

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

2 changes: 2 additions & 0 deletions native/wasmex/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,5 @@ wasmtime-wasi-http = "26.0.1"
wasi-common = "26.0.1"
wiggle = "26.0.1"
wat = "1.220.0"
wit-parser = "0.221.2"
convert_case = "0.6.0"
12 changes: 8 additions & 4 deletions native/wasmex/src/component_instance.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ use crate::component::ComponentInstanceResource;
use crate::store::ComponentStoreData;
use crate::store::ComponentStoreResource;

use convert_case::{Case, Casing};

use rustler::types::atom::nil;
use rustler::types::tuple;
use rustler::types::tuple::make_tuple;
Expand Down Expand Up @@ -63,7 +65,9 @@ pub fn component_call_function<'a>(
let _ = function.post_return(&mut *component_store);
Ok(encode_result(env, result))
}
Err(err) => Ok(env.error_tuple(format!("Error executing function: {err}"))),
Err(err) => Err(rustler::Error::Term(Box::new(format!(
"Error executing function: {err}"
)))),
Comment on lines +68 to +70
Copy link
Owner

Choose a reason for hiding this comment

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

👍

}
}

Expand Down Expand Up @@ -150,13 +154,13 @@ fn term_to_val(param_term: &Term, param_type: &Type) -> Result<Val, Error> {

fn term_to_field_name(key_term: &Term) -> String {
match key_term.get_type() {
TermType::Atom => key_term.atom_to_string().unwrap(),
_ => key_term.decode::<String>().unwrap(),
TermType::Atom => key_term.atom_to_string().unwrap().to_case(Case::Kebab),
_ => key_term.decode::<String>().unwrap().to_case(Case::Kebab),
Comment on lines +157 to +158
Copy link
Owner

Choose a reason for hiding this comment

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

looking at these unwrap calls: When/how would these trigger? can they be triggered through user input? Wouldn't be great if we crash the BEAM just because someone chose a weird field name 🤔

}
}

fn field_name_to_term<'a>(env: &rustler::Env<'a>, field_name: &str) -> Term<'a> {
rustler::serde::atoms::str_to_term(env, field_name).unwrap()
rustler::serde::atoms::str_to_term(env, &field_name.to_case(Case::Snake)).unwrap()
}

fn encode_result(env: rustler::Env, vals: Vec<Val>) -> Term {
Expand Down
1 change: 1 addition & 0 deletions native/wasmex/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,5 +11,6 @@ pub mod module;
pub mod pipe;
pub mod printable_term_type;
pub mod store;
pub mod wit;

rustler::init!("Elixir.Wasmex.Native");
18 changes: 18 additions & 0 deletions native/wasmex/src/wit.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
use rustler::{NifResult, Term};
use wit_parser::{Resolve, WorldItem};

#[rustler::nif(name = "wit_exported_functions")]
pub fn exported_functions(env: rustler::Env, path: String, wit: String) -> NifResult<Term> {
Copy link
Owner

@tessi tessi Dec 9, 2024

Choose a reason for hiding this comment

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

I know you care only about functions for this PR, but is it an option to return all exports of the module instead?

similar to module_exports for regular wasm modules.

We would share a common/similar API (which I like for consistency) and having that additional info will surely help us later and we don't have to break any API for those changes.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll take a look at module exports and see if we can consolidate, agree having a consistent api would be best.

let mut resolve = Resolve::new();
let id = resolve.push_str(path, &wit).unwrap();
let world_id = resolve.select_world(id, None).unwrap();
superchris marked this conversation as resolved.
Show resolved Hide resolved
let exports = &resolve.worlds[world_id].exports;
let exported_functions = exports
.iter()
.filter_map(|(_key, value)| match value {
WorldItem::Function(function) => Some((&function.name, function.params.len())),
Copy link
Owner

Choose a reason for hiding this comment

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

would be nifty if we could also return the types (params types and return types) for nicer introspection/debugging/tooling later

Copy link
Owner

@tessi tessi Dec 9, 2024

Choose a reason for hiding this comment

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

also to prepare for elixir types (whenever they become available) - would be ace if wasmex could created typed interfaces/function definitions right away

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, i did this way for expediency in getting something working we need a better API to get back data from wit parsing.

_ => None,
})
.collect::<Vec<(&String, usize)>>();
Ok(Term::map_from_pairs(env, exported_functions.as_slice()).unwrap())
}
5 changes: 5 additions & 0 deletions test/component_fixtures/hello_world.ex
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
defmodule HelloWorld do
@moduledoc false

use Wasmex.Components.Component, wit: "test/component_fixtures/hello_world/hello-world.wit"
Copy link
Owner

Choose a reason for hiding this comment

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

I know we touched this topic in a private conversation before, but let's bring this discussion here :D

I love the idea! Calling component modules must be easy and automated and what you did here is much much better than anything we already have in wasmex. I'm not 100% certain how to do it though and would love your thoughts!

I was (and still am) internally debating whether we should use macros (as you did here) or rather have code generation. Let's try to collect a few pros/cons and see what's better. (please add your thoughts - I'm not decided, and we could even end up doing both)

Macro

  • set-up: super smooth. just give it some wit and you're good
  • transparency: not great. you have to look at the wit definition (which is not elixir code) to see what this module even implements
  • level of magic: high. it's not clear what exactly happens. will it only return functions, will it do imported function, will it do other fancy wasm things to my module? what if the wit file is some complicated mess?! how will my module look like? what if I only want to expose a few selected exported functions and not all?
  • upgrade path: when we in wasmex want to add functionality (e.g. add imported functions) this would always be a breaking change to our users
  • debug'ability: not sure. What if we implement more functions in this module that in turn call into the generated ones. It get hard to reason about this module rather fast, no?
  • change tracking: if I am a user of wasmex and have such a wit module. Then changing my wit definition would result in this module changing too 🤔 Imagine being the reviewer of this PR in our users code. I could probably see what the wit-change is and could reason that this module changes too?! But it's very implicit and might lead to me missing a bug - e.g. when some other code calls into this generated module.

Code Generation

  • not as easy to set-up: probably requires some mix task to generate this modules skeleton (maybe use igniter?!)
  • any change in the wit definition results in two changes in the code base (the wit itself, and the generated module)
  • what if the wit definition changes, but the module was forgotten to be re-generated? this might introduce an inconsistency -- or maybe is a feature to develop the module independent from the initial wit 🤔
  • less magic, and by that also easier code reviews+debugging
  • maybe tooling has it easier too? Like what if the LSP wants to find the function implementation. with generated code it's easy to see where the fn is implemented --- with a macro it's not as much
  • it's more stuff to read and might look "scary". The macro hides away some "ugly" boilerplate whereas code generation create a larger diff that might be ugly to read
  • code generation is what elixir/José seems to prefer (e.g. mix phx.gen.auth, and igniter whereas macros are were heavily used in ruby codebases and José consciously decided against their magic when designing Elixir in many places)

Conclusion

So. I don't know. I guess it's a trade off as all things in life. Coming from a corporate thinking (my day job ruined me) code generation looks sweet. it's more explicit and less magic. code reviews and manual extensibility seems to be favoured and it looks to vibe with what (I think) Elixir intends to be. But macros are so much easier to set-up.

What are your thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

If it was a macro, would we still have compiled-time checking of the name of functions and number of arguments passed? I think that’s the main thing I would want, apart from type-checking.

If I change my WIT file, and the Elixir compiler alerts me via the macros changing then that’s a good UX imo.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm not sure I know or entirely understand your questions @RoyalIcing but in my experiments, things like completion in iex still work so there appear to be "normal" behaviour for the macro generated functions, since macros are expanded at compile time. I'm not sure this is a one size fits all answer here, so I kind of lean towards eventually supporting both options. That said, I think I'll continue on with the macro approach for the moment as I flesh things out as its an approach I'm pretty familiar with vs learning Igniter (which does sound fun but only so much time in day etc).

Copy link
Collaborator

@RoyalIcing RoyalIcing Dec 10, 2024

Choose a reason for hiding this comment

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

@superchris My thinking is round what happens when the WIT changes. If the WIT is consumed by the macros then that would lead to a compile time error for most changes, which is what you’d want for a good developer experience.

If you went with the code generation I’d want some compile-time check that verifies the WIT and generated code still agree anyway. So for me it would really come down to whether being able to read the interface as Elixir code would be helpful or not. If you had code generation would you ever want to alter that code? I’m thinking probably not?

Copy link
Owner

@tessi tessi Dec 10, 2024

Choose a reason for hiding this comment

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

huh! what a discussion here and on bluesky! considering the macro is a very thin wrapper right now, I'd say let's merge it!

we maybe want to expand this feature into more than that which might end up being code generation (e.g. to support an elixir module having only a sub-set of the exported functions implemented) but not now.

end
Comment on lines +1 to +5
Copy link
Owner

Choose a reason for hiding this comment

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

love it! would be great to see imported functions being support soon 🙌

oh and is it possible to move this module just above the test where we use it? (like an inlined module). this would make the test way easier to read IMO (since we don't have to jump files)

1 change: 1 addition & 0 deletions test/component_fixtures/hello_world/build.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
npx jco componentize -w hello-world.wit -o hello_world.wasm hello-world.js
4 changes: 4 additions & 0 deletions test/component_fixtures/hello_world/hello-world.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,4 +12,8 @@ export function multiGreet(who, times) {

export function greetMany(people) {
return people.map((person) => `Hello, ${person}!`);
}

export function echoKebab(kebabRecord) {
return kebabRecord;
}
4 changes: 4 additions & 0 deletions test/component_fixtures/hello_world/hello-world.wit
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
package local:hello-world;

world hello-world {
record kebab-record {
kebab-field: string
}
export greet: func(who: string) -> string;
export multi-greet: func(who: string, times: u16) -> list<string>;
export greet-many: func(people: list<string>) -> list<string>;
export echo-kebab: func(kebab-record: kebab-record) -> kebab-record;
}
Binary file modified test/component_fixtures/hello_world/hello_world.wasm
Binary file not shown.
14 changes: 14 additions & 0 deletions test/components/component_types_test.exs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
defmodule Wasm.Components.ComponentTypesTest do
use ExUnit.Case, async: true

alias Wasmex.Wasi.WasiP2Options

setup do
{:ok, store} = Wasmex.Components.Store.new()
component_bytes = File.read!("test/component_fixtures/component_types/component_types.wasm")
Expand Down Expand Up @@ -49,6 +51,18 @@ defmodule Wasm.Components.ComponentTypesTest do
])
end

test "record with kebab-field" do
{:ok, store} = Wasmex.Components.Store.new_wasi(%WasiP2Options{})
component_bytes = File.read!("test/component_fixtures/hello_world/hello_world.wasm")
{:ok, component} = Wasmex.Components.Component.new(store, component_bytes)
{:ok, instance} = Wasmex.Components.Instance.new(store, component)

assert {:ok, %{kebab_field: "foo"}} =
Wasmex.Components.Instance.call_function(instance, "echo-kebab", [
%{kebab_field: "foo"}
])
end

test "lists", %{instance: instance} do
assert {:ok, [1, 2, 3]} =
Wasmex.Components.Instance.call_function(instance, "id-list", [[1, 2, 3]])
Expand Down
12 changes: 12 additions & 0 deletions test/components/components_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,18 @@ defmodule Wasmex.ComponentsTest do
assert {:error, _error} = Wasmex.Components.call_function(component_pid, "garbage", ["wut"])
end

test "using the component macro" do
component_bytes = File.read!("test/component_fixtures/hello_world/hello_world.wasm")

component_pid =
start_supervised!({HelloWorld, %{bytes: component_bytes, wasi: %WasiP2Options{}}})

assert {:ok, "Hello, Elixir!"} = HelloWorld.greet(component_pid, "Elixir")

assert {:ok, ["Hello, Elixir!", "Hello, Elixir!"]} =
HelloWorld.multi_greet(component_pid, "Elixir", 2)
end

test "wasi interaction" do
component_bytes = File.read!("test/component_fixtures/wasi_p2_test/wasi-p2-test.wasm")

Expand Down
10 changes: 10 additions & 0 deletions test/components/wit_parser_test.exs
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
defmodule Components.WitParserTest do
use ExUnit.Case

test "exported_functions" do
wit = File.read!("test/component_fixtures/hello_world/hello-world.wit")

assert %{"greet" => 1, "greet-many" => 1, "multi-greet" => 2} =
Wasmex.Native.wit_exported_functions("hello-world.wit", wit)
end
end
Loading