Repository: mvdan/sh
Pull request #613 (2020-09-26 10:28:52)

shfmt: Don't add parentheses to functions, if they were not in the original source
Bash supports `foo() {}`, `function foo {}`, and `function foo() {}`.
Currently, shfmt always prints parentheses, i.e. `function foo {}` is always printed as `function foo() {}`.

This patch changes shfmt to only add parentheses, if they were in the original source.
Reasons:
- imo this may be unexpected for some users of Bash
- ksh only supports `foo() {}` and `function foo {}`, but not `function foo() {}`. This patch slightly increases compatibility with this and hopefully allows to run shfmt on more .ksh file (which is useful for [BashSupport Pro](https://www.bashsupport.com))

As this is my first PR for shfmt, this may not match the expectations or requirements :) Please let me know what should be changed or if this isn't how shfmt is supposed to work. Thansk!


Patch:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index 33d6a32c..afadf751 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -6,6 +6,7 @@
   - Add `-filename` to give a name to standard input
 - **syntax**
   - Rewrite arithmetic parsing to fix operator precedence
+  - Don't add parentheses to `function f {...}` declarations for ksh support
 
 ## [3.1.2] - 2020-06-26
 
diff --git a/syntax/nodes.go b/syntax/nodes.go
index 3130789c..d925b8dc 100644
--- a/syntax/nodes.go
+++ b/syntax/nodes.go
@@ -393,7 +393,8 @@ func (b *BinaryCmd) End() Pos { return b.Y.End() }
 // FuncDecl represents the declaration of a function.
 type FuncDecl struct {
 	Position Pos
-	RsrvWord bool // non-posix "function f()" style
+	RsrvWord bool // non-posix "function f" style
+	Parens   bool // with () parentheses, only meaningful with RsrvWord=true
 	Name     *Lit
 	Body     *Stmt
 }
diff --git a/syntax/parser.go b/syntax/parser.go
index c47c5dce..7174ce3b 100644
--- a/syntax/parser.go
+++ b/syntax/parser.go
@@ -1669,7 +1669,7 @@ func (p *Parser) gotStmtPipe(s *Stmt, binCmd bool) *Stmt {
 			if p.lang == LangPOSIX && !ValidName(name.Value) {
 				p.posErr(name.Pos(), "invalid func name")
 			}
-			p.funcDecl(s, name, name.ValuePos)
+			p.funcDecl(s, name, name.ValuePos, true)
 		} else {
 			p.callExpr(s, p.word(p.wps(name)), false)
 		}
@@ -2232,10 +2232,12 @@ func (p *Parser) bashFuncDecl(s *Stmt) {
 		p.followErr(fpos, "function", "a name")
 	}
 	name := p.lit(p.pos, p.val)
+	hasParens := false
 	if p.next(); p.got(leftParen) {
+		hasParens = true
 		p.follow(name.ValuePos, "foo(", rightParen)
 	}
-	p.funcDecl(s, name, fpos)
+	p.funcDecl(s, name, fpos, hasParens)
 }
 
 func (p *Parser) callExpr(s *Stmt, w *Word, assign bool) {
@@ -2305,10 +2307,11 @@ loop:
 	s.Cmd = ce
 }
 
-func (p *Parser) funcDecl(s *Stmt, name *Lit, pos Pos) {
+func (p *Parser) funcDecl(s *Stmt, name *Lit, pos Pos, withParens bool) {
 	fd := &FuncDecl{
 		Position: pos,
 		RsrvWord: pos != name.ValuePos,
+		Parens:   withParens,
 		Name:     name,
 	}
 	p.got(_Newl)
diff --git a/syntax/printer.go b/syntax/printer.go
index deb22100..d20e07d4 100644
--- a/syntax/printer.go
+++ b/syntax/printer.go
@@ -1065,11 +1065,13 @@ func (p *Printer) command(cmd Command, redirs []*Redirect) (startRedirs int) {
 			p.WriteString("function ")
 		}
 		p.writeLit(x.Name.Value)
-		p.WriteString("()")
+		if !x.RsrvWord || x.Parens {
+			p.WriteString("()")
+		}
 		if p.funcNextLine {
 			p.newline(Pos{})
 			p.indent()
-		} else if !p.minify {
+		} else if !x.Parens || !p.minify {
 			p.space()
 		}
 		p.line = x.Body.Pos().Line()


Test patch:
diff --git a/syntax/filetests_test.go b/syntax/filetests_test.go
index b3217b00..09aba5d9 100644
--- a/syntax/filetests_test.go
+++ b/syntax/filetests_test.go
@@ -713,16 +713,18 @@ var fileTests = []testCase{
 			"foo()\n{\na\nb\n}",
 		},
 		common: &FuncDecl{
-			Name: lit("foo"),
-			Body: stmt(block(litStmt("a"), litStmt("b"))),
+			Parens: true,
+			Name:   lit("foo"),
+			Body:   stmt(block(litStmt("a"), litStmt("b"))),
 		},
 	},
 	{
 		Strs: []string{"foo() { a; }\nbar", "foo() {\na\n}; bar"},
 		common: []Command{
 			&FuncDecl{
-				Name: lit("foo"),
-				Body: stmt(block(litStmt("a"))),
+				Parens: true,
+				Name:   lit("foo"),
+				Body:   stmt(block(litStmt("a"))),
 			},
 			litCall("bar"),
 		},
@@ -730,23 +732,36 @@ var fileTests = []testCase{
 	{
 		Strs: []string{"foO_123() { a; }"},
 		common: &FuncDecl{
-			Name: lit("foO_123"),
-			Body: stmt(block(litStmt("a"))),
+			Parens: true,
+			Name:   lit("foO_123"),
+			Body:   stmt(block(litStmt("a"))),
 		},
 	},
 	{
 		Strs: []string{"-foo_.,+-bar() { a; }"},
 		bsmk: &FuncDecl{
-			Name: lit("-foo_.,+-bar"),
-			Body: stmt(block(litStmt("a"))),
+			Parens: true,
+			Name:   lit("-foo_.,+-bar"),
+			Body:   stmt(block(litStmt("a"))),
 		},
 	},
 	{
 		Strs: []string{
 			"function foo() {\n\ta\n\tb\n}",
-			"function foo {\n\ta\n\tb\n}",
 			"function foo() { a; b; }",
 		},
+		bsmk: &FuncDecl{
+			RsrvWord: true,
+			Parens:   true,
+			Name:     lit("foo"),
+			Body:     stmt(block(litStmt("a"), litStmt("b"))),
+		},
+	},
+	{
+		Strs: []string{
+			"function foo {\n\ta\n\tb\n}",
+			"function foo { a; b; }",
+		},
 		bsmk: &FuncDecl{
 			RsrvWord: true,
 			Name:     lit("foo"),
@@ -757,6 +772,7 @@ var fileTests = []testCase{
 		Strs: []string{"function foo() (a)"},
 		bash: &FuncDecl{
 			RsrvWord: true,
+			Parens:   true,
 			Name:     lit("foo"),
 			Body:     stmt(subshell(litStmt("a"))),
 		},
@@ -4193,7 +4209,8 @@ var fileTests = []testCase{
 	{
 		Strs: []string{"foo() {\n\t<<EOF && { bar; }\nhdoc\nEOF\n}"},
 		common: &FuncDecl{
-			Name: lit("foo"),
+			Parens: true,
+			Name:   lit("foo"),
 			Body: stmt(block(stmt(&BinaryCmd{
 				Op: AndStmt,
 				X: &Stmt{Redirs: []*Redirect{{
diff --git a/syntax/printer_test.go b/syntax/printer_test.go
index 550bfd45..96ad89ef 100644
--- a/syntax/printer_test.go
+++ b/syntax/printer_test.go
@@ -782,6 +782,10 @@ func TestPrintFunctionNextLine(t *testing.T) {
 		},
 		{
 			"function foo {\n\tbar\n}",
+			"function foo\n{\n\tbar\n}",
+		},
+		{
+			"function foo() {\n\tbar\n}",
 			"function foo()\n{\n\tbar\n}",
 		},
 		{