io::Write::write_fmt: panic if the formatter fails when the stream does not fail by RalfJung · Pull Request #125012 · rust-lang/rust (original) (raw)

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 andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation26 Commits1 Checks6 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

RalfJung

@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

@rustbot rustbot added S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.

labels

May 11, 2024

@rust-log-analyzer

This comment has been minimized.

@RalfJung

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

Noratrieb

@@ -1,9 +1,11 @@
//@ run-pass
//@ needs-unwind

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you make it a should_panic unit test?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Mark-Simulacrum added the relnotes

Marks issues that should be documented in the release notes of the next release.

label

May 11, 2024

@Mark-Simulacrum

r=me with or without unit test conversion

@rust-log-analyzer

This comment has been minimized.

@RalfJung

@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

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

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 bors added S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

and removed S-waiting-on-review

Status: Awaiting review from the assignee but also interested parties.

labels

May 12, 2024

@bors

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

This was referenced

Jul 26, 2024

@khuey khuey mentioned this pull request

Sep 30, 2024

netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request

Oct 13, 2024

@he32

This is based on the pkgsrc-wip rust180 package, retaining the main pkgsrc changes as best as I could.

Pkgsrc changes:

Upstream chnages:

Version 1.80.1 (2024-08-08)

Version 1.80.0 (2024-07-25)

Language

Compiler

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.79.0 (2024-06-13)

Language

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Rustdoc

Misc

Compatibility Notes

Version 1.78.0 (2024-05-02)

Language

Compiler

Target changes:

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

These APIs are now stable in const contexts:

Cargo

Misc

Compatibility Notes

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Version 1.77.0 (2024-03-21)

Compiler

Refer to Rust's [platform support page][platform-support-doc] for more information on Rust's tiered platform support.

Libraries

Stabilized APIs

Cargo

Rustdoc

Misc

Internal Changes

These changes do not affect any public interfaces of Rust, but they represent significant improvements to the performance or internals of rustc and related tools.

Labels

merged-by-bors

This PR was explicitly merged by bors.

relnotes

Marks issues that should be documented in the release notes of the next release.

S-waiting-on-bors

Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

T-libs

Relevant to the library team, which will review and decide on the PR/issue.