Add std::io::Seek instance for std::io::Take

[melrief]

Library tracking issue #97227.

1. add a len field to Take to keep track of the original number of bytes that Take could read
2. add a position() method to return the current position of the cursor inside Take
3. implement std::io::Seek for std::io::Take

[rustbot]

r? @tgross35

rustbot has assigned @tgross35.
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

[tgross35]

API needs to start out as unstable where possible, the only exception is trait implementations because that part doesn't currently work.

[melrief]

I can't just change it to unstable because then the compiler complains that seek_io_take is both stable and unstable. I see a few options:

1. mark unstable the trait implementation
2. I have another feature for position
3. make position private (for now?)

[tgross35]

It can just be a separate feature gate

[melrief]

I've changed it to be unstable and put behind a different feature gate seek_io_take_position.

[tgross35]

It doesn't look like #97227 was ever accepted by libs-api. Could you file an API change proposal? This is an issue template at https://github.com/rust-lang/libs-team/issues. Please include the proposed position function here as well.

The trait implementation is insta-stable so it will need FCP if accepted.

[melrief]

@tgross35 I've created 555.

[programmerjake]

what's with the funny python-indexing-style logic?

ok, maybe your logic doesn't do that but it is written in a very confusing way with unsigned_abs everywhere.

[programmerjake]

a hopefully easier to understand implementation:
(I also implemented some other Seek methods and implemented the range-limited seeking as suggested in rust-lang/libs-team#555 (comment))

https://play.rust-lang.org/?version=nightly&mode=debug&edition=2024&gist=fa54e42bd394844eb9bf9eb5bcb87ecb

[melrief]

The usage of unsigned_abs was to deal with u64, i64 and the fact that I need to seek the inner from Current. I'm ok with your implementation though. I have one question about it: your implementation makes use of the unstable features unsigned_signed_diff and seek_stream_len, is it fine to use unstable features in the rust library? Should I just add the two unstable features to the crate attributes?

[programmerjake]

afaik it's generally fine to use unstable library features in the rust standard library, those are implemented in the same git repository, so if they need to change or are removed, the code using them can be updated at the same time.

[melrief]

@programmerjake I pushed your implementation with a132fcb. I also removed the tests that now fail and added the needed unstable feature to library/std/src/lib.rs.

[programmerjake]

nice! you'll probably want to add some tests that out-of-bounds seeks actually error.

[melrief]

Good point. I added five new tests with 6ee1a13:

1. take_seek_out_of_bounds_start checks Start with offset bigger than len
2. take_seek_out_of_bounds_end_forward checks End with positive non-zero offsewt
3. take_seek_out_of_bounds_end_before_start checks End with offset smaller than -len
4. take_seek_out_of_bounds_current_before_start checks Current with offset that pushes position before zero
5. take_seek_out_of_bounds_current_after_end checks Current with offset that pushes position after len

[programmerjake]

this looks generally pretty good to me, the next steps are to get someone to second the ACP and to get team signoff here (because the trait impl is insta-stable), I can't do either of those.

@rustbot label +needs-fcp,+T-libs-api

[programmerjake]

if you think you're done writing the code, you can comment @rustbot ready.

[melrief]

@rustbot ready

[programmerjake]

if you think you're done writing the code, you can comment @rustbot ready.

i guess i didn't notice it was already labeled as waiting on review...sorry, that wasn't necessary.

[melrief]

if you think you're done writing the code, you can comment @rustbot ready.

i guess i didn't notice it was already labeled as waiting on review...sorry, that did nothing.

No problem 😄. I created the PR and I should have checked.

[thaliaarchi]

Looks good to me (not a reviewer). The logic seems sound and it even handles the pathological case of adapting absolute offsets over isize::MAX to relative.

[thaliaarchi]

You could eliminate the range check from self.seek_relative by manually inlining it.

--- a/library/std/src/io/mod.rs
+++ b/library/std/src/io/mod.rs
@@ -3086,14 +3086,13 @@ fn seek(&mut self, pos: SeekFrom) -> Result<u64> {
         };
         while new_position != self.position() {
             if let Some(offset) = new_position.checked_signed_diff(self.position()) {
-                self.seek_relative(offset)?;
+                self.inner.seek_relative(offset)?;
+                self.limit = self.limit.wrapping_sub(offset as u64);
                 break;
             }
-            if new_position > self.position() {
-                self.seek_relative(i64::MAX)?;
-            } else {
-                self.seek_relative(i64::MIN)?;
-            }
+            let offset = if new_position > self.position() { i64::MAX } else { i64::MIN };
+            self.inner.seek_relative(offset)?;
+            self.limit = self.limit.wrapping_sub(offset as u64);
         }
         Ok(new_position)
     }

[programmerjake]

ok, though I'd be surprised if LLVM couldn't already do that optimization itself.

[melrief]

I pushed the manual inlining with 17989cb.

[thaliaarchi]

Since you update self.limit after the call to the inner user method (which may panic), I surveyed the other traits of Take<T> to see what order they update. It matches the convention of the other methods.

• Read::read, Read::read_buf, Seek::seek and Seek::seek_relative: update self.limit after inner call
• BufRead::consume: update self.limit before inner call
• BufRead::fill_buf: unchanged

Initially, I thought the inconsistency with consume should be (separately) addressed; however since consume is not fallible, but the others are, it makes sense to update it before the user method.