From 69e0a3e220300c5a4e199be679c15392372900d9 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 12:17:05 +0200 Subject: [PATCH 01/11] Add a failing test --- testdata/script/clothe-returns.txtar | 34 ++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100644 testdata/script/clothe-returns.txtar diff --git a/testdata/script/clothe-returns.txtar b/testdata/script/clothe-returns.txtar new file mode 100644 index 0000000..c6e9f1f --- /dev/null +++ b/testdata/script/clothe-returns.txtar @@ -0,0 +1,34 @@ +exec gofumpt -w foo.go +cmp foo.go foo.go.golden + +exec gofumpt -d foo.go.golden +! stdout . + +-- foo.go -- +package p + +func foo() (err error) { + if true { + return + } + if false { + return func() (err2 error) { + return + } + } + return +} +-- foo.go.golden -- +package p + +func foo() (err error) { + if true { + return err + } + if false { + return func() (err2 error) { + return err2 + } + } + return err +} \ No newline at end of file From ec26c1c161a5969e4291813cc53cbf94d1678ed2 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 14:03:06 +0200 Subject: [PATCH 02/11] Add 'clothe returns' transformation --- format/format.go | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/format/format.go b/format/format.go index 7316eb7..9927e57 100644 --- a/format/format.go +++ b/format/format.go @@ -702,6 +702,32 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { case *ast.AssignStmt: // Only remove lines between the assignment token and the first right-hand side expression f.removeLines(f.Line(node.TokPos), f.Line(node.Rhs[0].Pos())) + + // Clothe naked returns + case *ast.ReturnStmt: + if node.Results == nil { // We have either a naked return, or a function with no return values + parents, _ := astutil.PathEnclosingInterval(f.astFile, node.Pos(), node.End()) + var parentResults *ast.FieldList + // Find the nearest ancestor that is either a func declaration or func literal + parentLoop: + for _, parent := range parents { + switch p := parent.(type) { + case *ast.FuncDecl: + parentResults = p.Type.Results + break parentLoop + case *ast.FuncLit: + parentResults = p.Type.Results + break parentLoop + } + } + if fields := parentResults.NumFields(); fields > 0 { // The function has return values; let's clothe the return + node.Results = make([]ast.Expr, fields) + for i, result := range parentResults.List { + node.Results[i] = ast.NewIdent(result.Names[0].Name) + } + c.Replace(node) + } + } } } From f1a6eb439a0aa04521ee26deb1445e4c5b1d04b8 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 14:05:16 +0200 Subject: [PATCH 03/11] Update readme with 'clothe returns' example --- README.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/README.md b/README.md index 546706a..fbcdcb3 100644 --- a/README.md +++ b/README.md @@ -451,6 +451,26 @@ func Foo(bar, baz string) {} +#### Clothe naked returns + +**Naked returns are rewritten to include explicit return values** + +
Example + +```go +func Foo() (err error) { + return +} +``` + +```go +func Foo() (err error) { + return err +} +``` + +
+ ### Installation `gofumpt` is a replacement for `gofmt`, so you can simply `go install` it as From ea8e50ef85ebc1a835c98cf0c992241abbd88e92 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 14:12:30 +0200 Subject: [PATCH 04/11] Special case blank-named return values, and abort the transform --- format/format.go | 7 ++++++- testdata/script/clothe-returns.txtar | 10 +++++++++- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/format/format.go b/format/format.go index 9927e57..66a5989 100644 --- a/format/format.go +++ b/format/format.go @@ -723,7 +723,12 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { if fields := parentResults.NumFields(); fields > 0 { // The function has return values; let's clothe the return node.Results = make([]ast.Expr, fields) for i, result := range parentResults.List { - node.Results[i] = ast.NewIdent(result.Names[0].Name) + name := result.Names[0].Name + if name == "_" { // we can't handle blank names just yet, abort the transform + node.Results = nil + break + } + node.Results[i] = ast.NewIdent(name) } c.Replace(node) } diff --git a/testdata/script/clothe-returns.txtar b/testdata/script/clothe-returns.txtar index c6e9f1f..0d2fe09 100644 --- a/testdata/script/clothe-returns.txtar +++ b/testdata/script/clothe-returns.txtar @@ -18,6 +18,10 @@ func foo() (err error) { } return } + +func bar() (_ int, err error) { + return +} -- foo.go.golden -- package p @@ -31,4 +35,8 @@ func foo() (err error) { } } return err -} \ No newline at end of file +} + +func bar() (_ int, err error) { + return +} From 4d4be75dd74dd482288751c0704ba475c607a121 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 14:42:18 +0200 Subject: [PATCH 05/11] Clothe returns with multiple names for the same type in the result list i.e. `func() (a, b, c int)` --- format/format.go | 25 ++++++++++++++----------- testdata/script/clothe-returns.txtar | 8 ++++++++ 2 files changed, 22 insertions(+), 11 deletions(-) diff --git a/format/format.go b/format/format.go index 66a5989..f013929 100644 --- a/format/format.go +++ b/format/format.go @@ -707,28 +707,31 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { case *ast.ReturnStmt: if node.Results == nil { // We have either a naked return, or a function with no return values parents, _ := astutil.PathEnclosingInterval(f.astFile, node.Pos(), node.End()) - var parentResults *ast.FieldList + var results *ast.FieldList // Find the nearest ancestor that is either a func declaration or func literal parentLoop: for _, parent := range parents { switch p := parent.(type) { case *ast.FuncDecl: - parentResults = p.Type.Results + results = p.Type.Results break parentLoop case *ast.FuncLit: - parentResults = p.Type.Results + results = p.Type.Results break parentLoop } } - if fields := parentResults.NumFields(); fields > 0 { // The function has return values; let's clothe the return - node.Results = make([]ast.Expr, fields) - for i, result := range parentResults.List { - name := result.Names[0].Name - if name == "_" { // we can't handle blank names just yet, abort the transform - node.Results = nil - break + if fields := results.NumFields(); fields > 0 { // The function has return values; let's clothe the return + node.Results = make([]ast.Expr, 0, fields) + nameLoop: + for _, result := range results.List { + for _, ident := range result.Names { + name := ident.Name + if name == "_" { // we can't handle blank names just yet, abort the transform + node.Results = nil + break nameLoop + } + node.Results = append(node.Results, ast.NewIdent(name)) } - node.Results[i] = ast.NewIdent(name) } c.Replace(node) } diff --git a/testdata/script/clothe-returns.txtar b/testdata/script/clothe-returns.txtar index 0d2fe09..e15f4dd 100644 --- a/testdata/script/clothe-returns.txtar +++ b/testdata/script/clothe-returns.txtar @@ -22,6 +22,10 @@ func foo() (err error) { func bar() (_ int, err error) { return } + +func baz() (a, b, c int) { + return +} -- foo.go.golden -- package p @@ -40,3 +44,7 @@ func foo() (err error) { func bar() (_ int, err error) { return } + +func baz() (a, b, c int) { + return a, b, c +} From ee08c6c859d2a59f46675784bd988cc8e54d3e5b Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 14:54:21 +0200 Subject: [PATCH 06/11] Add failing test for comments being misplaced while clothing returns --- testdata/script/clothe-returns.txtar | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/testdata/script/clothe-returns.txtar b/testdata/script/clothe-returns.txtar index e15f4dd..172862e 100644 --- a/testdata/script/clothe-returns.txtar +++ b/testdata/script/clothe-returns.txtar @@ -26,6 +26,15 @@ func bar() (_ int, err error) { func baz() (a, b, c int) { return } + +func qux() (file string, b int, err error) { + if err == nil { + return + } + + // A comment + return +} -- foo.go.golden -- package p @@ -48,3 +57,12 @@ func bar() (_ int, err error) { func baz() (a, b, c int) { return a, b, c } + +func qux() (file string, b int, err error) { + if err == nil { + return file, b, err + } + + // A comment + return file, b, err +} From 0ed048bfe57aacc819dad0b274e4840a6b420617 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 15:40:03 +0200 Subject: [PATCH 07/11] Fix comment placement with an ugly hack --- format/format.go | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/format/format.go b/format/format.go index f013929..e068066 100644 --- a/format/format.go +++ b/format/format.go @@ -721,6 +721,7 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { } } if fields := results.NumFields(); fields > 0 { // The function has return values; let's clothe the return + var offset int node.Results = make([]ast.Expr, 0, fields) nameLoop: for _, result := range results.List { @@ -728,12 +729,21 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { name := ident.Name if name == "_" { // we can't handle blank names just yet, abort the transform node.Results = nil + offset = 0 break nameLoop } + offset += len(name) node.Results = append(node.Results, ast.NewIdent(name)) } } - c.Replace(node) + if len(node.Results) > 0 { + // An ugly hack to update the Pos of any comment that immediately + // follows a rewritten return statement. + for _, comment := range f.commentsBetween(node.End(), c.Parent().End()) { + comment.List[0].Slash = comment.Pos() + token.Pos(offset) + } + c.Replace(node) + } } } } From 46a2557f4318267c436d758ada0b1852c6ab1494 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Fri, 20 Oct 2023 15:43:07 +0200 Subject: [PATCH 08/11] Add failing test for comment placement --- testdata/script/clothe-returns.txtar | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/testdata/script/clothe-returns.txtar b/testdata/script/clothe-returns.txtar index 172862e..299189d 100644 --- a/testdata/script/clothe-returns.txtar +++ b/testdata/script/clothe-returns.txtar @@ -35,6 +35,9 @@ func qux() (file string, b int, err error) { // A comment return } + +// quux does quuxy things +func quux() {} -- foo.go.golden -- package p @@ -66,3 +69,6 @@ func qux() (file string, b int, err error) { // A comment return file, b, err } + +// quux does quuxy things +func quux() {} From def7745b8010541d0c274237f0a06aa1b080ba31 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Mon, 23 Oct 2023 18:15:48 +0200 Subject: [PATCH 09/11] Fix comment placement --- format/format.go | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/format/format.go b/format/format.go index e068066..d511499 100644 --- a/format/format.go +++ b/format/format.go @@ -721,7 +721,6 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { } } if fields := results.NumFields(); fields > 0 { // The function has return values; let's clothe the return - var offset int node.Results = make([]ast.Expr, 0, fields) nameLoop: for _, result := range results.List { @@ -729,19 +728,15 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { name := ident.Name if name == "_" { // we can't handle blank names just yet, abort the transform node.Results = nil - offset = 0 break nameLoop } - offset += len(name) - node.Results = append(node.Results, ast.NewIdent(name)) + node.Results = append(node.Results, &ast.Ident{ + NamePos: node.Pos(), // Use the Pos of the return statement, to not interfere with comment placement + Name: name, + }) } } if len(node.Results) > 0 { - // An ugly hack to update the Pos of any comment that immediately - // follows a rewritten return statement. - for _, comment := range f.commentsBetween(node.End(), c.Parent().End()) { - comment.List[0].Slash = comment.Pos() + token.Pos(offset) - } c.Replace(node) } } From 324c000d5579374542007ae9b92bf74bf41ec68c Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Mon, 29 Jan 2024 11:06:43 +0100 Subject: [PATCH 10/11] Invert a couple conditionals to reduce indentation --- format/format.go | 70 ++++++++++++++++++++++++++---------------------- 1 file changed, 38 insertions(+), 32 deletions(-) diff --git a/format/format.go b/format/format.go index d511499..c0dcd42 100644 --- a/format/format.go +++ b/format/format.go @@ -705,42 +705,48 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { // Clothe naked returns case *ast.ReturnStmt: - if node.Results == nil { // We have either a naked return, or a function with no return values - parents, _ := astutil.PathEnclosingInterval(f.astFile, node.Pos(), node.End()) - var results *ast.FieldList - // Find the nearest ancestor that is either a func declaration or func literal - parentLoop: - for _, parent := range parents { - switch p := parent.(type) { - case *ast.FuncDecl: - results = p.Type.Results - break parentLoop - case *ast.FuncLit: - results = p.Type.Results - break parentLoop - } + if node.Results != nil { + break + } + + // We have either a naked return, or a function with no return values + parents, _ := astutil.PathEnclosingInterval(f.astFile, node.Pos(), node.End()) + var results *ast.FieldList + // Find the nearest ancestor that is either a func declaration or func literal + parentLoop: + for _, parent := range parents { + switch p := parent.(type) { + case *ast.FuncDecl: + results = p.Type.Results + break parentLoop + case *ast.FuncLit: + results = p.Type.Results + break parentLoop } - if fields := results.NumFields(); fields > 0 { // The function has return values; let's clothe the return - node.Results = make([]ast.Expr, 0, fields) - nameLoop: - for _, result := range results.List { - for _, ident := range result.Names { - name := ident.Name - if name == "_" { // we can't handle blank names just yet, abort the transform - node.Results = nil - break nameLoop - } - node.Results = append(node.Results, &ast.Ident{ - NamePos: node.Pos(), // Use the Pos of the return statement, to not interfere with comment placement - Name: name, - }) - } - } - if len(node.Results) > 0 { - c.Replace(node) + } + if results.NumFields() == 0 { + break + } + + // The function has return values; let's clothe the return + node.Results = make([]ast.Expr, 0, results.NumFields()) + nameLoop: + for _, result := range results.List { + for _, ident := range result.Names { + name := ident.Name + if name == "_" { // we can't handle blank names just yet, abort the transform + node.Results = nil + break nameLoop } + node.Results = append(node.Results, &ast.Ident{ + NamePos: node.Pos(), // Use the Pos of the return statement, to not interfere with comment placement + Name: name, + }) } } + if len(node.Results) > 0 { + c.Replace(node) + } } } From 61a11135b7f9f24939b8ecc4a7941b55c87efc19 Mon Sep 17 00:00:00 2001 From: Jonathan Hall Date: Mon, 29 Jan 2024 13:38:54 +0100 Subject: [PATCH 11/11] Track parent function declarations/literals rather than relying on astutil.PathEnclosingInterval --- format/format.go | 24 +++++++++++++++++++++--- 1 file changed, 21 insertions(+), 3 deletions(-) diff --git a/format/format.go b/format/format.go index c0dcd42..08fc33f 100644 --- a/format/format.go +++ b/format/format.go @@ -190,6 +190,11 @@ type fumpter struct { blockLevel int minSplitFactor float64 + + // parentFuncs is a stack of parent function declarations or + // literals, used to determine return type information when clothing + // naked returns. + parentFuncs []ast.Node } func (f *fumpter) commentsBetween(p1, p2 token.Pos) []*ast.CommentGroup { @@ -336,6 +341,16 @@ var rxCommentDirective = regexp.MustCompile(`^([a-z-]+:[a-z]+|line\b|export\b|ex func (f *fumpter) applyPre(c *astutil.Cursor) { f.splitLongLine(c) + if c.Node() != nil && len(f.parentFuncs) > 0 { + // "pop" the last parent if it's no longer valid. + for i := len(f.parentFuncs) - 1; i >= 0; i-- { + if f.parentFuncs[i].End() < c.Node().Pos() { + f.parentFuncs = f.parentFuncs[:i] + break + } + } + } + switch node := c.Node().(type) { case *ast.File: // Join contiguous lone var/const/import lines. @@ -703,6 +718,10 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { // Only remove lines between the assignment token and the first right-hand side expression f.removeLines(f.Line(node.TokPos), f.Line(node.Rhs[0].Pos())) + case *ast.FuncDecl, *ast.FuncLit: + // Track the current function declaration or literal, to access + // return type information for clothing of naked returns. + f.parentFuncs = append(f.parentFuncs, node) // Clothe naked returns case *ast.ReturnStmt: if node.Results != nil { @@ -710,12 +729,11 @@ func (f *fumpter) applyPre(c *astutil.Cursor) { } // We have either a naked return, or a function with no return values - parents, _ := astutil.PathEnclosingInterval(f.astFile, node.Pos(), node.End()) var results *ast.FieldList // Find the nearest ancestor that is either a func declaration or func literal parentLoop: - for _, parent := range parents { - switch p := parent.(type) { + for i := len(f.parentFuncs) - 1; i >= 0; i-- { + switch p := f.parentFuncs[i].(type) { case *ast.FuncDecl: results = p.Type.Results break parentLoop