Skip to content

Avoid duplicating multiline trailing comments - #1310

Open
sean0407 wants to merge 1 commit into
EmmyLuaLs:mainfrom
sean0407:codex/emmlua-1299-formatter-comment-duplication
Open

sean0407 wants to merge 1 commit into
EmmyLuaLs:mainfrom
sean0407:codex/emmlua-1299-formatter-comment-duplication

Conversation

@sean0407

Copy link
Copy Markdown

Summary

Fixes issue #1299, where a multiline trailing comment node was emitted again after the statement suffix had already printed it. This caused every formatting pass to append another copy.

The formatter now treats the multiline comment node as inline trailing trivia, and a regression test checks both the initial output and second-pass stability.

Evidence

Reproduction:

printf 'local x = f() --[[c]] -- a\n-- b\nlocal y = 1\n' | luafmt

Before the fix, output duplicated --[[c]] -- a and -- b; the focused regression test failed. After the fix, the output matches the input and a second formatting pass is identical.

Checks run:

  • cargo test --locked -p emmylua_formatter — 496 passed, 0 failed.
  • cargo fmt --all --check — passed.

Files changed:

  • crates/emmylua_formatter/src/formatter/render/mod.rs
  • crates/emmylua_formatter/src/test/comment_tests.rs

Merge Danger

Low. The change only adjusts how inline trailing comment nodes are classified, with a focused formatter regression test. It does not alter parsing or other formatting rules.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

Summary

The change removes a guard that prevented multi-line block comments from being treated as inline trailing comments. This is a targeted fix for a duplication bug, but the removal may have broader side effects.


Issues & Recommendations

1. Removed guard may cause multi-line block comments to be misclassified (Potential regression)

Location: crates/emmylua_formatter/src/formatter/render/mod.rs:424

The removed condition:

&& !comment_node.text().contains_char('\n')

previously ensured that a comment containing a newline (i.e., a multi-line block comment like --[[\n...\n]]) could not be classified as an inline trailing comment. Removing it means such comments can now be treated as inline trailing comments.

Concern: A multi-line block comment placed after code on the same line, e.g.:

local x = f() --[[
  multi-line
  block
]]

will now be considered "inline trailing". Depending on how the renderer emits inline trailing comments (typically appended to the same output line), this could produce malformed output or incorrect indentation.

Recommendation: Verify with a test that multi-line block comments after code on the same line still format correctly. If the intent was only to fix the specific duplication case, consider a narrower guard, e.g.:

&& (!comment_node.text().contains_char('\n')
    || /* allow only when it's a single-line block comment */)

or explicitly check that the comment is a single-line block comment (--[[...]] without embedded newlines) rather than dropping the check entirely.


2. Test does not cover the multi-line block comment scenario

Location: crates/emmylua_formatter/src/test/comment_tests.rs:2396

The new test only covers single-line block comments (--[[c]]). Since the removed guard specifically targeted newline-containing comments, add a regression test such as:

let source = "local x = f() --[[\n  multi\n]]\nlocal y = 1\n";

and assert idempotency and expected layout.

Recommendation: Add a test for multi-line block comments to confirm the behavior change is intentional and correct.


3. Test name vs. assertion mismatch (minor)

The test is named test_inline_comments_before_following_comment_are_not_duplicated, but the assertions only check idempotency (first == source, second == first). The name implies a specific duplication scenario; consider renaming to something like test_inline_block_comment_idempotent or adding an explicit assertion that the comment appears exactly once (e.g., assert_eq!(first.matches("--[[c]]").count(), 1)).


4. Missing explanatory comment for the removed guard

Since the removed line was a non-obvious guard, it would help future maintainers to add a brief comment explaining why multi-line comments are now allowed here (e.g., "multi-line block comments are handled by the block-comment path; allowing them here fixes duplication when followed by another comment").


Verdict

The fix appears reasonable for the reported duplication bug, but the removal of the newline guard is a behavior change with potential edge cases. Recommend adding a multi-line block comment regression test before merging to confirm no formatting regressions.

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.

luafmt duplicates a trailing block comment + line comment when a comment line follows

1 participant