rustfmt ICE with expanded code

[ojeda]

Code

Minimized from expanded code (i.e. -Zunpretty=expanded) that we rustfmt in the Linux kernel to make it easier to read:

const _: () = builtin # offset_of(x, x);

Which makes sense, given:

https://github.com/rust-lang/rust/blob/7606c13961ddc1174b70638e934df0439b7dc515/src/tools/rustfmt/src/expr.rs#L401-L406

However, it would be nice to ignore (or format) these so that we can format expanded code.

Meta

It breaks in the latest stable (1.76.0) and nightly:

rustc 1.78.0-nightly (516b6162a 2024-03-03)

Error output

thread 'main' panicked at src/tools/rustfmt/src/expr.rs:405:13:
internal error: entered unreachable code

Backtrace

thread 'main' panicked at src/tools/rustfmt/src/expr.rs:405:13:
internal error: entered unreachable code
stack backtrace:
   0:     0x7fbfb27e405f - std::backtrace_rs::backtrace::libunwind::trace::h4257829118b815c0
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/../../backtrace/src/backtrace/libunwind.rs:104:5
   1:     0x7fbfb27e405f - std::backtrace_rs::backtrace::trace_unsynchronized::h8bf698b8571a7ffa
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/../../backtrace/src/backtrace/mod.rs:66:5
   2:     0x7fbfb27e405f - std::backtrace::Backtrace::create::hf360b329ac3984cf
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/backtrace.rs:331:13
   3:     0x7fbfb27e3fa0 - std::backtrace::Backtrace::force_capture::hce7c17c21d97619a
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/backtrace.rs:312:9
   4:     0x7fbfaf49641f - std[f9c89fb8bbe75f53]::panicking::update_hook::<alloc[7bfe078ffefd51e6]::boxed::Box<rustc_driver_impl[5cb1284730e04d20]::install_ice_hook::{closure#0}>>::{closure#0}
   5:     0x7fbfb27fef00 - <alloc::boxed::Box<F,A> as core::ops::function::Fn<Args>>::call::hd5a664d4e890a68c
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/alloc/src/boxed.rs:2030:9
   6:     0x7fbfb27fef00 - std::panicking::rust_panic_with_hook::h1329efa5bdc573dd
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:783:13
   7:     0x7fbfb27fec09 - std::panicking::begin_panic_handler::{{closure}}::h6ae91631ec1cf42d
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:649:13
   8:     0x7fbfb27fc186 - std::sys_common::backtrace::__rust_end_short_backtrace::hda139f8d0aadc9cd
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/sys_common/backtrace.rs:171:18
   9:     0x7fbfb27fe974 - rust_begin_unwind
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:645:5
  10:     0x7fbfb2849085 - core::panicking::panic_fmt::h862551fb494a270a
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/core/src/panicking.rs:72:14
  11:     0x7fbfb2849143 - core::panicking::panic::hddedaf52f5081cd3
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/core/src/panicking.rs:144:5
  12:     0x55e95779da61 - rustfmt_nightly[ce9e407bc4d9aa35]::expr::format_expr
  13:     0x55e957802286 - <rustfmt_nightly[ce9e407bc4d9aa35]::stmt::Stmt as rustfmt_nightly[ce9e407bc4d9aa35]::rewrite::Rewrite>::rewrite
  14:     0x55e9578193aa - <rustfmt_nightly[ce9e407bc4d9aa35]::visitor::FmtVisitor>::walk_stmts
  15:     0x55e957812870 - <rustfmt_nightly[ce9e407bc4d9aa35]::visitor::FmtVisitor>::visit_block
  16:     0x55e95779ecfb - rustfmt_nightly[ce9e407bc4d9aa35]::expr::rewrite_block_with_visitor
  17:     0x55e95779f0cc - rustfmt_nightly[ce9e407bc4d9aa35]::expr::rewrite_block_inner
  18:     0x55e95779d5fc - rustfmt_nightly[ce9e407bc4d9aa35]::expr::format_expr
  19:     0x55e9577a907f - rustfmt_nightly[ce9e407bc4d9aa35]::expr::rewrite_assign_rhs_expr::<rustc_ast[6684c6343d18f394]::ast::Expr>
  20:     0x55e9577ab367 - rustfmt_nightly[ce9e407bc4d9aa35]::expr::rewrite_assign_rhs_with_comments::<&alloc[7bfe078ffefd51e6]::string::String, rustc_ast[6684c6343d18f394]::ast::Expr>
  21:     0x55e9577b98fd - <rustfmt_nightly[ce9e407bc4d9aa35]::visitor::FmtVisitor>::visit_static
  22:     0x55e95781601d - <rustfmt_nightly[ce9e407bc4d9aa35]::visitor::FmtVisitor>::visit_item
  23:     0x55e9577f9f06 - <rustfmt_nightly[ce9e407bc4d9aa35]::visitor::FmtVisitor>::visit_items_with_reordering
  24:     0x55e957819bdb - <rustfmt_nightly[ce9e407bc4d9aa35]::visitor::FmtVisitor>::format_separate_mod
  25:     0x55e9576961ce - <rustfmt_nightly[ce9e407bc4d9aa35]::Session<std[f9c89fb8bbe75f53]::io::stdio::Stdout>>::format_input_inner::{closure#0}
  26:     0x55e9576ab731 - rustfmt[22febc4b220ec6a2]::format_and_emit_report::<std[f9c89fb8bbe75f53]::io::stdio::Stdout>
  27:     0x55e9576a9bcc - rustfmt[22febc4b220ec6a2]::execute
  28:     0x55e9576a529b - rustfmt[22febc4b220ec6a2]::main
  29:     0x55e95768fa83 - std[f9c89fb8bbe75f53]::sys_common::backtrace::__rust_begin_short_backtrace::<fn(), ()>
  30:     0x55e957691d39 - std[f9c89fb8bbe75f53]::rt::lang_start::<()>::{closure#0}
  31:     0x7fbfb27e1043 - core::ops::function::impls::<impl core::ops::function::FnOnce<A> for &F>::call_once::he5f334fe0a08c913
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/core/src/ops/function.rs:284:13
  32:     0x7fbfb27e1043 - std::panicking::try::do_call::h231e398ce39da676
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:552:40
  33:     0x7fbfb27e1043 - std::panicking::try::h46412093dcdab127
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:516:19
  34:     0x7fbfb27e1043 - std::panic::catch_unwind::hfbb2d28c473eb4dd
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panic.rs:146:14
  35:     0x7fbfb27e1043 - std::rt::lang_start_internal::{{closure}}::h083e0c5f8fcad290
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/rt.rs:148:48
  36:     0x7fbfb27e1043 - std::panicking::try::do_call::h37fd7901d6535c55
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:552:40
  37:     0x7fbfb27e1043 - std::panicking::try::h7c485b04206d8759
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panicking.rs:516:19
  38:     0x7fbfb27e1043 - std::panic::catch_unwind::hfea1c5c7d30edbf3
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/panic.rs:146:14
  39:     0x7fbfb27e1043 - std::rt::lang_start_internal::hc4df6736fe95cc4a
                               at /rustc/516b6162a2ea8e66678c09e8243ebd83e4b8eeea/library/std/src/rt.rs:148:20
  40:     0x55e9576ac8f5 - main
  41:     0x7fbfac319d90 - <unknown>
  42:     0x7fbfac319e40 - __libc_start_main
  43:     0x55e95767f269 - <unknown>
  44:                0x0 - <unknown>


rustc version: 1.78.0-nightly (516b6162a 2024-03-03)
platform: x86_64-unknown-linux-gnu

Cc: @metaspace (who found it when trying to expand his rnull driver)
From: https://rust-for-linux.zulipchat.com/#narrow/stream/291565-Help/topic/Using.20the.20.2Ersi.20target/near/423973701.

[ytmimi]

Pretty sure this is a duplicate of #5885. I think the quickest fix for now would be to return None instead of calling unreachable!()

[ojeda]

Indeed, looks like it, thanks!

If ignoring (or similar) is possible, that is great. In general, what would be best is knowing that rustfmt supports the -Zunpretty=expanded output.

[ytmimi]

To the best of my knowledge, as long as the output from -Zunpretty=expanded is valid, parsable rust code then rustfmt should be able to handle it, but I'm going to err on the side of caution and say it depends on the input you give rustfmt. I haven't really used -Zunpretty=expanded and I don't think we have any tests setup to more thoroughly verify that.

One caveat would be a case like this where there was an assumption that certain AST nodes would never appear directly in the AST, and instead would be expanded later in the compilation process. There might also be cases where rustfmt doesn't handle experimental, nightly-only syntax.

Another thing to keep in mind is that the max_width is a hard constraint for rustfmt. Depending on how verbose / nested the expansion is rustfmt might not be able to format certain AST nodes if they can't fit within the max_width limit.

[ojeda]

Thanks for the analysis! Yeah, -Zunpretty=expanded is a feature that people is finding useful in the kernel, so it would be nice to make sure it is possible to format the output with rustfmt to make it a bit more readable (though it is also true that some of the output is already fairly readable). Would it make sense to test the formatting in tests for -Zunpretty=expanded itself, for instance, or here?

I think it is reasonable that the output of rustfmt here is best-effort, i.e. if something isn't as pretty as in the "normal code" case, or some lines aren't even formatted at all, I think that would be fine. In other words, getting most of a file formatted is already better than getting nothing.

If you want, we could reuse this issue to make it more about the general idea of ensuring it can process (to some degree) -Zunpretty=expanded code without ICE'ing.

[ytmimi]

Overall I think makes sense to add one off code snippets like the one you included in the issue description as test cases for rustfmt, but I don't know if we'd set up exhaustive -Zunpretty=expanded tests in rustfmt (at least not at this time).

It's hard to guarantee that rustfmt can handle all the -Zunpretty=expanded code you throw at it. After all, there are plenty of bugs formatting unexpanded code 😅 and I'm sure all of them would apply to -Zunpretty=expanded input too. We've also had instances where seemingly unrelated changes in the compiler suddenly cause crashes in nightly rustfmt so the chance of an ICE is never really 0%.

I agree that it would be better if rustfmt could process the -Zunpretty=expanded input without ICE'ing, and right now I think it makes sense to tackle each ICE as it pops up. If you or anyone else working on the kernel finds that there are other inputs that cause rustfmt to crash or behave strangely it would be great to open up issues as they arise!

In this case I think the fix for this one is something like this:

diff --git a/src/expr.rs b/src/expr.rs
index 147f4b31..6a21d88a 100644
--- a/src/expr.rs
+++ b/src/expr.rs
@@ -404,8 +404,11 @@ pub(crate) fn format_expr(
         ast::ExprKind::FormatArgs(..)
         | ast::ExprKind::IncludedBytes(..)
         | ast::ExprKind::OffsetOf(..) => {
-            // These do not occur in the AST because macros aren't expanded.
-            unreachable!()
+            // These don't normally occur in the AST because macros aren't expanded. However,
+            // rustfmt tries to parse macro arguments when formatting macros, so it's not totally
+            // impossible for rustfmt to come across one of these nodes when formatting a file.
+            // Also, rustfmt might get passed the output from `-Zunpretty=expanded`.
+            None
         }
         ast::ExprKind::Err => None,
     };

[ojeda]

Thanks! That makes sense -- we will report the ICEs then. I added a note about it on our linked issue.

Of course, bugs can happen :) It was more about knowing whether expanded output was intended to be handled or not.

[Tanjaint21]

@rustbot claim

[y86-dev]

I just hit this issue with 1.79.0-nightly (1684a753d 2024-04-01) and I am a bit confused, should this already be in nightly?

[ytmimi]

@y86-dev I can understand the confusion. Fixes applied directly to rustfmt won't be reflected in nightly until we sync rustfmt up with rust-lang/rust

[ojeda]

For reference: I can see it fixed in the current nightly (1.81.0), but not the current beta (1.80.0).

[calebcartwright]

1.80 beta was cut before the subtree sync with the fix. i don't recall offhand where 1.80 is on the release train but if it's not too late then we could see if the relevant teams would support a backport to beta to pull it in

[ojeda]

Ah, sorry, I wrote the message for reference since I was taking a look to see in which version it got fixed.

But, of course, getting it into 1.80.0 would be nice, since some people want to use the feature and it has been a while. There are a bit more than 3 weeks until its release.