Skip to content

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

Closed
tbu- wants to merge 1 commit intorust-lang:masterfrom
tbu-:pr_io_arc
Closed

Add Read, Write and Seek impls for Arc<T> where appropriate#94744
tbu- wants to merge 1 commit intorust-lang:masterfrom
tbu-:pr_io_arc

Conversation

@tbu-
Copy link
Copy Markdown
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.

@tbu-
Add Read, Write and Seek impls for Arc<T> where appropriate
2576c23 
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
Copy Markdown
Contributor

rust-highfive commented Mar 8, 2022

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
Copy Markdown
Contributor

SNCPlay42 commented Mar 8, 2022

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)

dtolnay
dtolnay requested changes Mar 8, 2022
View reviewed changes
Copy link
Copy Markdown
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.

@dtolnay dtolnay closed this Mar 8, 2022
@dtolnay dtolnay mentioned this pull request Mar 8, 2022
Closed
@jonhoo
Copy link
Copy Markdown
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- tbu- mentioned this pull request Mar 8, 2022
Merged
@m-ou-se m-ou-se mentioned this pull request Mar 9, 2022
Closed
tbu- added a commit to tbu-/rust that referenced this pull request Jun 23, 2023
@tbu-
Add Read, Write and Seek impls for Arc<File> where appropriate
11fecf6 
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
@TaKO8Ki
Rollup merge of rust-lang#94748 - tbu-:pr_file_arc, r=Amanieu
1dba5af 
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
@bors
Auto merge of rust-lang#94748 - tbu-:pr_file_arc, r=Amanieu
05b82e5 
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.
This was referenced Dec 12, 2024
Open
Open
@fintelia fintelia mentioned this pull request Apr 23, 2026
Merged
jhpratt added a commit to jhpratt/rust that referenced this pull request Apr 24, 2026
@jhpratt
Rollup merge of rust-lang#155684 - bushrat011899:blanket_io_seek_for_…
e002c6c 
…ref, r=jhpratt

Generalize IO Traits for `Arc<T>` where `&T: IoTrait`

ACP: rust-lang/libs-team#755
Tracking issue: rust-lang#154046
Related: rust-lang#94744

## Description

After experimenting with rust-lang#155625, I noticed `Seek` and `SeekFrom` can almost be moved to `core::io`. Unfortunately, the implementation of `Seek` for `Arc<File>` is a blocker for such a move, since `Arc` is not a fundamental type. This PR attempts to resolve this potential blocker by replacing the implementation with a more general alternative. An internal trait `IoHandle` has been added which types can implement to opt-in to `Read`/`Write`/`Seek` implementations for `Arc<Self>` as long as `&Self` implements said trait. Note that `BufRead` is excluded as the signature for `fill_buf` would require returning from a temporary.

Since this "blanket" implementation only applies to a single type which already implements the same traits, I believe this should have no user-facing impact.

If this PR was merged, rust-lang#134190 could be replaced with a 2 line PR:
```rust
impl IoHandle for TcpStream {}
impl IoHandle for UnixStream {}
```
Likewise for any other types, a table of which can be found [here](rust-lang/libs-team#504 (comment)). This is out of scope for this PR to avoid the need for an ACP.

---

## Notes

* See [this comment](rust-lang#154046 (comment)) for further details.
* No AI tooling of any kind was used during the creation of this PR.
rust-timer added a commit that referenced this pull request Apr 24, 2026
@rust-timer
Unrolled build for #155684
ed234f4 
Rollup merge of #155684 - bushrat011899:blanket_io_seek_for_ref, r=jhpratt

Generalize IO Traits for `Arc<T>` where `&T: IoTrait`

ACP: rust-lang/libs-team#755
Tracking issue: #154046
Related: #94744

## Description

After experimenting with #155625, I noticed `Seek` and `SeekFrom` can almost be moved to `core::io`. Unfortunately, the implementation of `Seek` for `Arc<File>` is a blocker for such a move, since `Arc` is not a fundamental type. This PR attempts to resolve this potential blocker by replacing the implementation with a more general alternative. An internal trait `IoHandle` has been added which types can implement to opt-in to `Read`/`Write`/`Seek` implementations for `Arc<Self>` as long as `&Self` implements said trait. Note that `BufRead` is excluded as the signature for `fill_buf` would require returning from a temporary.

Since this "blanket" implementation only applies to a single type which already implements the same traits, I believe this should have no user-facing impact.

If this PR was merged, #134190 could be replaced with a 2 line PR:
```rust
impl IoHandle for TcpStream {}
impl IoHandle for UnixStream {}
```
Likewise for any other types, a table of which can be found [here](rust-lang/libs-team#504 (comment)). This is out of scope for this PR to avoid the need for an ACP.

---

## Notes

* See [this comment](#154046 (comment)) for further details.
* No AI tooling of any kind was used during the creation of this PR.
github-actions Bot pushed a commit to rust-lang/rustc-dev-guide that referenced this pull request Apr 27, 2026
@jhpratt
Rollup merge of #155684 - bushrat011899:blanket_io_seek_for_ref, r=jh…
f622cf6 
…pratt

Generalize IO Traits for `Arc<T>` where `&T: IoTrait`

ACP: rust-lang/libs-team#755
Tracking issue: rust-lang/rust#154046
Related: rust-lang/rust#94744

## Description

After experimenting with rust-lang/rust#155625, I noticed `Seek` and `SeekFrom` can almost be moved to `core::io`. Unfortunately, the implementation of `Seek` for `Arc<File>` is a blocker for such a move, since `Arc` is not a fundamental type. This PR attempts to resolve this potential blocker by replacing the implementation with a more general alternative. An internal trait `IoHandle` has been added which types can implement to opt-in to `Read`/`Write`/`Seek` implementations for `Arc<Self>` as long as `&Self` implements said trait. Note that `BufRead` is excluded as the signature for `fill_buf` would require returning from a temporary.

Since this "blanket" implementation only applies to a single type which already implements the same traits, I believe this should have no user-facing impact.

If this PR was merged, rust-lang/rust#134190 could be replaced with a 2 line PR:
```rust
impl IoHandle for TcpStream {}
impl IoHandle for UnixStream {}
```
Likewise for any other types, a table of which can be found [here](rust-lang/libs-team#504 (comment)). This is out of scope for this PR to avoid the need for an ACP.

---

## Notes

* See [this comment](rust-lang/rust#154046 (comment)) for further details.
* No AI tooling of any kind was used during the creation of this PR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dtolnay dtolnay dtolnay requested changes

Assignees

@dtolnay dtolnay

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

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

5 participants

@tbu- @rust-highfive @SNCPlay42 @jonhoo @dtolnay