-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Add support webidl namespaces. #678
Add support webidl namespaces. #678
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @derekdreery! I think this is on the right track.
However, I'm unsure we want to land this without tests, which will be hard to write without integration in one of our two frontends (webidl and proc-macro). Maybe @alexcrichton has a stronger opinion?
General thoughts: If we add support for modules to the proc-macro frontend, and two different #[wasm_bindgen] extern { ... }
blocks reference the same module, it will be emitted as two duplicate modules in the final code, which will cause errors. I don't see a way around this (other than putting the extern blocks in different modules), and so we would have to document the limitation very clearly.
With the webidl frontend, we can do some pre/post processing to ensure that there aren't duplicate modules, just the single canonical one.
I'm planning to write tests once I've done the webidl-to-backend bit (which I've just started), I wouldn't expect you to accept this without tests. However, I think it's hard to just test the codegen without manually creating AST in the test - once the webidl bit is working we can test both at the same time. |
Yep, sounds good to me! |
Let me/alex/ohanar know when you're ready for another round of review :) |
There is another option here: The webidl suggests that the console methods are pseudo-static methods on the Console type. So instead of generating a module, we could generate a For example: the console webidl is [Exposed=(Window,Worker,WorkerDebugger,Worklet,System),
ClassString="Console",
ProtoObjectHack]
namespace console {
//...
} So the operations are sort-of methods on a global singleton So there are 3 options for generating bindings: extern {
pub static console: Console;
}
// later..
console.log("something"); or using static (js meaning) methods: pub struct Console;
impl Console {
pub fn log(val: JsValue);
}
// later..
Console::log("something") or using a module and bare methods pub mod console {
pub fn log(val: JsValue);
}
// later..
console::log("something") |
I had written out a comment about how I prefer modules, but then I discovered this in the webidl spec:
and verified that our webidls only have operations and attributes within namespaces. That means we don't need to worry about nested types/interfaces/mixins. Which means that the first and second options are looking a lot more tempting, and require much less invasive changes to wasm-bindgen. Additionally, if you carefully read these steps on creating an operation you'll find that namespace operations do not get the namespace object passed as a All of that said, I am partial to using static methods, the equivalent of #[wasm_bindgen]
extern {
type console;
#[wasm_bindgen(static_method_of = console)]
fn log(...);
}
console::log(...); because it matches what |
There is a problem with naming here though: while |
The main problem with using static methods rather than modules (at least as I see it), is that you can't import a static method into your local namespace. I.e. pub mod console_module {
pub fn log(...) { ... }
}
// vs
pub struct ConsoleType;
impl ConsoleType {
pub fn log(...) { ... }
}
use console_module::log; // this works
use ConsoleType::log; // this doesn't |
I've almost finished implementing this as per the module version, (finished modulo bugs/errors), and anyone who wants to change over to one of the other variants will find it much easier after this PR has merged (since you can just go in and edit the AST transformation from webidl to backend), so I would consider punting on the decision and seeing what people's experience of the current version is. The reason I've stopped is I'm getting the following error
which is easy to solve, but I was wondering if an As always, I love to find out I'm wrong, so tell me if I am :). Also please don't merge this until I have added tests, and implemented partial namespaces (easy because I already did all the first_pass stuff). I need to find something other than |
This problem also comes up with external C libraries - so maybe people would be comfortable with it. I don't know. |
- Tried `cargo doc` and seen methods generated. - Added test with a few method calls to the console operations.
Ok @fitzgen @alexcrichton ready for review :). EDIT The library definitely isn't working right now - when I try to run the functions I get an error
I'm using an altered version of the hello_world example to test @ https://github.com/derekdreery/test_console_wasm . I'm not sure how to make progress. EDIT I think it's related to the fact that we are importing the module from another file - wasm-bindgen doesn't like this, but it's happy if we declare the module in the same file (see the bindings I manually constructed). |
I have no issue running your altered hello_world example. I remember when I first started looking at this project, I was getting a bunch of validation errors, that eventually went away when I restarted my system. I never really figured out what was causing the issue. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally this looks really good to me. I would really like to see some more testing here though. You could add something to the webidl-tests
crate if nothing is quite ready in web-sys
.
.operations | ||
.entry(identifier) | ||
.and_modify(|operation_data| operation_data.overloaded = true) | ||
.or_insert_with(Default::default) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could just use or_default
here.
pub(crate) operations: BTreeMap<Option<&'src str>, OperationData<'src>>, | ||
} | ||
|
||
impl<'src> NamespaceData<'src> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not just make this the implementation for Default
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason it's the way it currently is is that I felt implementing Default
is misleading - there isn't an 'obvious' default value for the partial
field. I thought that having separate methods helped convey meaning.
In fact, if I had followed my own plan I would not implement Default at all, I'd have 2 methods for partial and non-partial. I'll change to this so you can see what I mean, if you don't like it I can change it to just the Default :).
return Ok(()); | ||
} | ||
|
||
let rust_name = rust_ident(self.identifier.0.to_snake_case().as_str()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is repeated a lot. Maybe make a snake_case_ident
function in utils? (It would match the camel_case_ident
function`.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the camel_case_ident function exists to handle cases like HTMLBR
where the generic converter cannot deduce there should be a space, like HTML_BR
.
@ohanar I've tried updating everything (rust, npm, cargo) and building from scratch, and I still get the offest error. Very strange, and unfortunate since I can't debug what I've done. Anyway I can make the changes you suggested. What tests would you like - it's hard to test the output of the log functions as they don't return anything? |
I'm not sure what's causing the build error now - it's failing in chrome but not firefox. |
So it seems like the issue you are running up against is some upstream bug (see webpack/webpack#7760 and xtuc/webassemblyjs#407). As for tests, you could add something like this to namespace Math {
double pow(double base, double exponent);
} And then assert some things with it in a corresponding rust file. |
yep using webpack from I'll work on some tests. |
I've added some tests - I'm not sure they ran though because when I typed |
Try |
@afdw that worked (well it errored, but that's better than not running). |
Should be ready for review again. |
Looks good! |
Thanks for sticking with this PR @derekdreery, and thanks for the detailed reviews @ohanar! |
I'm opening this early to get feedback.
This PR aims to get namespaces supported, so you can do
So far it can generate code from the AST, which I reconstructed from the following code snippet which I checked compiles correctly.