[webidl] Use sequences instead of arrays

[wingo]

Arrays are no longer part of the current WebIDL standard. This change adapts the test IDL file appropriately, relaxes the WebIDL parser to continue to allow sequence attributes (until FrozenArray is implemented), and adapts the bindings generator.

The bindings generator changes result in more usage of type objects instead of interpreting aspects about argument and return types from the name.

See #8476.

[wingo]

@kripken I have corresponding changes to ammo.js here: kripken/ammo.js#263

@jgravelle-google you mentioned that you have some internal uses of webidl_binder.py; would you mind checking to see that they still work with this change? See the changes to test.idl in this PR. The generated .cpp / .js files do not change for test.idl; for ammo.js, the .cpp changed in what I think is an innocuous way (see ammo.js pr).

[wingo]

test-upstream-other check failure appears to be unrelated (test_binaryen_metadce_* things)

[kripken]

Arrays are no longer part of the current WebIDL standard. This change adapts the test IDL file appropriately, relaxes the WebIDL parser to continue to allow sequence attributes (until FrozenArray is implemented), and adapts the bindings generator.

It sounds like arrays were removed, sequences were added, but sequences are also going to be removed? I don't follow WebIDL development closely, but that sounds a lot of churn for the basic list type - is that typical?

[VirtualTim]

Current spec is here: https://www.w3.org/TR/WebIDL/#es-sequence.
Proposed spec is here: https://heycam.github.io/webidl/#es-sequence
Proposed spec for FrozenArrays: https://heycam.github.io/webidl/#es-frozen-array

It looks to me like sequences are not being removed, and if we want to conform to the (future) spec we'll need support for both sequences and FrozenArrays.

Oh, and this will obviously break existing projects that use webidl arrays. I think being more spec-compliant is worth it, but I think we should notify people somehow? Maybe the doco should be updated? https://github.com/emscripten-core/emscripten/blob/incoming/site/source/docs/porting/connecting_cpp_and_javascript/WebIDL-Binder.rst
Or release notes? https://github.com/emscripten-core/emscripten/blob/incoming/ChangeLog.md

[wingo]

In practice my understanding is that the current spec is https://heycam.github.io/webidl/#es-sequence. The w3 version is out of date. @Ms2ger can perhaps elaborate.

Sequences are a part of the current standard. They are also part of the WebIDL.py that is in emscripten. Arrays are gone, so it makes sense to move towards sequences.

Sequences are mostly the same thing as arrays except they are always passed by value, from a WebIDL spec point of view. So it doesn't make sense to have a sequence as an attribute, because you can't mutate its members -- if you got its value, it would give you a copy of the array, and then you would be mutating some copy! But, the emscripten WebIDL bindings generator wants to allow in-place mutation (and that's fine). The thing that most closely corresponds to that is this FrozenArray construct, but even Mozilla's parser doesn't support it. So, instead of making some kind of larger change here, my proposal is to allow sequences as attributes for the purposes of emscripten.

In some future we can consider using WebIDL.py in the opposite way, to parse IDL that describes platform interfaces (e.g. WebGL), as part of the ⛄ bindings proposal (https://github.com/WebAssembly/webidl-bindings/blob/master/proposals/webidl-bindings/Explainer.md, discussed in recent Wasm CG F2F). This would let games and other graphics-heavy emscripten apps avoid trampolining through JavaScript. So it makes sense to have a WebIDL parser that's closer to the standard. This concern goes in the opposite way too, for exposing emscripten-exported interfaces to other modules in the system (other wasm modules or callback interfaces from the platform).

I am sympathetic as regards churn, but I think your surprise @kripken is based on a misunderstanding -- sequences are here to stay, it was just in the attribute case that we might consider moving to FrozenArray in the future.

[wingo]

@VirtualTim definitely makes sense to update the release notes and documentation. We can also point people to the standalone WebIDL bindings generator if they need to stick with the old thing: https://gitlab.com/wingo/webidl-embindgen/tree/a13ce3d68328594eccac59a181bc76d3cfe82837 It's independent of emscripten.

[Ms2ger]

First of all - yes, please avoid documents on w3.org/TR/; they are by definition out-of-date. (The WebIDL one even more so than in the general case.)

My understanding is that using sequence<T> as operation arguments is fine for emscripten, and the interesting case is where T[] is used as the type of a read-write attribute. This situation, AIUI, does not involve actual JavaScript Array object in emscripten.

The two built-in kinds for lists in current WebIDL (sequence and FrozenArray) are both defined to create Array objects when converting to JavaScript values, and neither supports changing the backing storage. (sequence always returns a new object and doesn't keep track of it in the host; FrozenArray might keep track of the object and return it several times, but the object is frozen to ensure it can't be modified.)

It seems like the current emscripten behaviour is more similar to, for example, HTMLOptionsCollection which uses (something like)

interface HTMLOptionsCollection {
  getter HTMLOptionElement? (unsigned long index);
  setter void (unsigned long index, HTMLOptionElement? option);
};

which corresponds on the C++ side to something like

class HTMLOptionsCollection {
  HTMLOptionElement* get(uint32_t index);
  void set(uint32_t index, HTMLOptionElement* value);
}

... so the behaviour can, as far as I understand it, already be implemented in WebIDL as it exists today - without using sequence or FrozenArray. This is of course quite tedious, so we could extend WebIDL(-as-used-in-emscripten) with new syntax to autogenerate this arraylike interface and its implementation. This new syntax, to come full circle, could simply be T[].

This would mean the IDL used here would be rejected by a conforming WebIDL parser; however, a conforming WebIDL parser would also reject a sequence used as the type of an attribute, so this doesn't seem all that problematic. If we'd like to avoid that, we could attempt to add T[] to the WebIDL grammar, with a prose restriction on using it in the web platform.

Thoughts?

[VirtualTim]

@VirtualTim definitely makes sense to update the release notes and documentation. We can also point people to the standalone WebIDL bindings generator if they need to stick with the old thing: https://gitlab.com/wingo/webidl-embindgen/tree/a13ce3d68328594eccac59a181bc76d3cfe82837 It's independent of emscripten.

Actually I've changed my mind. I'm also making some changes to the WebIDL generator (#8706, #8859), so I think it would be better to merge these, then open a separate PR to update the doco, and try and get these all done in one release. That way people will only need to update their bindings once.

[jgravelle-google]

@jgravelle-google you mentioned that you have some internal uses of webidl_binder.py; would you mind checking to see that they still work with this change?

I don't remember mentioning this, which is to say I don't know of any, so I have no useful data here either way.

[kripken]

Thanks for the details everyone! Sounds good to me. And specifically sounds good to try to get the changes into one release, to minimize churn for users (as much as we can).

[kripken]

Updating the changelog in this PR makes sense too I think. But as mentioned earlier we can hopefully have all the changelog changes in one release, and can mention them all at once to users.

[kripken]

these lines look surprising - is it saying that a function returning sequence<long> returns long? Or does inner mean something else?

[wingo]

Ah, very good point! I didn't think that webidl bindings supported returning array values, and that attribute getters for arrays were the only case. I was wrong. I was confused by the fact that the getter for render_function was called with the array type's .name property; but of course this is without the []. Instead for array (or rather, sequence) attribute getters I should call render_function with the sequence's .inner.type and remove this case.

[wingo]

I fixed handling of sequence return types, which now behaves as you'd expect.

[kripken]

the const code seems like a new addition - I don't see any previous handling of const that it replaces? Is that a bugfix?

[kripken]

lgtm aside from that one question, nice!

[sbc100]

The deletion of the incoming branch triggered the automatic closing of this branch. My apologies. This was an unexpected side effect.

Re-opening and re-targeting to master.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.

[stale]

This issue has been automatically marked as stale because there has been no activity in the past year. It will be closed automatically if no further activity occurs in the next 30 days. Feel free to re-open at any time if this issue is still relevant.