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

Draft
wants to merge 2 commits 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().

  • This pr returns None early and then leave entire enum unformatted instead of calling unwrap().
  • visit_enum is refactored with format_enum that returns Rewrite. Later pr will replace this rewrite with RewriteResult

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

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

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

- 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
Copy link
Contributor Author

Thank you for listing all the related issues! I included only the cases that originally lead to a rustfmt crash.

@ding-young
Copy link
Contributor Author

About refactoring visit_enum, I think introducing format_enum would make it easier to propagate errors later, as we wouldn't need to visit all the push_str and push_rewrite calls scattered throughout. I personally feel it may be better for users to have their enum variants formatted even if generics aren’t, and vice versa, while listing all rewrite errors if possible. Anyway, I think we can leave that for later consideration.

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 ding-young marked this pull request as draft November 29, 2024 15:25
@ytmimi
Copy link
Contributor

ytmimi commented Nov 29, 2024

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

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