Add `std::io::Seek` instance for `std::io::Take` by melrief · Pull Request #138023 · rust-lang/rust (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

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

Closes: rust-lang/libs-team#555

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

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

Comment on lines 2861 to 2862

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can just be a separate feature gate

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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.

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

This comment was marked as outdated.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

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

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

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.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 { /* ... */

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.

joshtriplett removed the T-libs

Relevant to the library team, which will review and decide on the PR/issue.

label

Apr 29, 2025

Starting an FCP since this is insta-stable.

@rfcbot merge

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

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.

Labels

disposition-merge

This issue / PR is in PFCP or FCP with a disposition to merge it.

needs-fcp

This change is insta-stable, so needs a completed FCP to proceed.

proposed-final-comment-period

Proposed to merge/close by relevant subteam, see T- label. Will enter FCP once signed off.

S-waiting-on-team

Status: Awaiting decision from the relevant subteam (see the T- label).

T-libs-api

Relevant to the library API team, which will review and decide on the PR/issue.