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

Remove AsyncRead::initializer #1761

Closed
Nemo157 opened this issue Jul 22, 2019 · 11 comments · Fixed by #1845
Closed

Remove AsyncRead::initializer #1761

Nemo157 opened this issue Jul 22, 2019 · 11 comments · Fixed by #1845

Comments

@Nemo157
Copy link
Member

Nemo157 commented Jul 22, 2019

The tracking issue for std::io::Read::initializer makes it sound like that API is definitely going to be changed a lot, it would be good to remove the similar API here until the std API is closer to its final form.

@cramertj
Copy link
Member

Yeah, it seems like there's some push towards having freeze() instead. cc @seanmonstar @RalfJung

@RalfJung
Copy link
Member

There was a push at rust-lang/rust#58363, but there are two issues with this:

Note however that the current initializer API is incompatible with rust-lang/unsafe-code-guidelines#77, at least if rust-lang/unsafe-code-guidelines#71 ends up deciding that integers must be initialized to be valid.

@seanmonstar
Copy link
Contributor

It seems to me like we're at an awkward point with regards to the desired functionality here.

  • If it's decided that &mut [u8] to uninitialized memory is UB, then we need to add methods like unsafe fn read_uninit(&mut self, buf: &mut MaybeUninit<[u8]>).
  • However, if it is decided to NOT be UB, then we don't need those methods, and don't need to suffer the churn.

@Nemo157
Copy link
Member Author

Nemo157 commented Jul 25, 2019

Whether or not an &mut [u8] pointing to uninitialized memory is UB, there was earlier discussion in the tracking issue about how the ceremony in the current API doesn't provide much utility and it could be replaced with a simpler API. It would be disappointing if AsyncRead ended up with a different, more complicated API than Read.

@taiki-e
Copy link
Member

taiki-e commented Aug 30, 2019

I think we should choose one of the following:

@cramertj
Copy link
Member

+1 for making it unstable. The "simpler API" is unsound, though, so I don't want to do that.

@carllerche
Copy link
Member

@cramertj what is unsound about it?

@cramertj
Copy link
Member

unsafe fn may_read_buffer(&self) -> bool { true } is incorrect because it is unsafe to implement, not unsafe to call. The only way today to make something unsafe to implement is via a separate unsafe trait, or something which is forced to call an unsafe function, at which point you wind up back at the current initializer API.

@carllerche
Copy link
Member

@cramertj makes sense, thanks for the explanation. Is the intent to document the unsafe requirements you describe as part of https://github.com/rust-lang/unsafe-code-guidelines or some other location?

@cramertj
Copy link
Member

The requirement that unsafe fn in traits is only unsafe to call, not unsafe to implement? I'm not aware of an existing UCG proposal for that, but I'd certainly be interested in one, yes.

@RalfJung
Copy link
Member

That seems more like Reference material than UCG. There's no open questions, just things are a bit surprising (rust-lang/rfcs#2585 would improve things IMO by making the role of unsafe fn clearer).

bors bot added a commit to async-rs/async-std that referenced this issue Oct 1, 2019
255: Update futures-preview to 0.3.0-alpha.19 r=skade a=taiki-e

Depends on http-rs/surf#74

Refs:
* rust-lang/futures-rs#1761
* rust-lang/futures-rs#1845


cc @skade @stjepang @yoshuawuyts 

Co-authored-by: Taiki Endo <te316e89@gmail.com>
bors bot added a commit to async-rs/async-std that referenced this issue Oct 1, 2019
255: Update futures-preview to 0.3.0-alpha.19 r=skade a=taiki-e

Depends on http-rs/surf#74

Refs:
* rust-lang/futures-rs#1761
* rust-lang/futures-rs#1845


cc @skade @stjepang @yoshuawuyts 

Co-authored-by: Taiki Endo <te316e89@gmail.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

Successfully merging a pull request may close this issue.

6 participants