Bug 232734 – (GPUDeviceLostReason or undefined) in an IDL cannot be compiled

• Component: Bindings
• Status: NEW

[Myles C. Maxfield]

(GPUDeviceLostReason or undefined) is in WebGPU.

[Radar WebKit Bug Importer]

<rdar://problem/85318712>

[Dan Glastonbury]

Created attachment 453360 [details]
Patch

[Dan Glastonbury]

I implemented this as the spec specifies, although `GPUDeviceLostReason?` maybe be more idiomatic IDL.

[Sam Weinig]

Comment on attachment 453360 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=453360&action=review

This should include new tests in Source/WebCore/bindings/scripts/tests/ (or at least extend one of the existing tests).

> Source/WebCore/bindings/IDLTypes.h:89
> +struct IDLUnionUndefined : IDLType<std::monostate> {
> +};

It would be nice if could find a way to just have a single type representing undefined. I expect that the other one uses IDLType<void> as its base, which is why you aren't using it, but I'd like to understand what would have to change if updated IDLUndefined to this. Probably would need a specialization of toJS and CallbackResult. We already special case undefined for operations in the code generator.

Another option would be to extend IDLUndefined and add a new type alias override for union member like we already have for things like ParameterType, SequenceStorageType, etc.

If we go with this, please move the trailing } to the previous line to match IDLNull.

> Source/WebCore/bindings/js/JSDOMConvertUnion.h:417
> +template<> struct JSConverter<IDLUnionUndefined> {

This should go in its own JSDOMConvertUnionUndefined.h

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6911
> +    my $numberOfUndefinedMembers =
> +        GetNumberOfUndefinedMemberTypes($idlUnionType);

This should be on one line.

> Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6912
> +    assert("Union types must only hav 0 or 1 undefined types.") if $numberOfUndefinedMembers > 1;

Typo. "hav" -> "have"

That said, I am not sure checking for multiple undefined types is necessary. In general we expect all the types to be unique (or rather distinguishable https://webidl.spec.whatwg.org/#dfn-distinguishable), and we do the nullable check because you can have disperate types that are nullable (e.g. long? and object?) which we want to avoid.

I don't remember if we currently check for mutual distinguishability, but that would be the place where this would happen.

[Sam Weinig]

Not making this union specific would have the additional benefit that it should work for operation arguments as well (which I would be don't work today), e.g. float `foo(undefined arg1, boolean arg2)`, which we would probably want the c++ to model as `float foo(std::monostate arg1, bool arg2)`.

[Dan Glastonbury]

(In reply to Sam Weinig from comment #4)
> Comment on attachment 453360 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=453360&action=review
> 
> This should include new tests in Source/WebCore/bindings/scripts/tests/ (or
> at least extend one of the existing tests).

Thanks. I'll add some tests.

> > Source/WebCore/bindings/IDLTypes.h:89
> > +struct IDLUnionUndefined : IDLType<std::monostate> {
> > +};
> 
> It would be nice if could find a way to just have a single type representing
> undefined. I expect that the other one uses IDLType<void> as its base, which
> is why you aren't using it, but I'd like to understand what would have to
> change if updated IDLUndefined to this. Probably would need a specialization
> of toJS and CallbackResult. We already special case undefined for operations
> in the code generator.

The problem is that `void` is to a valid type for `std::variant`, you need to use `std::monostate`, and it must be the first type if you want a default constructible variant with an "uninitialised state".

> Another option would be to extend IDLUndefined and add a new type alias
> override for union member like we already have for things like
> ParameterType, SequenceStorageType, etc.

I did consider that. If your happy that I add another type alias, such as `UnionStorageType`, I'll make that change.

> If we go with this, please move the trailing } to the previous line to match
> IDLNull.
> 
> > Source/WebCore/bindings/js/JSDOMConvertUnion.h:417
> > +template<> struct JSConverter<IDLUnionUndefined> {
> 
> This should go in its own JSDOMConvertUnionUndefined.h

> 
> > Source/WebCore/bindings/scripts/CodeGeneratorJS.pm:6912
> > +    assert("Union types must only hav 0 or 1 undefined types.") if $numberOfUndefinedMembers > 1;
> 
> Typo. "hav" -> "have"

Oops.

> That said, I am not sure checking for multiple undefined types is necessary.
> In general we expect all the types to be unique (or rather distinguishable
> https://webidl.spec.whatwg.org/#dfn-distinguishable), and we do the nullable
> check because you can have disperate types that are nullable (e.g. long? and
> object?) which we want to avoid.

I added the check for undefined to move it to the front of the list. I cribbed the check of 0-or-1 from the Nullable check but I can remove that part.

> I don't remember if we currently check for mutual distinguishability, but
> that would be the place where this would happen.

Thanks.

[Dan Glastonbury]

(In reply to Sam Weinig from comment #5)
> Not making this union specific would have the additional benefit that it
> should work for operation arguments as well (which I would be don't work
> today), e.g. float `foo(undefined arg1, boolean arg2)`, which we would
> probably want the c++ to model as `float foo(std::monostate arg1, bool
> arg2)`.

Is that allowed by the spec? https://webidl.spec.whatwg.org/#idl-undefined

```
undefined must not be used as the type of an argument in any circumstance (in an operation, callback function, constructor operation, etc), or as the type of a dictionary member, whether directly or in a union. Instead, use an optional argument or a non-required dictionary member.
```

[Sam Weinig]

(In reply to Dan Glastonbury from comment #7)
> (In reply to Sam Weinig from comment #5)
> > Not making this union specific would have the additional benefit that it
> > should work for operation arguments as well (which I would be don't work
> > today), e.g. float `foo(undefined arg1, boolean arg2)`, which we would
> > probably want the c++ to model as `float foo(std::monostate arg1, bool
> > arg2)`.
> 
> Is that allowed by the spec? https://webidl.spec.whatwg.org/#idl-undefined
> 
> ```
> undefined must not be used as the type of an argument in any circumstance
> (in an operation, callback function, constructor operation, etc), or as the
> type of a dictionary member, whether directly or in a union. Instead, use an
> optional argument or a non-required dictionary member.
> ```

Ah, fair point. I did a quick scan of the grammar but missed this.

[Myles C. Maxfield]

*** Bug 236723 has been marked as a duplicate of this bug. ***

[Ahmad Saleem]

Chromium / Blink -

https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/webgpu/gpu_device_lost_info.idl?q=GPUDeviceLostReason%20IDL

and Gecko - https://searchfox.org/mozilla-central/source/testing/web-platform/tests/interfaces/webgpu.idl

Webkit - https://github.com/WebKit/WebKit/blob/004c145fcf53a9781a9d225eaf65d0c6443a6994/Source/WebCore/Modules/WebGPU/GPUDeviceLostReason.idl

______

Just want to add updated IDL files links to all browsers. Thanks!

[Chris Dumez]

Looks like Ryosuke would need this for https://bugs.webkit.org/show_bug.cgi?id=255778 too.

[Dan Glastonbury]

I believe it was decided that `GPUDeviceLostReason or undefined` is not idiomatic IDL and should be `GPUDeviceLostReason?`

[Ryosuke Niwa]

(In reply to Dan Glastonbury from comment #12)
> I believe it was decided that `GPUDeviceLostReason or undefined` is not
> idiomatic IDL and should be `GPUDeviceLostReason?`

I'm proposing the same for customElements.getName (https://bugs.webkit.org/show_bug.cgi?id=255778).