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.

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.

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

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

@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.

@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)).

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.

Thank you for all the follow up here!

@bors r+

📌 Commit c8f5ff8 has been approved by tgross35

It is now in the queue for this repository.

⌛ Testing commit c8f5ff8 with merge 60dabef...

☀️ Test successful - checks-actions
Approved by: tgross35
Pushing 60dabef to master...

What is this?

This is an experimental post-merge analysis report that shows differences in test outcomes between the merged PR and its parent PR.

Comparing 59372f2 (parent) -> 60dabef (this PR)

Test differences

Show 26 test diffs

Stage 0

• io::tests::take_seek: [missing] -> pass (J0)
• io::tests::take_seek_big_offsets: [missing] -> pass (J0)
• io::tests::take_seek_error: [missing] -> pass (J0)

Stage 1

• io::tests::take_seek: [missing] -> pass (J1)
• io::tests::take_seek_big_offsets: [missing] -> pass (J1)
• io::tests::take_seek_error: [missing] -> pass (J1)

Additionally, 20 doctest diffs were found. These are ignored, as they are noisy.

Job group index

• J0: mingw-check
• J1: aarch64-apple, aarch64-gnu, arm-android, armhf-gnu, dist-i586-gnu-i586-i686-musl, i686-gnu-1, i686-gnu-nopt-1, i686-msvc-1, test-various, x86_64-apple-1, x86_64-gnu, x86_64-gnu-aux, x86_64-gnu-llvm-19-1, x86_64-gnu-llvm-19-2, x86_64-gnu-llvm-19-3, x86_64-gnu-llvm-20-1, x86_64-gnu-llvm-20-2, x86_64-gnu-llvm-20-3, x86_64-gnu-nopt, x86_64-gnu-stable, x86_64-mingw-1, x86_64-msvc-1

Test dashboard

Run

cargo run --manifest-path src/ci/citool/Cargo.toml -- \
    test-dashboard 60dabef95a3de3ec974dcb50926e4bfe743f078f --output-dir test-dashboard

And then open test-dashboard/index.html in your browser to see an overview of all executed tests.

Job duration changes

1. dist-apple-various: 7550.3s -> 5433.2s (-28.0%)
2. x86_64-apple-1: 7720.3s -> 9297.2s (20.4%)
3. dist-x86_64-apple: 9511.7s -> 10868.2s (14.3%)
4. dist-arm-linux: 5329.2s -> 4612.6s (-13.4%)
5. x86_64-gnu-llvm-19-1: 5714.7s -> 5245.4s (-8.2%)
6. x86_64-apple-2: 5849.2s -> 6262.4s (7.1%)
7. aarch64-apple: 4489.8s -> 4220.8s (-6.0%)
8. dist-armhf-linux: 5376.0s -> 5069.2s (-5.7%)
9. dist-aarch64-apple: 5803.4s -> 6093.3s (5.0%)
10. dist-x86_64-musl: 7568.7s -> 7195.8s (-4.9%)

How to interpret the job duration changes?

Job durations can vary a lot, based on the actual runner instance
that executed the job, system noise, invalidated caches, etc. The table above is provided
mostly for t-infra members, for simpler debugging of potential CI slow-downs.

Finished benchmarking commit (60dabef): comparison URL.

Overall result: ❌ regressions - please read the text below

Our benchmarks found a performance regression caused by this PR.
This might be an actual regression, but it can also be just noise.

Next Steps:

• If the regression was expected or you think it can be justified,
  please write a comment with sufficient written justification, and add
  @rustbot label: +perf-regression-triaged to it, to mark the regression as triaged.
• If you think that you know of a way to resolve the regression, try to create
  a new PR with a fix for the regression.
• If you do not understand the regression or you think that it is just noise,
  you can ask the @rust-lang/wg-compiler-performance working group for help (members of this group
  were already notified of this PR).

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

	mean	range	count
Regressions ❌ (primary)	2.0%	[1.0%, 3.0%]	2
Regressions ❌ (secondary)	1.0%	[1.0%, 1.0%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.0%	[1.0%, 3.0%]	2

Max RSS (memory usage)

Results (secondary 2.0%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	5.8%	[4.4%, 7.2%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.5%	[-0.6%, -0.4%]	3
All ❌✅ (primary)	-	-	0

Cycles

Results (primary 3.0%, secondary -1.7%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	3.0%	[3.0%, 3.0%]	1
Regressions ❌ (secondary)	0.8%	[0.8%, 0.8%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-1.9%	[-3.6%, -0.9%]	10
All ❌✅ (primary)	3.0%	[3.0%, 3.0%]	1

Binary size

Results (primary 1.1%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.1%	[1.1%, 1.1%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.1%	[1.1%, 1.1%]	1

Bootstrap: 775.652s -> 775.977s (0.04%)
Artifact size: 365.59 MiB -> 365.61 MiB (0.01%)

since this is stabilizing new API, shouldn't this get the relnotes label?

Noise in newly added benchmarks.

@rustbot label: +perf-regression-triaged