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

Unnecessary Fn bound for SizeError::map_src. #2009

Closed
conradludgate opened this issue Nov 4, 2024 · 1 comment
Closed

Unnecessary Fn bound for SizeError::map_src. #2009

conradludgate opened this issue Nov 4, 2024 · 1 comment

Comments

@conradludgate
Copy link

I am implementing a small wrapper over read_from_bytes that allows working over types like Bytes instead of &[u8]. As such I want to change the Src bound to be Bytes instead of &[u8].

However, the bound on map_src is unnecessarily a Fn(Src) -> NewSrc which implies it would be callable multiple times. This is inconsistent with other forms of map which use FnOnce. This is a problem for me as I cannot simply map_src(|_| bytes_buf) as that moves ownership, giving me the error

error[E0507]: cannot move out of `buf`, a captured variable in an `Fn` closure
   --> proxy/src/protocol2.rs:371:40
    |
363 |         let buf = if self.len() < len {
    |             --- captured outer variable
...
371 |         res.map_err(|e| e.map_src(|()| buf))
    |                                   ---- ^^^ move occurs because `buf` has type `bytes::BytesMut`, which does not implement the `Copy` trait
    |                                   |
    |                                   captured by this `Fn` closure
    |
help: consider cloning the value if the performance cost is acceptable
    |
371 |         res.map_err(|e| e.map_src(|()| buf.clone()))
    |                                           ++++++++

Easy to fix here with a clone, sure. But it's not ideal.

I have a feeling this can be fixed without a breaking change, but I'm not 100% certain.

jswrenn added a commit that referenced this issue Nov 4, 2024
github-merge-queue bot pushed a commit that referenced this issue Nov 4, 2024
* Relax `map_src` argument bound to `FnOnce`

Fixes #2009

* Release v0.8.9
@joshlf
Copy link
Member

joshlf commented Nov 4, 2024

This is fixed in 0.8.9.

@joshlf joshlf closed this as completed Nov 4, 2024
joshlf pushed a commit that referenced this issue Nov 5, 2024
* Relax `map_src` argument bound to `FnOnce`

Fixes #2009

* Release v0.8.9
github-merge-queue bot pushed a commit that referenced this issue Nov 6, 2024
* Relax `map_src` argument bound to `FnOnce`

Fixes #2009

* Release v0.8.9

Co-authored-by: Jack Wrenn <jswrenn@amazon.com>
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

No branches or pull requests

2 participants