Skip to content

Keep necessary parentheses on the LHS of a hung binary operation#1136

Open
sarathfrancis90 wants to merge 1 commit into
JohnnyMorganz:mainfrom
sarathfrancis90:fix-hanging-binop-lhs-parentheses
Open

Keep necessary parentheses on the LHS of a hung binary operation#1136
sarathfrancis90 wants to merge 1 commit into
JohnnyMorganz:mainfrom
sarathfrancis90:fix-hanging-binop-lhs-parentheses

Conversation

@sarathfrancis90

Copy link
Copy Markdown

Two parentheses bugs that were fixed once already come back as soon as the expression is long enough to be hung over multiple lines:

-- input
local someVeryLongVariableNameHere = (-someExtremelyLongOperandNameOnTheLeft) ^ someOtherExtremelyLongOperandNameOnTheRight

-- 2.5.2 output: this is now -(a ^ b)
local someVeryLongVariableNameHere = -someExtremelyLongOperandNameOnTheLeft
	^ someOtherExtremelyLongOperandNameOnTheRight

and, with syntax = "Luau":

-- input
local isNegative = someExtremelyLongVariableNameHere ~= nil and (someExtremelyLongVariableNameHereAsWellOkayThenFine :: number) < someExtremelyLongThresholdVariableNameHereTooRight

-- 2.5.2 output: does not parse, `number < ...` is read as a generic type instantiation
local isNegative = someExtremelyLongVariableNameHere ~= nil
	and someExtremelyLongVariableNameHereAsWellOkayThenFine :: number
		< someExtremelyLongThresholdVariableNameHereTooRight

These are the same cases as #623 and #940. check_excess_parentheses handles both correctly, but only if it is told the expression is the LHS of a binop — and the hanging paths never do. hang_binop_expression forwards whatever context the expression as a whole was hung in, and format_hanging_expression_ passes UnaryOrBinary, so the BinaryLHS/BinaryLHSExponent guards never fire once hanging kicks in.

I pulled the context choice into a small helper and used it at each LHS call site. The direction is one-way — these contexts only ever keep more parentheses, never remove them — so the blast radius is small: across the whole test corpus one existing snapshot changed, multiline-expression-comments-1, where (not strictFiltering) now keeps its parentheses like the non-hung (not true) and false case above it already does.

Found this while checking that formatting is a fixed point (format(format(x)) == format(x)) over the test inputs with various configs; the Luau one showed up as StyLua failing to re-read its own output.

Tested with cargo test on each feature in the CI matrix, plus clippy and rustfmt.

check_excess_parentheses already knows that parentheses on the LHS of a
binop must be kept for `(-X) ^ Y` and `(expr :: T) < Y`, but the hanging
code paths never told it they were formatting an LHS: they passed down
either the context the whole expression was hung in, or UnaryOrBinary.
As soon as the expression was multilined the parentheses were dropped
again, changing the meaning of `(-X) ^ Y` and producing a syntax error
for the Luau type assertion.

Derive the LHS context from the binop at every LHS call site instead.
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.

1 participant