Deduplicate panic_fmt #88860

nbdd0121 · 2021-09-11T16:41:56Z

std's begin_panic_fmt and core's panic_fmt are duplicates. Merge them to declutter code and remove a lang item.

rust-highfive · 2021-09-11T16:41:58Z

(rust-highfive has picked a reviewer for you, use r? to override)

klensy · 2021-09-11T16:53:43Z

library/std/src/panic.rs

-        $crate::rt::begin_panic_fmt(&$crate::const_format_args!($fmt, $($arg)+))
+        $crate::rt::begin_panic_fmt($crate::const_format_args!($fmt, $($arg)+))


Why?

Ahh, begin_panic_fmt get params by reference, when panic_fmt takes by value. It's actually interesting to change panic_fmt to get it by reference too, as reference should be shorter than Arguments struct.

It's an existing inconsistency between libcore and libstd API. The inconsistency actually makes the compiler/rustc_const_eval/src/const_eval/machine.rs's begin_panic_fmt -> const_panic_fmt replacement unsound (but since const panic doesn't allow arguments today this unsound path is never hit).

Ahh, begin_panic_fmt get params by reference, when panic_fmt takes by value. It's actually interesting to change panic_fmt to get it by reference too, as reference should be shorter that Arguments struct.

I actually don't think there'll be any meaningful difference. Size of Arguments would make it be passed by reference anyway.

Plus, many existing methods like write_fmt by value and Arguments is Copy so I think it's actually good to take it by value for consistency.

print-type-size type: `std::fmt::Arguments`: 48 bytes, alignment: 8 bytes print-type-size field `.pieces`: 16 bytes print-type-size field `.fmt`: 16 bytes print-type-size field `.args`: 16 bytes

What are you suggesting by listing the numbers? As I've said, the size of Arguments (is larger than 2 usize) would make it be passed by reference. So codegen will behave exactly the same with or without the & (https://godbolt.org/z/h69MKjdsn), and for semantics and consistency it should be Arguments rather than &Arguments.

Sorry, i didn't knew that there some optimization that pass big Copy types by ref even if i explicitly pass it by value.

Value passed into PanicInfo::internal_constructor by Option'ing reference anyway, so there is no sense to copying it before that. (this comment about pub fn panic_fmt(fmt: fmt::Arguments<'_>) -> !)

Tried to check that in small example, looks like parameter really isn't copied in panic_fmt (didn't checked this in stdlib). I remember that i played somewhere with fmt::Arguments and saw needless copying in stdlib.

bors · 2021-09-17T18:50:03Z

☔ The latest upstream changes (presumably #89047) made this pull request unmergeable. Please resolve the merge conflicts.

nbdd0121 · 2021-10-18T16:47:13Z

@rustbot label: +T-libs

m-ou-se · 2021-10-19T09:02:57Z

Thanks!

@bors r+

bors · 2021-10-19T09:02:59Z

📌 Commit 5636a13 has been approved by m-ou-se

m-ou-se · 2021-10-19T09:06:29Z

Note that clippy also has a reference to begin_panic_fmt that can be removed.

Deduplicate panic_fmt std's begin_panic_fmt and core's panic_fmt are duplicates. Merge them to declutter code and remove a lang item.

nbdd0121 · 2021-10-19T12:59:54Z

Note that clippy also has a reference to begin_panic_fmt that can be removed.

Done

m-ou-se · 2021-10-19T13:55:32Z

(I haven't looked at the failure above in detail yet, but note that x86_64-gnu-tools only runs if there are changes in e.g. src/tools/clippy; meaning that it wasn't run before your last commit. It might be that the first commit already broke clippy's test.)

nbdd0121 · 2021-10-19T14:00:01Z

It seems that clippy is already sort of broken without my changes. if_then_panic wouldn't trigger with 2021 edition or if the crate is compiled with #![no_std].

nbdd0121 · 2021-10-19T14:02:06Z

Hmm, this lint is introduced after this PR is submitted.. Need a rebase to find out..

std's begin_panic_fmt and core's panic_fmt are duplicates. Merge them to declutter code and remove a lang item.

m-ou-se · 2021-10-19T14:03:46Z

Yeah, clippy tends to depend heavily on the exact details of the std macros to recognize them, and doesn't seem to recognize core's macros. Rust 2021 uses core::panic with std as well, which clippy doesn't understand.

Every time we change the panic macro in std, clippy breaks. We should probably do something about that.

m-ou-se · 2021-10-19T14:04:47Z

I'm guessing that it trips on the & that got removed, which makes it unable to recognize the panic message and arguments anymore.

nbdd0121 · 2021-10-19T14:15:08Z

https://github.com/rust-lang/rust-clippy/blob/389a74b31aabca4aac3289763eeb2eccedd1b988/clippy_utils/src/higher.rs#L717 -- That's a very detailed assumption about what it expands to 🙃.

So AddrOf is indeed matched. And I guess the let ExprKind::Block is the reason it won't recognize core's macro. Another data point on why the brace should be removed.

m-ou-se · 2021-10-19T15:06:06Z

@bors r+

bors · 2021-10-19T15:06:08Z

📌 Commit 7bd93df has been approved by m-ou-se

Rollup of 10 pull requests Successful merges: - rust-lang#86479 (Automatic exponential formatting in Debug) - rust-lang#87404 (Add support for artifact size profiling) - rust-lang#87769 (Alloc features cleanup) - rust-lang#88789 (remove unnecessary bound on Zip specialization impl) - rust-lang#88860 (Deduplicate panic_fmt) - rust-lang#90009 (Make more `From` impls `const` (libcore)) - rust-lang#90018 (Fix rustdoc UI for very long type names) - rust-lang#90025 (Revert rust-lang#86011 to fix an incorrect bound check) - rust-lang#90036 (Remove border-bottom from most docblocks.) - rust-lang#90060 (Update RELEASES.md) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Deduplicate panic_fmt std's begin_panic_fmt and core's panic_fmt are duplicates. Merge them to declutter code and remove a lang item.

This reverts commit f702499, reversing changes made to 84fe598.

Fix manual_assert and match_wild_err_arm for `#![no_std]` and Rust 2021 Rust 2015 `std::panic!` has a wrapping block while `core::panic!` and Rust 2021 `std::panic!` does not. See rust-lang/rust#88919 for details. Note that the test won't pass until clippy changes in rust-lang/rust#88860 is synced. --- changelog: Fix [`manual_assert`] and [`match_wild_err_arm`] for `#![no_std]` and Rust 2021. Fixes #7723

rust-highfive assigned dtolnay Sep 11, 2021

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 11, 2021

This comment has been minimized.

Sign in to view

klensy reviewed Sep 11, 2021

View reviewed changes

nbdd0121 force-pushed the panic branch from c14860c to 1570e1e Compare September 11, 2021 17:02

nbdd0121 force-pushed the panic branch from 1570e1e to 5636a13 Compare September 17, 2021 18:54

JohnCSimon added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 3, 2021

apiraino added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Oct 14, 2021

rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Oct 18, 2021

Mark-Simulacrum assigned Mark-Simulacrum and unassigned dtolnay Oct 18, 2021

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 Oct 19, 2021

m-ou-se assigned m-ou-se and unassigned Mark-Simulacrum Oct 19, 2021

m-ou-se added A-runtime Area: std's runtime and "pre-main" init for handling backtraces, unwinds, stack overflows and removed T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 19, 2021

GuillaumeGomez mentioned this pull request Oct 19, 2021

Rollup of 7 pull requests #90049

Closed

This comment has been minimized.

Sign in to view

Deduplicate panic_fmt

9370156

std's begin_panic_fmt and core's panic_fmt are duplicates. Merge them to declutter code and remove a lang item.

Remove begin_panic_fmt from clippy

7bd93df

nbdd0121 force-pushed the panic branch from 3d736e6 to 7bd93df Compare October 19, 2021 14:16

m-ou-se mentioned this pull request Oct 19, 2021

Clippy breaks when we change anything about the std macros rust-lang/rust-clippy#7843

Closed

nbdd0121 mentioned this pull request Oct 19, 2021

Remove braces from Rust 2015 panic macro #88919

Closed

JohnTitor mentioned this pull request Oct 19, 2021

Rollup of 10 pull requests #90067

Merged

bors merged commit f702499 into rust-lang:master Oct 20, 2021

rustbot added this to the 1.58.0 milestone Oct 20, 2021

nbdd0121 deleted the panic branch October 20, 2021 16:29

nbdd0121 mentioned this pull request Oct 20, 2021

Fix manual_assert and match_wild_err_arm for #![no_std] and Rust 2021 rust-lang/rust-clippy#7851

Merged

flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 21, 2021

Rollup merge of rust-lang#88860 - nbdd0121:panic, r=m-ou-se

30cc96e

Deduplicate panic_fmt std's begin_panic_fmt and core's panic_fmt are duplicates. Merge them to declutter code and remove a lang item.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Oct 27, 2021

Revert "Rollup merge of rust-lang#88860 - nbdd0121:panic, r=m-ou-se"

d751558

This reverts commit f702499, reversing changes made to 84fe598.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deduplicate panic_fmt #88860

Deduplicate panic_fmt #88860

nbdd0121 commented Sep 11, 2021

rust-highfive commented Sep 11, 2021

This comment has been minimized.

klensy Sep 11, 2021 •

edited

Loading

nbdd0121 Sep 11, 2021 •

edited

Loading

nbdd0121 Sep 11, 2021

klensy Sep 11, 2021

nbdd0121 Sep 11, 2021

klensy Sep 11, 2021 •

edited

Loading

klensy Sep 11, 2021

bors commented Sep 17, 2021

nbdd0121 commented Oct 18, 2021

m-ou-se commented Oct 19, 2021

bors commented Oct 19, 2021

m-ou-se commented Oct 19, 2021 •

edited

Loading

nbdd0121 commented Oct 19, 2021

This comment has been minimized.

m-ou-se commented Oct 19, 2021

nbdd0121 commented Oct 19, 2021

nbdd0121 commented Oct 19, 2021

m-ou-se commented Oct 19, 2021

m-ou-se commented Oct 19, 2021

nbdd0121 commented Oct 19, 2021

m-ou-se commented Oct 19, 2021

bors commented Oct 19, 2021

		$crate::rt::begin_panic_fmt(&$crate::const_format_args!($fmt, $($arg)+))
		$crate::rt::begin_panic_fmt($crate::const_format_args!($fmt, $($arg)+))

Deduplicate panic_fmt #88860

Deduplicate panic_fmt #88860

Conversation

nbdd0121 commented Sep 11, 2021

rust-highfive commented Sep 11, 2021

This comment has been minimized.

klensy Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

nbdd0121 Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

nbdd0121 Sep 11, 2021

Choose a reason for hiding this comment

klensy Sep 11, 2021

Choose a reason for hiding this comment

nbdd0121 Sep 11, 2021

Choose a reason for hiding this comment

klensy Sep 11, 2021 • edited Loading

Choose a reason for hiding this comment

klensy Sep 11, 2021

Choose a reason for hiding this comment

bors commented Sep 17, 2021

nbdd0121 commented Oct 18, 2021

m-ou-se commented Oct 19, 2021

bors commented Oct 19, 2021

m-ou-se commented Oct 19, 2021 • edited Loading

nbdd0121 commented Oct 19, 2021

This comment has been minimized.

m-ou-se commented Oct 19, 2021

nbdd0121 commented Oct 19, 2021

nbdd0121 commented Oct 19, 2021

m-ou-se commented Oct 19, 2021

m-ou-se commented Oct 19, 2021

nbdd0121 commented Oct 19, 2021

m-ou-se commented Oct 19, 2021

bors commented Oct 19, 2021

klensy Sep 11, 2021 •

edited

Loading

nbdd0121 Sep 11, 2021 •

edited

Loading

klensy Sep 11, 2021 •

edited

Loading

m-ou-se commented Oct 19, 2021 •

edited

Loading