Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Seek instance for io::Take #97230

Closed
wants to merge 3 commits into from
Closed

Conversation

wangbj
Copy link

@wangbj wangbj commented May 20, 2022

Library tracking issue #97227.

@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs to request review from a libs-api team reviewer. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 20, 2022
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Mark-Simulacrum (or someone else) soon.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 20, 2022
@rust-log-analyzer

This comment has been minimized.

@wangbj
Copy link
Author

wangbj commented May 20, 2022

Note I added

#[unstable(feature = "seek_io_take", issue = "37214")]

Befreo impl<T: Seek> for Take<T> but rustc complaines:

     = note: `#[deny(ineffective_unstable_trait_impl)]` on by default
     = note: see issue #55436 <https://github.com/rust-lang/rust/issues/55436> for more information

@the8472
Copy link
Member

the8472 commented May 21, 2022

SeekFrom allows seeking beyond the end of an object. This could be observable by doing (&file).take(n).seek(m) and then dropping the Take but continuing to operate on the file.

Since some branches clamp the seek value via min I think test-cases and documentation should cover what happens when the underlying source starts with a seek position beyond its end and when one tries to seek past its end. Some of those cases might be surprising or inconsistent with what SeekFrom says.

@wangbj
Copy link
Author

wangbj commented May 22, 2022

@the8472 You are right if .take(N) return fewer than N bytes indeed cause issues (when calling seek). What would you suggest for the path forward? Do you think we could save stream_position() in struct Take?

@compiler-errors
Copy link
Member

This is an impl of a public trait on a public type, and therefore insta-stable. Not sure if needs a tracking issue, but it is a T-libs-api issue instead of a T-libs one, per this comment.

r? rust-lang/libs-api @rustbot label +T-libs-api -T-libs

@rustbot rustbot added the T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. label May 28, 2022
@rustbot rustbot removed the T-libs Relevant to the library team, which will review and decide on the PR/issue. label May 28, 2022
@rust-log-analyzer

This comment has been minimized.

@compiler-errors
Copy link
Member

@wangbj heads up that merge commits are not allowed in PRs. I recommend git pull --rebase instead in the future, and you should probably try to rework your git history so that merge commit is no longer included. See: https://rustc-dev-guide.rust-lang.org/git.html#no-merge-policy

@rust-log-analyzer
Copy link
Collaborator

The job mingw-check failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
configure: rust.debug-assertions := True
configure: rust.overflow-checks := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/library/std/src/io/tests.rs at line 618:
         assert_eq!(bytes, *b"01");
         assert_eq!(stream.stream_position().unwrap(), 2);
         assert_eq!(stream.seek(SeekFrom::Current(2)).unwrap(), 4);
-        assert_eq!(stream.seek(SeekFrom::Current(-5)).unwrap_err().kind(), io::ErrorKind::InvalidInput);
+        assert_eq!(
+            stream.seek(SeekFrom::Current(-5)).unwrap_err().kind(),
+            io::ErrorKind::InvalidInput
+        );
         assert_eq!(stream.seek(SeekFrom::Start(1)).unwrap(), 1);
         assert!(stream.read_exact(&mut bytes).is_ok());
         assert_eq!(bytes, *b"12");
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2021" "--unstable-features" "--skip-children" "--check" "/checkout/library/std/src/io/readbuf/tests.rs" "/checkout/library/std/src/io/util/tests.rs" "/checkout/library/std/src/io/cursor/tests.rs" "/checkout/library/std/src/io/readbuf.rs" "/checkout/library/std/src/io/stdio.rs" "/checkout/library/std/src/io/tests.rs" "/checkout/library/std/src/io/stdio/tests.rs" "/checkout/library/std/src/sys_common/thread_local_key/tests.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.

@wangbj
Copy link
Author

wangbj commented Jun 6, 2022

Close in flavor of #97807 97807

@wangbj wangbj closed this Jun 6, 2022
@wangbj wangbj deleted the seek_io_take branch June 6, 2022 21:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants