WebIDL Enum Support #433

Slowki · 2018-07-10T02:04:17Z

Adds basic support for enums to the WebIDL interface generator.

Per the consensus on #260 the enums are represented as normal Rust enums. Right now I'm just having it panic when an invalid string is converted, I was thinking of adding something along the lines of this nice hack instead, but I wanted some feedback first to see if there's a better approach.

ohanar · 2018-07-10T05:39:41Z

I haven't had the time to look through your code thoroughly (will make sure to get a chance tomorrow), but upon a first pass through everything seems fine.

I think the hack you suggested sounds like a good approach, nothing better comes to mind immediately (when I commented on the issue, I think I was thinking of doing the same thing).

alexcrichton · 2018-07-10T14:04:09Z

crates/backend/src/codegen.rs

+
+                fn into_abi(self, extra: &mut ::wasm_bindgen::convert::Stack) -> Self::Abi {
+                    match self {
+                        #(#variant_paths_ref => ::wasm_bindgen::JsValue::from_str(#variant_strings).into_abi(extra),)*


Could this use the From impl below to convert this to a JsValue?

alexcrichton · 2018-07-10T14:04:34Z

Nice! Just one small comment but otherwise this looks great to me

fitzgen

Thanks for the pull request @Slowki :)

fitzgen · 2018-07-10T17:01:22Z

crates/backend/src/codegen.rs

+                fn from(obj: ::wasm_bindgen::JsValue) -> #name {
+                    let obj_str = match obj.as_string() {
+                        Some(string_value) => string_value,
+                        None => panic!("Can't convert a non-string into {}", #name_string),


I agree that when we receive a non-string value or non-enum string value as an argument to a function that expects this enum, we should panic.

However, I don't think we should implement From<JsValue> for these and have it panic. This is too much of a footgun for folks who are just reading docs and see the impl and don't know what it does under the hood.

Instead, I think we should make a normal, non-trait pub fn from_js_value(JsValue) -> Option<Self> method, and the FromWasmAbi trait impl can unwrap the result. This gives us the same behavior when sending them to and from the ABI, but without the footgun.

I think panicking in FromWasmAbi is something we should avoid so that we can recover in cases where JS functions return invalid string values.

Example

foo.webidl

enum Foo { "bar", "baz" } Foo getFoo();

foo.js

getFoo() { return "quux" }

somefile.rs

// Panics match getFoo() { Foo::Bar => ..., Foo::Baz => ..., }

Possible Solutions

We could use this idiom from libstd and return __Invalid instead of panicking

enum Foo { Bar, Baz, #[doc(hidden)] __Invalid, }

We could just expose a catch-all variant

enum Foo { Bar, Baz, InvalidValue(String), }

Or we could make WebIDL functions which return enums return Options.

+1. I definitely wouldn't want my web app to panic if a browser gets updated with an updated version of a standard, and I haven't gotten around to upgrading my copy of web-sys.

The main downside to including the invalid value in the actual enum is that the enum no longer can be copy. I would be inclined to either just include an __Invalid variant, or have WebIDL functions returning enums to instead return Results, with an error type that contains the invalid string.

I like the Result solution the best, it seems like it's the most convenient for the end user. The only downside is that functions that throw will end up returning Result<Result<Enum, String>, ...>, but there might be a nice way to flatten that out.

Another advantage of having non exhaustive enums is that we won't have to do breaking version bumps when updating WebIDLs.

I am working on adding support for partial and mixin interfaces right now. As part of those changes, I am doing a two pass process that would make it trivial to implement the Result return type solution. (This is in part to fix a bug in the current crate where interface objects are being passed by value, rather than by reference, as per the spec.)

I should have time today to finish up a first draft, and submit a PR. It is based off of this branch, so I would return to the non-exhaustive enum problem in a follow-up PR (especially if you want to go down the Result path).

@ohanar Sounds good. I'll make from_js_value visible and resolve the merge conflict, then I'm good with this being merged.

@fitzgen That's a good point, definitely a huge + for non-exhaustive over Results.

Add enum support to the WebIDL interface generator.

* Remove From<JSValue> for ENUM * Add `from_js_value` method which returns an Option<ENUM>

fitzgen

Thanks!

ohanar

I am happy with merging this now, and revisiting the issue of how to handle non-exhaustive enums in the future.

alexcrichton · 2018-07-11T14:53:04Z

crates/backend/src/codegen.rs

+            }
+
+            impl #name {
+                fn from_js_value(obj: ::wasm_bindgen::JsValue) -> Option<#name> {


This may want to use #vis out in front to allow this to be used outside the module perhaps?

Make from_js_value match the visibility of the enum it's associated with

Slowki · 2018-07-11T19:38:39Z

Feel free to merge, I'll make a separate PR for the non-exhaustive enums issue.

Update the enum test to match the new constructor return values

fitzgen · 2018-07-11T21:31:12Z

Feel free to merge, I'll make a separate PR for the non-exhaustive enums issue.

SGTM! Thanks @Slowki :)

Slowki force-pushed the feat/basic-enum-support branch from 28adf58 to 5afca02 Compare July 10, 2018 02:37

ohanar self-requested a review July 10, 2018 04:31

alexcrichton reviewed Jul 10, 2018

View reviewed changes

fitzgen reviewed Jul 10, 2018

View reviewed changes

Slowki added 3 commits July 10, 2018 20:28

webidl: initial enum support

a981dfd

Add enum support to the WebIDL interface generator.

webidl: replace From trait with from_js_value method for enums

c56b1b0

* Remove From<JSValue> for ENUM * Add `from_js_value` method which returns an Option<ENUM>

webidl: use JSValue::from in IntoWasmAbi for enums

6608828

Slowki force-pushed the feat/basic-enum-support branch from 5afca02 to 6608828 Compare July 11, 2018 00:29

fitzgen approved these changes Jul 11, 2018

View reviewed changes

ohanar reviewed Jul 11, 2018

View reviewed changes

alexcrichton reviewed Jul 11, 2018

View reviewed changes

Slowki added 2 commits July 11, 2018 15:34

Merge branch 'master' into feat/basic-enum-support

4c7b130

webidl: make from_js_value visible for enums

fac73a2

Make from_js_value match the visibility of the enum it's associated with

webidl: update enum test to unwrap constructor results

5520da1

Update the enum test to match the new constructor return values

fitzgen merged commit 9c7b15e into rustwasm:master Jul 11, 2018

Slowki deleted the feat/basic-enum-support branch July 11, 2018 22:31

Slowki mentioned this pull request Jul 14, 2018

WebIDL: Handle Invalid Enum Returns #477

Merged

Uh oh!

WebIDL Enum Support #433

WebIDL Enum Support #433

Uh oh!

Conversation

Slowki commented Jul 10, 2018

Uh oh!

ohanar commented Jul 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alexcrichton commented Jul 10, 2018

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Slowki Jul 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Example

Possible Solutions

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fitzgen left a comment

Choose a reason for hiding this comment

Uh oh!

ohanar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Slowki commented Jul 11, 2018

Uh oh!

fitzgen commented Jul 11, 2018

Uh oh!

Uh oh!

Slowki Jul 11, 2018 •

edited

Loading