whatwg / html

HTML Standard
https://html.spec.whatwg.org/multipage/
Other
8.1k stars 2.66k forks source link

Reflected IDL attributes of type FrozenArray<T>? don't create a FrozenArray #10219

Open petervanderbeken opened 7 months ago

petervanderbeken commented 7 months ago

What is the issue with the HTML Standard?

The getter steps for such attributes starts with:

  1. Let elements be this's attr-associated elements.

  2. If the contents of elements is equal to the contents of this's cached attr-associated elements, then return this's cached attr-associated elements.

  3. Let elementsAsFrozenArray be elements, converted to a FrozenArray<T>?.

Note that elements is a list created in compute the attr-associated elements. elementsAsFrozenArray is a FrozenArray<T>?. Step 3 refers to https://webidl.spec.whatwg.org/#es-type-mapping for converting the list to the FrozenArray<T>?, but that doesn't generally convert from an Infra type to a WebIDL type (in this case list to FrozenArray<T>?). I suppose it could use create a frozen array, but that should then be explicitly called.

I also wonder if step 2 is well-defined. After step 4 cached attr-associated elements is a FrozenArray<T>? value:

  1. Set this's cached attr-associated elements to elementsAsFrozenArray.

So step 2 compares the contents of a list to the contents of a FrozenArray<T>?, but a FrozenArray<T>?' value is a reference to an object that holds a fixed length array of unmodifiable values. I suppose it could compare to that array held by the object, but that would now contain JavaScript values (created in step 1 of create a frozen array).

@annevk, am I missing something?

annevk commented 7 months ago

I think you are correct that this is bogus.

annevk commented 7 months ago

I've put up some improvements in #10221 (also to address some feedback Olli at one point gave).

@mrego and @alice should probably look at the remainder and can maybe help review what I wrote.

alice commented 6 months ago

Belatedly looking at this; apologies for the delay.

Thanks for making the edits in #10221. I am particularly a fan of the change to "get the attr-associated elements".

The "cached attr-associated elements object" part is, I guess, necessary, but it seems like it could just as well have been a change to how FrozenArray is specified. At least, the implementation in Blink works like this:

I don't know enough about how FrozenArray is used elsewhere to know whether it would be possible to specify FrozenArray such that its array is an array of the IDL objects, and the process of converting an IDL FrozenArray is where the conversion of each element in array to the corresponding JavaScript object happens, but that's certainly how it works in Blink right now (at least as far as I understand it).

In that case, step 3 of the getter could quite straightforwardly call "create a frozen array", I think.

I guess that would still leave open the question of how to precisely word step 2; it might need to be something like "if elements contains the same sequence of elements as this's cached attr-associated elements"?

annevk commented 6 months ago

That might be possible with some changes to Web IDL. It's already roughly how you describe it I think. And yeah, ideally we define the equality operation a bit better. Unfortunately we don't have good helpers for that.

alice commented 6 months ago

I guess the tricky part is that the "Frozen" part of "FrozenArray" is a JavaScript concept: https://tc39.es/ecma262/multipage/abstract-operations.html#sec-setintegritylevel

So I guess to specify the IDL FrozenArray the way it's implemented in Blink, you'd have to have some kind of notion of the IDL FrozenArray being basically a container for an immutable sequence and a frozen JavaScript object containing the equivalent T values? As opposed to what it is now, which is really just the JavaScript object.

At which point, unless it's useful for something else for FrozenArray to work that way, we may as well stick with what you've written. I think if you squint, the Blink implementation is equivalent :P

domenic commented 6 months ago

I think that's doable though. Web IDL types are already defined to be wrappers around various things, e.g. we let Promise<T> be represented by PromiseCapability records, which is basically a tuple of (actual JS promise object, resolve function, reject function).

In general there's a lot of possible improvements to the way FrozenArray<> is defined in IDL; see e.g. the discussion in https://github.com/whatwg/webidl/issues/810.