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

Support [NoInterfaceObject] in web-sys #1449

Merged
merged 1 commit into from
Apr 15, 2019

Conversation

alexcrichton
Copy link
Contributor

This commit enables [NoInterfaceObject] annotated interfaces in
web-sys. The NoInterfaceObject attribute means that there's not
actually a JS class for the object, but all of its properties and such
can still be accessed structually and invoked. This should help provide
more bindings for some more common types on the web!

Note that this builds on recent features to ensure that dyn_into and
friends always fail for NoInterfaceObject objects because they don't
actually have a class.

Closes #893
Closes #1257
Closes #1315

@RReverser
Copy link
Member

Note that this builds on recent features to ensure that dyn_into and
friends always fail for NoInterfaceObject objects because they don't
actually have a class.

I'm not very familiar with NoInterfaceObject semantics to be honest, but is that behaviour really intentional? Or would it require some structural comparison instead for duck-typing?

@alexcrichton
Copy link
Contributor Author

According to the WebIDL spec it looks like it's just duck typing for these interfaces (and intentional that it is). It's also noted though that this is a legacy feature which shouldn't be used by anything new, so it's just compat with older APIs for now.

@RReverser
Copy link
Member

it looks like it's just duck typing for these interfaces (and intentional that it is)

I see. So that's why I wonder if false is correct or one has to actually check the keys to make sure that the object corresponds to the duck interface. Although that would be too expensive I guess.

Another alternative is maybe panicking in is_type_of to make it clear that one can convert to/from these objects only using unchecked_ APIs?

@Pauan
Copy link
Contributor

Pauan commented Apr 12, 2019

Although that would be too expensive I guess.

Since it's only run on dyn_into (and similar), I think expensive-but-correct is better than fast-but-useless.

Another alternative is maybe panicking in is_type_of to make it clear that one can convert to/from these objects only using unchecked_ APIs?

I think that's a reasonable compromise. I like it.

@alexcrichton
Copy link
Contributor Author

FWIW I think we in general just need to expose the bindings found in the WebIDL as opposed to ensuring that all the conversions all make sense one way or another. It's already the case that calling is_instance_of will cause a JS exception to be thrown because most of these types aren't actually defined in JS so x instanceof ANGLE_instance_array will fail.

I think that gracefully failing here is probably better than panicking because it's true that there's no instances of this class. If a panic happens it'd be due to an unwrap which would still be loud and in theory easy to spot.

I don't think we can probe for the structure of an object to see if it's of the right type, because although it may have properties there's no guarantee they're functions with the right arguments and right return values. I think that unconditionally returning false and failing all conversions is the right behavior here.

@RReverser
Copy link
Member

Your arguments make sense to me. Neither approach seems actually 100% correct, just implementing different tradeoffs, so I don't have a strong opinion on which one to choose.

Perhaps the only good thing we could do in the future is actually separating types that have underlying named class vs those that don't, into own traits, so that these cases would be forbidden at compile-time.

@alexcrichton alexcrichton requested a review from fitzgen April 15, 2019 15:41
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

I have no IFace but I must scream

... it was an attempt :-p

@@ -45,6 +45,7 @@ pub(crate) struct FirstPassRecord<'src> {
pub(crate) struct InterfaceData<'src> {
/// Whether only partial interfaces were encountered
pub(crate) partial: bool,
pub(crate) has_no_interface: bool,
Copy link
Member

Choose a reason for hiding this comment

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

the negative is a bit funky (vs has_interface: bool) but I guess it makes sense in this case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nah seems reasonable to me to switch!

This commit enables `[NoInterfaceObject]` annotated interfaces in
`web-sys`. The `NoInterfaceObject` attribute means that there's not
actually a JS class for the object, but all of its properties and such
can still be accessed structually and invoked. This should help provide
more bindings for some more common types on the web!

Note that this builds on recent features to ensure that `dyn_into` and
friends always fail for `NoInterfaceObject` objects because they don't
actually have a class.

Closes rustwasm#893
Closes rustwasm#1257
Closes rustwasm#1315
@alexcrichton alexcrichton force-pushed the no-interface-objects branch from f6be58d to 01a5223 Compare April 15, 2019 17:36
@alexcrichton alexcrichton merged commit 3ab9bb1 into rustwasm:master Apr 15, 2019
@alexcrichton alexcrichton deleted the no-interface-objects branch April 15, 2019 17:36
bors bot added a commit to grovesNL/glow that referenced this pull request Oct 15, 2019
51: Add WebGL Extensions r=grovesNL a=TannerRogalsky

This enables and tracks all supported WebGL extensions. It also uses those extensions to add vertex array objects and instanced drawing to WebGL1 when supported.

Some areas of potential interest:
- stdweb's VAO extension implementation returns a different type than the non-extension implementation. I've resolved this by tracking the extension resource separately but with the same key type.
- WebGL extensions are all `[NoInterfaceObject]`s and aren't represented as JS classes and therefore can't be cast into Rust classes in the normal way (using `JsCast::dyn_into`). Instead `unchecked_into` is used along with the web_sys-provided Rust classes to duck-type these extensions. As I understand it, [this is the expected way](rustwasm/wasm-bindgen#1449).
- I've employed a macro to cut down on code duplication around the web_sys extension creation because their WebGL1 and WebGL2 context share an API but not a useful trait.

I'm of course open to any suggestions on how to improve this.

Co-authored-by: Tanner Rogalsky <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants