Implement io::Seek for io::Empty

[oberien]

Fix #78029

[rust-highfive]

r? @shepmaster

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

[nagisa]

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]

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:

rust/library/std/src/io/cursor.rs

Lines 210 to 234 in dda2b5e

	fn seek(&mut self, style: SeekFrom) -> io::Result<u64> {
	let (base_pos, offset) = match style {
	SeekFrom::Start(n) => {
	self.pos = n;
	return Ok(n);
	}
	SeekFrom::End(n) => (self.inner.as_ref().len() as u64, n),
	SeekFrom::Current(n) => (self.pos, n),
	};
	let new_pos = if offset >= 0 {
	base_pos.checked_add(offset as u64)
	} else {
	base_pos.checked_sub((offset.wrapping_neg()) as u64)
	};
	match new_pos {
	Some(n) => {
	self.pos = n;
	Ok(self.pos)
	}
	None => Err(Error::new(
	ErrorKind::InvalidInput,
	"invalid seek to a negative or overflowing position",
	)),
	}
	}

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.

[pickfire]

Looks good to me.

[shepmaster]

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

r? @KodrAus

[oberien]

I rebased to the current master.

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

[oberien]

I rebased to the current master.

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

[jyn514]

Would love to get eyes on this :) It would help quite a bit with algesten/ureq#252.

[KodrAus]

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

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

• @Amanieu
• @BurntSushi
• @KodrAus
• @dtolnay
• @m-ou-se
• @sfackler
• @withoutboats

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.

[sfackler]

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]

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]

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

[sfackler]

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

[oberien]

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]

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.

[m-ou-se]

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.

That same code would break in the same way on File::open("/dev/null").

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

I don't mind either way.

Let's change that then before merging this.

[rust-log-analyzer]

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

Click to see the possible cause of the failure (guessed by this bot)

   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]

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

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

[m-ou-se]

Great! Thank you!

@bors r+ rollup

[bors]

📌 Commit f1cd179 has been approved by m-ou-se