Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

What's the idiomatic way to reject a promise with structured data? #1223

Closed
litherum opened this issue Oct 19, 2022 · 6 comments
Closed

What's the idiomatic way to reject a promise with structured data? #1223

litherum opened this issue Oct 19, 2022 · 6 comments

Comments

@litherum
Copy link

litherum commented Oct 19, 2022

Hello!

In WebGPU, the website provides shader source code as a string to the browser, for the browser to compile it. The compilation process is asynchronous and returns a promise, because it runs in the GPU Process. Of course, that shader source code string may be erroneous; if the compilation process fails, it's fairly natural to reject the promise. When we reject the promise, we'd like to provide structured data to the website that includes errors, warnings, message strings, line + column information, maybe things like the name of the function that had the error inside it, possibly error codes, etc.

This is oversimplified a lot, but an author's code would want to look somewhat vaguely like this:

compileShader(`
    struct Foo {
        /* members here, details not important */
    }
    fn bar(foo: Foo) {
        let baz = foo * 7; // Structs can't be multiplied by 7; this is a compile error
    }
`).then(function(compiledShader) {
    /* use the compiledShader */
}, function(compilerErrors) {
    console.log("There was an error inside the shader function " + compilerErrors.failedFunction);
});

However, https://www.w3.org/2001/tag/doc/promises-guide#reasons-should-be-errors says:

Promise rejection reasons should always be instances of the JavaScript Error type, just like synchronously-thrown exceptions should always be instances of Error. This generally means using either one of the built-in JavaScript error types, or using DOMException.

It's unclear how to attach structured data to an Error or a DOMException. Should we subclass DOMException as e.g. RTCError does? Or maybe we should attach a dictionary to the cause field of Error?

@marcoscaceres
Copy link
Member

I'm not sure how unique this is in the platform, but this seems to layer two problems:

  1. a potential error at the call site (compileShader()).
  2. a compilation error at the GPU level as the cause of the exception.

Expressed a bit differently:

const shaderProgram = `
    fn bar(foo: Foo) {
        let baz = foo * 7; // Structs can't be multiplied by 7; this is a compile error
    }
`;
try {
  await compileShader(shaderProgram);
} catch (err) {
   // err is maybe TypeError or some new GPUError, as the type of input was bad. 
   // err.cause as SyntaxError in shaderProgram at some line, column.
}

If a TypeError or some new GPUError would depend on the various different ways compileShader() can fail.

The rationale being that the .cause would contain the "structured data" represented as a SyntaxError for the column/line number of shaderProgram.

Similarly, the TypeError or GPUError would give you the stack for where .compileShader() was called from.

I'm imagining that then developer tools could show more clearly where the error was in shaderProgram by linking everything together (same way that dev tools does today when running code in console).

Please let me know if I'm overcomplicating this.

@domenic
Copy link
Member

domenic commented Oct 20, 2022

Good timing! We are adding a pattern exactly for this case in #1211 .

I don't think using cause is appropriate here, because there was no inner exception thrown by some other subsystem, which this is wrapping. Also, SyntaxError does not contain column or line numbers.

domenic added a commit that referenced this issue Oct 20, 2022
Also be clearer about how exception names are meant to be used, at least for now (pending discussion in #1219).

See discussion in #1168 (comment). Closes #1223.

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.
domenic added a commit that referenced this issue Oct 20, 2022
Also be clearer about how exception names are meant to be used, at least for now (pending discussion in #1219).

See discussion in #1168 (comment). Closes #1223.

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.
@marcoscaceres
Copy link
Member

Ah, never mind them 😊 Having a look at #1211.

@litherum
Copy link
Author

@domenic so, sounds like we should subclass DOMException?

interface GPUException : DOMException {
  readonly attribute DOMString failedFunction;
  ... other things too ...
};

Is this ready to go? We'd be happy to add this to the WebGPU spec as soon as you give the go-ahead.

@domenic
Copy link
Member

domenic commented Nov 1, 2022

Yes, that's the idea! Be sure to follow the full pattern shown in #1211, including the guidance in the following:

*   The [=identifier=] of the [=interface=] must end with <code>Error</code>, and must not be any
    of the names in the <a><code>DOMException</code> names table</a>.
*   The [=interface=] must have a [=constructor operation=] which sets the instance's
    [=DOMException/name=] to the interface's [=identifier=].
*   Their [=constructor operation=] must take as its first parameter an [=optional=] {{DOMString}}
    named |message| defaulting to the empty string, and must set the instance's
    [=DOMException/message=] to |message|.
*   Their [=constructor operation=] should take as its second parameter a [=dictionary=] containing
    the additional information that needs to be exposed.
*   They should have [=read only=] [=attributes=], whose names are the same as the members of the
    constructor dictionary, which return the values accepted by the constructor operation.
*   They should be [=serializable objects=], whose [=serialization steps=] and
    [=deserialization steps=] preserve the additional information.

(The PR also contains a full example of how to do this.)

Actually merging that PR is blocked on a tooling issue (plinss/widlparser#79), but you can follow its example ahead of time with no issues.

@domenic
Copy link
Member

domenic commented Nov 1, 2022

Although I guess your spec build might also be blocked by the same tooling issue, at least if you're using Bikeshed... :-/.

domenic added a commit that referenced this issue Jun 1, 2023
Also be clearer about how exception names are meant to be used, at least for now (pending discussion in #1219).

See discussion in #1168 (comment). Closes #1223.

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.
domenic added a commit that referenced this issue Jun 1, 2023
Also be clearer about how exception names are meant to be used, at least for now (pending discussion in #1219).

See discussion in #1168 (comment). Closes #1223.

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.
domenic added a commit that referenced this issue Jun 1, 2023
Also be clearer about how exception names are meant to be used, at least for now (pending discussion in #1219).

See discussion in #1168 (comment). Closes #1223.

This also includes some updates to all exception creation and throwing, such as reflecting how messages are actually passed along in practice, or using Web IDL infrastructure to create DOMException instances instead of Construct()ing them.
@domenic domenic closed this as completed in fcbaa09 Jun 5, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

3 participants