Remove panics in Deref impl of QueueWriteBufferView and BufferViewMut

[botahamec]

Checklist

• Run cargo clippy.
• Run RUSTFLAGS=--cfg=web_sys_unstable_apis cargo clippy --target wasm32-unknown-unknown if applicable.
• Add change to CHANGELOG.md. See simple instructions inside file.

Connections
closes #3134

Description

According to the documentation for Queue::write_buffer_with:

The returned value can be dereferenced to a &mut [u8]; dereferencing it to a &[u8] panics!

According to the documentation for Deref:

this trait should never fail. Failure during dereferencing can be extremely confusing when Deref is invoked implicitly.

A similar problem appears in BufferViewMut.

The way I solved this problem was to get rid of the implementations of Deref and DerefMut for each, implement AsMut for QueueWriteBufferView, and add the read method to BufferViewMut

Testing
I ran cargo nextest run.

[nical]

Sorry I'm chiming in a bit late, didn't see #3134.

Considering we can read from the &mut deref, I think that we should just keep things simple, remove the panic and simply make sure to document the behavior well (reading the bytes may be slow and won't yield the real content of the buffer).
I'm not fond of the panic in the first place since it solves at best half of the problem it tries to solve.

Thoughts @teoxoy @cwfitzgerald ?

[teoxoy]

Considering we can read from the &mut deref, I think that we should just keep things simple, remove the panic and simply make sure to document the behavior well (reading the bytes may be slow and won't yield the real content of the buffer).

I'd say that this is quite difficult to document since Deref/DerefMut happen implicitly.
This PR looks good to me, makes things more idiomatic.

@nical are you concerned about the ergonomics being slightly worse?

[cwfitzgerald]

I am having trouble coming to a conclusion of which option I actually like more. Deref* makes it really easy to use - lets it slot into various apis that take anything that impls Deref (notably Encase). And it's not like we don't have other giant performance footguns that you can easily write. At the end of the day, as long as the panic goes away (as that is notably unidomatic. I think we'll be fine.

[codecov-commenter]

Codecov Report

Merging #3336 (003d998) into master (48fbb92) will decrease coverage by 0.20%.
The diff coverage is 8.33%.

@@            Coverage Diff             @@
##           master    #3336      +/-   ##
==========================================
- Coverage   64.70%   64.49%   -0.21%     
==========================================
  Files          66       86      +20     
  Lines       37336    42721    +5385     
==========================================
+ Hits        24159    27554    +3395     
- Misses      13177    15167    +1990     

Impacted Files	Coverage Δ
wgpu/src/lib.rs	51.01% <8.33%> (-0.33%)	⬇️
wgpu-core/src/hub.rs	60.67% <0.00%> (-3.91%)	⬇️
wgpu-core/src/instance.rs	65.67% <0.00%> (-2.04%)	⬇️
wgpu-hal/src/dx12/conv.rs	84.22% <0.00%> (ø)
wgpu-hal/src/dx12/mod.rs	27.27% <0.00%> (ø)
wgpu-hal/src/dx12/command.rs	73.46% <0.00%> (ø)
wgpu-hal/src/dx11/adapter.rs	0.00% <0.00%> (ø)
wgpu-hal/src/dx12/adapter.rs	81.38% <0.00%> (ø)
wgpu-hal/src/dx11/command.rs	0.00% <0.00%> (ø)
... and 23 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[nical]

In my opinion it's irrelevant whether someone ends up calling Deref or DerefMut (explicitly or implicitly), what matters is that they don't read form the (& or &mut) reference. AsMut has the same problem, it doesn't prevent or discourage reading, it just forces you to type "as_mut()" before doing so. As a result we still need to document that reading has likely undesired effects (potentially slow uncached memory reads and not providing access to the real buffer data).

Deref could be useful, here. We should not panic because it is not idiomatic, but we can log a performance warning. If we remove that hook, it will be as easy for accidental reads to happen but we won'be able to put a red flag on some of them.

@nical are you concerned about the ergonomics being slightly worse?

Yes-ish. My primary concern is that I don't think the way we are trying to paper over the issue helps (I don't think Rust can let us express something that would be helpful for this). The ergonomics cost is not something I'll lose sleep over but at the same time since I'm unconvinced that it helps in a meaningful way, my preference would be to keep things simple.

The problem isn't important enough to press further so if my arguments have not swayed you, don't feel bad about merging this as is.

[teoxoy]

I'm in the same boat as @cwfitzgerald (don't think there is a satisfactory solution).

We should not panic because it is not idiomatic, but we can log a performance warning.

This sounds better to me if we keep the Deref/DerefMut; log a warning instead of panicking.

@botahamec how do you feel about keeping the current API and logging a warning instead of panicking?

[botahamec]

@teoxoy That works for me. I assume that's referring to QueueWriteBufferView. Should BufferViewMut stay the way I currently have it in this PR?

[nical]

I'd say let them both have the familiar Deref/DerefMut API.

[botahamec]

@nical Does that make sense for BufferViewMut? Currently Deref panics only if self.readable is false. Is it similar to QueueWriteBufferView where it will just give nonsense data if self.readable is false?

[cwfitzgerald]

I believe it will give something resembling the correct data, just may be horrendously slow to read from.

[nical]

Yep, like @cwfitzgerald said. It makes sense for BufferViewMut for the same reason it makes sense for QueueWriteBufferView (For some definition of "makes sense" since it's hard to express what we really want to express in rust).

[botahamec]

I added back Deref and DerefMut. Let me know if anyone has any other comments

[botahamec]

Oh I guess readable isn't being used anymore on BufferViewMut. Should I remove it? I could also just add a method that uses it.

[cwfitzgerald]

One nit, then g2g

[teoxoy]

What about logging out warnings? We don't seem to do that yet but was what we previously talked about.

[botahamec]

@teoxoy Oh yes! You're right, I completely forgot. I'll get to that.

[teoxoy]

LGTM, thanks!

[teoxoy]

@botahamec looks like CI is unhappy

[botahamec]

Hmm, it's weird that my editor didn't show any problems. Maybe that's a sign to put less faith in rust-analyzer. Hopefully it works now.

[botahamec]

Ok, coincidentally, cargo deny generated an error because of a recent vulnerability in tokio #3368