fix: handle synthetic comments in commentBefore function#2745
Conversation
There was a problem hiding this comment.
Code Review
This pull request modifies the commentBefore function in printer/printer.go to correctly handle synthetic comments attached via CommentedStmts when their offset matches the statement's first token offset. It also adds a corresponding unit test TestCommentedNodesInEventHandler in printer/printer_test.go to verify this behavior. No review comments were provided, so there is no additional feedback.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2745 +/- ##
==========================================
+ Coverage 94.09% 94.20% +0.10%
==========================================
Files 32 32
Lines 10074 10074
==========================================
+ Hits 9479 9490 +11
+ Misses 425 419 -6
+ Partials 170 165 -5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Overview
This PR fixes a bug where synthetic comments attached to statements via CommentedStmts were not being printed before their associated statement. The root cause: the original commentBefore check used strict < comparison, so a synthetic comment with Slash set to exactly the statement's first-token offset (offset equal, not less-than) was silently dropped.
Code Change Analysis
printer/printer.go:167–171 — commentBefore
The two changes to the return expression are both necessary:
<→<=— correct fix for the core bug: a synthetic comment whosePos()equals the statement's offset is now treated as a leading comment.- Added
p.commentOffset != infinityguard — critical correctness requirement. Without it, the new<=would fire whenp.flushis called at EOF withOffset: infinityandp.commentOffsetis alsoinfinity(no more comments). The old<happened to be safe there; the new<=is not.
The combination is correct. The comment in the code explains the <= change but omits mention of why the != infinity guard is needed — a reader might think it is defensive boilerplate when it is load-bearing.
Suggestion: Expand the inline comment to document the infinity guard:
// For synthetic comments attached via CommentedStmts, the comment offset may
// be exactly the same as the statement's first token offset. Treat it as a
// leading comment to preserve expected placement.
// The != infinity guard is required because flush() calls this with
// Offset: infinity; without it, <= would falsely match when no comments remain.
return p.commentOffset != infinity && p.commentOffset <= next.Offset && (!p.impliedSemi || !p.commentNewline)printer/nodes.go:1417–1424 — stmt (unchanged, context only)
The injection point where setComment is called before the switch. setComment only fires when p.useNodeComments is true — the PR relies on this path being active, which it is when CommentedNodes is used. This looks correct.
Test Coverage
TestCommentedNodesInEventHandler correctly reproduces the bug scenario:
- Parses a
.goxfile with anonStart =>event handler (the exact construct that triggered the issue). - Sets
Slash: eventStmt.Pos()to co-locate the synthetic comment with the statement (the key precondition). - Asserts the comment text immediately precedes
onStart =>in the output.
Minor observations:
- The test uses
strings.Containsrather than an exact match, which is appropriate since the full file has surrounding content. ShadowEntryaccess is guarded with nil checks before use — good defensive test setup.- The test does not cover the
infinityedge case directly. This is acceptable: the guard is already exercised implicitly by every test that callsFprintto completion (all of which invoke the finalflushat EOF). - Existing
TestCommentedNodessubtests continue to cover the strict<cases, providing regression coverage.
Summary
The fix is correct, minimal, and well-targeted. The new test covers the bug scenario precisely. The only actionable suggestion is to document why the != infinity guard is load-bearing in the inline comment, so a future reader does not mistake it for optional boilerplate and remove it.
|
please create an issue before creating a PR. |
created: #2747 |
fix: #2747