Parser: attach comments to ExpressionStatement; handle Module.file_leading_comments (BT-976)#1013
Merged
Parser: attach comments to ExpressionStatement; handle Module.file_leading_comments (BT-976)#1013
Conversation
…ading_comments (BT-976) - Add collect_trailing_comment() — reads trailing_trivia from the last consumed token to capture end-of-line `//` comments - Update parse_module(), parse_method_body(), parse_block() to call collect_comment_attachment() before and collect_trailing_comment() after each expression parse, replacing ExpressionStatement::bare() - Handle empty-module edge case: file-level comments with no items are collected into Module.file_leading_comments; non-empty modules carry leading comments on the first item as ExpressionStatement.comments - Remove old manual comment collection loop from parse_module() that was incorrectly storing all file comments in file_leading_comments - Update 65 parser snapshots to reflect comments now correctly attached to ExpressionStatement nodes instead of file_leading_comments - Add 5 tests covering all new cases (BT-976 acceptance criteria) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughParser now collects and attaches per-statement leading and trailing comments into AST nodes: expression statements gain a Changes
Sequence Diagram(s)sequenceDiagram
participant Parser
participant TokenStream
participant AST as Module/Nodes
Parser->>TokenStream: collect_comment_attachment() (leading)
Parser->>TokenStream: parse expression statement (consume tokens)
TokenStream-->>Parser: tokens consumed
Parser->>Parser: collect_trailing_comment() (inspect last token.trailing_trivia)
Parser->>AST: construct ExpressionStatement { comments, expression }
Parser->>AST: append to Module.items or set Module.file_leading_comments (if empty)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/beamtalk-core/src/source_analysis/parser/mod.rs (1)
754-757:⚠️ Potential issue | 🟠 MajorLeading comments are dropped when the first item is a standalone method.
For non-empty modules, Line 796 clears
file_leading_comments, but standalone-method parsing does not attach leading trivia from the class-name token. Result: comments like// notebeforeCounter >> ...are lost.💡 Proposed fix in
parse_modulestandalone-method branch} else if self.is_at_standalone_method_definition() { - let method_def = self.parse_standalone_method_definition(); + let pending_comments = self.collect_comment_attachment().leading; + let mut method_def = self.parse_standalone_method_definition(); + if !pending_comments.is_empty() { + let mut merged = pending_comments; + merged.append(&mut method_def.method.comments.leading); + method_def.method.comments.leading = merged; + } method_definitions.push(method_def); } else {Please add a regression test for
// commentfollowed by a standalone method definition to lock this behavior.Also applies to: 792-800
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/beamtalk-core/src/source_analysis/parser/mod.rs` around lines 754 - 757, parse_module currently drops leading comments when the first item is a standalone method because the standalone-method branch calls parse_standalone_method_definition() but never attaches file_leading_comments or preserves leading trivia from the class-name token; update the standalone-method branch in parse_module (the is_at_standalone_method_definition() case) to capture and attach leading trivia/comments (same way other branches do by consuming file_leading_comments and/or copying leading trivia from the class-name token into the MethodDefinition node) and ensure file_leading_comments is cleared only after attaching them; add a regression test that parses a module starting with a line comment (e.g. "// comment") followed by a standalone method like "Counter >> ..." and asserts the parsed method retains that leading comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@crates/beamtalk-core/src/source_analysis/parser/declarations.rs`:
- Around line 799-802: The bug is that after let expr = self.parse_expression()
you always call comments.trailing = self.collect_trailing_comment(), which can
attach stale trailing trivia when parse_expression() made no progress; fix by
only collecting trailing comments if the parser advanced or the expression is
not the “no-progress” (error/empty) case — e.g. check whether parse_expression
consumed any tokens (or use expr.is_error() combined with a consumed_tokens
flag) before calling collect_trailing_comment(). Update the same pattern in the
loops that use collect_comment_attachment / parse_expression in functions in
expressions.rs and mod.rs so trailing comments are only captured when
parse_expression actually consumed input.
---
Outside diff comments:
In `@crates/beamtalk-core/src/source_analysis/parser/mod.rs`:
- Around line 754-757: parse_module currently drops leading comments when the
first item is a standalone method because the standalone-method branch calls
parse_standalone_method_definition() but never attaches file_leading_comments or
preserves leading trivia from the class-name token; update the standalone-method
branch in parse_module (the is_at_standalone_method_definition() case) to
capture and attach leading trivia/comments (same way other branches do by
consuming file_leading_comments and/or copying leading trivia from the
class-name token into the MethodDefinition node) and ensure
file_leading_comments is cleared only after attaching them; add a regression
test that parses a module starting with a line comment (e.g. "// comment")
followed by a standalone method like "Counter >> ..." and asserts the parsed
method retains that leading comment.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (65)
test-package-compiler/tests/snapshots/compiler_tests__abstract_class_spawn_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__actor_spawn_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__actor_spawn_with_args_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__actor_state_mutation_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__async_keyword_message_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__async_unary_message_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__async_with_await_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__binary_operators_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__blocks_no_args_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__boundary_deeply_nested_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__boundary_long_identifiers_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__boundary_mixed_errors_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__boundary_unicode_identifiers_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__cascade_complex_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__cascades_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__character_literals_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__class_definition_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__class_methods_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__comment_handling_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__control_flow_mutations_errors_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__control_flow_mutations_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__empty_blocks_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__empty_method_body_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__error_message_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__error_recovery_invalid_syntax_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__error_recovery_malformed_message_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__error_recovery_unterminated_string_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__expect_directive_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__future_pattern_matching_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__future_string_interpolation_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__hello_world_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__intrinsic_keyword_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__map_literals_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__method_lookup_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__multi_keyword_complex_args_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__nested_blocks_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__nested_keyword_messages_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__sealed_class_violation_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__sealed_method_override_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__stdlib_class_dictionary_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__stdlib_class_list_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__stdlib_class_set_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__stdlib_class_tuple_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__string_operations_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__typed_class_warnings_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__typed_methods_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__typed_value_type_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__unary_operators_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__unicode_string_literals_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__value_type_multi_expr_method_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__value_type_param_collision_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__while_true_simple_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__whitespace_handling_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__workspace_binding_cascade_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__workspace_binding_send_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_array_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_block_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_boolean_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_dictionary_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_integer_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_list_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_nil_object_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_nil_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_set_parser.snapis excluded by!**/*.snaptest-package-compiler/tests/snapshots/compiler_tests__ws_stdlib_string_parser.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
crates/beamtalk-core/src/source_analysis/parser/declarations.rscrates/beamtalk-core/src/source_analysis/parser/expressions.rscrates/beamtalk-core/src/source_analysis/parser/mod.rs
- Guard collect_trailing_comment() with progress check in parse_module(), parse_method_body(), and parse_block() — avoids attaching stale trailing trivia when parse_expression() consumes no tokens - Fix leading comment attachment for standalone method definitions: parse_standalone_method_definition() now calls collect_comment_attachment() on the class-name token before advancing, and prepends those comments to the parsed method's comments.leading (previously they were dropped because parse_identifier() consumed the token without reading its trivia) - Add regression test: standalone_method_leading_comment_attached Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Owner
Author
|
CodeRabbit outside-diff finding (mod.rs:754-757) — Fixed in 58b0cf6 Root cause: Fix: |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/beamtalk-core/src/source_analysis/parser/mod.rs (1)
758-771: Consider extracting statement-with-comments parsing into a shared helper.The same sequence now exists in
parse_module,parse_method_body, andparse_block(collect leading → parse expression → guarded trailing → buildExpressionStatement). Centralizing it would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@crates/beamtalk-core/src/source_analysis/parser/mod.rs` around lines 758 - 771, Multiple places (parse_module, parse_method_body, parse_block) duplicate the pattern of collecting leading comments, parsing an expression, conditionally collecting trailing comments based on token consumption, and constructing an ExpressionStatement; extract this into a shared helper (e.g., parse_expression_statement_with_comments) that calls collect_comment_attachment(), calls parse_expression(), uses the pos_before/current check to decide whether to call collect_trailing_comment(), and returns an ExpressionStatement struct; replace the duplicated sequences in parse_module, parse_method_body, and parse_block with calls to this new helper to centralize behavior and avoid drift.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@crates/beamtalk-core/src/source_analysis/parser/mod.rs`:
- Around line 758-771: Multiple places (parse_module, parse_method_body,
parse_block) duplicate the pattern of collecting leading comments, parsing an
expression, conditionally collecting trailing comments based on token
consumption, and constructing an ExpressionStatement; extract this into a shared
helper (e.g., parse_expression_statement_with_comments) that calls
collect_comment_attachment(), calls parse_expression(), uses the
pos_before/current check to decide whether to call collect_trailing_comment(),
and returns an ExpressionStatement struct; replace the duplicated sequences in
parse_module, parse_method_body, and parse_block with calls to this new helper
to centralize behavior and avoid drift.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
crates/beamtalk-core/src/source_analysis/parser/declarations.rscrates/beamtalk-core/src/source_analysis/parser/expressions.rscrates/beamtalk-core/src/source_analysis/parser/mod.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
collect_trailing_comment()to capture end-of-line// commentsfrom the last consumed token's trailing triviaparse_module(),parse_method_body(), andparse_block()to attach leading and trailing comments to eachExpressionStatement(replacesExpressionStatement::bare())Module.file_leading_commentsis only populated when the module has no items; non-empty modules carry leading comments on the first itemparse_module()that incorrectly put all file comments intofile_leading_commentsExpressionStatementnodesTest plan
expression_statement_leading_comment_in_method_body—//between statements attaches to second statementexpression_statement_trailing_comment— inline//attaches astrailingempty_module_file_leading_comments_populated— comments-only file populatesfile_leading_commentsnon_empty_module_file_leading_comments_empty— comments attach to first expression, notfile_leading_commentsblock_body_expression_statement_leading_comment— comments in block bodies attach correctlyLinear: https://linear.app/beamtalk/issue/BT-976
🤖 Generated with Claude Code
Summary by CodeRabbit