diff --git a/crates/emmylua_formatter/src/formatter/expr.rs b/crates/emmylua_formatter/src/formatter/expr.rs index bd8ce7eaa..909ed35fb 100644 --- a/crates/emmylua_formatter/src/formatter/expr.rs +++ b/crates/emmylua_formatter/src/formatter/expr.rs @@ -23,8 +23,9 @@ use super::sequence::{ }; use super::spacing::{SpaceRule, space_around_binary_op}; use super::trivia::{ - count_blank_lines_before, has_non_trivia_before_on_same_line_tokenwise, - node_has_direct_comment_child, source_line_prefix_width, trailing_gap_requests_alignment, + attached_source_prefix_width, count_blank_lines_before, + has_non_trivia_before_on_same_line_tokenwise, node_has_direct_comment_child, + source_line_prefix_width, trailing_gap_requests_alignment, }; #[derive(Clone, Copy, Default)] @@ -1509,7 +1510,7 @@ fn format_table_expr(ctx: &FormatContext, plan: &FormatPlan, expr: &LuaTableExpr let mut flat_layout = layout; flat_layout.strategy = ExpandStrategy::Never; let flat_docs = format_delimited_sequence(ctx, flat_layout); - if ir::ir_flat_width(&flat_docs) + source_line_prefix_width(expr.syntax()) + if ir::ir_flat_width(&flat_docs) + attached_source_prefix_width(expr.syntax()) <= ctx.config.layout.max_line_width { flat_docs @@ -1568,7 +1569,7 @@ fn format_table_expr(ctx: &FormatContext, plan: &FormatPlan, expr: &LuaTableExpr let mut flat_layout = layout.clone(); flat_layout.strategy = ExpandStrategy::Never; let flat_docs = format_delimited_sequence(ctx, flat_layout); - if ir::ir_flat_width(&flat_docs) + source_line_prefix_width(expr.syntax()) + if ir::ir_flat_width(&flat_docs) + attached_source_prefix_width(expr.syntax()) <= ctx.config.layout.max_line_width { return flat_docs; diff --git a/crates/emmylua_formatter/src/formatter/layout/mod.rs b/crates/emmylua_formatter/src/formatter/layout/mod.rs index 10a850ee4..9f0f493a2 100644 --- a/crates/emmylua_formatter/src/formatter/layout/mod.rs +++ b/crates/emmylua_formatter/src/formatter/layout/mod.rs @@ -320,10 +320,10 @@ fn analyze_call_arg_list_layout(ctx: &FormatContext, args: &LuaCallArgList, plan is_multiline_block_like_expr(arg) || matches!(arg, LuaExpr::TableExpr(table) if table.syntax().text().contains_char('\n')) }); - let first_line_prefix_width = arg_exprs - .first() - .map(|arg| source_line_prefix_width(arg.syntax())) - .unwrap_or(0); + // The candidates are measured from the opening `(`, so the prefix is that token's column. The first + // argument's column would depend on this list's own previous layout (on the `(` line or below it), + // and the next pass could then choose differently (#1302). + let first_line_prefix_width = source_line_prefix_width(args.syntax()); let first_arg_multiline_block = arg_exprs.first().is_some_and(is_multiline_block_like_expr); let first_arg_multiline_table = matches!(arg_exprs.first(), Some(LuaExpr::TableExpr(table)) if table.syntax().text().contains_char('\n')); let mut single_inline_block_arg_index = None; diff --git a/crates/emmylua_formatter/src/formatter/trivia.rs b/crates/emmylua_formatter/src/formatter/trivia.rs index eeb24c153..e14f468f5 100644 --- a/crates/emmylua_formatter/src/formatter/trivia.rs +++ b/crates/emmylua_formatter/src/formatter/trivia.rs @@ -1,4 +1,4 @@ -use emmylua_parser::{LuaKind, LuaSyntaxKind, LuaSyntaxNode, LuaTokenKind}; +use emmylua_parser::{LuaKind, LuaSyntaxKind, LuaSyntaxNode, LuaSyntaxToken, LuaTokenKind}; use rowan::TextRange; /// Count how many blank lines appear before a node. @@ -55,10 +55,33 @@ pub fn has_non_trivia_before_on_same_line_tokenwise(node: &LuaSyntaxNode) -> boo } pub fn source_line_prefix_width(node: &LuaSyntaxNode) -> usize { + node.first_token() + .map_or(0, |token| source_token_prefix_width(&token)) +} + +/// Where `node` (a table's `{`) will start when it follows `=` or `return`, which the formatter always attaches +/// it to: that token's source column, its width and a space. The node's own source column would depend on +/// whether it sat on that token's line, and the next pass could then decide differently (#1302). After any +/// other token (`,`, `(`) the node may go on a line of its own, so its own column is kept. +pub fn attached_source_prefix_width(node: &LuaSyntaxNode) -> usize { + let mut previous = node.first_token().and_then(|token| token.prev_token()); + while let Some(token) = previous { + match token.kind().to_token() { + LuaTokenKind::TkWhitespace | LuaTokenKind::TkEndOfLine => previous = token.prev_token(), + LuaTokenKind::TkAssign | LuaTokenKind::TkReturn => { + // Both tokens are ASCII, so their char count is their width; `+ 1` is the space after them. + return source_token_prefix_width(&token) + token.text().chars().count() + 1; + } + _ => break, + } + } + source_line_prefix_width(node) +} + +/// The source column of `token`: the width of its line before it. +fn source_token_prefix_width(token: &LuaSyntaxToken) -> usize { let mut width = 0usize; - let Some(mut token) = node.first_token() else { - return 0; - }; + let mut token = token.clone(); while let Some(prev) = token.prev_token() { let text = prev.text(); diff --git a/crates/emmylua_formatter/src/test/expression_tests.rs b/crates/emmylua_formatter/src/test/expression_tests.rs index e86ac033b..90b094430 100644 --- a/crates/emmylua_formatter/src/test/expression_tests.rs +++ b/crates/emmylua_formatter/src/test/expression_tests.rs @@ -994,9 +994,11 @@ end, 4, 5, 6, 7, 8, 9, 10 #[test] fn test_non_first_multiline_callback_tail_refills_after_end() { + // The packed first line `test(1, "hello", function ()` is 28 columns. Measured from the `1` it came + // out 29; measured from the `(` (#1302) it is exact, so 27 is the widest limit that still expands. let config = LuaFormatConfig { layout: LayoutConfig { - max_line_width: 28, + max_line_width: 27, ..Default::default() }, ..Default::default() diff --git a/crates/emmylua_formatter/src/test/misc_tests.rs b/crates/emmylua_formatter/src/test/misc_tests.rs index e44a470d9..8cbafb311 100644 --- a/crates/emmylua_formatter/src/test/misc_tests.rs +++ b/crates/emmylua_formatter/src/test/misc_tests.rs @@ -2,7 +2,7 @@ mod tests { use crate::{ SourceText, assert_format, - config::{LayoutConfig, LuaFormatConfig}, + config::{IndentConfig, IndentKind, LayoutConfig, LuaFormatConfig}, reformat_lua_code, }; use emmylua_parser::LuaLanguageLevel; @@ -832,4 +832,93 @@ end assert_eq!(first, second); } + + // ========== width estimates independent of the source layout (#1302) ========== + + /// Format `text`, assert that formatting the result again changes nothing, and return it. + fn format_idempotent(text: &str, config: &LuaFormatConfig) -> String { + let level = LuaLanguageLevel::default(); + let first = reformat_lua_code(&SourceText { text, level }, config); + let second = reformat_lua_code( + &SourceText { + text: &first, + level, + }, + config, + ); + assert_eq!(first, second, "not idempotent for:\n{text}"); + first + } + + fn two_space_config(max_line_width: usize) -> LuaFormatConfig { + LuaFormatConfig { + indent: IndentConfig { + kind: IndentKind::Space, + width: 2, + }, + layout: LayoutConfig { + max_line_width, + ..Default::default() + }, + ..Default::default() + } + } + + #[test] + fn test_call_args_layout_does_not_depend_on_first_arg_source_column() { + // The packed layout's first line is exactly 120 columns. Its first argument sits right after the + // `(` there, and on a line of its own in the expanded layout; estimating the line from that column + // made each layout format into the other forever. + let config = two_space_config(120); + let packed = r#"function M.evidence(rows) + local facts = Seq.from(rows) + :map(function(row) + local p = row[2] + return Seq.concat({ p.tail ~= '' and p.tail or '[no name]', 'ft=' .. (p.filetype ~= '' and p.filetype or '-') }, { + 'lines=' .. p.lines + }, p.marks ~= nil and { 'marks=' .. p.marks } or {}, p.modified and { 'MODIFIED' } or {} + ):join ' ' + end) + :totable() +end +"#; + let expanded = r#"function M.evidence(rows) + local facts = Seq.from(rows) + :map(function(row) + local p = row[2] + return Seq.concat( + { p.tail ~= '' and p.tail or '[no name]', 'ft=' .. (p.filetype ~= '' and p.filetype or '-') }, + { + 'lines=' .. p.lines + }, + p.marks ~= nil and { 'marks=' .. p.marks } or {}, + p.modified and { 'MODIFIED' } or {} + ):join ' ' + end) + :totable() +end +"#; + let from_packed = format_idempotent(packed, &config); + let from_expanded = format_idempotent(expanded, &config); + assert_eq!(from_packed, from_expanded); + assert!( + from_packed.lines().all(|line| line.len() <= 120), + "{from_packed}" + ); + } + + #[test] + fn test_table_after_assign_layout_does_not_depend_on_source_line() { + // Flat, the statement is 123 columns. With the table on its own line in the source, its column there + // (2) was taken for its column in the output (14), so it was joined flat past the limit. + let config = two_space_config(120); + let fields = "{ red = '31', green = '32', yellow = '33', blue = '34', magenta = '35', cyan = '36', dim = '90', bold = '1' }"; + let same_line = format_idempotent(&format!("local CODES = {fields}\n"), &config); + let next_line = format_idempotent(&format!("local CODES =\n {fields}\n"), &config); + assert_eq!(same_line, next_line); + assert!( + next_line.lines().all(|line| line.len() <= 120), + "{next_line}" + ); + } }