Github Implement io::Seek for io::Empty by oberien · Pull Request #78044 · rust-...

Contributor

rust-highfive commented on Oct 17, 2020

r? @shepmaster

(rust_highfive has picked a reviewer for you, use r? to override)

nagisa commented on Oct 17, 2020

This makes sense to me, though exact behaviour when the seek would go past the 0-th position might need further discussion.

• io::Cursor currently returns Err if seek is past the end;
• File on UNIX will work just fine with seeking past the end and will properly set the position to that location;

And this introduces the 3rd behaviour where seeking past the end silently works, but does not update the position either.

oberien commented on Oct 17, 2020 •

The documentation of Seek states

A seek beyond the end of a stream is allowed, but behavior is defined by the implementation. […]

Errors

Seeking to a negative offset is considered an error.

From that documentation my impression was that seeking beyond the end is totally fine. I think that's also what Cursor is doing:

In that implementation, Cursor::seek only errors if the offset would underflow (go below 0 → seek to a negative position) or overflow (go above u64::max_value()). To me this seems similar to the behaviour you describe for File on unix.

I agree that it might make sense to mirror that behaviour of Seek to actually seek to that offset and return it as the new offset. However, right now io::Empty is a unit-struct and thus a zero-sized type. Adding a pos: u64 member to it would increase its size to 8 bytes, which shouldn't be necessary for most use-cases. Thus I settled on the behaviour of this PR to always seek to position 0, erroring when seeking below 0, without the need for any overhead.
This could break code which assumes that the internal position is actually updated (e.g. seeking to SeekFrom::Current(x), then seeking back with SeekFrom::Current(-x)). However, the documentation explicitly states that seeking beyond the end is implementation defined, so any such code would be in violation of that documentation.

shepmaster commented on Oct 19, 2020

Since traits are insta-stable, this probably needs a team sign-off.

r? @KodrAus

This comment has been hidden.

oberien commented on Nov 11, 2020

I rebased to the current master.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

This comment has been hidden.

oberien commented on Nov 19, 2020

I rebased to the current master.

@rustbot modify labels: +S-waiting-on-review -S-waiting-on-author

library/std/src/io/util.rs

Outdated

Show resolved

KodrAus commented 17 days ago

@rfcbot fcp merge

This implements Seek for io::Empty such that:

• The stream length is reported as 0.
• Seeking with a negative value returns an error.
• Seeking with any positive value will return Ok, but won't update the position.

rfcbot commented 17 days ago •

Team member @KodrAus 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.

m-ou-se commented 10 days ago

The documentation of the Seek trait says:

A seek beyond the end of a stream is allowed, but behavior is defined by the implementation.

Does this sentence mean that all implementations always need to return Ok for any positive offset, and only the behaviour afterwards is implementation defined? Or is returning Err also a possible implementation-specific behaviour? (But then what does 'allowed' mean here?)

sfackler commented 10 days ago

I would interpret that as saying that the Seek impl shouldn't panic if you try to seek past the end, but could either return success or failure.

m-ou-se commented 10 days ago

Having seek(Current(100)); succeed, but seek(Current(-100)); fail afterwards doesn't seem great. I think either the first seek should fail, or both seek calls should succeed.

Some data points:

• Seeking in any direction on /dev/null on Linux always succeeds, even if the offset is negative. It always reports a position of 0. (So both calls would succeed. (Even in reverse order.))

• Seeking on a regular file or Cursor doesn't accept negative offsets, but it does allow seek(Current(100)); seek(Current(-100));, as it tracks the current position. (So both calls would succeed, when done in that order.)

However, since the only stable promise we're making here is that io::Empty implements Seek, without making any promises about the behaviour of negative or past-the-end seeks, merging this seems fine to me.

rfcbot commented 10 days ago

This is now entering its final comment period, as per the review above.

sfackler commented 10 days ago

Mirroring /dev/null's behavior seems pretty reasonable to me personally.

oberien commented 10 days ago •

I agree that mirroring /dev/null could make sense here, as it feels more intuitive. I decided to error on negative seeks as the documentation of Seek::seek states

Errors

Seeking to a negative offset is considered an error.

I'm not sure if this is considered a guarantee, but there could be code that relies on seek(Current(100)); seek(Current(-101)) to error. For example a function consuming a stream backwards could have the exit condition be an Err returned by the seek, which would result in an infinite loop if we were to mirror /dev/null.

I don't mind either way.

rfcbot commented 7 hours ago

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.

The RFC will be merged soon.

library/std/src/io/util.rs

Outdated

Show resolved

rust-log-analyzer commented 2 hours ago

The job x86_64-gnu-llvm-9 failed! Check out the build log: (web) (plain)

   Compiling miniz_oxide v0.4.0
   Compiling hashbrown v0.9.0
   Compiling object v0.22.0
   Compiling addr2line v0.14.0
error: unused imports: `ErrorKind`, `Error`
  |
  |
8 |     self, BufRead, Error, ErrorKind, Initializer, IoSlice, IoSliceMut, Read, Seek, SeekFrom, Write,
  |                    ^^^^^  ^^^^^^^^^
  |
  = note: `-D unused-imports` implied by `-D warnings`
error: aborting due to previous error

error: could not compile `std`

rust-log-analyzer commented 1 hour ago

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

configure: rust.channel         := nightly
configure: rust.debug-assertions := 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: 
---
    Checking rand v0.7.3
    Checking alloc v0.0.0 (/checkout/library/alloc)
    Checking core v0.0.0 (/checkout/library/core)
    Checking std v0.0.0 (/checkout/library/std)
error: use of associated function `core::num::<impl u64>::max_value` that will be deprecated in a future Rust version: replaced by the `MAX` associated constant on this type
  --> library/std/src/io/util/tests.rs:37:45
   |
37 |     assert!(matches!(e.seek(SeekFrom::Start(u64::max_value())), Ok(0)));
   |
   |
   = note: `-D deprecated-in-future` implied by `-D warnings`

error: use of associated function `core::num::<impl i64>::min_value` that will be deprecated in a future Rust version: replaced by the `MIN` associated constant on this type
  --> library/std/src/io/util/tests.rs:39:43
   |
39 |     assert!(matches!(e.seek(SeekFrom::End(i64::min_value())), Ok(0)));


error: use of associated function `core::num::<impl i64>::max_value` that will be deprecated in a future Rust version: replaced by the `MAX` associated constant on this type
  --> library/std/src/io/util/tests.rs:43:43
   |
43 |     assert!(matches!(e.seek(SeekFrom::End(i64::max_value())), Ok(0)));


error: use of associated function `core::num::<impl i64>::min_value` that will be deprecated in a future Rust version: replaced by the `MIN` associated constant on this type
  --> library/std/src/io/util/tests.rs:45:47
   |
45 |     assert!(matches!(e.seek(SeekFrom::Current(i64::min_value())), Ok(0)));


error: use of associated function `core::num::<impl i64>::max_value` that will be deprecated in a future Rust version: replaced by the `MAX` associated constant on this type
  --> library/std/src/io/util/tests.rs:49:47
   |
49 |     assert!(matches!(e.seek(SeekFrom::Current(i64::max_value())), Ok(0)));

error: aborting due to 5 previous errors

error: could not compile `std`
error: could not compile `std`

To learn more, run the command again with --verbose.
warning: build failed, waiting for other jobs to finish...
error: build failed
command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-Zbinary-dep-depinfo" "-j" "16" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace compiler-builtins-c" "--manifest-path" "/checkout/library/test/Cargo.toml" "--all-targets" "-p" "test" "-p" "std" "-p" "unwind" "-p" "alloc" "-p" "proc_macro" "-p" "core" "-p" "panic_unwind" "-p" "panic_abort" "-p" "term" "--message-format" "json-render-diagnostics"
failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu --all-targets
Build completed unsuccessfully in 0:00:42

oberien commented 19 minutes ago

I changed io::Empty::seek to follow /dev/null and squashed the commits back together into a single one.

m-ou-se commented 18 minutes ago

Great! Thank you!

@bors r+ rollup

bors commented 18 minutes ago

Commit f1cd179 has been approved by m-ou-se