Use generic trait implementations for Cursor when possible. by nwin · Pull Request #27197 · rust-lang/rust

nwin · 2015-07-21T18:08:17Z

This is a revival of #23364. Github didn’t recognize my updated branch there.

The cursor implementation now uses AsRef which means that fixed-sized array can now be used with Cursor. Besides that, the generic implementation simplifies the code as the macro can be avoided.

The only drawback is, that specialized implementation for fixed-sized arrays are now ruled out unless RFC#1210 is accepted & implemented.

Box<[u8]> cannot be used yet, but that should be mitigated by implementing AsRef for Box and friends. I will submit a separate PR for that later as it is an orthogonal issue.

rust-highfive · 2015-07-21T18:08:25Z

r? @aturon

(rust_highfive has picked a reviewer for you, use r? to override)

bors · 2015-07-22T20:51:10Z

☔ The latest upstream changes (presumably #27219) made this pull request unmergeable. Please resolve the merge conflicts.

nwin · 2015-07-22T21:05:52Z

rebased

aturon · 2015-07-27T14:43:48Z

@nwin, sorry for the reviewing delay -- I was away on vacation last week.

This change looks good to me, but I'd also like to get @alexcrichton's opinion. We don't currently use AsRef in this way in IO -- in many cases we can't -- but it seems pretty reasonable to do so here.

alexcrichton · 2015-07-27T17:30:17Z

I do feel somewhat more comfortable with this than before (using AsRef instead of Deref). I don't think this is a strictly backwards-compatible change though (in terms of coherence) because I could have impl Read for Cursor<MyType> locally I believe and this would start not compiling if MyType implemented AsRef<[u8]>. I'm currently running a crater run to see if this actually causes any breakage in practice.

Unfortunately this also doesn't enable things like Cursor<Box<[u8]>> because the AsRef trait isn't implemented for many smart pointers (and I think we ran into trouble doing so).

nwin · 2015-07-27T19:50:04Z

Do you remember what was holding back something like

impl<T: ?Sized,V:AsRef<T> + ?Sized>AsRef<T>forBox<V>{#[inline]fnas_ref(&self) -> &T{(&*self).as_ref()}}

That would have been my next PR.

alexcrichton · 2015-07-27T21:04:59Z

I believe the killer aspect was that the meaning of <Box<Option<T>>::as_ref changed. Before it would return Option<&T> and after the addition of the trait impl it would return &Option<T>.

alexcrichton · 2015-07-27T21:56:44Z

Looks like this causes one regression on crates.io

aturon · 2015-07-27T22:24:05Z

That breakage seems acceptable under our semver guidelines.

@nwin, I'm wondering if you can elaborate on the motivation here. Is it mainly to clean up the implementation, or is there a use case you've run into that needs this additional flexibility?

nwin · 2015-07-28T06:47:27Z

@alexcrichton Ah the Box thing is annoying. On the other hand we are already having impl<'a, T, U> AsRef<U> for &'a T where T: AsRef<U> + ?Sized, U: ?Sized (which is causing this regression I guess), so it would be natural to have the same forward implementation for smart pointers.

@aturon As I already elaborated in #23364, my main motivation was to enable the usage of Box<[T]>, Arc<[T]> etc. Initially I used Deref for that but it was brought up that AsRef is the more natural trait to do that as it also covers fixed-sized arrays (currently only up to a length of 32 though :( ). Thus a follow-up PR would have been the implementation of AsRef for Box and friends. I didn’t get any feedback on this thread so I just went ahead.

As it appears now, this is not so trivial but we are also in the unfortunate situation that it is currently impossible to write generic code over &[u8], Vec<u8> and SmartPointer<[u8]>. Afaik the first two types were part of the motivation to introduce AsRef in the first place.

nwin · 2015-08-29T06:57:54Z

I updated this PR such that is uses Borrow instead. I didn’t squash the commits yet.

alexcrichton · 2015-08-31T06:41:08Z

Thanks @nwin! I'm on board with this change as-is, but I'm gonna flag this with T-libs so it comes up during triage to get some more eyes on it.

alexcrichton · 2015-09-17T06:22:13Z

The libs team discussed this during triage today, and the conclusion was that we're currently pretty on-the-fence about this. @gankro pointed out that the use of Borrow is out of line with how it's only used for collections today, and @aturon echo'd the thought indicating that AsRef really is the right solution here.

The conclusion was to reevaluate implementations of AsRef on smart pointers today with the help of crater. If the breakage (although present) is relatively minimal we may reconsider adding those implementations. As a result I'm going to do a crater run of adding AsRef implementations for smart pointers and report back with the results!

Gankra · 2015-09-17T12:18:01Z

(note that Borrow isn't totally a collections thing today -- Cow being the notable exception)

alexcrichton · 2015-09-18T16:14:53Z

Ok, the crater report indicates a total of 5 root regressions, but 4 of those were timeouts (cc @brson, common to see?) and only one of them was related to the addition of AsRef. There were three regressions caused by dependencies on that one root regression which may also need to be investigated as well.

cc #26096, the original issue about the regression. This may be showing up more externally than on crates.io as well because the use of smart pointers tends to happen more in apps than libs perhaps.

brson · 2015-09-21T19:50:47Z

@alexcrichton Yeah, timeouts are common.

These common traits were left off originally by accident from these smart pointers, and a past attempt (rust-lang#26008) to add them was later reverted (rust-lang#26160) due to unexpected breakge (rust-lang#26096) occurring. The specific breakage in worry is the meaning of this return value changed: let a: Box<Option<T>> = ...; a.as_ref() Currently this returns `Option<&T>` but after this change it will return `&Option<T>` because the `AsRef::as_ref` method shares the same name as `Option::as_ref`. A [crater report][crater] of this change, however, has shown that the fallout of this change is quite minimal. These trait implementations are "the right impls to add" to these smart pointers and would enable various generalizations such as those in rust-lang#27197. [crater]: https://gist.github.com/anonymous/0ba4c3512b07641c0f99 This commit is a breaking change for the above reasons mentioned, and the mitigation strategies look like any of: Option::as_ref(&a) a.as_ref().as_ref() (*a).as_ref()

alexcrichton · 2015-10-02T16:02:41Z

As an update, the libs team talked about this and decided to move forward with AsRef and AsMut implementations on smart pointers (#28811), so if that gets merged then this should be ok to merge going back to AsRef. Thanks for being patient @nwin!

nwin · 2015-10-02T19:46:15Z

Great. I removed my latest commit and rebased onto master.

process::tests::test_inherit_env fails it also doesn’t pass on master, probably some OSX 10.11 issue…

These common traits were left off originally by accident from these smart pointers, and a past attempt (#26008) to add them was later reverted (#26160) due to unexpected breakge (#26096) occurring. The specific breakage in worry is the meaning of this return value changed: let a: Box<Option<T>> = ...; a.as_ref() Currently this returns `Option<&T>` but after this change it will return `&Option<T>` because the `AsRef::as_ref` method shares the same name as `Option::as_ref`. A [crater report][crater] of this change, however, has shown that the fallout of this change is quite minimal. These trait implementations are "the right impls to add" to these smart pointers and would enable various generalizations such as those in #27197. [crater]: https://gist.github.com/anonymous/0ba4c3512b07641c0f99 This commit is a breaking change for the above reasons mentioned, and the mitigation strategies look like any of: Option::as_ref(&a) a.as_ref().as_ref() (*a).as_ref()

alexcrichton · 2015-10-08T17:29:29Z

OK, now that #28811 has merged, I'm going to r+ this, thanks again for the patience @nwin!

@bors: r+ ea1f886

This is a revival of #23364. Github didn’t recognize my updated branch there. The cursor implementation now uses `AsRef` which means that fixed-sized array can now be used with `Cursor`. Besides that, the generic implementation simplifies the code as the macro can be avoided. The only drawback is, that specialized implementation for fixed-sized arrays are now ruled out unless [RFC#1210](rust-lang/rfcs#1210) is accepted & implemented. `Box<[u8]>` cannot be used yet, but that should be mitigated by [implementing `AsRef` for `Box` and friends](https://internals.rust-lang.org/t/forward-implement-traits-on-smart-pointers-make-smart-pointers-more-transparent/2380/3). I will submit a separate PR for that later as it is an orthogonal issue.

bors · 2015-10-08T20:53:25Z

⌛ Testing commit ea1f886 with merge 11eda66...

bors · 2015-10-08T22:40:49Z

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

This introduces an unfortunate point of difference between futures::io::AsyncWrite and std::io::Write, but I think the increased ergonomics around writing to statically sized in memory buffers (presumably just for test purposes) is useful. `impl<T: AsRef<[u8]>> Read for Cursor<T>` was added in rust-lang/rust#27197, I'm not sure why `impl<T: AsMut<[u8]>> Write for Cursor<T>` wasn't added at the same time; I would propose doing this change in `std` and just piggybacking off it here, but the breakage is almost certainly not worth it by this point.

This introduces an unfortunate point of difference between `futures::io::AsyncWrite` and `std::io::Write`, but I think the increased ergonomics around writing to statically sized in memory buffers (presumably just for test purposes) is useful. `impl<T: AsRef<[u8]>> Read for Cursor<T>` was added in rust-lang/rust#27197, I'm not sure why `impl<T: AsMut<[u8]>> Write for Cursor<T>` wasn't added at the same time; I would propose doing this change in `std` and just piggybacking off it here, but the breakage is almost certainly not worth it by this point.

…lnay Implement `Write for Cursor<[u8; N]>`, plus `A: Allocator` cursor support This implements `Write for Cursor<[u8; N]>`, and also adds support for generic `A: Allocator` in `Box` and `Vec` cursors. This was inspired by a user questioning why they couldn't write a `Cursor<[u8; N]>`: https://users.rust-lang.org/t/why-vec-and-not-u8-makes-cursor-have-write/68210 Related history: - rust-lang#27197 switched `AsRef<[u8]>` for reading and seeking - rust-lang#67415 tried to use `AsMut<[u8]>` for writing, but did not specialize `Vec`.

rust-highfive assigned aturon Jul 21, 2015

nwin force-pushed the generic-cursor-impl branch from 0fc5b20 to 9cbb867Compare July 22, 2015 21:03

alexcrichton mentioned this pull request Aug 20, 2015
Implement Read, BufRead, Write and Seek for Cursor<Box<[u8]>> #27897
Merged

nwin force-pushed the generic-cursor-impl branch from 9cbb867 to 7704ff1Compare August 29, 2015 06:58

alexcrichton added the T-libs-api [DEPRECATED; DO NOT USE] label Aug 31, 2015

alexcrichton mentioned this pull request Sep 25, 2015
Implement AsMut for Vec #28663
Merged

brson added relnotes Marks issues that should be documented in the release notes of the next release. regression-from-stable-to-nightly Performance or correctness regression from stable to nightly. labels Sep 30, 2015

alexcrichton mentioned this pull request Oct 2, 2015
std: Add AsRef/AsMut impls to Box/Rc/Arc #28811
Merged

Use generic trait implementations for Cursor when possible.
ea1f886

nwin force-pushed the generic-cursor-impl branch from 7704ff1 to ea1f886Compare October 2, 2015 19:15

bors merged commit ea1f886 into rust-lang:masterOct 8, 2015

nwin deleted the generic-cursor-impl branch October 9, 2015 13:33

Nemo157 mentioned this pull request Jul 21, 2018
Implement AsyncWrite for the generic Cursor<T: AsMut<[u8]>> rust-lang/futures-rs#1109
Merged

kennytm mentioned this pull request Dec 19, 2019
Update cursor.rs #67415
Closed

cuviper mentioned this pull request Jan 8, 2022
Implement Write for Cursor<[u8; N]>, plus A: Allocator cursor support #92663
Merged

Uh oh!

Uh oh!

Conversation

nwin commented Jul 21, 2015

Uh oh!

rust-highfive commented Jul 21, 2015

Uh oh!

bors commented Jul 22, 2015

Uh oh!

nwin commented Jul 22, 2015

Uh oh!

aturon commented Jul 27, 2015

Uh oh!

alexcrichton commented Jul 27, 2015

Uh oh!

nwin commented Jul 27, 2015

Uh oh!

alexcrichton commented Jul 27, 2015

Uh oh!

alexcrichton commented Jul 27, 2015

Uh oh!

aturon commented Jul 27, 2015

Uh oh!

nwin commented Jul 28, 2015

Uh oh!

nwin commented Aug 29, 2015

Uh oh!

alexcrichton commented Aug 31, 2015

Uh oh!

alexcrichton commented Sep 17, 2015

Uh oh!

Gankra commented Sep 17, 2015

Uh oh!

alexcrichton commented Sep 18, 2015

Uh oh!

brson commented Sep 21, 2015

Uh oh!

alexcrichton commented Oct 2, 2015

Uh oh!

nwin commented Oct 2, 2015

Uh oh!

alexcrichton commented Oct 8, 2015

Uh oh!

bors commented Oct 8, 2015

Uh oh!

bors commented Oct 8, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants