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

[melrief]

Library tracking issue #97227.
ACP: rust-lang/libs-team#555

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

Closes: rust-lang/libs-team#555

[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]

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.

[tgross35]

ACP was accepted rust-lang/libs-team#555 (comment)

[programmerjake]

the tracking issue #97227 needs to be updated to include position and seek_io_take_position.
other than that, lgtm!

[tgross35]

@melrief could you squash?

Also, what is the point of position? In cases where this may change (Seek), it is already available via stream_position, it doesn't seem very useful on its own.

[tgross35]

@rust-lang/libs-api this adds the new insta-stable API so will need FCP:

impl<T: Seek> Seek for Take<T> { /* ... */

This was accepted in ACP rust-lang/libs-team#555. The tracking issue #97227 has been around much longer, with an abandoned implementation in #97230 and #97807.

@rustbot label +I-libs-api-nominated

Also to consider is whether or not we need the position function (added unstably here) since it is directly exposed in the cases where the result is interesting, via Seek (#138023 (comment)).

[programmerjake]

Also, what is the point of position? In cases where this may change (Seek), it is already available via stream_position, it doesn't seem very useful on its own.

imo it's useful because it's not fallible, this is kinda similar to how we have std::str::CharIndices::offset() even though you could easily get that same information via iter.clone().next().unwrap_or((iter.as_str().len(), ' ')).0.

[joshtriplett]

Starting an FCP since this is insta-stable.

@rfcbot merge

[rfcbot]

Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members:

• @Amanieu
• @BurntSushi
• @dtolnay
• @joshtriplett
• @m-ou-se

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.