Implement Read, BufRead, Write and Seek for Cursor<Box<[u8]>>

[sfackler]

No description provided.

[rust-highfive]

r? @aturon

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

[bluss]

There could be a doc comment here on the impl block explaining the semantics. It should be visible on the type's rustdoc page in the current implementation (so on Cursor's page).

The vector growing and the box merely filling is something that we probably want to put into the documentation somehow.

[sfackler]

Agreed - I have personally been a bit confused as to the exact semantics of some of the impls here. I believe that docs on the impl itself will not be rendered but docs on methods in the impl will.

[bluss]

It's actually the opposite in my experience. #24838

[alexcrichton]

Ah this should actually be 1.5.0 now (oh how time flies!)

[alexcrichton]

I'm a little worried about the interactions between this and #27197, as I think that attempting to generalize these implementations is probably a good idea and don't want to gain too many impl blocks here. I think that using the generalization of Deref is a bit too powerful (and a bit unidiomatic), and as found in #27197 AsRef won't work as it's not implemented for smart pointers. Perhaps, though, the BufRead, Read, and Seek implementations could be generalized for T: Borrow<[u8]>? That's implemented for all smart pointer types and would give us Cursor<Box<[u8]>> for free (reading at least).

For writing, we could generalize over T: BorrowMut<[u8]> to get the &mut [u8] and Box<[u8]> implementations, but it would invalidate the current Vec<u8> implementation (e.g. it wouldn't allow the vector to grow). With specialization, however, we should be able to have this overlap.

In that sense I would be ok exploring using Borrow for the reading side of things and adding an explicit impl of Write for Cursor<Box<[u8]>>. How does that sound?

cc @nwin

[nwin]

I agree that the write part can’t be generalized easily, so I think it would be ok to implement Cursor for Box<[u8]>.

I'm not sure about the Borrow-part, as already discussed in #23364, it would not work with fixed-sized arrays. Furthermore Borrow has been implemented for Box only very recently (not in May at least) so it this really the appropriate trait for this task? I would be in favor of using AsRef and forward-implement it for all smart pointers as this would cover all slice-like structures I can think of.

[alexcrichton]

@nwin unfortunately AsRef is off the table due to the breakage of as_ref on Box<Option<T>>

[nwin]

@alexcrichton: That is really unfortunate because then I do not see a nice solution.

Seems we have the following options:

• Implement Cursor for Box<[T]> without forgetting Rc<[T]>, Arc<[T]> and reminding everybody to also implement it for any custom smart pointer aka. Gc<[T]>
• Use Deref<Target=AsRef<[u8]>> and not care about raw [T; N] (Cursor::new([0; 2]))
• Use Borrow and not care about any [T; N]

[sfackler]

None of Rc<[T]>, Arc<[T]> and Gc<[T]> can be used in a Cursor since they don't return mutable references to their interior.

[sfackler]

Er, that only applies to Write impls, sorry.

[alexcrichton]

Given #27197 (which I'd personally be ok accepting) that would alter this to just adding Write for Box<[u8]>, which I think is fine. Let's wait to see what happens over there though.

[alexcrichton]

The libs team is still holding off on this until #27197 is resolved (as a status update)

[alexcrichton]

@sfackler now that #28811 has landed which has unblocked #27197 which will in turn unblock this, just a heads up that this may need a rebase soon! I think that the parts we agreed to add were Write for Cursor<Box<[u8]>> (having the same semantics as &mut [u8], and that should do the trick!

[bors]

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

[sfackler]

rebased

[alexcrichton]

r=me with minor nit about the stable since

[sfackler]

@bors r=alexcrichton

[bors]

📌 Commit 6b244d5 has been approved by alexcrichton

[bors]

⌛ Testing commit 6b244d5 with merge 3034541...

[bors]

☀️ 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