C# future simple codegen #1357
C# future simple codegen #1357
Conversation
yowl
force-pushed
the
csharp-future-simple
branch
from
b14b14b to
c3fce61
Compare
|
Thanks for doing this, @yowl! I'm planning to review it by the end of the week. |
dicej
left a comment
dicej
left a comment
There was a problem hiding this comment.
Thanks for doing this! Looks like a good start; please see my comments inline.
| Instruction::FutureLift { payload: _, ty: _ } => { | ||
| // TODO get the prefix for the type | ||
| let sig_type_name = "Void"; | ||
| uwriteln!(self.src, "var reader = new {}.FutureReader{}({});", self.interface_gen.name, sig_type_name, operands[0]); |
There was a problem hiding this comment.
I'd recommend using e.g. self.locals.tmp("reader") here to generate a unique variable name so as to avoid clashes.
| let op = &operands[0]; | ||
| self.interface_gen.add_future(self.func_name); | ||
|
|
||
| results.push(format!("{op}.Handle")); |
There was a problem hiding this comment.
FutureReader should probably have a TakeHandle() method that zeros out the handle field (and asserts that it wasn't already zero) before returning the original value so that the application won't accidentally try to use the no-longer-valid handle.
| uwrite!( | ||
| self.csharp_interop_src, | ||
| r#" | ||
| [global::System.Runtime.InteropServices.DllImportAttribute("$root", EntryPoint = "[waitable-set-new]"), global::System.Runtime.InteropServices.WasmImportLinkageAttribute] | ||
| internal static extern int WaitableSetNew(); | ||
| "# | ||
| ); | ||
|
|
||
| uwrite!( | ||
| self.csharp_interop_src, | ||
| r#" | ||
| [global::System.Runtime.InteropServices.DllImportAttribute("$root", EntryPoint = "[waitable-join]"), global::System.Runtime.InteropServices.WasmImportLinkageAttribute] | ||
| internal static extern void WaitableJoin(int waitable, int set); | ||
| "# | ||
| ); | ||
|
|
||
| uwrite!( | ||
| self.csharp_interop_src, | ||
| r#" | ||
| [global::System.Runtime.InteropServices.DllImportAttribute("$root", EntryPoint = "[waitable-set-wait]"), global::System.Runtime.InteropServices.WasmImportLinkageAttribute] | ||
| internal static unsafe extern int WaitableSetWait(int waitable, int* waitableHandlePtr); | ||
| "# | ||
| ); | ||
|
|
||
| uwrite!( | ||
| self.csharp_interop_src, | ||
| r#" | ||
| [global::System.Runtime.InteropServices.DllImportAttribute("$root", EntryPoint = "[waitable-set-drop]"), global::System.Runtime.InteropServices.WasmImportLinkageAttribute] | ||
| internal static unsafe extern void WaitableSetDrop(int waitable); | ||
| "# | ||
| ); | ||
|
|
There was a problem hiding this comment.
These could be moved to a standalone e.g. AsyncSupport.cs file that gets copied into the output since they don't depend on the WIT in any way.
| .interface_fragments | ||
| .push(InterfaceFragment { | ||
| csharp_src: format!(r#" | ||
| public class FutureReader{future_type_name} : FutureReader |
There was a problem hiding this comment.
Have you considered making FutureReader and FutureWriter generic classes rather than generating separate classes for each payload type? That's what we do in Rust and in Go, for example. Note how we use "vtable"-style structure in both Rust and Go to specialize the implementations internally. Happy to discuss further if this isn't clear.
| return "FutureReader".to_owned(); | ||
| } else { | ||
| return format!("Task<{name}>"); | ||
| return format!("FutureReader<{name}>"); |
There was a problem hiding this comment.
This suggests that FutureReader is generic, but it isn't, is it?
| self.csharp_interop_src, | ||
| r#" | ||
| [global::System.Runtime.InteropServices.DllImportAttribute("{import_module_name}", EntryPoint = "[future-new-0][async]{future}"), global::System.Runtime.InteropServices.WasmImportLinkageAttribute] | ||
| internal static extern ulong {camel_name}VoidNew(); |
There was a problem hiding this comment.
Using camel_name (which is derived from the name of an arbitrary function in which this future type happened to appear) is confusing IMO.
It's true that we use that function name as part of the Wasm import name because that's the only practical way to uniquely communicate to wit-component which future type we're referring to. However, that's purely an implementation detail that shouldn't be exposed to users. Instead, we should generate names like FutureNew for a future, FutureStringNew for a future<string>, etc. And if the same future type shows up in more than one function, we should only emit one copy. See the Rust, C, Go, and/or Python generators for examples.
2 participants
This PR adds enough code gen to support the simple-future wit runtime test. As for the async PR, this is pretty much the minimum PR in terms of future support. I've not tackled the typed canonical methods except to add a "void" implementation which is hard coded as the one to use.
Have followed the c test cases rather than the rust ones.
Also changed Export and Import in namespaces to be uppercase and moved resources and other methods to the appropriate import or export class. Some types are still produced from the import side, and have introduced a concept of a bidirectional type (enum, flags) that sit above the import/export split.
The current codegen produced is at https://github.com/yowl/wit-bindgen-simple-future