io::Write::write_fmt: panic if the formatter fails when the stream does not fail

[RalfJung]

Follow-up to #124954

[rustbot]

r? @Mark-Simulacrum

rustbot has assigned @Mark-Simulacrum.
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

[RalfJung]

That test explicitly violates the existing std contract. It should probably be converted into a panic test?

[Noratrieb]

can you make it a should_panic unit test?

[RalfJung]

Then I'd have to split it in 3 tests -- the parts that shouldn't panic, and the two panics.
I'd rather not do that.

[Mark-Simulacrum]

r=me with or without unit test conversion

[kpreid]

I think this might not be the right choice. IO errors, broadly, might happen for any operation for many reasons, and it feels “unhelpfully pedantic” to convert an error into a panic when there's no reason that it has to be a panic. The usual reasons to panic are

• there is no error return path available,
• recovering from the error would be incorrect, or
• it clearly indicates a bug in the crate that is panicking or its caller,

but this is none of those, because the panic is in std, but the bug could be in a crate that is neither std nor the crate that is invoking io::Write::write_fmt(). What about changing only the message to make it clear that this is a bug in the formatting routine?

On the other hand, this is straightforwardly a logic bug that is easy to not write (simply don't ever construct a fmt::Error unless you're implementing a fmt::Write stream), and fmt trait implementations can have plenty of other logic bugs that panic or abort.

[workingjubilee]

This seems like it is going to introduce non-elided panic branches into all the places where it is hardest to test for codegen. Is there a soundness-based motivation for the panic here?

[workingjubilee]

As for format!, it does not return an error, I believe.

[RalfJung]

This seems like it is going to introduce non-elided panic branches into all the places where it is hardest to test for codegen.

I don't know what you mean by that.

Is there a soundness-based motivation for the panic here?

No.

[RalfJung]

I think this might not be the right choice. IO errors, broadly, might happen for any operation for many reasons, and it feels “unhelpfully pedantic” to convert an error into a panic when there's no reason that it has to be a panic.

This is not an IO error that's being converted here. Any such error in the formatter is a lingering panic anyway when used with format! (and likely with whatever form the "append format-args to String" operation will take).

So I would say it's helpfully pedantic. But I take your point, it is an avoidable panic.
We could tie it to the library UB checks... that's kind of a hack, it's not a UB check, but it would avoid the panic in release builds.

[workingjubilee]

@saethlin someone said your name /hj

[saethlin]

This seems like it is going to introduce non-elided panic branches into all the places where it is hardest to test for codegen.

I also do not know what you mean by "hard to test for codegen". Looking at generated code is easy.

[workingjubilee]

@saethlin fair enough! my main thought was "but panicking involves formatting..." and that kind of problem.

[saethlin]

In general, the standard library formatting code tends to end up with a huge amount of indirection anyway. From a bottom-up optimization view I think the codegen is pretty much unsalvagable. The call graphs from this might be a bit goofy, but we're already doing panicky string slicing in the function that reports panics from string slicing, and even Arguments::new currently recurses: #117804

I agree it's goofy and I'd like things to be different, but I think this PR adds some nice error detection capability which is probably the best sort of thing to do here.

[workingjubilee]

Alright, with a vote of "confidence" like that I'm fine with this, then, as long as we're also happy with letting this go in the future if it turns out to be a bad idea.

[RalfJung]

Yeah ofc, as with everything -- if it causes issues we can always revert. The joy of "undo". :)

@bors r=Mark-Simulacrum,workingjubilee

[bors]

📌 Commit e00f27b has been approved by Mark-Simulacrum,workingjubilee

It is now in the queue for this repository.

[bors]

⌛ Testing commit e00f27b with merge 4fd98a4...

[bors]

☀️ Test successful - checks-actions
Approved by: Mark-Simulacrum,workingjubilee
Pushing 4fd98a4 to master...

[rust-timer]

Finished benchmarking commit (4fd98a4): comparison URL.

Overall result: ❌✅ regressions and improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.6%	[0.6%, 0.6%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.4%	[-0.4%, -0.4%]	1
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.4%, 0.6%]	2

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	10.6%	[10.6%, 10.6%]	1
Regressions ❌ (secondary)	2.8%	[2.8%, 2.8%]	1
Improvements ✅ (primary)	-2.7%	[-2.7%, -2.7%]	1
Improvements ✅ (secondary)	-5.0%	[-5.0%, -5.0%]	1
All ❌✅ (primary)	4.0%	[-2.7%, 10.6%]	2

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.2%	[0.0%, 0.3%]	8
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-0.1%	[-0.1%, -0.1%]	5
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.1%	[-0.1%, 0.3%]	13

Bootstrap: 676.352s -> 677.159s (0.12%)
Artifact size: 316.07 MiB -> 315.94 MiB (-0.04%)

[RalfJung]

Uh, I did not expect this to have a perf impact on the compiler...

[Noratrieb]

a tiny change on one benchmark, the benchmark doesn't seem to be very noisy recently from the graph but i don't think this is worth investigating.

[workingjubilee]

I'm more impressed on the 10% diff in RSS on regex (...but it's only like 30MB of a 300MB execution if I'm reading the tea leaves correctly, I don't think it's worth worrying about).

[RalfJung]

RSS is very noisy, I have no idea where the threshold for significance is there.