fix panic on failure to format generics in enum #6396
Conversation
ding-young force-pushed the fix-panic-on-enum branch from 97e8897 to 733e1ef Compare 
| @ding-young thanks for picking this one up! I wonder if now would be a good time to refactor Let me know what you think? |
| Can we also add this smaller reproducible case from the original issue: enum Node where P::<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S<S>>>>>>>>>>>>>>>>>>>>>>>>: { Cons, } |
| @ding-young there were also some linked issues like #6137, #6318, #6378. Would be good to check these issues out as well. #6378 doesn't seem to have a minimal test case so feel free to ignore that one. |
ding-young force-pushed the fix-panic-on-enum branch from 733e1ef to ff6ebd3 Compare 
| Thank you for listing all the related issues! I included only the cases that originally lead to a rustfmt crash. |
| About refactoring I still need to take a closer look to ensure the original behavior is preserved, so I’ll leave it as a draft, but it would be great if you could briefly check whether I’m refactoring in the right direction. |
| @ding-young thanks for looking into this refactor. I won't have time to review this PR this weekend, but I'll try to set aside some time for a review next week. |
| Is this PR still the primary PR for fixing this bug? Or #6630? It would seem I'm another person who has hit the same issue! This PR fixes Rustfmt for me, but without it I can't format the project! Let me know if there is anything I can do to help move things forward! |
I was originally perplexed as to why the crate wasn't already formatted, but quickly found out why... Though the PR seems to have languished for some time, I've run a patched version of rustfmt that completes without crashing: rust-lang/rustfmt#6396
#31) * style: run (patched) rustfmt I was originally perplexed as to why the crate wasn't already formatted, but quickly found out why... Though the PR seems to have languished for some time, I've run a patched version of rustfmt that completes without crashing: rust-lang/rustfmt#6396 * style: fix `mismatched_lifetime_syntaxes` lint * style: remove unnecessary macro imports This is a strange case — I had to look this up myself! Because you have a `#[macro_use]` above the `params` module in `lib.rs`, all of the macros in `params.rs` are automatically made available crate-wide. This import of the macro in `scan_properties.rs` is therefore redundant! The same logic applies to `impl_param_described`. https://lukaswirth.dev/tlborm/decl-macros/minutiae/scoping.html * style: fix clippy `derivable_impls` For `enum`s, you can add a tag and `#[derive(Default)]` instead of manually implementing the same code: https://doc.rust-lang.org/std/default/trait.Default.html#enums * style: fix clippy `single_match` * style: fix clippy `redundant_closure` * style: clippy fix `unnecessary_unwrap` * style: clippy fix `manual_is_multiple_of` NOTE: This method was added in a recent version of Rust (1.87.0), so changing to this code means that our minimum supported Rust version would become that! If you'd rather keep compatibility with older-than-stable versions of Rust, we should leave this unchanged! * chore(edition): run `cargo update` * style(edition): run `cargo --fix edition` Performs some automatic migration to the 2024 edition of Rust. Will be followed by a cleanup commit. * style(edition): bump to Rust 2024 * style(edition): reformat using (patched) rustfmt This is now a different, newer PR that supports the 2024 edition: rust-lang/rustfmt#6630 * style(edition): `expr_2021` -> `expr` in macros The 2024 edition changed `expr`s to accept `const` and `_` expressions, but because no macros in `mzdata` included any `const` or `_` keywords in their syntax, no change needs to be made. * style(edition): `match` back to `if let` None of the migrated code seemed to depend on any exact `Drop` timing, so the overly-cautious `if let` -> `match` migration was unnecessary. * style(edition): change `gen` to `this_generation` In the 2024 edition, `gen` became a reserved Rust keyword. Using `r#gen` is still possible, but probably not best practice, so I've renamed the old `gen` variables. * style: fix clippy `let_and_return` * style: fix clippy `collapsible_if`
- related issues: rust-lang#5738, rust-lang#6137, rust-lang#6318, rust-lang#6378 - instead of calling unwrap(), restore original snippet when we fail to format generics in enum - we need to propagate this rewrite failure later
- introduce format_enum that returns Rewrite - early return when it fails to format the generics in enum
ding-young force-pushed the fix-panic-on-enum branch from af03169 to c85a3dd Compare | @avrabe Hi! I've added your test case on #6630. Haven't run it on your repository, though. Would you take a look? @TheLostLambda I lost some contexts on my pr, but maybe this pr is the primary fix, I guess. To push this change pr forward, we’ll need to find someone who has permission to merge it. I guess the core maintainers have been busy lately. |
| Would you mind reviewing this? @camsteffen @jieyouxu |
4 participants
fixes #5738
Description
Above issue reports panic when formatting complex enum with generic parameters.
This is because rustfmt simply calls
unwrap()onRewrite(RewriteResultin future), the return value fromformat_generics.It is better not to panic on this sort of rewrite failure.
Therefore, this pr restores original snippet when we fail to formatgenericsin enum instead of callingunwrap().Noneearly and then leave entire enum unformatted instead of callingunwrap().visit_enumis refactored withformat_enumthat returnsRewrite. Later pr will replace this rewrite withRewriteResultTODO
visitor.rs?