[liziye627-design]

Fixes #1300.

When a call's arguments or a table's fields were broken onto separate lines, a block comment written before the comma (a --[[@as T]], b) was emitted as a trailing comment after the comma (a, --[[@as T]]). For an --[[@as T]] cast that moves the cast onto the next item.

extract_trailing_comment now records whether the comment is a block comment (--[[ ]] / --[=[ ]=]) that is followed by a ,/;. Such a comment is appended to the item's doc, so it stays before the separator. Every other trailing comment (line comments, a comment on the last item, a comment after the comma) is handled as before. This covers call arguments and table fields, the two lists named in the issue; function parameter lists use a separate path and are unchanged.

-- before
g(
    a, --[[@as T]]
    b
)
-- after
g(
    a --[[@as T]],
    b
)

Tests (in expression_tests.rs): call-arg and table-field cases, plus an idempotency check that includes a cast on the last argument. The two layout tests fail on main and pass with the change.

• cargo test -p emmylua_formatter: 498 passed
• cargo fmt --all -- --check and cargo clippy -p emmylua_formatter --all-targets -- -D warnings: clean
• pre-commit run --all (pre-commit 4.7.0 in a venv): exit 0, every hook passed (fmt, schema, prettier, whitespace/EOF/yaml/toml/json checks, typos); the broken-symlink hook was skipped with no files to check. The working tree was unchanged afterwards.

Relation to #1310: that PR also touches comment handling in this crate (render/mod.rs, multiline trailing comments, #1299). This PR changes formatter/expr.rs and expression_tests.rs only. git merge-tree against #1310's head merges without conflicts, and cargo test -p emmylua_formatter passes on the merged result (499 tests).

Drafted with AI assistance; the tests above were run locally.

[github-actions]

Code Review

Summary

The change fixes a real bug (Issue #1300): block comments like --[[@as T]] placed before a comma were being moved after the comma, causing them to attach to the next list item. The approach of tracking before_separator and inlining such comments into the item's doc is sound. Tests are well-targeted and include an idempotency check.

Issues & Suggestions

1. is_block_comment is fragile / duplicated logic

fn is_block_comment(node: &LuaSyntaxNode) -> bool {
    let text = node.text().to_string();
    let Some(rest) = text.strip_prefix("--[") else { return false; };
    rest.trim_start_matches('=').starts_with('[')
}

• Allocation: node.text().to_string() allocates on every call. Use node.text().to_string() only if needed, or better, inspect the token kind. LuaComment typically has a CommentKind/is_block accessor — prefer that over string parsing.
• Correctness edge case: --[=foo (malformed) would return false correctly, but --[==[ vs --[===[ are both handled. However --[ followed by [ with no = (i.e. --[[) works, but --[ + =* + [ where the closing bracket count differs is not validated — acceptable since the parser already validated it, but relying on string parsing duplicates parser knowledge.
• Suggestion: use the parser's comment kind if available (e.g. LuaComment::cast(node).map(|c| c.is_block())), or at minimum avoid the allocation with node.text().to_string() → node.text().to_string() replaced by node.text().to_string() … actually use node.text().to_string() only once; better: node.text().to_string() → node.text().to_string(). Concretely: let text = node.text().to_string(); → let text = node.text().to_string(); is unavoidable with current API, but consider node.text().to_string() → node.text().to_string(). Recommendation: check for an existing is_block_comment helper in the codebase and reuse it.

2. separator_follows scans unbounded siblings

fn separator_follows(node: &LuaSyntaxNode) -> bool {
    let mut next = node.next_sibling_or_token();
    while let Some(sibling) = next {
        match sibling.kind() {
            LuaKind::Token(LuaTokenKind::TkWhitespace) => {}
            LuaKind::Token(LuaTokenKind::TkSemicolon) | LuaKind::Token(LuaTokenKind::TkComma) => return true,
            _ => return false,
        }
        next = sibling.next_sibling_or_token();
    }
    false
}

• The loop is effectively bounded by the first non-whitespace token, so it terminates quickly — fine.
• But: it does not check for a newline in the whitespace. A comment followed by a newline then a comma on the next line would be treated as before_separator = true, incorrectly inlining the comment before the separator. The doc comment says "on the same line" but the code doesn't enforce it. Add a newline check on whitespace tokens (e.g. if sibling.text().contains('\n') { return false; }).

3. extract_trailing_comment — seen_separator semantics

LuaKind::Token(LuaTokenKind::TkSemicolon) | LuaKind::Token(LuaTokenKind::TkComma) => {
    seen_separator = true;
}

• Once a separator is seen, the loop continues scanning up to 4 siblings. If a comment appears after the separator, before_separator is correctly false. Good.
• However, the loop bound for _ in 0..4 combined with seen_separator means a comment after , + whitespace + comment is found (2 iterations), fine. But if there are multiple whitespace tokens (unlikely) it could miss. Low risk.
• Minor: seen_separator is only meaningful for the before_separator computation; consider a comment explaining why the loop continues past the separator (to still extract the trailing comment for placement after the separator). The current code is correct but non-obvious.

4. attach_comment_before_separator mutates doc and returns Option

fn attach_comment_before_separator(
    doc: &mut Vec<DocIR>,
    comment: Option<ExtractedTrailingComment>,
) -> Option<(Vec<DocIR>, TextRange)> {
    let comment = comment?;
    if comment.before_separator {
        doc.push(ir::space());
        doc.extend(comment.docs);
        return None;
    }
    Some((comment.docs, comment.range))
}

• Naming: the function both attaches (when before separator) and returns (when after). The name suggests only attaching. Consider split_trailing_comment or document the dual behavior clearly (the doc comment does, but the name is misleading).
• Spacing: doc.push(ir::space()) unconditionally adds a space before the comment. For a block comment immediately adjacent to the expression (e.g. a--[[@as T]]), this changes a--[[...]] to a --[[...]]. That's likely desirable (matches render_comment_with_spacing), but verify it doesn't double-space if the doc already ends with a space. Since format_expr output typically doesn't end with trailing space, this is fine — but worth a test for a--[[@as T]], (no space in source).
• Consistency: render_comment_with_spacing is used for the comment docs; the manual ir::space() may conflict with the spacing config (line_comment_min_spaces_before). For block comments this is probably fine, but consider using the same spacing helper for consistency.

5. consumed_comment_ranges push moved before formatting

In both collect_call_arg_entries and collect_table_entries, the range is now pushed before format_expr/format_table_field_ir is called. This is fine (order of push doesn't matter), but note that previously the push happened only when trailing_comment was Some. Now it happens when extracted is Some — equivalent. No issue.

6. Test coverage gaps

• No test for a--[[@as T]], (no space before comment) — verify spacing normalization.
• No test for a block comment before ; separator (only , tested).
• No test for a block comment followed by a newline then comma (the separator_follows newline concern in #2).
• No test for a line comment before a comma (e.g. a -- comment\n, b) — should remain after separator / not be inlined. The is_block_comment guard handles this, but a regression test would be valuable.
• No test for nested/multiple block comments.

7. Minor: ExtractedTrailingComment visibility

The struct is private (no pub), used only within the module — correct. But it's defined between collect_* functions and extract_trailing_comment; consider placing it near extract_trailing_comment for readability. Cosmetic.

8. is_block_comment vs separator_follows argument types

is_block_comment(&child) where child is a LuaSyntaxNode (from node.children()), and is_block_comment(comment_node) where comment_node is LuaSyntaxNode. Consistent. Fine.

Recommendations (priority order)

1. Fix separator_follows to reject newlines in whitespace (correctness bug for multi-line cases).
2. Reuse existing block-comment detection if available; otherwise avoid the allocation and add a comment about parser validation.
3. Add tests for: no-space-before-comment, ; separator, line-comment-before-comma (negative case), and newline-before-comma.
4. Rename attach_comment_before_separator to reflect dual behavior (e.g. split_comment_by_separator).
5. Consider using the spacing helper instead of raw ir::space() for consistency with render_comment_with_spacing.

Overall the fix is well-structured and the idempotency test is a good safeguard. The main correctness concern is #2 (newline handling in separator_follows).

[CppCXY]

这个检查消耗太大了, 应该判断firstToken是否是LuaTokenKind::TkDocLongStart