From 19b2d28062b78738d5576463e7fd8ba23e8fce55 Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Mon, 24 Jul 2023 16:39:50 +0200 Subject: [PATCH 1/7] Use go modules, analysis package and add test * Use go modules * Use `analysis.Analyzer` for the linter * Introduce running mode to use from `golangci-lint` * Add tests using the testing package and assert fixer * Add standalone linter (`cmd/whitespace`) --- README.md | 4 +- cmd/whitespace/main.go | 10 + go.mod | 10 + go.sum | 8 + main.go | 162 ---------- .../src/whitespace_multiline/multiline.go | 85 ++++++ .../whitespace_multiline/multiline.go.golden | 85 ++++++ .../whitespace_no_multiline/no_multiline.go | 39 +++ .../no_multiline.go.golden | 35 +++ whitespace.go | 284 ++++++++++++++++++ whitespace_test.go | 29 ++ 11 files changed, 588 insertions(+), 163 deletions(-) create mode 100644 cmd/whitespace/main.go create mode 100644 go.mod create mode 100644 go.sum delete mode 100644 main.go create mode 100644 testdata/src/whitespace_multiline/multiline.go create mode 100644 testdata/src/whitespace_multiline/multiline.go.golden create mode 100644 testdata/src/whitespace_no_multiline/no_multiline.go create mode 100644 testdata/src/whitespace_no_multiline/no_multiline.go.golden create mode 100644 whitespace.go create mode 100644 whitespace_test.go diff --git a/README.md b/README.md index 2a88f13..f99ecce 100644 --- a/README.md +++ b/README.md @@ -4,4 +4,6 @@ Whitespace is a linter that checks for unnecessary newlines at the start and end ## Installation guide -Whitespace is included in [golangci-lint](https://github.com/golangci/golangci-lint/). Install it and enable whitespace. +To install as a standalone linter, run `go install github.com/ultraware/whitespace`. + +Whitespace is also included in [golangci-lint](https://github.com/golangci/golangci-lint/). Install it and enable whitespace. diff --git a/cmd/whitespace/main.go b/cmd/whitespace/main.go new file mode 100644 index 0000000..73a78ed --- /dev/null +++ b/cmd/whitespace/main.go @@ -0,0 +1,10 @@ +package main + +import ( + "github.com/ultraware/whitespace" + "golang.org/x/tools/go/analysis/singlechecker" +) + +func main() { + singlechecker.Main(whitespace.NewAnalyzer(nil)) +} diff --git a/go.mod b/go.mod new file mode 100644 index 0000000..306a134 --- /dev/null +++ b/go.mod @@ -0,0 +1,10 @@ +module github.com/ultraware/whitespace + +go 1.20 + +require golang.org/x/tools v0.12.0 + +require ( + golang.org/x/mod v0.12.0 // indirect + golang.org/x/sys v0.11.0 // indirect +) diff --git a/go.sum b/go.sum new file mode 100644 index 0000000..06d171f --- /dev/null +++ b/go.sum @@ -0,0 +1,8 @@ +golang.org/x/mod v0.12.0 h1:rmsUpXtvNzj340zd98LZ4KntptpfRHwpFOHG188oHXc= +golang.org/x/mod v0.12.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= +golang.org/x/sys v0.11.0 h1:eG7RXZHdqOJ1i+0lgLgCpSXAp6M3LYlAo6osgSi0xOM= +golang.org/x/sys v0.11.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/tools v0.11.0 h1:EMCa6U9S2LtZXLAMoWiR/R8dAQFRqbAitmbJ2UKhoi8= +golang.org/x/tools v0.11.0/go.mod h1:anzJrxPjNtfgiYQYirP2CPGzGLxrH2u2QBhn6Bf3qY8= +golang.org/x/tools v0.12.0 h1:YW6HUoUmYBpwSgyaGaZq1fHjrBjX1rlpZ54T6mu2kss= +golang.org/x/tools v0.12.0/go.mod h1:Sc0INKfu04TlqNoRA1hgpFZbhYXHPr4V5DzpSBTPqQM= diff --git a/main.go b/main.go deleted file mode 100644 index a5ff70a..0000000 --- a/main.go +++ /dev/null @@ -1,162 +0,0 @@ -package whitespace - -import ( - "go/ast" - "go/token" -) - -// Message contains a message -type Message struct { - Pos token.Position - Type MessageType - Message string -} - -// MessageType describes what should happen to fix the warning -type MessageType uint8 - -// List of MessageTypes -const ( - MessageTypeLeading MessageType = iota + 1 - MessageTypeTrailing - MessageTypeAddAfter -) - -// Settings contains settings for edge-cases -type Settings struct { - MultiIf bool - MultiFunc bool -} - -// Run runs this linter on the provided code -func Run(file *ast.File, fset *token.FileSet, settings Settings) []Message { - var messages []Message - - for _, f := range file.Decls { - decl, ok := f.(*ast.FuncDecl) - if !ok || decl.Body == nil { // decl.Body can be nil for e.g. cgo - continue - } - - vis := visitor{file.Comments, fset, nil, make(map[*ast.BlockStmt]bool), settings} - ast.Walk(&vis, decl) - - messages = append(messages, vis.messages...) - } - - return messages -} - -type visitor struct { - comments []*ast.CommentGroup - fset *token.FileSet - messages []Message - wantNewline map[*ast.BlockStmt]bool - settings Settings -} - -func (v *visitor) Visit(node ast.Node) ast.Visitor { - if node == nil { - return v - } - - if stmt, ok := node.(*ast.IfStmt); ok && v.settings.MultiIf { - checkMultiLine(v, stmt.Body, stmt.Cond) - } - - if stmt, ok := node.(*ast.FuncLit); ok && v.settings.MultiFunc { - checkMultiLine(v, stmt.Body, stmt.Type) - } - - if stmt, ok := node.(*ast.FuncDecl); ok && v.settings.MultiFunc { - checkMultiLine(v, stmt.Body, stmt.Type) - } - - if stmt, ok := node.(*ast.BlockStmt); ok { - wantNewline := v.wantNewline[stmt] - - comments := v.comments - if wantNewline { - comments = nil // Comments also count as a newline if we want a newline - } - first, last := firstAndLast(comments, v.fset, stmt.Pos(), stmt.End(), stmt.List) - - startMsg := checkStart(v.fset, stmt.Lbrace, first) - - if wantNewline && startMsg == nil { - v.messages = append(v.messages, Message{v.fset.PositionFor(stmt.Pos(), false), MessageTypeAddAfter, `multi-line statement should be followed by a newline`}) - } else if !wantNewline && startMsg != nil { - v.messages = append(v.messages, *startMsg) - } - - if msg := checkEnd(v.fset, stmt.Rbrace, last); msg != nil { - v.messages = append(v.messages, *msg) - } - } - - return v -} - -func checkMultiLine(v *visitor, body *ast.BlockStmt, stmtStart ast.Node) { - start, end := posLine(v.fset, stmtStart.Pos()), posLine(v.fset, stmtStart.End()) - - if end > start { // Check only multi line conditions - v.wantNewline[body] = true - } -} - -func posLine(fset *token.FileSet, pos token.Pos) int { - return fset.PositionFor(pos, false).Line -} - -func firstAndLast(comments []*ast.CommentGroup, fset *token.FileSet, start, end token.Pos, stmts []ast.Stmt) (ast.Node, ast.Node) { - if len(stmts) == 0 { - return nil, nil - } - - first, last := ast.Node(stmts[0]), ast.Node(stmts[len(stmts)-1]) - - for _, c := range comments { - if posLine(fset, c.Pos()) == posLine(fset, start) || posLine(fset, c.End()) == posLine(fset, end) { - continue - } - - if c.Pos() < start || c.End() > end { - continue - } - if c.Pos() < first.Pos() { - first = c - } - if c.End() > last.End() { - last = c - } - } - - return first, last -} - -func checkStart(fset *token.FileSet, start token.Pos, first ast.Node) *Message { - if first == nil { - return nil - } - - if posLine(fset, start)+1 < posLine(fset, first.Pos()) { - pos := fset.PositionFor(start, false) - return &Message{pos, MessageTypeLeading, `unnecessary leading newline`} - } - - return nil -} - -func checkEnd(fset *token.FileSet, end token.Pos, last ast.Node) *Message { - if last == nil { - return nil - } - - if posLine(fset, end)-1 > posLine(fset, last.End()) { - pos := fset.PositionFor(end, false) - return &Message{pos, MessageTypeTrailing, `unnecessary trailing newline`} - } - - return nil -} diff --git a/testdata/src/whitespace_multiline/multiline.go b/testdata/src/whitespace_multiline/multiline.go new file mode 100644 index 0000000..3459d7b --- /dev/null +++ b/testdata/src/whitespace_multiline/multiline.go @@ -0,0 +1,85 @@ +package whitespace + +import "fmt" + +// No comments, only whitespace. +func fn1() { // want "unnecessary leading newline" + + fmt.Println("Hello, World") + +} // want "unnecessary trailing newline" + +// Whitespace before leading and after trailing comment. +func fn2() { // want "unnecessary leading newline" + + // Some leading comments - this should s till yield error. + fmt.Println("Hello, World") + // And some trailing comments as well! + +} // want "unnecessary trailing newline" + +// Whitespace after leading and before trailing comment - no diagnostic. +func fn3() { + // This is fine, newline is after commetn + + fmt.Println("Hello, World") + + // And so is this! +} + +// MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) without newlinew. +func fn4( + arg1 int, + arg2 int, +) { // want "multi-line statement should be followed by a newline" + fmt.Println("Hello, World") + + if true && + false { // want "multi-line statement should be followed by a newline" + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { // want "multi-line statement should be followed by a newline" + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { // want "multi-line statement should be followed by a newline" + _ = func( + arg1 int, + arg2 int, + ) { // want "multi-line statement should be followed by a newline" + fmt.Println("Hello, World") + } + } +} + + +// MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) with comments counting +// as newlines. +func fn5( + arg1 int, + arg2 int, +) { + // Comment count as newline. + fmt.Println("Hello, World") + + if true && + false { + // Comment count as newline. + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { + // Comment count as newline. + fmt.Println("Hello, World") + } +} diff --git a/testdata/src/whitespace_multiline/multiline.go.golden b/testdata/src/whitespace_multiline/multiline.go.golden new file mode 100644 index 0000000..fd43156 --- /dev/null +++ b/testdata/src/whitespace_multiline/multiline.go.golden @@ -0,0 +1,85 @@ +package whitespace + +import "fmt" + +// No comments, only whitespace. +func fn1() { // want "unnecessary leading newline" + fmt.Println("Hello, World") +} // want "unnecessary trailing newline" + +// Whitespace before leading and after trailing comment. +func fn2() { // want "unnecessary leading newline" + // Some leading comments - this should s till yield error. + fmt.Println("Hello, World") + // And some trailing comments as well! +} // want "unnecessary trailing newline" + +// Whitespace after leading and before trailing comment - no diagnostic. +func fn3() { + // This is fine, newline is after commetn + + fmt.Println("Hello, World") + + // And so is this! +} + +// MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) without newlinew. +func fn4( + arg1 int, + arg2 int, +) { // want "multi-line statement should be followed by a newline" + + fmt.Println("Hello, World") + + if true && + false { // want "multi-line statement should be followed by a newline" + + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { // want "multi-line statement should be followed by a newline" + + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { // want "multi-line statement should be followed by a newline" + + _ = func( + arg1 int, + arg2 int, + ) { // want "multi-line statement should be followed by a newline" + + fmt.Println("Hello, World") + } + } +} + +// MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) with comments counting +// as newlines. +func fn5( + arg1 int, + arg2 int, +) { + // Comment count as newline. + fmt.Println("Hello, World") + + if true && + false { + // Comment count as newline. + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { + // Comment count as newline. + fmt.Println("Hello, World") + } +} diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go b/testdata/src/whitespace_no_multiline/no_multiline.go new file mode 100644 index 0000000..8b9b9e2 --- /dev/null +++ b/testdata/src/whitespace_no_multiline/no_multiline.go @@ -0,0 +1,39 @@ +package whitespace + +import "fmt" + +// MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) want to remove newlinne. +func fn1( + arg1 int, + arg2 int, +) { // want "unnecessary leading newline" + + fmt.Println("Hello, World") + + if true && + false { // want "unnecessary leading newline" + + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { // want "unnecessary leading newline" + + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { + _ = func( + arg1 int, + arg2 int, + ) { // want "unnecessary leading newline" + + fmt.Println("Hello, World") + } + } +} diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go.golden b/testdata/src/whitespace_no_multiline/no_multiline.go.golden new file mode 100644 index 0000000..113ef36 --- /dev/null +++ b/testdata/src/whitespace_no_multiline/no_multiline.go.golden @@ -0,0 +1,35 @@ +package whitespace + +import "fmt" + +// MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) want to remove newlinne. +func fn1( + arg1 int, + arg2 int, +) { // want "unnecessary leading newline" + fmt.Println("Hello, World") + + if true && + false { // want "unnecessary leading newline" + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { // want "unnecessary leading newline" + fmt.Println("Hello, World") + } + + _ = func( + arg1 int, + arg2 int, + ) { + _ = func( + arg1 int, + arg2 int, + ) { // want "unnecessary leading newline" + fmt.Println("Hello, World") + } + } +} diff --git a/whitespace.go b/whitespace.go new file mode 100644 index 0000000..b9c66b2 --- /dev/null +++ b/whitespace.go @@ -0,0 +1,284 @@ +package whitespace + +import ( + "flag" + "go/ast" + "go/token" + "strings" + + "golang.org/x/tools/go/analysis" +) + +// MessageType describes what should happen to fix the warning. +type MessageType uint8 + +// List of MessageTypes. +const ( + MessageTypeRemove MessageType = iota + 1 + MessageTypeAdd +) + +// RunningMode describes the mode the linter is run in. This can be either +// native or golangci-lint. +type RunningMode uint8 + +const ( + RunningModeNative RunningMode = iota + RunningModeGolangCI +) + +// Message contains a message and diagnostic information. +type Message struct { + // Diagnostic is what position the diagnostic should be put at. This isn't + // always the same as the fix start, f.ex. when we fix trailing newlines we + // put the diagnostic at the right bracket but we fix between the end of the + // last statement and the bracket. + Diagnostic token.Pos + + // FixStart is the span start of the fix. + FixStart token.Pos + + // FixEnd is the span end of the fix. + FixEnd token.Pos + + // LineNumber represent the actual line number in the file. This is set when + // finding the diagnostic to make it easier to suggest fixes in + // golangci-lint. + LineNumber int + + // MessageType represents the type of message it is. + MessageType MessageType + + // Message is the diagnostic to show. + Message string +} + +// Settings contains settings for edge-cases. +type Settings struct { + Mode RunningMode + MultiIf bool + MultiFunc bool +} + +// NewAnalyzer creates a new whitespace analyzer. +func NewAnalyzer(settings *Settings) *analysis.Analyzer { + if settings == nil { + settings = &Settings{} + } + + return &analysis.Analyzer{ + Name: "whitespace", + Doc: "Whitespace is a linter that checks for unnecessary newlines at the start and end of functions, if, for, etc.", + Flags: flags(settings), + Run: func(p *analysis.Pass) (any, error) { + Run(p, settings) + return nil, nil + }, + RunDespiteErrors: true, + } +} + +func flags(settings *Settings) flag.FlagSet { + flags := flag.NewFlagSet("", flag.ExitOnError) + flags.BoolVar(&settings.MultiIf, "multi-if", settings.MultiIf, "Check that multi line if-statements have a leading newline") + flags.BoolVar(&settings.MultiFunc, "multi-func", settings.MultiFunc, "Check that multi line functions have a leading newline") + + return *flags +} + +func Run(pass *analysis.Pass, settings *Settings) []Message { + messages := []Message{} + + for _, file := range pass.Files { + filename := pass.Fset.Position(file.Pos()).Filename + if !strings.HasSuffix(filename, ".go") { + continue + } + + fileMessages := runFile(file, pass.Fset, *settings) + + if settings.Mode == RunningModeGolangCI { + messages = append(messages, fileMessages...) + continue + } + + for _, message := range fileMessages { + pass.Report(analysis.Diagnostic{ + Pos: message.Diagnostic, + Category: "whitespace", + Message: message.Message, + SuggestedFixes: []analysis.SuggestedFix{ + { + TextEdits: []analysis.TextEdit{ + { + Pos: message.FixStart, + End: message.FixEnd, + NewText: []byte("\n"), + }, + }, + }, + }, + }) + } + } + + return messages +} + +func runFile(file *ast.File, fset *token.FileSet, settings Settings) []Message { + var messages []Message + + for _, f := range file.Decls { + decl, ok := f.(*ast.FuncDecl) + if !ok || decl.Body == nil { // decl.Body can be nil for e.g. cgo + continue + } + + vis := visitor{file.Comments, fset, nil, make(map[*ast.BlockStmt]bool), settings} + ast.Walk(&vis, decl) + + messages = append(messages, vis.messages...) + } + + return messages +} + +type visitor struct { + comments []*ast.CommentGroup + fset *token.FileSet + messages []Message + wantNewline map[*ast.BlockStmt]bool + settings Settings +} + +func (v *visitor) Visit(node ast.Node) ast.Visitor { + if node == nil { + return v + } + + if stmt, ok := node.(*ast.IfStmt); ok && v.settings.MultiIf { + checkMultiLine(v, stmt.Body, stmt.Cond) + } + + if stmt, ok := node.(*ast.FuncLit); ok && v.settings.MultiFunc { + checkMultiLine(v, stmt.Body, stmt.Type) + } + + if stmt, ok := node.(*ast.FuncDecl); ok && v.settings.MultiFunc { + checkMultiLine(v, stmt.Body, stmt.Type) + } + + if stmt, ok := node.(*ast.BlockStmt); ok { + wantNewline := v.wantNewline[stmt] + + comments := v.comments + if wantNewline { + comments = nil // Comments also count as a newline if we want a newline + } + + first, last := firstAndLast(comments, v.fset, stmt.Pos(), stmt.End(), stmt.List) + startMsg := checkStart(v.fset, stmt.Lbrace, first) + + if wantNewline && startMsg == nil && len(stmt.List) >= 1 { + v.messages = append(v.messages, Message{ + Diagnostic: stmt.Lbrace, + FixStart: stmt.List[0].Pos(), + FixEnd: stmt.List[0].Pos(), + LineNumber: v.fset.PositionFor(stmt.List[0].Pos(), false).Line, + MessageType: MessageTypeAdd, + Message: "multi-line statement should be followed by a newline", + }) + } else if !wantNewline && startMsg != nil { + v.messages = append(v.messages, *startMsg) + } + + if msg := checkEnd(v.fset, stmt.Rbrace, last); msg != nil { + v.messages = append(v.messages, *msg) + } + } + + return v +} + +func checkMultiLine(v *visitor, body *ast.BlockStmt, stmtStart ast.Node) { + start, end := posLine(v.fset, stmtStart.Pos()), posLine(v.fset, stmtStart.End()) + + if end > start { // Check only multi line conditions + v.wantNewline[body] = true + } +} + +func posLine(fset *token.FileSet, pos token.Pos) int { + return fset.PositionFor(pos, false).Line +} + +func firstAndLast(comments []*ast.CommentGroup, fset *token.FileSet, start, end token.Pos, stmts []ast.Stmt) (ast.Node, ast.Node) { + if len(stmts) == 0 { + return nil, nil + } + + first, last := ast.Node(stmts[0]), ast.Node(stmts[len(stmts)-1]) + + for _, c := range comments { + if posLine(fset, c.Pos()) == posLine(fset, start) || posLine(fset, c.End()) == posLine(fset, end) { + continue + } + + if c.Pos() < start || c.End() > end { + continue + } + if c.Pos() < first.Pos() { + first = c + } + if c.End() > last.End() { + last = c + } + } + + return first, last +} + +func checkStart(fset *token.FileSet, start token.Pos, first ast.Node) *Message { + if first == nil { + return nil + } + + if posLine(fset, start)+1 < posLine(fset, first.Pos()) { + // We need to know what column we're at to mark or fix start at the + // first node position minus its columns mins the size of a newline. + firstColumn := fset.PositionFor(first.Pos(), false).Column + + return &Message{ + Diagnostic: start, + // We remove 3 for the first statement position to account for the + // newline but not having to consider potential comments between the + // left bracket and the first statement. + FixStart: first.Pos() - token.Pos(firstColumn) - 1, + FixEnd: first.Pos(), + LineNumber: fset.PositionFor(first.Pos(), false).Line - 1, + MessageType: MessageTypeRemove, + Message: "unnecessary leading newline", + } + } + + return nil +} + +func checkEnd(fset *token.FileSet, end token.Pos, last ast.Node) *Message { + if last == nil { + return nil + } + + if posLine(fset, end)-1 > posLine(fset, last.End()) { + return &Message{ + Diagnostic: end, + FixStart: last.End(), + FixEnd: end, + LineNumber: fset.PositionFor(last.End(), false).Line + 1, + MessageType: MessageTypeRemove, + Message: "unnecessary trailing newline", + } + } + + return nil +} diff --git a/whitespace_test.go b/whitespace_test.go new file mode 100644 index 0000000..bbcc4d6 --- /dev/null +++ b/whitespace_test.go @@ -0,0 +1,29 @@ +package whitespace + +import ( + "testing" + + "golang.org/x/tools/go/analysis/analysistest" +) + +func TestWantMultiline(t *testing.T) { + testdata := analysistest.TestData() + analyzer := NewAnalyzer(&Settings{ + Mode: RunningModeNative, + MultiIf: true, + MultiFunc: true, + }) + + analysistest.RunWithSuggestedFixes(t, testdata, analyzer, "whitespace_multiline") +} + +func TestNoMultiline(t *testing.T) { + testdata := analysistest.TestData() + analyzer := NewAnalyzer(&Settings{ + Mode: RunningModeNative, + MultiIf: false, + MultiFunc: false, + }) + + analysistest.RunWithSuggestedFixes(t, testdata, analyzer, "whitespace_no_multiline") +} From e33481951eac1d65da8548619d96cdcc22a6eb2d Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Fri, 11 Aug 2023 23:14:05 +0200 Subject: [PATCH 2/7] Fix comments and typos, add another test --- testdata/src/whitespace_multiline/multiline.go | 6 +++--- .../src/whitespace_multiline/multiline.go.golden | 6 +++--- .../src/whitespace_no_multiline/no_multiline.go | 11 +++++++++++ .../no_multiline.go.golden | 9 +++++++++ whitespace.go | 15 +++++++-------- 5 files changed, 33 insertions(+), 14 deletions(-) diff --git a/testdata/src/whitespace_multiline/multiline.go b/testdata/src/whitespace_multiline/multiline.go index 3459d7b..cdf2b9c 100644 --- a/testdata/src/whitespace_multiline/multiline.go +++ b/testdata/src/whitespace_multiline/multiline.go @@ -12,7 +12,7 @@ func fn1() { // want "unnecessary leading newline" // Whitespace before leading and after trailing comment. func fn2() { // want "unnecessary leading newline" - // Some leading comments - this should s till yield error. + // Some leading comments - this should yield error. fmt.Println("Hello, World") // And some trailing comments as well! @@ -20,11 +20,11 @@ func fn2() { // want "unnecessary leading newline" // Whitespace after leading and before trailing comment - no diagnostic. func fn3() { - // This is fine, newline is after commetn + // This is fine, newline is after comment fmt.Println("Hello, World") - // And so is this! + // This is fine, newline before comment } // MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) without newlinew. diff --git a/testdata/src/whitespace_multiline/multiline.go.golden b/testdata/src/whitespace_multiline/multiline.go.golden index fd43156..d0ca256 100644 --- a/testdata/src/whitespace_multiline/multiline.go.golden +++ b/testdata/src/whitespace_multiline/multiline.go.golden @@ -9,18 +9,18 @@ func fn1() { // want "unnecessary leading newline" // Whitespace before leading and after trailing comment. func fn2() { // want "unnecessary leading newline" - // Some leading comments - this should s till yield error. + // Some leading comments - this should yield error. fmt.Println("Hello, World") // And some trailing comments as well! } // want "unnecessary trailing newline" // Whitespace after leading and before trailing comment - no diagnostic. func fn3() { - // This is fine, newline is after commetn + // This is fine, newline is after comment fmt.Println("Hello, World") - // And so is this! + // This is fine, newline before comment } // MultiFunc (FuncDecl), MultiIf and MultiFunc(FuncLit) without newlinew. diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go b/testdata/src/whitespace_no_multiline/no_multiline.go index 8b9b9e2..60baedd 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go +++ b/testdata/src/whitespace_no_multiline/no_multiline.go @@ -37,3 +37,14 @@ func fn1( } } } + +// MultiFunc (FuncDecl) with comment. +func fn2( + arg1 int, + arg2 int, +) { // want "unnecessary leading newline" + + // A comment. + fmt.Println("Hello, World") +} + diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go.golden b/testdata/src/whitespace_no_multiline/no_multiline.go.golden index 113ef36..ff89593 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go.golden +++ b/testdata/src/whitespace_no_multiline/no_multiline.go.golden @@ -33,3 +33,12 @@ func fn1( } } } + +// MultiFunc (FuncDecl) with comment. +func fn2( + arg1 int, + arg2 int, +) { // want "unnecessary leading newline" + // A comment. + fmt.Println("Hello, World") +} diff --git a/whitespace.go b/whitespace.go index b9c66b2..d3c7d15 100644 --- a/whitespace.go +++ b/whitespace.go @@ -244,18 +244,17 @@ func checkStart(fset *token.FileSet, start token.Pos, first ast.Node) *Message { } if posLine(fset, start)+1 < posLine(fset, first.Pos()) { - // We need to know what column we're at to mark or fix start at the - // first node position minus its columns mins the size of a newline. - firstColumn := fset.PositionFor(first.Pos(), false).Column + firstPosition := fset.PositionFor(first.Pos(), false) return &Message{ Diagnostic: start, - // We remove 3 for the first statement position to account for the - // newline but not having to consider potential comments between the - // left bracket and the first statement. - FixStart: first.Pos() - token.Pos(firstColumn) - 1, + // We remove all the tabs/spaces + 1 for the newline for the first + // statement position to account for the newline but not having to + // consider potential comments between the left bracket and the + // first statement. + FixStart: first.Pos() - token.Pos(firstPosition.Column) - 1, FixEnd: first.Pos(), - LineNumber: fset.PositionFor(first.Pos(), false).Line - 1, + LineNumber: firstPosition.Line - 1, MessageType: MessageTypeRemove, Message: "unnecessary leading newline", } From 6064ab32449d475b3de012259bb25ee3e7b2c02b Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Fri, 11 Aug 2023 23:18:51 +0200 Subject: [PATCH 3/7] Add GitHub actions --- .github/workflows/go.yaml | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 .github/workflows/go.yaml diff --git a/.github/workflows/go.yaml b/.github/workflows/go.yaml new file mode 100644 index 0000000..0142102 --- /dev/null +++ b/.github/workflows/go.yaml @@ -0,0 +1,22 @@ +--- +name: Build and test +on: + push: + pull_request: + +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v3 + + - name: Set up Go + uses: actions/setup-go@v4 + with: + go-version: '1.21' + + - name: Build + run: go build -v ./... + + - name: Test + run: go test -v ./... From d9dd7ed2b510ff9409a7b39a40a9a98cca917160 Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Tue, 3 Oct 2023 22:24:46 +0200 Subject: [PATCH 4/7] Make linter work on non formatted files Instead of relying on the file being `gofmt`:ed and only containing one empty line at most this commit will keep track of comments after the opening bracket of block statements and set the fix start to where this comment ends. This means that we will fix all the way from the left bracket or comment til the first statement no matter how many empty lines this is. This will also keep a list of lines affected by the linter to support fixing multiple empty lines in `golangci-lint`. --- .../whitespace_no_multiline/no_multiline.go | 46 +++++++++++++ .../no_multiline.go.golden | 23 +++++++ whitespace.go | 66 ++++++++++++------- 3 files changed, 111 insertions(+), 24 deletions(-) diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go b/testdata/src/whitespace_no_multiline/no_multiline.go index 60baedd..ca5b74a 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go +++ b/testdata/src/whitespace_no_multiline/no_multiline.go @@ -48,3 +48,49 @@ func fn2( fmt.Println("Hello, World") } +// MultiFunc (FuncDecl) that's not `gofmt`:ed. +func fn3( + arg1 int, + arg2 int, +) { // want "unnecessary leading newline" + + + + // A comment. + fmt.Println("Hello, World") + + if true { // want "unnecessary leading newline" + + + + fmt.Println("No cmoments") + + + } // want "unnecessary trailing newline" + // Also at end + + + +} // want "unnecessary trailing newline" + +// Regular func (FuncDecl) that's not `gofmt`:ed. +func fn4() { // want "unnecessary leading newline" + + + + + fmt.Println("Hello, World") + + if true { // want "unnecessary leading newline" + + + + fmt.Println("No cmoments") + + + } // want "unnecessary trailing newline" + + + + +} // want "unnecessary trailing newline" diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go.golden b/testdata/src/whitespace_no_multiline/no_multiline.go.golden index ff89593..2fd4cfd 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go.golden +++ b/testdata/src/whitespace_no_multiline/no_multiline.go.golden @@ -42,3 +42,26 @@ func fn2( // A comment. fmt.Println("Hello, World") } + +// MultiFunc (FuncDecl) that's not `gofmt`:ed. +func fn3( + arg1 int, + arg2 int, +) { // want "unnecessary leading newline" + // A comment. + fmt.Println("Hello, World") + + if true { // want "unnecessary leading newline" + fmt.Println("No cmoments") + } // want "unnecessary trailing newline" + // Also at end +} // want "unnecessary trailing newline" + +// Regular func (FuncDecl) that's not `gofmt`:ed. +func fn4() { // want "unnecessary leading newline" + fmt.Println("Hello, World") + + if true { // want "unnecessary leading newline" + fmt.Println("No cmoments") + } // want "unnecessary trailing newline" +} // want "unnecessary trailing newline" diff --git a/whitespace.go b/whitespace.go index d3c7d15..6c6442c 100644 --- a/whitespace.go +++ b/whitespace.go @@ -41,10 +41,10 @@ type Message struct { // FixEnd is the span end of the fix. FixEnd token.Pos - // LineNumber represent the actual line number in the file. This is set when - // finding the diagnostic to make it easier to suggest fixes in + // LineNumbers represent the actual line number in the file. This is set + // when finding the diagnostic to make it easier to suggest fixes in // golangci-lint. - LineNumber int + LineNumbers []int // MessageType represents the type of message it is. MessageType MessageType @@ -176,15 +176,15 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor { comments = nil // Comments also count as a newline if we want a newline } - first, last := firstAndLast(comments, v.fset, stmt.Pos(), stmt.End(), stmt.List) - startMsg := checkStart(v.fset, stmt.Lbrace, first) + opening, first, last := firstAndLast(comments, v.fset, stmt) + startMsg := checkStart(v.fset, opening, first) if wantNewline && startMsg == nil && len(stmt.List) >= 1 { v.messages = append(v.messages, Message{ - Diagnostic: stmt.Lbrace, + Diagnostic: opening, FixStart: stmt.List[0].Pos(), FixEnd: stmt.List[0].Pos(), - LineNumber: v.fset.PositionFor(stmt.List[0].Pos(), false).Line, + LineNumbers: []int{v.fset.PositionFor(stmt.List[0].Pos(), false).Line}, MessageType: MessageTypeAdd, Message: "multi-line statement should be followed by a newline", }) @@ -212,30 +212,42 @@ func posLine(fset *token.FileSet, pos token.Pos) int { return fset.PositionFor(pos, false).Line } -func firstAndLast(comments []*ast.CommentGroup, fset *token.FileSet, start, end token.Pos, stmts []ast.Stmt) (ast.Node, ast.Node) { - if len(stmts) == 0 { - return nil, nil +func firstAndLast(comments []*ast.CommentGroup, fset *token.FileSet, stmt *ast.BlockStmt) (token.Pos, ast.Node, ast.Node) { + openingPos := stmt.Lbrace + 1 + + if len(stmt.List) == 0 { + return openingPos, nil, nil } - first, last := ast.Node(stmts[0]), ast.Node(stmts[len(stmts)-1]) + first, last := ast.Node(stmt.List[0]), ast.Node(stmt.List[len(stmt.List)-1]) for _, c := range comments { - if posLine(fset, c.Pos()) == posLine(fset, start) || posLine(fset, c.End()) == posLine(fset, end) { + // If the comment is on the same line as the opening pos (initially the + // left bracket) but it starts after the pos the comment must be after + // the bracket and where that comment ends should be considered where + // the fix should start. + if posLine(fset, c.End()) == posLine(fset, openingPos) && c.Pos() > openingPos { + openingPos = c.End() + } + + if posLine(fset, c.Pos()) == posLine(fset, stmt.Pos()) || posLine(fset, c.End()) == posLine(fset, stmt.End()) { continue } - if c.Pos() < start || c.End() > end { + if c.Pos() < stmt.Pos() || c.End() > stmt.End() { continue } + if c.Pos() < first.Pos() { first = c } + if c.End() > last.End() { last = c } } - return first, last + return openingPos, first, last } func checkStart(fset *token.FileSet, start token.Pos, first ast.Node) *Message { @@ -244,17 +256,11 @@ func checkStart(fset *token.FileSet, start token.Pos, first ast.Node) *Message { } if posLine(fset, start)+1 < posLine(fset, first.Pos()) { - firstPosition := fset.PositionFor(first.Pos(), false) - return &Message{ - Diagnostic: start, - // We remove all the tabs/spaces + 1 for the newline for the first - // statement position to account for the newline but not having to - // consider potential comments between the left bracket and the - // first statement. - FixStart: first.Pos() - token.Pos(firstPosition.Column) - 1, + Diagnostic: start, + FixStart: start, FixEnd: first.Pos(), - LineNumber: firstPosition.Line - 1, + LineNumbers: linesBetween(fset, start, first.Pos()), MessageType: MessageTypeRemove, Message: "unnecessary leading newline", } @@ -273,7 +279,7 @@ func checkEnd(fset *token.FileSet, end token.Pos, last ast.Node) *Message { Diagnostic: end, FixStart: last.End(), FixEnd: end, - LineNumber: fset.PositionFor(last.End(), false).Line + 1, + LineNumbers: linesBetween(fset, last.End(), end), MessageType: MessageTypeRemove, Message: "unnecessary trailing newline", } @@ -281,3 +287,15 @@ func checkEnd(fset *token.FileSet, end token.Pos, last ast.Node) *Message { return nil } + +func linesBetween(fset *token.FileSet, a, b token.Pos) []int { + lines := []int{} + aPosition := fset.PositionFor(a, false) + bPosition := fset.PositionFor(b, false) + + for i := aPosition.Line + 1; i < bPosition.Line; i++ { + lines = append(lines, i) + } + + return lines +} From b5ade394300e02d786021e868c691eb02d1e78a7 Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Wed, 4 Oct 2023 22:08:31 +0200 Subject: [PATCH 5/7] Fix typos, add tests from PR --- .../whitespace_no_multiline/no_multiline.go | 59 ++++++++++++++++++- .../no_multiline.go.golden | 38 ++++++++++++ whitespace.go | 2 +- 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go b/testdata/src/whitespace_no_multiline/no_multiline.go index ca5b74a..1a77fca 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go +++ b/testdata/src/whitespace_no_multiline/no_multiline.go @@ -85,7 +85,7 @@ func fn4() { // want "unnecessary leading newline" - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" @@ -93,4 +93,61 @@ func fn4() { // want "unnecessary leading newline" +} // want "unnecessary trailing newline" + +// Regular func (FuncDecl) that's not `gofmt`:ed. with comments +func fn5() { + // A comment that should still exist after this + // This one should also still exist + + + + fmt.Println("Hello, World") + + if true { + // A comment that should still exist after this + // This one should also still exist + + + + fmt.Println("No cmoments") + + + } // want "unnecessary trailing newline" + + + + +} // want "unnecessary trailing newline" + +// Regular func (FuncDecl) that's not `gofmt`:ed. with comment blocks +func fn6() { + /* + Lorem ipsum dolor sit amet, consectetur adipiscing elit. + Curabitur ornare dolor at nulla ultrices cursus. + Mauris pharetra metus ac condimentum sodales. + Fusce viverra libero vitae tellus dictum, sed congue risus sodales. + */ + + + + fmt.Println("Hello, World") + + if true { + /* + Lorem ipsum dolor sit amet, consectetur adipiscing elit. + Curabitur ornare dolor at nulla ultrices cursus. + Mauris pharetra metus ac condimentum sodales. + Fusce viverra libero vitae tellus dictum, sed congue risus sodales. + */ + + + fmt.Println("No cmoments") + + + } // want "unnecessary trailing newline" + + + + } // want "unnecessary trailing newline" diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go.golden b/testdata/src/whitespace_no_multiline/no_multiline.go.golden index 2fd4cfd..b2fc497 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go.golden +++ b/testdata/src/whitespace_no_multiline/no_multiline.go.golden @@ -62,6 +62,44 @@ func fn4() { // want "unnecessary leading newline" fmt.Println("Hello, World") if true { // want "unnecessary leading newline" + fmt.Println("No comments") + } // want "unnecessary trailing newline" +} // want "unnecessary trailing newline" + +// Regular func (FuncDecl) that's not `gofmt`:ed. with comments +func fn5() { + // A comment that should still exist after this + // This one should also still exist + + fmt.Println("Hello, World") + + if true { + // A comment that should still exist after this + // This one should also still exist + + fmt.Println("No cmoments") + } // want "unnecessary trailing newline" +} // want "unnecessary trailing newline" + +// Regular func (FuncDecl) that's not `gofmt`:ed. with comment blocks +func fn6() { + /* + Lorem ipsum dolor sit amet, consectetur adipiscing elit. + Curabitur ornare dolor at nulla ultrices cursus. + Mauris pharetra metus ac condimentum sodales. + Fusce viverra libero vitae tellus dictum, sed congue risus sodales. + */ + + fmt.Println("Hello, World") + + if true { + /* + Lorem ipsum dolor sit amet, consectetur adipiscing elit. + Curabitur ornare dolor at nulla ultrices cursus. + Mauris pharetra metus ac condimentum sodales. + Fusce viverra libero vitae tellus dictum, sed congue risus sodales. + */ + fmt.Println("No cmoments") } // want "unnecessary trailing newline" } // want "unnecessary trailing newline" diff --git a/whitespace.go b/whitespace.go index 6c6442c..9ec4acd 100644 --- a/whitespace.go +++ b/whitespace.go @@ -41,7 +41,7 @@ type Message struct { // FixEnd is the span end of the fix. FixEnd token.Pos - // LineNumbers represent the actual line number in the file. This is set + // LineNumbers represent the actual line numbers in the file. This is set // when finding the diagnostic to make it easier to suggest fixes in // golangci-lint. LineNumbers []int From 4f3cb80810f5c084327f79d0a26d56cf89c4316c Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Mon, 30 Oct 2023 22:28:51 +0100 Subject: [PATCH 6/7] Multiline comments ending after opening pos is ok --- .../whitespace_no_multiline/no_multiline.go | 18 ++++++++++++++++++ .../no_multiline.go.golden | 17 +++++++++++++++++ whitespace.go | 10 ++++++++-- 3 files changed, 43 insertions(+), 2 deletions(-) diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go b/testdata/src/whitespace_no_multiline/no_multiline.go index 1a77fca..e542fc6 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go +++ b/testdata/src/whitespace_no_multiline/no_multiline.go @@ -151,3 +151,21 @@ func fn6() { } // want "unnecessary trailing newline" + +func fn7() { /* Multiline spaning 1 line */ // want "unnecessary leading newline" + + fmt.Println("No cmoments") +} + +func fn8() { /* Multiline spaning + over several lines */ + + fmt.Println("No cmoments") +} + +func fn9() { /* Multiline spaning + + over several lines */ + + fmt.Println("No cmoments") +} diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go.golden b/testdata/src/whitespace_no_multiline/no_multiline.go.golden index b2fc497..93bc99c 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go.golden +++ b/testdata/src/whitespace_no_multiline/no_multiline.go.golden @@ -103,3 +103,20 @@ func fn6() { fmt.Println("No cmoments") } // want "unnecessary trailing newline" } // want "unnecessary trailing newline" + +func fn7() { /* Multiline spaning 1 line */ // want "unnecessary leading newline" + fmt.Println("No cmoments") +} + +func fn8() { /* Multiline spaning + over several lines */ + + fmt.Println("No cmoments") +} + +func fn9() { /* Multiline spaning + + over several lines */ + + fmt.Println("No cmoments") +} diff --git a/whitespace.go b/whitespace.go index 9ec4acd..350e9b7 100644 --- a/whitespace.go +++ b/whitespace.go @@ -226,8 +226,14 @@ func firstAndLast(comments []*ast.CommentGroup, fset *token.FileSet, stmt *ast.B // left bracket) but it starts after the pos the comment must be after // the bracket and where that comment ends should be considered where // the fix should start. - if posLine(fset, c.End()) == posLine(fset, openingPos) && c.Pos() > openingPos { - openingPos = c.End() + if posLine(fset, c.Pos()) == posLine(fset, openingPos) && c.Pos() > openingPos { + if posLine(fset, c.End()) != posLine(fset, openingPos) { + // This is a multiline comment that spans from the `LBrace` line + // to a line further down. This should always be seen as ok! + first = c + } else { + openingPos = c.End() + } } if posLine(fset, c.Pos()) == posLine(fset, stmt.Pos()) || posLine(fset, c.End()) == posLine(fset, stmt.End()) { From 7f92e178a77223910f1a757ee946e2263f53f488 Mon Sep 17 00:00:00 2001 From: Simon Sawert Date: Tue, 31 Oct 2023 09:25:57 +0100 Subject: [PATCH 7/7] Fix typo --- testdata/src/whitespace_no_multiline/no_multiline.go | 12 ++++++------ .../whitespace_no_multiline/no_multiline.go.golden | 12 ++++++------ 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go b/testdata/src/whitespace_no_multiline/no_multiline.go index e542fc6..4038fb3 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go +++ b/testdata/src/whitespace_no_multiline/no_multiline.go @@ -63,7 +63,7 @@ func fn3( - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" @@ -110,7 +110,7 @@ func fn5() { - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" @@ -142,7 +142,7 @@ func fn6() { */ - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" @@ -154,18 +154,18 @@ func fn6() { func fn7() { /* Multiline spaning 1 line */ // want "unnecessary leading newline" - fmt.Println("No cmoments") + fmt.Println("No comments") } func fn8() { /* Multiline spaning over several lines */ - fmt.Println("No cmoments") + fmt.Println("No comments") } func fn9() { /* Multiline spaning over several lines */ - fmt.Println("No cmoments") + fmt.Println("No comments") } diff --git a/testdata/src/whitespace_no_multiline/no_multiline.go.golden b/testdata/src/whitespace_no_multiline/no_multiline.go.golden index 93bc99c..6cbf7de 100644 --- a/testdata/src/whitespace_no_multiline/no_multiline.go.golden +++ b/testdata/src/whitespace_no_multiline/no_multiline.go.golden @@ -52,7 +52,7 @@ func fn3( fmt.Println("Hello, World") if true { // want "unnecessary leading newline" - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" // Also at end } // want "unnecessary trailing newline" @@ -77,7 +77,7 @@ func fn5() { // A comment that should still exist after this // This one should also still exist - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" } // want "unnecessary trailing newline" @@ -100,23 +100,23 @@ func fn6() { Fusce viverra libero vitae tellus dictum, sed congue risus sodales. */ - fmt.Println("No cmoments") + fmt.Println("No comments") } // want "unnecessary trailing newline" } // want "unnecessary trailing newline" func fn7() { /* Multiline spaning 1 line */ // want "unnecessary leading newline" - fmt.Println("No cmoments") + fmt.Println("No comments") } func fn8() { /* Multiline spaning over several lines */ - fmt.Println("No cmoments") + fmt.Println("No comments") } func fn9() { /* Multiline spaning over several lines */ - fmt.Println("No cmoments") + fmt.Println("No comments") }