Implement default methods for io::Empty and io::Sink

[thaliaarchi]

Implements default methods of io::Read, io::BufRead, and io::Write for io::Empty and io::Sink. These implementations are equivalent to the defaults, except in doing less unnecessary work.

Read::read_to_string and BufRead::read_line both have a redundant call to str::from_utf8 which can't be inlined from core and Write::write_all_vectored has slicing logic which can't be simplified (See on Compiler Explorer). The rest are optimized to the minimal with -C opt-level=3, but this PR gives that benefit to unoptimized builds.

This includes an implementation of Write::write_fmt which just ignores the fmt::Arguments<'_>. This could be problematic whenever a user formatting impl is impure, but the docs do not guarantee that the args will be expanded.

Tracked in #136756.

r? @m-ou-se

[m-ou-se]

Would it be a valid implementation of Write::write_fmt if the fmt::Arguments<'_> were just ignored? This would be problematic whenever a user formatting impl is impure. But is it actually guaranteed that the args will be expanded?

As far as I know, we technically don't guarantee that the write_fmt method uses the fmt::Arguments exactly once. Skipping it entirely when the output is ignored anyway seems entirely reasonable to me.

ping @rust-lang/libs-api - Please speak up if you don't think it's okay that write!(io::sink(), "{x}") just skips calling Display::fmt on x.

[thaliaarchi]

I've added nop Write::write_fmt.

@rustbot label -S-waiting-on-author +S-waiting-on-review

[thaliaarchi]

Test write-fmt-error expects a panic for Sink::write_fmt, given an Err-returning fmt impl:

rust/tests/ui/write-fmt-errors.rs

Lines 36 to 41 in ed49386

	let res = catch_unwind(|| write!(sink(), "{} {} {}", 1, ErrorDisplay, "bar"));
	let err = res.expect_err("formatter error did not lead to panic").downcast::<&str>().unwrap();
	assert!(
	err.contains("formatting trait implementation returned an error"),
	"unexpected panic: {}", err
	);

Should I change the test or revert the write_fmt changes?

[m-ou-se]

I don't think the purpose of that test is to test the behaviour of io::sink() specifically. It should be fine to change it to something that actually writes. (E.g. &mut Vec<u8>.)

[thaliaarchi]

I've added tests for the new impls.

I also decided to keep Empty::is_read_vectored as the default false. I see it this way: Empty has nothing to read, so we don't need to read that nothing in larger batches; Sink can write large amounts cheaply, so we should batch those writes as large as possible.

In the standard library, only Chain<T, U> non-trivially uses is_read_vectored. It returns true when either of its inner Readers returns true. With Empty in a chain, if Empty::is_read_vectored() == true, it would force the other to use vectored reads:

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

Lines 2729 to 2731 in f04bbc6

	fn is_read_vectored(&self) -> bool {
	self.first.is_read_vectored() || self.second.is_read_vectored()
	}

This could be somewhat improved by making it conditional on whether the first has finished (but then constant propagation is probably worsened):

fn is_read_vectored(&self) -> bool {
    if !self.done_first { self.first.is_read_vectored() } else { self.second.is_read_vectored() }
}

In the standard library, only BufWriter<W> and LineWriterShim<'a, W> non-trivially use is_write_vectored. They select a writing strategy based on the inner writer's is_write_vectored. With Sink::is_write_vectored() == true (as currently), it does less work, because it batches more (nop) writes together. But, vectored or not, a buffered sink is admittedly dumb.

[thaliaarchi]

@m-ou-se Hey, friendly 2-week ping. Would you mind taking a look at the changes since your last review?

[m-ou-se]

@bors r+

[bors]

📌 Commit 523b9d9 has been approved by m-ou-se

It is now in the queue for this repository.

[bors]

🌲 The tree is currently closed for pull requests below priority 100. This pull request will be tested once the tree is reopened.