Add `std::io::Seek` instance for `std::io::Take` #138023

melrief · 2025-03-04T23:46:58Z

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

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

rustbot · 2025-03-04T23:47:02Z

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 · 2025-03-05T00:55:59Z

library/std/src/io/mod.rs

+    #[stable(feature = "seek_io_take", since = "CURRENT_RUSTC_VERSION")]
+    pub fn position(&self) -> u64 {


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

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:

mark unstable the trait implementation

I have another feature for position

make position private (for now?)

It can just be a separate feature gate

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

tgross35 · 2025-03-05T00:57:07Z

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 · 2025-03-05T02:53:45Z

@tgross35 I've created 555.

programmerjake · 2025-03-05T04:00:32Z

library/std/src/io/mod.rs

+        let offset_from_start = match pos {
+            SeekFrom::Start(offset) => offset,
+            SeekFrom::End(offset) => {
+                if offset > 0 {


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.

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

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?

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.

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

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

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

take_seek_out_of_bounds_start checks Start with offset bigger than len

take_seek_out_of_bounds_end_forward checks End with positive non-zero offsewt

take_seek_out_of_bounds_end_before_start checks End with offset smaller than -len

take_seek_out_of_bounds_current_before_start checks Current with offset that pushes position before zero

take_seek_out_of_bounds_current_after_end checks Current with offset that pushes position after len

programmerjake · 2025-03-07T17:29:11Z

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 · 2025-03-07T17:36:28Z

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

melrief · 2025-03-07T18:01:20Z

@rustbot ready

programmerjake · 2025-03-07T19:09:00Z

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 · 2025-03-07T19:10:57Z

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.

library/std/src/io/mod.rs

thaliaarchi · 2025-03-10T23:57:49Z

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 · 2025-04-24T02:58:46Z

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 · 2025-04-24T04:58:07Z

@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 · 2025-04-24T04:59:52Z

@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 · 2025-04-24T06:27:03Z

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.

rfcbot · 2025-05-16T14:59:49Z

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

tgross35

One nit then r=me

library/std/src/io/tests.rs

tgross35 · 2025-05-19T13:53:04Z

library/std/src/io/tests.rs

+    let buf = Cursor::new(b"0123456789");
+    let mut take = buf.take(2);
+    assert!(take.seek(SeekFrom::Start(3)).is_err());
+    assert!(take.seek(SeekFrom::End(1)).is_err());
+    assert!(take.seek(SeekFrom::End(-3)).is_err());
+    assert!(take.seek(SeekFrom::Current(-1)).is_err());
+    assert!(take.seek(SeekFrom::Current(3)).is_err());


Sorry, one more question. With .take(2), isn't SeekFrom::Start(3) one beyond the error condition rather than the first error? I.e. SeekFrom::Start(2) and SeekFrom::Current(2) should also fail.

The current implementation assumes that there is a valid position after the last element of the Take and you can seek to it. The rationale is to keep Read and Seek consistent. When you read the last element of a Take, the position is one after the last element. To stay consistent, seek allows to move to this position. No element can be read from that point ofc . I can add a test of this behavior if you want.

Note that there is no documented requirement in seek for this behavior. I just picked the one that made sense to me. I'm happy to change this if you think it's not correct.

Thanks for explaining, makes complete sense. A test wouldn’t hurt if you have one in mind, happy to merge after.

I've added https://github.com/rust-lang/rust/pull/138023/files#diff-7978b30f81fdec805305c6965b5f90ee70139a7f15e9f81a4f95734f715c7afcR441 and https://github.com/rust-lang/rust/pull/138023/files#diff-7978b30f81fdec805305c6965b5f90ee70139a7f15e9f81a4f95734f715c7afcR474 that checks that nothing can be read from the end of the Take.

tgross35 · 2025-05-19T16:44:27Z

Thank you for all the follow up here!

@bors r+

bors · 2025-05-19T16:44:29Z

📌 Commit c8f5ff8 has been approved by tgross35

It is now in the queue for this repository.

bors · 2025-05-19T19:28:19Z

⌛ Testing commit c8f5ff8 with merge 60dabef...

bors · 2025-05-19T22:40:08Z

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

github-actions · 2025-05-19T22:43:01Z

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

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

dist-apple-various: 7550.3s -> 5433.2s (-28.0%)
x86_64-apple-1: 7720.3s -> 9297.2s (20.4%)
dist-x86_64-apple: 9511.7s -> 10868.2s (14.3%)
dist-arm-linux: 5329.2s -> 4612.6s (-13.4%)
x86_64-gnu-llvm-19-1: 5714.7s -> 5245.4s (-8.2%)
x86_64-apple-2: 5849.2s -> 6262.4s (7.1%)
aarch64-apple: 4489.8s -> 4220.8s (-6.0%)
dist-armhf-linux: 5376.0s -> 5069.2s (-5.7%)
dist-aarch64-apple: 5803.4s -> 6093.3s (5.0%)
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.

rust-timer · 2025-05-20T00:10:16Z

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%)

programmerjake · 2025-05-20T00:15:14Z

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

Kobzol · 2025-05-20T06:58:10Z

Noise in newly added benchmarks.

@rustbot label: +perf-regression-triaged

…tgross35 Add `std::io::Seek` instance for `std::io::Take` Library tracking issue [rust-lang#97227](rust-lang#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 assigned tgross35 Mar 4, 2025

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Mar 4, 2025

tgross35 reviewed Mar 5, 2025

View reviewed changes

tgross35 added the S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. label Mar 5, 2025

melrief mentioned this pull request Mar 5, 2025

Add std::io::Seek instance for std::io::Take<T> when T: Seek rust-lang/libs-team#555

Closed

programmerjake reviewed Mar 5, 2025

View reviewed changes

rustbot added needs-fcp This change is insta-stable, so needs a completed FCP to proceed. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. labels Mar 7, 2025

thaliaarchi reviewed Mar 10, 2025

View reviewed changes

library/std/src/io/mod.rs Outdated Show resolved Hide resolved

tgross35 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 8, 2025

tgross35 requested a review from programmerjake April 24, 2025 02:58

tgross35 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-ACP Status: PR has an ACP and is waiting for the ACP to complete. labels Apr 24, 2025

programmerjake approved these changes Apr 24, 2025

View reviewed changes

rustbot added the I-libs-api-nominated Nominated for discussion during a libs-api team meeting. label Apr 24, 2025

tgross35 removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 26, 2025

rfcbot removed the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label May 16, 2025

rfcbot added the to-announce Announce this issue on triage meeting label May 16, 2025

rustbot mentioned this pull request May 16, 2025

Tracking issue for release notes of #138023: Add std::io::Seek instance for std::io::Take #141091

Open

tgross35 reviewed May 17, 2025

View reviewed changes

library/std/src/io/tests.rs Outdated Show resolved Hide resolved

melrief force-pushed the 97227_impl_Seek_for_Take branch from e2ec4b6 to 3ae6577 Compare May 19, 2025 11:53

This comment has been minimized.

Sign in to view

melrief force-pushed the 97227_impl_Seek_for_Take branch from 3ae6577 to 7e44db3 Compare May 19, 2025 12:04

tgross35 reviewed May 19, 2025

View reviewed changes

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

c8f5ff8

melrief force-pushed the 97227_impl_Seek_for_Take branch from 7e44db3 to c8f5ff8 Compare May 19, 2025 15:59

tgross35 approved these changes May 19, 2025

View reviewed changes

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label May 19, 2025

bors added the merged-by-bors This PR was explicitly merged by bors. label May 19, 2025

bors merged commit 60dabef into rust-lang:master May 19, 2025
7 checks passed

rustbot added this to the 1.89.0 milestone May 19, 2025

melrief deleted the 97227_impl_Seek_for_Take branch May 19, 2025 23:30

rustbot added the perf-regression Performance regression. label May 20, 2025

programmerjake mentioned this pull request May 20, 2025

Tracking Issue for seek_io_take and seek_io_take_position #97227

Open

1 task

rustbot added the perf-regression-triaged The performance regression has been triaged. label May 20, 2025

tgross35 added the relnotes Marks issues that should be documented in the release notes of the next release. label May 20, 2025

		#[stable(feature = "seek_io_take", since = "CURRENT_RUSTC_VERSION")]
		pub fn position(&self) -> u64 {

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

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

Uh oh!

Conversation

melrief commented Mar 4, 2025 • edited by tgross35 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Mar 4, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Mar 5, 2025

Uh oh!

melrief commented Mar 5, 2025

Uh oh!

This comment was marked as outdated.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

melrief Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

programmerjake commented Mar 7, 2025

Uh oh!

programmerjake commented Mar 7, 2025

Uh oh!

melrief commented Mar 7, 2025

Uh oh!

programmerjake commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melrief commented Mar 7, 2025

Uh oh!

thaliaarchi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaliaarchi commented Mar 10, 2025

Uh oh!

tgross35 commented Apr 24, 2025

Uh oh!

programmerjake left a comment

Choose a reason for hiding this comment

Uh oh!

tgross35 commented Apr 24, 2025

Uh oh!

tgross35 commented Apr 24, 2025

Uh oh!

programmerjake commented Apr 24, 2025

Uh oh!

rfcbot commented May 16, 2025

Uh oh!

tgross35 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Add `std::io::Seek` instance for `std::io::Take` #138023

Add `std::io::Seek` instance for `std::io::Take` #138023

melrief commented Mar 4, 2025 •

edited by tgross35

Loading

melrief Mar 6, 2025 •

edited

Loading

programmerjake commented Mar 7, 2025 •

edited

Loading

melrief May 19, 2025 •

edited

Loading