Skip to content

Generalize io for Arc#130675

Closed
CAD97 wants to merge 1 commit intorust-lang:masterfrom
CAD97:arcio
Closed

Generalize io for Arc#130675
CAD97 wants to merge 1 commit intorust-lang:masterfrom
CAD97:arcio

Conversation

@CAD97
Copy link
Copy Markdown
Contributor

@CAD97 CAD97 commented Sep 21, 2024

There's no fundamental reason to limit the io trait implementations on Arc to only File; they can equally be applied to any Arc<T> where &T: Trait. Arc isn't a fundamental type, so it's impossible for downstream crates to implement Read for Arc<LocalType>, thus this blanket impl is a non-breaking one (if I remembered the API evolution rules correctly).

ATTN: this PR makes insta-stable API changes.

Now that the impl is generalized it might be more proper to live in a different file, but I did this from the web editor.

@rustbot
Copy link
Copy Markdown
Collaborator

rustbot commented Sep 21, 2024

r? @Noratrieb

rustbot has assigned @Noratrieb.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Sep 21, 2024
@Noratrieb
Copy link
Copy Markdown
Member

r? libs-api

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label Sep 21, 2024
@rustbot rustbot assigned m-ou-se and unassigned Noratrieb Sep 21, 2024
@lolbinarycat
Copy link
Copy Markdown
Contributor

I actually ran into this today when adding blanket impls in one of by projects (i have a subtrait of Write and i tried adding a blanket impl on Arc)

@CAD97
Copy link
Copy Markdown
Contributor Author

CAD97 commented Sep 22, 2024

On second thought — this is a bad/dangerous implementation due to impls such as Read for &[u8], which adjusts the input reference. While it's easy to accidentally think &T: Trait means T: Trait but replacing &mut self with &self, more implementations are possible than that.

It does feel like there should be a way for such forwarding to be done, but this isn't it.

@CAD97 CAD97 closed this Sep 22, 2024
@lolbinarycat
Copy link
Copy Markdown
Contributor

yep, &mut &T is mutable, &&mut T is not

@CAD97 CAD97 deleted the arcio branch September 22, 2024 04:18
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. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants