Expose the full Result type to the rust map_async callback

[nical]

Checklist

• Run cargo clippy.
• Add change to CHANGELOG.md. See simple instructions inside file.

Description

map_async communicates errors in two ways:

• via the callback, it communicates all errors, including errors happening at validation time and later.
• via the result returned by the method which only reports the validation errors that can be caught immediately.

In practice I don't think that handling errors from the return value of map_async is practical because it only reports a subset of errors. Unfortunately the callback, where all error handling can be concentrated, takes a simplified representation of the error which is ffi-friendly but not as useful as the Result type.

We only need the ffi-friendly status for the C version of the callback, so in the PR I am proposing exposing the full error type to the rust callback.

Testing

None.

[nical]

Another change that I did not make in this PR but that I would like to push for is to not make map_async return a Result at all.

Today, map_async not returning Err does not mean that mapping will happen without error, it only means that we haven't caught an error yet.

Users could decide to handle some errors in callback and some at the map_async call site, but that's fairly messy and error prone as handling only a subset of the errors in both place makes it easy to forget to handle some errors (or errors that wgpu will add in future releases).

Thoughts?

[jimblandy]

I get this build error from cargo test --workspace:

error[E0631]: type mismatch in function arguments
   --> player/tests/test.rs:116:25
    |
58  | fn map_callback(status: wgc::resource::BufferMapAsyncStatus) {
    | ------------------------------------------------------------ found signature of `fn(BufferMapAsyncStatus) -> _`
...
115 |                     callback: wgc::resource::BufferMapCallback::from_rust(
    |                               ------------------------------------------- required by a bound introduced by this call
116 |                         Box::new(map_callback)
    |                         ^^^^^^^^^^^^^^^^^^^^^^ expected signature of `fn(Result<(), BufferAccessError>) -> _`
    |
    = note: required for the cast to the object type `dyn FnOnce(Result<(), BufferAccessError>) + Send`

[jimblandy]

Thoughts?

Piping all results through the callback is certainly a faithful translation of the WebGPU spec's mapAsync returning a promise: the only way the caller receives any information about the outcome of the operation is through that promise. And since wgpu has the web backend, its API is similarly constrained. The way wgpu::backend::direct's implementation of Context::buffer_map_async reports some errors via the callback and others via the error scope stack does not seem ideal.

The only thing that worries me about changing wgpu_core's API is that wgpu_core is supposed to serve as the common ground for all the different languages' wgpu bindings. Those other bindings should pipe everything through the callback, but I don't know if they do. I guess, if I really believe that they should be pressured to make their APIs behave this way, I should support making buffer_map_async return ().

[jimblandy]

Needs cargo test --workspace fixed. Rebasing on recent master should fix the macos build problems.

[nical]

Since I spend more time n code that use wgpu-core than wgpu I hadn't noticed that wgpu was only exposing a sort of placeholder error type (BufferAsyncError), and that it is already not returning an error in BufferSlice::map_async. I added a commit that has BufferSlice::map_async expose BufferAccessError instead of BufferAsyncError.

Edit: that breaks because BufferAccessError is in core which the web backend doesn't have access to. Looking around I think I might be going against the intent of how errors should be reported by wgpu since there are a number of other placeholder-looking error types in wgpu.

[nical]

I removed the commit that replaces BufferAsyncError in favor of BufferAccessError for now. I'll revisit when I better understand the error handlign story.

[nical]

I rebased the PR and applied crowlCats's suggestion. The PR still only affects wgpu-core (and not wgpu). If I remember correctly the main decision to make for wgpu is how to deal with the web backend. It looks like the web backend isn't set up to report specific errors, so the options are:

• A) have the web backend always just report BufferAccessError::Failed in case of error while the native backend gets detailed errors.
• B) figure out a way for the web backend to report more specific errors so they both get detailed errors.

It boils down to whether it is more important to improve the situation on native or to keep native and web on equal footing.
I'm pretty lazy so my suggestion is to go for A first and hope that someone will find the will to implement B (if possible at all).

I suggest landing this PR as is (unless there are other review comments to address of course) since it's uncontroversial. I'm happy to make the wgpu change (A) in another PR if folks agree with the direction.

[grovesNL]

Thank you! The changes to wgpu-core seem reasonable to me.

It boils down to whether it is more important to improve the situation on native or to keep native and web on equal footing.

I think we should generally aim for equal footing by default, and expose opt-in native-only functionality when there's a significant benefit. e.g., ideally out of memory errors would have the same error handling story between native and web if possible, and contain the same amount of detail - unless extra detail would significantly benefit the native backend.

[cwfitzgerald]

G2G outside one comment

[cwfitzgerald]

Thanks!

Stale