Conversation
|
This change is |
PR SummaryLow Risk Overview When a zero-width Adds fixture Reviewed by Cursor Bugbot for commit bf6ae2a. Bugbot is set up for automated code reviews on this repo. Configure here. |
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files and all commit messages, and made 1 comment.
Reviewable status: 2 of 4 files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
crates/cairo-lang-formatter/test_data/expected_results/empty_block_comment.cairo line 4 at r1 (raw file):
} fn glued_in_input() { // no leading space in input
We should also test it is not idempotent. Maybe even at the tester level
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 2 files.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on orizi and TomerStarkware).
An empty block whose sole content is a comment (`fn f() { // c }`) formatted the
comment glued to the opening brace (`{// c`): the empty statement list is skipped,
so its wrapping break points are dropped, and a leading comment gets no leading
space. It was also non-idempotent (a second pass inserted the space). When skipping
an empty statement list whose closing brace carries a comment, insert a space so it
reads `{ // c` and formatting is idempotent.
c8e5fb7 to
bf6ae2a
Compare
orizi
left a comment
There was a problem hiding this comment.
@orizi made 1 comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on eytan-starkware and TomerStarkware).
crates/cairo-lang-formatter/test_data/expected_results/empty_block_comment.cairo line 4 at r1 (raw file):
Previously, eytan-starkware wrote…
We should also test it is not idempotent. Maybe even at the tester level
Done.
eytan-starkware
left a comment
There was a problem hiding this comment.
@eytan-starkware reviewed 4 files and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on TomerStarkware).
Summary
When an empty
StatementListblock body has a comment attached to the closing brace, the formatter was omitting the space between{and the comment, producing{// commentinstead of{ // comment. A space is now inserted before the closing brace's trivia in this case, ensuring idempotent formatting.Type of change
Please check one:
Why is this change needed?
When a block body is empty (zero-width
StatementList) and the closing}carries a leading comment in its trivia, the formatter skipped the empty node without inserting any space. This caused the comment to be glued directly to the opening brace ({// comment), which is both visually incorrect and non-idempotent — re-running the formatter on its own output would produce a different result.What was the behavior or documentation before?
An empty block with a comment on the closing brace was formatted as:
What is the behavior or documentation after?
The formatter now inserts a space before the comment, producing:
This applies to all empty block bodies, including nested constructs like
loop { // comment in loop }.Related issue or discussion (if any)
Additional context
A new test case (
empty_block_comment.cairo) covers three scenarios: a top-level empty function body with a comment, input that already lacks the leading space, and a nestedloopblock with a comment.