google / closure-compiler

A JavaScript checker and optimizer.
https://developers.google.com/closure/compiler/
Apache License 2.0
7.41k stars 1.15k forks source link

Define ArrayBufferView and TypedArray as interfaces rather than classes #4181

Open niloc132 opened 4 months ago

niloc132 commented 4 months ago

Technically, the former appears to be a typedef and the latter might be a template for classes? But neither exists as a named function/constructor. Earlier versions of the spec do indicate that both were at one point interfaces though.

This impacts https://github.com/google/elemental2, which then tries to use both types on the right side of JS instanceof operations, which will fail ("Right-hand side of 'instanceof' is not an object").

As an alternative, one or both could be a typedef as the second link below seems to say? The archive.org link shows the older version where both were defined as interfaces.

https://tc39.es/ecma262/multipage/ordinary-and-exotic-objects-behaviours.html#typedarray https://webidl.spec.whatwg.org/#ArrayBufferView http://web.archive.org/web/20150108144329/https://www.khronos.org/registry/typedarray/specs/latest/

gkdn commented 4 months ago

@mollyibot Could you patch the PR internally and see if it causes any problems?

gkdn commented 3 months ago

@jDramaix Could you first provide general feedback on the direction before asking Colin to invest more time adapting the solution?

niloc132 commented 3 months ago

I apologize for the churn, and am happy to spend some time exploring to make this work.

The issue I'm seeking to resolve is that when jsinterop-generator builds elemental2, it sees these types and emits classes for them rather than interfaces, and so when a typedarray of some kind is used generically, the compiler emits casts, and those will fail since there is no constructor called TypedArray or ArrayBufferView in the global context.

Note that there is a constructor called TypedArray, so it is not fully accurate to call this an interface:

> (new Uint8Array()).__proto__.__proto__.constructor 
< function TypedArray()

However, this is not accessible on the "global this", so cannot be used when performing instanceof checks. In theory GWT/J2CL could make the call above to find it, but that seems like more trouble than it would be worth?

Please let me know if there's another straightforward direction I can pursue here. I did not start trying to build elemental2 yet from this change, imagining incorrectly that it would just change the inheritance structure to the desired format. I can take a closer in a week or two, and hold off on this change for now, or if you see a fruitful direction for me to try I would be happy to follow that?

--

At a glance, I'm not clear how to override the closure-compiler repo used by elemental2 - I followed the steps provided, but my changes to closure-compiler don't seem to be reflected in the generated output (both in that I still see public class Float64Array extends TypedArray, and I don't see the failures @mollyibot mentioned above). Given that closure-compiler is now a bazel project, it looks like I want something more like

local_repository(
    name = "com_google_javascript_closure_compiler",
    path = "/home/colin/workspace/closure-compiler",
#    build_file = "jscomp.BUILD",
)

but that doesn't quite get it right - it looks like transitive dependencies from closure aren't picked up this way (@google_bazel_common, @com_github_johnynek_bazel_jar_jar, etc), and inlining most of closure's WORKSPACE then results in other issues...

jDramaix commented 3 months ago

I agree that defining TypedArray and ArrayBufferView as interfaces is the correct approach. This aligns with how they're defined in JavaScript.

To have this change compiling successfully, we need to modify several things:

Note that this change is likely to be a breaking change internally, so it may require some time before merging it.

jDramaix commented 3 months ago

At a glance, I'm not clear how to override the closure-compiler repo used by elemental2 -

We used the extern files coming the closure compiler jar embedded in rules_closure We expect to have a @com_google_javascript_closure_compiler//:externs target representing a zip file containing all the extern files.
You should be able to override that at the end of the WORKSPACE file