Implement support for WebIDL dictionaries #702

alexcrichton · 2018-08-14T17:22:26Z

This commit adds support for generating bindings for dictionaries defined in
WebIDL. Dictionaries are associative arrays which are simply objects in JS with
named keys and some values. In Rust given a dictionary like:

dictionary Foo {
    long field;
};

we'll generate a struct like:

pub struct Foo {
    obj: js_sys::Object,
}

impl Foo {
    pub fn new() -> Foo { /* make a blank object */ }

    pub fn field(&mut self, val: i32) -> &mut Self {
        // set the field using `js_sys::Reflect`
    }
}

// plus a bunch of AsRef, From, and wasm abi impls

At the same time this adds support for partial dictionaries and dictionary
inheritance. All dictionary fields are optional by default and hence only have
builder-style setters, but dictionaries can also have required fields. Required
fields are exposed as arguments to the new constructor.

Closes #241

ohanar

Mostly looks good. The main thing that I'm concerned about is exposing the JS Object that is being used under the hood, I'm not sure that is necessarily forward compatible with host binding support. I would remove the conversion impls there, and add getters so that dictionaries are usable as a return type (getters should take into account default values).

ohanar · 2018-08-14T19:46:49Z

crates/webidl/src/lib.rs

+                    }
+                }
+            }
+            dst[start..].sort_by_key(|f| f.name.clone());


This is sorting by the snake case identifier, rather than the original identifier, which could produce the wrong result in edge cases:

dictionary Example { boolean fooBaz; boolean foo_bar; };

That is correctly ordered per the spec, but this line will reorder it to [foo_bar, foo_baz].

Good point! I'll add a comment because I think this is largely good enough for now, it's basically just generating the order of required fields for the constructor in any case.

ohanar · 2018-08-14T19:48:36Z

crates/backend/src/codegen.rs

+                    &JsValue::from(val),
+                );
+                self
+            }


Without corresponding getters, if a dictionary is returned, it is clumsy to use (have to upcast it to an Object, which I have concerns about).

True! The limitation to not providing getters right now is that there isn't a great way to compile them in and generate code which works. I think we'll definitely want to add getters though! (this is just an initial implementation)

ohanar · 2018-08-14T19:52:10Z

crates/backend/src/codegen.rs

+                }
+
+                // interop w/ Object
+                impl From<#name> for Object {


I don't think there is any guarantee that we could do this with the host bindings proposal. In particular, I don't see any mention that WebIDL dictionaries have to be ECMAScript Objects. (This is really more of a concern for the AsRef and AsMut impls than the conversion impl, but I think we should implement all or none.)

ohanar · 2018-08-14T19:53:43Z

crates/backend/src/codegen.rs

+        let name = &self.name;
+        let ty = &self.ty;
+        (quote! {
+            pub fn #name(&mut self, val: #ty) -> &mut Self {


You use pub here, even though the Dictionary might not have a public visibility.

True! Dictionaries are only generated by WebIDL though so they'll always be pub anyway. I think I'll remove the visibility field there and just hardcode everything to public

ohanar · 2018-08-14T19:55:03Z

crates/webidl/src/first_pass.rs

+impl<'src> FirstPass<'src, ()> for weedle::PartialDictionaryDefinition<'src> {
+    fn first_pass(&'src self, record: &mut FirstPassRecord<'src>, (): ()) -> Result<()> {
+        record.dictionaries.entry(self.identifier.0)
+            .or_insert_with(Default::default)


Maybe or_default() here (and above)?

ohanar · 2018-08-14T20:23:34Z

crates/backend/src/codegen.rs

+                impl<'a> OptionIntoWasmAbi for &'a #name {
+                    fn none() -> Self::Abi { <&'a Object>::none() }
+                }
+                impl<'a> OptionFromWasmAbi for #name {


Unused lifetime.

ohanar · 2018-08-14T20:25:14Z

crates/backend/src/codegen.rs

+                        #name { obj: Object::from_abi(abi, extra) }
+                    }
+                }
+


Missing RefFromWasmAbi.

eminence · 2018-08-14T21:07:59Z

One places that uses a Dictionary is AudioContext.webidl:

dictionary AudioContextOptions {
             float        sampleRate = 0;
};

[Pref="dom.webaudio.enabled",
 Constructor(optional AudioContextOptions contextOptions)]
interface AudioContext : BaseAudioContext {
 //...
}

While the AudioContextOptions dictionary gets correctly generated, and a no-arg constructor is generated for the AudioContext struct, there is no AudioContext constructor that accepts an AudioContextOptions struct. Is this expected? (edit: expected given the state of this PR. That is, are the other known issues that would prevent the other constructor from being generated?)

fitzgen · 2018-08-14T19:10:14Z

crates/backend/src/codegen.rs

            }
        }
+        for d in self.dictionaries.iter() {
+            d.to_tokens(tokens);


Is ToTokens not implemented for &[T] or something? It seems like we shouldn't need to write this loop out by hand.

Nah it's not currently implemented because I think it's ambiguous whether you want bracket delimiters or everything just concatenated together

fitzgen · 2018-08-14T19:14:09Z

crates/backend/src/codegen.rs

+            .map(|f| &f.ty)
+            .collect::<Vec<_>>();
+        let required_names2 = required_names;
+        let required_names3 = required_names;


Is this still necessary even though required_names is a reference (and therefore is Copy)? I thought the trick of making a thing a reference usually fixed the ownership issues with quasi quoting...

Unfortunately yeah, it has to do with the mystery of the quote! macro, although I'm not quite sure why. If the same variable name is used below it generates a bunch of scary compiler errors.

alexcrichton · 2018-08-14T21:34:33Z

Thanks for taking a look @ohanar! It's a good point that I don't think we quite know how this is going to map to the host bindings proposal. We could definitely be more conservative and expose none of the internal details about Object.

I'm not sure what the best route here is. I don't think we know how this is gonna end up with host bindings (these functions may just not ever be bound with host bindings?). In any case though I think it's definitely the case that we're going to publish multiple major versions of web-sys. There's a lot of API design decisions we're only guessing about and we're definitely going to get some of them wrong. In that sense I'd probably be in favor of exposing the impls today with the assumption that we may very well reverse this decision in the future if a better binding strategy comes about.

alexcrichton · 2018-08-14T22:33:04Z

@eminence turned out to be a bug, should be fixed now!

alexcrichton · 2018-08-14T22:56:11Z

crates/webidl/src/idl_type.rs

+            // Note that these map to `f64` because JS doesn't really have a
+            // native type for u64, so WebIDL surely just means a JS number
+            IdlType::LongLong => Some(ident_ty(raw_ident("f64"))),
+            IdlType::UnsignedLongLong => Some(ident_ty(raw_ident("f64"))),


This is a pretty subtle update in this PR, but lots of dictionaries apparently have long long specified in them. It's unclear what WebIDL could mean other than f64 for JS because 64-bit integers aren't possible, so these two now map to f64

From the spec:

The long long type is a signed integer type that has values in the range [−9223372036854775808, 9223372036854775807].

In other words, WebIDL says a long long is a 64-bit integer. Presumably with host-bindings this won't be an issue, it is however an issue with current day shims. I don't think we should change all the signatures of methods just because of modern day limitations.

ohanar · 2018-08-14T23:46:43Z

@alexcrichton My vote would be to be on the more conservative side. I don't see that much value added by having the ability to convert to an Object, so the cost/value ratio doesn't really add up for me.

alexcrichton · 2018-08-15T06:30:13Z

@ohanar makes sense yeah, but the JS object under the hood is currently what's powering dictionaries-in-dictionaries, so I'm not sure we can avoid exposing this detail without removing that functionality? I don't want to be too conservative at the loss of functionality in the sense that we have tons of time to iterate on this crate.

The most recent commit should also avoid modifying the WebIDL and manually skips unimplemented dictionaries and i64/u64 in dictionaries.

ohanar · 2018-08-15T08:08:48Z

Well, really conversion to a JSValue is needed to power dictionaries-in-dictionaries, not conversion to js_sys::Object (which was my main complaint).

alexcrichton · 2018-08-15T15:10:48Z

@ohanar oh interesting, can you expand on why JsValue is ok but Object isn't? I figured that we'd want to remove both to be conservative but having a conversion to JsValue seems like it'd be natural to have a conversion to Object as well (in that adding Object isn't really that much more of a promise in the API, the conversion to JsValue is already the big one)

eminence · 2018-08-15T15:16:56Z

With the latest fix, I confirm that the missing web audio constructors are now available, and that I was able to successfully get a sine-wave demo working! This obviously isn't a rigorous test of every dictionary, but I'm very happy that this particular one is working 😃 🎶

This commit adds support for generating bindings for dictionaries defined in WebIDL. Dictionaries are associative arrays which are simply objects in JS with named keys and some values. In Rust given a dictionary like: dictionary Foo { long field; }; we'll generate a struct like: pub struct Foo { obj: js_sys::Object, } impl Foo { pub fn new() -> Foo { /* make a blank object */ } pub fn field(&mut self, val: i32) -> &mut Self { // set the field using `js_sys::Reflect` } } // plus a bunch of AsRef, From, and wasm abi impls At the same time this adds support for partial dictionaries and dictionary inheritance. All dictionary fields are optional by default and hence only have builder-style setters, but dictionaries can also have required fields. Required fields are exposed as arguments to the `new` constructor. Closes wasm-bindgen#241

alexcrichton · 2018-08-16T00:10:00Z

I've gone ahead and removed the asref/asmut/from implementations for Object

ohanar · 2018-08-16T20:05:46Z

So I guess my concern is that Objects are much more general maps than dictionaries (which are more akin to structs), and I would be a bit surprised if they were re-used for host bindings, just out of concern of efficiency. On the other hand, I would be less surprised if dictionaries were still opaque data structures, so they would presumably be a subtype of anyref, which will likely be the underlying type for a JsValue in the future.

This is obviously me making guesses, but I generally want to take as conservative of an approach as reasonably possible, just so we can minimize potential breakage in the future.

alexcrichton mentioned this pull request Aug 14, 2018

Support creating bindings for WebIDL dictionaries #241

Closed

ohanar suggested changes Aug 14, 2018

View reviewed changes

ohanar reviewed Aug 14, 2018

View reviewed changes

crates/backend/src/codegen.rs

#name { obj: Object::from_abi(abi, extra) }

}

}

Copy link

Contributor

ohanar Aug 14, 2018

Choose a reason for hiding this comment

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

Missing RefFromWasmAbi.

fitzgen approved these changes Aug 14, 2018

View reviewed changes

alexcrichton force-pushed the dictionaries branch from c1b3964 to 7e2a90b Compare August 14, 2018 21:31

alexcrichton force-pushed the dictionaries branch from 7e2a90b to 205bca9 Compare August 14, 2018 22:31

alexcrichton force-pushed the dictionaries branch from 205bca9 to e4e5271 Compare August 14, 2018 22:44

alexcrichton commented Aug 14, 2018

View reviewed changes

alexcrichton force-pushed the dictionaries branch from e4e5271 to 1ddb758 Compare August 15, 2018 06:03

alexcrichton force-pushed the dictionaries branch from 1ddb758 to 6af417e Compare August 16, 2018 00:04

alexcrichton force-pushed the dictionaries branch from 6af417e to d6e4819 Compare August 16, 2018 00:08

ohanar approved these changes Aug 16, 2018

View reviewed changes

ohanar merged commit 25b6f5d into wasm-bindgen:master Aug 16, 2018

alexcrichton deleted the dictionaries branch August 20, 2018 21:25

Sixshaman mentioned this pull request Mar 27, 2022

The order of constructor arguments in dictionary bindings is not obvious #2845

Open

daxpedda mentioned this pull request Jun 6, 2023

Interning internal enum representations and field names #3468

Closed

Uh oh!

Implement support for WebIDL dictionaries #702

Implement support for WebIDL dictionaries #702

Uh oh!

Conversation

alexcrichton commented Aug 14, 2018

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!

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!

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!

eminence commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

alexcrichton commented Aug 14, 2018

Uh oh!

alexcrichton commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ohanar Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ohanar commented Aug 14, 2018

Uh oh!

alexcrichton commented Aug 15, 2018

Uh oh!

ohanar commented Aug 15, 2018

Uh oh!

alexcrichton commented Aug 15, 2018

Uh oh!

eminence commented Aug 15, 2018

Uh oh!

alexcrichton commented Aug 16, 2018

Uh oh!

ohanar commented Aug 16, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

eminence commented Aug 14, 2018 •

edited

Loading

ohanar Aug 14, 2018 •

edited

Loading