Skip to content
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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix panic on failure to format generics in enum #6396

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ding-young
Copy link
Contributor

@ding-young ding-young commented Nov 17, 2024

fixes #5738

Description

Above issue reports panic when formatting complex enum with generic parameters.
This is because rustfmt simply calls unwrap() on Rewrite (RewriteResult in future), the return value from format_generics.
It is better not to panic on this sort of rewrite failure.

Therefore, this pr restores original snippet when we fail to format generics in enum instead of calling unwrap().
We'll need to propagate this rewrite failure later, too.

TODO

  • Revisit test case
  • Any other solutions ? Calling existing functions provided in visitor.rs?
  • check whether version gate is required (originally it would've panic, so I'm not sure if this is a formatting change)
  • check other related issues like Crash during rustfmt #6378

- instead of calling unwrap(), restore original snippet when we fail to format generics in enum
- we need to propagate this rewrite failure later
@ytmimi
Copy link
Contributor

ytmimi commented Nov 21, 2024

@ding-young thanks for picking this one up! I wonder if now would be a good time to refactor visit_enum, and introduce a format_enum function, which we can return a RewriteResult from, similar to how visit_static and visit_struct are implemented. Then if we fail to format the generics we can just return early with an error and leave the enum unformatted.

Let me know what you think?

@ytmimi
Copy link
Contributor

ytmimi commented Nov 21, 2024

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,
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we double check that all of these enums trigger the bug. Probably best to remove any that don't.

@ytmimi
Copy link
Contributor

ytmimi commented Nov 21, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

panic: Option::unwrap() on a None value
3 participants