Sitelet https://github.com/gpuweb/gpuweb/pull/2750
Skip to content

Expose different error types directly instead of via polymorphism - #2750

Closed
kainino0x wants to merge 3 commits into
gpuweb:mainfrom
kainino0x:no-error-polymorphism
Closed

kainino0x wants to merge 3 commits into
gpuweb:mainfrom
kainino0x:no-error-polymorphism

Conversation

@kainino0x

@kainino0x kainino0x commented Apr 12, 2022 •

Copy link
Copy Markdown
Contributor

This makes it easier to programmatically handle possible future error
types, especially for marshalling into other languages (like C).

Fixes #1884


Preview | Diff

This makes it easier to programmatically handle possible future error
types, especially for marshalling into other languages (like C).

Fixes gpuweb#1884
@kainino0x kainino0x added tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue for webgpu editors meeting labels Apr 12, 2022
@kainino0x

Copy link
Copy Markdown
Contributor Author

Adding to editor meeting agenda to propose this to the tacit resolution queue.
(I won't be there next week, but I support this PR or a variant where we keep a separate GPUErrorFilter enum.)

@github-actions

Copy link
Copy Markdown
Contributor

Previews, as seen when this build job started (3b7ecbd):
WebGPU | IDL
WGSL
Explainer

Comment thread spec/index.bs
Comment thread spec/index.bs
@kainino0x
kainino0x requested a review from toji April 27, 2022 21:52

@toji toji left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sill LGTM! Thanks!

@kainino0x kainino0x added tacit resolution queue Editors have agreed and intend to land if no feedback is given and removed for webgpu editors meeting tacit resolution candidate Editors may be able to resolve and move to tacit resolution queue labels Apr 27, 2022
@kainino0x kainino0x removed the tacit resolution queue Editors have agreed and intend to land if no feedback is given label May 5, 2022
@kainino0x

Copy link
Copy Markdown
Contributor Author

meeting: Doesn't seem JavaScripty - and can do the same thing with error.constructor.name today.

I hadn't thought of doing this (thanks Kelsey for thinking of it) and knowing it, I'm inclined to close this PR. Maybe I'll add a note into the spec mentioning that you can do this.

@kainino0x

Copy link
Copy Markdown
Contributor Author

OK, I thought about this some more. Fundamentally, using error.constructor.name can work. But from a compatibility perspective, I think that using polymorphism here is more hazardous: the API has outputs of type GPUError (return value of popErrorScope, GPUUncapturedErrorEvent.error attribute). Changing the spec to add a new type to the GPUError union (start returning new types from those APIs) arguably constitutes a breaking change, as older applications wouldn't be set up to handle them.

Now, of course, returning objects with a new, never-before-seen value for error.type also constitutes a breaking change in the same way, but I think the pattern makes it less likely for applications to break down when that happens.

To improve this I've pushed a commit to this PR that changes the GPUError.type from an enum to a DOMString, to signal that it's an open type.

@toji

toji commented May 9, 2022

Copy link
Copy Markdown
Member

We discussed this on the editors call today. I'm not a fan of making the type an open string because it's something the API is returning, not that we're accepting from the developer, so we should be able to make stronger guarantees.

After talking it over some more we decided that a good path forward would be to use a common base class for the errors that had a message member so that developers can reliably use the uncapturederror event to create logs without needing to know the exact details of the error. The other place that errors may surface, push/popErrorScope(), requires that you specify the type of error you want to capture, so you have to know about the error type in advance which removes concerns about accidentally breaking if new error types are introduced.

@kainino0x
kainino0x deleted the no-error-polymorphism branch June 15, 2022 23:52
juj added a commit to juj/wasm_webgpu that referenced this pull request Aug 17, 2022
…eb#2853

WebGPU spec still not great, does not have a general error code. See also gpuweb/gpuweb#2750 and gpuweb/gpuweb#1884 .
@juj

juj commented Aug 17, 2022

Copy link
Copy Markdown

Updated https://github.com/juj/wasm_webgpu/ to this change today. I notice that this spec change does not really help things out here, the original issues reported in #1884 are still present.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make error scope objects have same shape as DOMExceptions?

3 participants