Skip to content

Conversation

@hawkadrian
Copy link
Contributor

Summary

Replaces panic! in set_color function with a graceful fallback that returns uncolored text for unhandled token kinds. This prevents crashes when encountering tokens like TokenAt, TokenDollar, TokenPub, or newly added tokens that aren't explicitly handled in the match expression.


Type of change

Please check one:

  • Bug fix (fixes incorrect behavior)
  • New feature
  • Performance improvement
  • Documentation change with concrete technical impact
  • Style, wording, formatting, or typo-only change

⚠️ Note:
To keep maintainer workload sustainable, we generally do not accept PRs that
are only minor wording, grammar, formatting, or style changes.
Such PRs may be closed without detailed review.


Why is this change needed?

The set_color function in colored_printer.rs uses panic! for unhandled SyntaxKind variants. Several existing tokens (e.g., TokenAt, TokenDollar, TokenPub, TokenMacro, TokenContinue, TokenBreak, assignment operators like TokenDivEq, TokenMinusEq, etc.) are not explicitly handled, which causes the program to crash when these tokens are encountered during colored printing. Additionally, any future token additions to the language would trigger the same panic.

This is particularly problematic because print_colored is used in test utilities, and crashes prevent proper debugging output.


What was the behavior or documentation before?

When set_color encountered an unhandled token kind (either existing unhandled tokens or newly added ones), it would call panic!("Unexpected syntax kind: {kind:?}"), causing the entire program to crash. The TODO comment on line 126 acknowledged this limitation but didn't provide a solution.


What is the behavior or documentation after?

Unhandled token kinds now return text.clear() (uncolored text) instead of panicking. This allows the colored printer to continue functioning even when encountering tokens that aren't explicitly colorized, making the code resilient to new token additions without requiring immediate updates to the color mapping.


Related issue or discussion (if any)

Addresses the TODO comment on line 126: // TODO(yuval): Can this be made exhaustive?


Additional context

The change is minimal and maintains backward compatibility. All existing handled tokens continue to work as before. The fallback behavior (uncolored text) is consistent with how other "neutral" tokens like TokenEndOfFile, TokenMissing, and TokenEmpty are handled in the same function.

All tests pass (69 tests), and the code compiles without warnings.

@reviewable-StarkWare
Copy link

This change is Reviewable

Copy link
Collaborator

@orizi orizi left a comment

Choose a reason for hiding this comment

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

@orizi made 1 comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @hawkadrian).


a discussion (no related file):
since the printer isn't used in any of the production tools - rather panic and find the missing kinds not handled when actually used.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants