Bug 232734 - (GPUDeviceLostReason or undefined) in an IDL cannot be compiled
Summary: (GPUDeviceLostReason or undefined) in an IDL cannot be compiled
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Bindings (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Dan Glastonbury
URL:
Keywords: InRadar
: 236723 (view as bug list)
Depends on:
Blocks: 232558
  Show dependency treegraph
 
Reported: 2021-11-04 16:26 PDT by Myles C. Maxfield
Modified: 2022-03-02 16:21 PST (History)
7 users (show)

See Also:


Attachments
Patch (6.78 KB, patch)
2022-02-27 16:36 PST, Dan Glastonbury
sam: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Myles C. Maxfield 2021-11-04 16:26:34 PDT
(GPUDeviceLostReason or undefined) is in WebGPU.
Comment 1 Radar WebKit Bug Importer 2021-11-11 15:27:41 PST
<rdar://problem/85318712>
Comment 2 Dan Glastonbury 2022-02-27 16:36:20 PST
Created attachment 453360 [details]
Patch
Comment 3 Dan Glastonbury 2022-02-27 16:39:12 PST
I implemented this as the spec specifies, although `GPUDeviceLostReason?` maybe be more idiomatic IDL.
Comment 4 Sam Weinig 2022-02-27 17:49:28 PST
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.
Comment 5 Sam Weinig 2022-02-27 17:54:28 PST
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)`.
Comment 6 Dan Glastonbury 2022-02-27 17:58:14 PST
(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.
Comment 7 Dan Glastonbury 2022-02-27 19:13:31 PST
(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.
```
Comment 8 Sam Weinig 2022-02-28 08:23:54 PST
(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.
Comment 9 Myles C. Maxfield 2022-03-02 16:03:08 PST
*** Bug 236723 has been marked as a duplicate of this bug. ***