Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix unstable formatting of tables at column width boundary #437

Merged
merged 7 commits into from
Apr 2, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
### Fixed
- Fixed leading trivia on semicolon lost when semicolon is removed ([#431](https://github.com/JohnnyMorganz/StyLua/issues/431))
- Fixed shape calculation of the RHS of a binary expression not correctly reset when hanging, causing it to expand unnecessarily ([#432](https://github.com/JohnnyMorganz/StyLua/issues/432))
- Fixed unstable formatting of tables at column width boundary ([#436](https://github.com/JohnnyMorganz/StyLua/issues/436))

## [0.13.0] - 2022-03-31
### Added
Expand Down
97 changes: 64 additions & 33 deletions src/formatters/table.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use crate::{
expression::{format_expression, hang_expression, is_brackets_string},
general::{format_contained_span, format_end_token, format_token_reference, EndTokenType},
trivia::{strip_trivia, FormatTriviaType, UpdateLeadingTrivia, UpdateTrailingTrivia},
trivia_util,
trivia_util::{self, table_field_trailing_trivia},
},
shape::Shape,
};
Expand All @@ -16,7 +16,7 @@ use full_moon::{
Expression, Field, TableConstructor, Value,
},
node::Node,
tokenizer::{Token, TokenReference, TokenType},
tokenizer::{Token, TokenKind, TokenReference, TokenType},
};

/// Used to provide information about the table
Expand Down Expand Up @@ -418,6 +418,7 @@ fn should_expand(table_constructor: &TableConstructor) -> bool {
return true;
}
}

false
}
}
Expand All @@ -427,6 +428,8 @@ pub fn format_table_constructor(
table_constructor: &TableConstructor,
shape: Shape,
) -> TableConstructor {
const BRACE_LEN: usize = "{".len();

let (start_brace, end_brace) = table_constructor.braces().tokens();

// Determine if we need to force the table multiline
Expand All @@ -437,38 +440,66 @@ pub fn format_table_constructor(
(true, _) => TableType::MultiLine,

(false, Some(_)) => {
// Compare the difference between the position of the start brace and the end brace to
// guess how long the table is. This heuristic is very naiive, since it relies on the input.
// If the input is badly formatted (e.g. lots of spaces in the table), then it would flag this over width.
// However, this is currently our best solution: attempting to format the input onto a single line to
// see if we are over width (both completely and in a fail-fast shape.over_budget() check) leads to
// exponential time complexity with respect to how deep the table is.
// TODO: find an improved heuristic whilst comparing against benchmarks
let braces_range = (
// Use the position of the last trivia in case there is some present (e.g. whitespace)
// So that we don't include an extra space
if let Some(token) = start_brace.leading_trivia().last() {
token.end_position().bytes()
} else {
start_brace.token().end_position().bytes()
},
end_brace.token().start_position().bytes(),
);
let singleline_shape = shape + (braces_range.1 - braces_range.0) + 3; // 4 = two braces + single space before last brace

match singleline_shape.over_budget() {
true => TableType::MultiLine,
false => {
// Determine if there was a new line at the end of the start brace
// If so, then we should always be multiline
if start_brace
.trailing_trivia()
.any(trivia_util::trivia_is_newline)
{
TableType::MultiLine
// Determine if there was a new line at the end of the start brace
// If so, then we should always be multiline
if start_brace
.trailing_trivia()
.any(trivia_util::trivia_is_newline)
{
TableType::MultiLine
} else {
// Compare the difference between the position of the start brace and the end brace to
// guess how long the table is. This heuristic is very naiive, since it relies on the input.
// If the input is badly formatted (e.g. lots of spaces in the table), then it would flag this over width.
// However, this is currently our best solution: attempting to format the input onto a single line to
// see if we are over width (both completely and in a fail-fast shape.over_budget() check) leads to
// exponential time complexity with respect to how deep the table is.
// TODO: find an improved heuristic whilst comparing against benchmarks
let braces_range = (
// Use the position of the last trivia in case there is some present (e.g. whitespace)
// So that we don't include an extra space
if let Some(token) = start_brace.leading_trivia().last() {
token.end_position().bytes()
} else {
TableType::SingleLine
}
start_brace.token().end_position().bytes()
},
end_brace.token().start_position().bytes(),
);

let last_field = table_constructor
.fields()
.last()
.expect("atleast one field must be present");

// See if we need to +1 because we will be adding spaces
let additional_shape = match (
start_brace
.trailing_trivia()
.any(|x| x.token_kind() == TokenKind::Whitespace),
// A space will be present on the end of the last field, not the start of the end brace
match (last_field.value(), last_field.punctuation()) {
(_, Some(token)) => token
.trailing_trivia()
.any(|x| x.token_kind() == TokenKind::Whitespace),
(field, None) => table_field_trailing_trivia(field)
.iter()
.any(|x| x.token_kind() == TokenKind::Whitespace),
},
) {
(true, true) => 0,
(true, false) | (false, true) => 1,
(false, false) => 2,
};

let singleline_shape = shape
+ (braces_range.1 - braces_range.0)
+ additional_shape
+ BRACE_LEN
+ BRACE_LEN;

match singleline_shape.over_budget() {
true => TableType::MultiLine,
false => TableType::SingleLine,
}
}
}
Expand Down
9 changes: 9 additions & 0 deletions src/formatters/trivia_util.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1078,6 +1078,15 @@ pub fn table_fields_contains_comments(table_constructor: &TableConstructor) -> b
})
}

pub fn table_field_trailing_trivia(field: &Field) -> Vec<Token> {
match field {
Field::ExpressionKey { value, .. } => get_expression_trailing_trivia(value),
Field::NameKey { value, .. } => get_expression_trailing_trivia(value),
Field::NoKey(expression) => get_expression_leading_trivia(expression),
other => panic!("unknown node {:?}", other),
}
}

// Checks to see whether an expression contains comments inline inside of it
// This can only happen if the expression is a BinOp
// We should ignore any comments which are trailing for the whole expression, as they are not inline
Expand Down
4 changes: 4 additions & 0 deletions tests/inputs/table-7.lua
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
-- https://github.com/JohnnyMorganz/StyLua/issues/436
local OffsetEnum = {aValue = 10, anotherValue = 11, yetAnotherValue = 12, reset = 0, postReset = 1, aaaaaaaaaaa = true}

local OffsetEnum = { aValue = 10, anotherValue = 11, yetAnotherValue = 12, reset = 0, postReset = 1, aaaaaaaaa = true }
18 changes: 18 additions & 0 deletions tests/snapshots/tests__standard@table-7.lua.snap
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
---
source: tests/tests.rs
assertion_line: 12
expression: format(&contents)

---
-- https://github.com/JohnnyMorganz/StyLua/issues/436
local OffsetEnum = {
aValue = 10,
anotherValue = 11,
yetAnotherValue = 12,
reset = 0,
postReset = 1,
aaaaaaaaaaa = true,
}

local OffsetEnum = { aValue = 10, anotherValue = 11, yetAnotherValue = 12, reset = 0, postReset = 1, aaaaaaaaa = true }