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

Add Read, Write and Seek impls for Arc<T> where appropriate #94744

Closed
wants to merge 1 commit into from

Conversation

tbu-
Copy link
Contributor

@tbu- tbu- commented Mar 8, 2022

If &T implements these traits, Arc<T> has no reason not to do so
either. This is useful for operating system handles like File or
TcpStream which don't need a mutable reference to implement these
traits.

Fix #53835.

If `&T` implements these traits, `Arc<T>` has no reason not to do so
either. This is useful for operating system handles like `File` or
`TcpStream` which don't need a mutable reference to implement these
traits.

Fix rust-lang#53835.
@rust-highfive
Copy link
Collaborator

r? @dtolnay

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 8, 2022
@SNCPlay42
Copy link
Contributor

This is incorrect, or at least has very surprising behaviour, if R = [u8]. impl Read for &[u8] expects mutable borrows of the same [u8] to be passed to read each time it is called, and updates the mutably borrowed slice to track the position in the buffer.

But this impl will create a new &[u8] pointing to the whole buffer each time read is called, with the effect that repeated calls to read will each read from the beginning of the buffer each time.

(playground demonstration)

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

I was about to comment the same as @SNCPlay42. I don't think any of these impls behave correctly in general. Mutations of the &mut &R by the underlying trait impls are just thrown away.

FYI @rust-lang/libs-api @jonhoo in case anyone sees a way to rescue this.

Impls for specific types like Arc<File> can work but I'm not sure which ones make sense / are needed.

@jonhoo
Copy link
Contributor

jonhoo commented Mar 8, 2022

Oh, interesting, I hadn't though of that — good catch! In that case I'd be in favor of at least adding the impls for the concrete types we know where this would be really useful (like File and TcpStream).

tbu- added a commit to tbu-/rust that referenced this pull request Jun 23, 2023
If `&T` implements these traits, `Arc<T>` has no reason not to do so
either. This is useful for operating system handles like `File` or
`TcpStream` which don't need a mutable reference to implement these
traits.

CC rust-lang#53835.
CC rust-lang#94744.
TaKO8Ki added a commit to TaKO8Ki/rust that referenced this pull request Jun 27, 2023
Add `Read`, `Write` and `Seek` impls for `Arc<File>` where appropriate

If `&T` implements these traits, `Arc<T>` has no reason not to do so
either. This is useful for operating system handles like `File` or
`TcpStream` which don't need a mutable reference to implement these
traits.

CC rust-lang#53835.
CC rust-lang#94744.
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 10, 2023
Add `Read`, `Write` and `Seek` impls for `Arc<File>` where appropriate

If `&T` implements these traits, `Arc<T>` has no reason not to do so
either. This is useful for operating system handles like `File` or
`TcpStream` which don't need a mutable reference to implement these
traits.

CC rust-lang#53835.
CC rust-lang#94744.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide Read and Write access through Arc<T> if &T: Read/Write
5 participants