From 7d0260daca23d124b2b0429d0975d89729443bef Mon Sep 17 00:00:00 2001 From: riacataquian Date: Tue, 31 May 2022 19:30:24 +0800 Subject: [PATCH 1/5] syntax: fix whitespace on nested subshells we want white space on nested subshells if its in a single line since its ambiguous, eg we want `( (` over `((` this shouldn't be the case for multiple lines- fix the logic that adds white space to only do so if the two subshells are in the same line fixes #814 --- syntax/printer.go | 7 +++++++ syntax/printer_test.go | 8 ++++++++ 2 files changed, 15 insertions(+) diff --git a/syntax/printer.go b/syntax/printer.go index 7dc183a02..cbb310c83 100644 --- a/syntax/printer.go +++ b/syntax/printer.go @@ -1081,6 +1081,13 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) { p.WriteByte('(') if len(x.Stmts) > 0 && startsWithLparen(x.Stmts[0]) { p.wantSpace = spaceRequired + if nested, ok := x.Stmts[0].Cmd.(*Subshell); ok { + // we only want to keep the space between two nested subshells' open brackets + // if its in a single line to avoid ambiguity + if x.Lparen.Line() != nested.Pos().Line() { + p.wantSpace = spaceNotRequired + } + } } else { p.wantSpace = spaceNotRequired } diff --git a/syntax/printer_test.go b/syntax/printer_test.go index 59483f689..4e12e6113 100644 --- a/syntax/printer_test.go +++ b/syntax/printer_test.go @@ -616,6 +616,14 @@ var printTests = []printCase{ "`declare`", "$(declare)", }, + { + "(\n(foo >redir)\n)", + "(\n\t(foo >redir)\n)", + }, + { + "( (foo >redir) )", + "( (foo >redir))", + }, } func TestPrintWeirdFormat(t *testing.T) { From 52458f0abf8be2e33860e9e83c82a2452eaeb4ce Mon Sep 17 00:00:00 2001 From: riacataquian Date: Thu, 2 Jun 2022 00:29:47 +0800 Subject: [PATCH 2/5] handle more scenarios and node types the fix fall short for some scenarios such as when we actually want new line, eg there's more than one statement; fix the logic to also not add white space for such scenarios also add more tests for the different types --- syntax/printer.go | 13 ++++++------- syntax/printer_test.go | 14 ++++++++++++-- 2 files changed, 18 insertions(+), 9 deletions(-) diff --git a/syntax/printer.go b/syntax/printer.go index cbb310c83..6de213ffe 100644 --- a/syntax/printer.go +++ b/syntax/printer.go @@ -1079,14 +1079,13 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) { p.ifClause(x, false) case *Subshell: p.WriteByte('(') - if len(x.Stmts) > 0 && startsWithLparen(x.Stmts[0]) { + stmts := x.Stmts + if len(stmts) > 0 && startsWithLparen(stmts[0]) { p.wantSpace = spaceRequired - if nested, ok := x.Stmts[0].Cmd.(*Subshell); ok { - // we only want to keep the space between two nested subshells' open brackets - // if its in a single line to avoid ambiguity - if x.Lparen.Line() != nested.Pos().Line() { - p.wantSpace = spaceNotRequired - } + // we only want to keep the space between two nested subshells' open brackets + // if its in a single line to avoid ambiguity + if x.Lparen.Line() != stmts[0].Pos().Line() || len(stmts) > 1 { + p.wantSpace = spaceNotRequired } } else { p.wantSpace = spaceNotRequired diff --git a/syntax/printer_test.go b/syntax/printer_test.go index 4e12e6113..e7b327a2b 100644 --- a/syntax/printer_test.go +++ b/syntax/printer_test.go @@ -621,9 +621,19 @@ var printTests = []printCase{ "(\n\t(foo >redir)\n)", }, { - "( (foo >redir) )", - "( (foo >redir))", + "( (foo) )", + "( (foo))", }, + { + "( {foo}; bar; )", + "(\n\t{foo}\n\tbar\n)", + }, + { + "( ((foo++)) )", + "( ((foo++)))", + }, + samePrint("(\n\t((foo++))\n)"), + samePrint("(foo && bar)"), } func TestPrintWeirdFormat(t *testing.T) { From 7a1540e02e4b2a13c0be5d86e804d4b2b326407d Mon Sep 17 00:00:00 2001 From: riacataquian Date: Thu, 2 Jun 2022 23:47:12 +0800 Subject: [PATCH 3/5] adds more test cases --- syntax/printer_test.go | 40 ++++++++++++++++++++++------------------ 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/syntax/printer_test.go b/syntax/printer_test.go index e7b327a2b..0fda28ac2 100644 --- a/syntax/printer_test.go +++ b/syntax/printer_test.go @@ -616,24 +616,6 @@ var printTests = []printCase{ "`declare`", "$(declare)", }, - { - "(\n(foo >redir)\n)", - "(\n\t(foo >redir)\n)", - }, - { - "( (foo) )", - "( (foo))", - }, - { - "( {foo}; bar; )", - "(\n\t{foo}\n\tbar\n)", - }, - { - "( ((foo++)) )", - "( ((foo++)))", - }, - samePrint("(\n\t((foo++))\n)"), - samePrint("(foo && bar)"), } func TestPrintWeirdFormat(t *testing.T) { @@ -1321,6 +1303,28 @@ func TestPrintManyStmts(t *testing.T) { {"foo\nbar <redir)\n)", + "(\n\t(foo >redir)\n)\n", + }, + { + "( (foo) )", + "( (foo))\n", + }, + { + "( (foo); bar )", + "(\n\t(foo)\n\tbar\n)\n", + }, + { + "( ((foo++)) )", + "( ((foo++)))\n", + }, + { + "( ((foo++)); bar )", + "(\n\t((foo++))\n\tbar\n)\n", + }, + samePrint("(\n\t((foo++))\n)\n"), + samePrint("(foo && bar)\n"), } parser := NewParser(KeepComments(true)) printer := NewPrinter() From 8e4f46fc45ff1319f3cad818f5a78cd53ab2c17a Mon Sep 17 00:00:00 2001 From: riacataquian Date: Sun, 5 Jun 2022 23:03:30 +0800 Subject: [PATCH 4/5] fix syntax for singleLine and minify printer opts on instances when command is a SubShell, `startsWithLparen` condition is met and there's more than one statement, we want: - space between the statements if `SingleLine` is enabled - new line between the statements if `Minify` is enabled otherwise we'd get `reached ) without matching (( with ))` syntax errors; rightfully so, because of the root issue we've been trying to solve, being `((` is ambiguous --- syntax/printer.go | 7 ++++++- syntax/printer_test.go | 44 +++++++++++++++++++++--------------------- 2 files changed, 28 insertions(+), 23 deletions(-) diff --git a/syntax/printer.go b/syntax/printer.go index 6de213ffe..607211f32 100644 --- a/syntax/printer.go +++ b/syntax/printer.go @@ -1084,12 +1084,17 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) { p.wantSpace = spaceRequired // we only want to keep the space between two nested subshells' open brackets // if its in a single line to avoid ambiguity - if x.Lparen.Line() != stmts[0].Pos().Line() || len(stmts) > 1 { + if x.Lparen.Line() != stmts[0].Pos().Line() || len(stmts) > 1 && !p.singleLine { p.wantSpace = spaceNotRequired + + if p.minify { + p.newline(stmts[0].Pos()) + } } } else { p.wantSpace = spaceNotRequired } + p.spacePad(stmtsPos(x.Stmts, x.Last)) p.nestedStmts(x.Stmts, x.Last, x.Rparen) p.wantSpace = spaceNotRequired diff --git a/syntax/printer_test.go b/syntax/printer_test.go index 0fda28ac2..30607cd8b 100644 --- a/syntax/printer_test.go +++ b/syntax/printer_test.go @@ -616,6 +616,28 @@ var printTests = []printCase{ "`declare`", "$(declare)", }, + { + "(\n(foo >redir))", + "(\n\t(foo >redir)\n)", + }, + { + "( (foo) )", + "( (foo))", + }, + { + "( (foo); bar )", + "(\n\t(foo)\n\tbar\n)", + }, + { + "( ((foo++)) )", + "( ((foo++)))", + }, + { + "( ((foo++)); bar )", + "(\n\t((foo++))\n\tbar\n)", + }, + samePrint("(\n\t((foo++))\n)"), + samePrint("(foo && bar)"), } func TestPrintWeirdFormat(t *testing.T) { @@ -1303,28 +1325,6 @@ func TestPrintManyStmts(t *testing.T) { {"foo\nbar <redir)\n)", - "(\n\t(foo >redir)\n)\n", - }, - { - "( (foo) )", - "( (foo))\n", - }, - { - "( (foo); bar )", - "(\n\t(foo)\n\tbar\n)\n", - }, - { - "( ((foo++)) )", - "( ((foo++)))\n", - }, - { - "( ((foo++)); bar )", - "(\n\t((foo++))\n\tbar\n)\n", - }, - samePrint("(\n\t((foo++))\n)\n"), - samePrint("(foo && bar)\n"), } parser := NewParser(KeepComments(true)) printer := NewPrinter() From 23457f2acd20947f8b5eb02f201bd5e29a4278e9 Mon Sep 17 00:00:00 2001 From: riacataquian Date: Mon, 6 Jun 2022 20:54:31 +0800 Subject: [PATCH 5/5] resolve comments - make the if conditions more readable by adding parentheses separators - reword comment on the double parens ambiguity - make use of the `mustNewLine` variable to tell `stmtList` we must write a new line, instead of explicitly writing it up front --- syntax/printer.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/syntax/printer.go b/syntax/printer.go index 607211f32..17a664d30 100644 --- a/syntax/printer.go +++ b/syntax/printer.go @@ -1082,13 +1082,13 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) { stmts := x.Stmts if len(stmts) > 0 && startsWithLparen(stmts[0]) { p.wantSpace = spaceRequired - // we only want to keep the space between two nested subshells' open brackets - // if its in a single line to avoid ambiguity - if x.Lparen.Line() != stmts[0].Pos().Line() || len(stmts) > 1 && !p.singleLine { + // Add a space between nested parentheses if we're printing them in a single line, + // to avoid the ambiguity between `((` and `( (`. + if (x.Lparen.Line() != stmts[0].Pos().Line() || len(stmts) > 1) && !p.singleLine { p.wantSpace = spaceNotRequired if p.minify { - p.newline(stmts[0].Pos()) + p.mustNewline = true } } } else { @@ -1349,7 +1349,7 @@ func (p *Printer) stmtList(stmts []*Stmt, last []Comment) { // statement. p.comments(c) } - if !p.minify || p.wantSpace == spaceRequired { + if p.mustNewline || !p.minify || p.wantSpace == spaceRequired { p.newlines(pos) } p.line = pos.Line()