fix(formatter): estimate call-arg and attached-table line widths from stable columns

[mareklangiewicz]

Summary

Fixes #1302. Layout choices add a node's source column to a candidate's flat width. Twice that column moved with the layout being chosen, so the next pass chose differently:

 analyze_call_arg_list_layout                    # candidates are measured from `(`
-  prefix = column of the first argument          # after `(` when packed, own line when expanded
+  prefix = column of `(`
 format_table_expr (key = value fields)
-  fits = flat width + column of `{`              # `{` on the next line in the source → col 2
+  fits = flat width + column after `=`/`return`  # where the formatter attaches it → col 14

• Call args: the luafmt is not idempotent: layout of a nested call alternates between two states forever #1302 input alternated packed ↔ expanded forever: its packed first line is exactly 120 columns, counted as 121 from packed input and ~104 from expanded input. Now it is a fixed point after one pass.
• Tables: local CODES =\n { red = '31', … } was joined flat to 123 columns at max_line_width = 120, then expanded on the next pass. Now both source shapes give the expanded table at once. Only = and return are used: after , or ( the table may go on its own line, and using those made things worse.

test_non_first_multiline_callback_tail_refills_after_end moves from width 28 to 27: its packed first line test(1, "hello", function () is exactly 28 columns, which the old estimate counted as 29. At 27 it still expects the one-per-line layout it was written for.

Evidence

• Before: the two new tests in misc_tests.rs fail: each formats both source shapes of a case and requires the same output and a stable second pass.
  After: cargo test -p emmylua_formatter: 492 passed; cargo clippy -p emmylua_formatter --all-targets: clean.
• On a 168-file Lua codebase (2-space indent, width 120), files still changed by a second pass: 25 → 22; by a fourth: 2 → 0.

Not fixed here: most of the remaining second-pass changes are a callback that is one line in the source getting chain indentation on pass 1 and being attached on pass 2 (is_multiline_block_like_expr reads the source's newlines). That converges by pass 2 or 3 and is a different mechanism.

Merge Danger

Door: two-way

Blast Radius: layout of call argument lists and of key = value tables after =/return, near the line-width limit

🤖 Generated with Claude Code

https://claude.ai/code/session_01WCiE5WPKMyzj1s6ytKuoD2

[github-actions]

Code Review

Summary

The change addresses a real idempotency bug (#1302) where width estimates depended on the source layout, causing the formatter to oscillate between two layouts. The approach is sound: introduce attached_source_prefix_width for nodes that the formatter always attaches to =/return, and use the opening token's column for call-arg lists. Overall the change looks correct and well-tested.

Issues & Suggestions

1. attached_source_prefix_width — potential off-by-one / width semantics

return source_token_prefix_width(&token) + token.text().chars().count() + 1;

• The + 1 assumes a single space between the token and the node. This is consistent with the formatter attaching with one space, but it's a magic constant. Consider a named constant or a comment explaining the + 1 is the separating space.
• token.text().chars().count() counts characters, not display columns. For =/return this is fine (ASCII), but if the helper is ever reused for other tokens, this could diverge from source_token_prefix_width which presumably measures width differently. Worth a doc note that this is only valid for ASCII tokens.

2. source_token_prefix_width clones the token unnecessarily

let mut token = token.clone();

LuaSyntaxToken is a cheap rowan handle (Rc-based), so the clone is fine, but the original code took node.first_token() by value. Since the function only needs prev_token() traversal, you could take &LuaSyntaxToken and avoid the clone entirely:

fn source_token_prefix_width(token: &LuaSyntaxToken) -> usize {
    let mut width = 0usize;
    let mut cur = token.clone(); // or restructure to avoid
    ...
}

Minor, but the clone is avoidable if you restructure the loop to start from token.prev_token().

3. attached_source_prefix_width — behavior when previous token is on a different line

The loop skips TkWhitespace and TkEndOfLine, then checks for TkAssign/TkReturn. If the = is on a previous line (e.g. local x =\n {...}), the function still returns the ='s column + width + 1, which is the column the { would occupy if placed on the = line. That matches the documented intent ("the formatter always attaches it to"), so this is correct — but the doc comment could be clearer that it deliberately ignores the source line break.

4. analyze_call_arg_list_layout — semantic change worth confirming

let first_line_prefix_width = source_line_prefix_width(args.syntax());

Previously this used the first argument's column; now it uses the ('s column. The comment explains the reasoning well. However:

• args.syntax() is the LuaCallArgList; source_line_prefix_width calls first_token(), which should be the (. Confirm that LuaCallArgList's first token is indeed ( and not something else (e.g. a leading trivia token). If the node's first token could be whitespace/comment, the width would be wrong.
• The variable name first_line_prefix_width is now slightly misleading since it's no longer derived from the first argument. Consider renaming to open_paren_prefix_width or list_prefix_width for clarity.

5. Test change in expression_tests.rs

-                max_line_width: 28,
+                max_line_width: 27,

This tightens the limit by 1. Since the width computation changed, this is likely a necessary adjustment, but it's worth verifying the test still exercises the intended boundary (i.e., that 27 is the exact threshold and 28 would now produce a different layout). A brief comment explaining why 27 is the boundary would help future maintainers.

6. New tests — good coverage, minor nits

• format_twice is a nice helper. Consider asserting first == second inside it to make the idempotency contract explicit, rather than relying on each caller to do so.
• The tests assert line.len() <= 120, but len() is bytes, not display columns. For ASCII test content this is fine; if non-ASCII is ever added, this would be misleading. Consider using a column-aware check or noting the ASCII assumption.
• The expanded fixture in test_call_args_layout_does_not_depend_on_first_arg_source_column is a hand-written expected output. If the formatter's output changes slightly, this test becomes brittle. That's acceptable for a regression test, but consider deriving expanded from packed via a second config if feasible.

Verdict

The core logic is correct and the fix is well-motivated. Main actionable items:

1. Clarify the + 1 magic constant and ASCII assumption in attached_source_prefix_width.
2. Rename first_line_prefix_width to reflect it's now the ( column.
3. Verify args.syntax().first_token() is the ( (not trivia).
4. Add a comment on the 28 → 27 test threshold change.