From 7317ed14854f95e34e23c8ad0f6bff0324dc4def Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Wed, 17 May 2023 19:35:01 +0000 Subject: [PATCH 1/5] Implement //nolint parsing similar to golangci-lint Look for nolint comments and collect the ranges of where they apply. If a node immediately follows a range with the same column, expand the range to include the node. Add tests that verify errors are filtered out. --- go/tools/builders/BUILD.bazel | 10 +++ go/tools/builders/nogo_main.go | 82 +++++++++++++++++-- go/tools/builders/nolint.go | 52 ++++++++++++ go/tools/builders/nolint_test.go | 53 ++++++++++++ tests/core/nogo/nolint/BUILD.bazel | 6 ++ tests/core/nogo/nolint/README.rst | 14 ++++ tests/core/nogo/nolint/nolint_test.go | 112 ++++++++++++++++++++++++++ 7 files changed, 322 insertions(+), 7 deletions(-) create mode 100644 go/tools/builders/nolint.go create mode 100644 go/tools/builders/nolint_test.go create mode 100644 tests/core/nogo/nolint/BUILD.bazel create mode 100644 tests/core/nogo/nolint/README.rst create mode 100644 tests/core/nogo/nolint/nolint_test.go diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel index 5bbfbd67f6..28724714e8 100644 --- a/go/tools/builders/BUILD.bazel +++ b/go/tools/builders/BUILD.bazel @@ -37,6 +37,15 @@ go_test( rundir = ".", ) +go_test( + name = "nolint_test", + size = "small", + srcs = [ + "nolint.go", + "nolint_test.go", + ], +) + filegroup( name = "builder_srcs", srcs = [ @@ -82,6 +91,7 @@ go_source( "nogo_main.go", "nogo_typeparams_go117.go", "nogo_typeparams_go118.go", + "nolint.go", "pack.go", ], # //go/tools/builders:nogo_srcs is considered a different target by diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index b9423fb450..f04ca33027 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -202,6 +202,55 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil act.pkg = pkg } + // Process nolint directives similar to golangci-lint. + for _, f := range pkg.syntax { + // First, gather all comments and apply a range to the entire comment block. + // This will cover inline comments and allow us to detect comments above a + // node later. + for _, group := range f.Comments { + for _, comm := range group.List { + linters, ok := parseNolint(comm.Text) + if !ok { + continue + } + nl := &Range{ + from: pkg.fset.Position(group.Pos()), + to: pkg.fset.Position(group.End()).Line, + } + for analyzer, act := range actions { + if linters.Contains(analyzer.Name) { + act.nolint = append(act.nolint, nl) + } + } + } + } + + // For each node in the ast, check if the previous line is covered by a + // range for the linter and expand it. + ast.Inspect(f, func(n ast.Node) bool { + if n == nil { + return true + } + + nodeStart := pkg.fset.Position(n.Pos()) + nodeEnd := pkg.fset.Position(n.End()) + for _, act := range actions { + for _, rng := range act.nolint { + if rng.from.Filename != nodeStart.Filename { + continue + } + if rng.to != nodeStart.Line-1 || rng.from.Column != nodeStart.Column { + continue + } + // Expand the range to cover this node + rng.to = nodeEnd.Line + } + } + + return true + }) + } + // Execute the analyzers. execAll(roots) @@ -211,6 +260,11 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil return diagnostics, facts, nil } +type Range struct { + from token.Position + to int +} + // An action represents one unit of analysis work: the application of // one analysis to one package. Actions form a DAG within a // package (as different analyzers are applied, either in sequence or @@ -226,6 +280,7 @@ type action struct { diagnostics []analysis.Diagnostic usesFacts bool err error + nolint []*Range } func (act *action) String() string { @@ -279,13 +334,26 @@ func (act *action) execOnce() { factFilter[reflect.TypeOf(f)] = true } pass := &analysis.Pass{ - Analyzer: act.a, - Fset: act.pkg.fset, - Files: act.pkg.syntax, - Pkg: act.pkg.types, - TypesInfo: act.pkg.typesInfo, - ResultOf: inputs, - Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, + Analyzer: act.a, + Fset: act.pkg.fset, + Files: act.pkg.syntax, + Pkg: act.pkg.types, + TypesInfo: act.pkg.typesInfo, + ResultOf: inputs, + Report: func(d analysis.Diagnostic) { + pos := act.pkg.fset.Position(d.Pos) + for _, rng := range act.nolint { + if pos.Filename != rng.from.Filename { + continue + } + if pos.Line < rng.from.Line || pos.Line > rng.to { + continue + } + // Found a nolint range. Ignore the issue. + return + } + act.diagnostics = append(act.diagnostics, d) + }, ImportPackageFact: act.pkg.facts.ImportPackageFact, ExportPackageFact: act.pkg.facts.ExportPackageFact, ImportObjectFact: act.pkg.facts.ImportObjectFact, diff --git a/go/tools/builders/nolint.go b/go/tools/builders/nolint.go new file mode 100644 index 0000000000..d3225bce4f --- /dev/null +++ b/go/tools/builders/nolint.go @@ -0,0 +1,52 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import "strings" + +type Linters []string + +func (l Linters) Contains(s string) bool { + if len(l) == 0 { + return true + } + for _, name := range l { + if s == name { + return true + } + } + return false +} + +// From a comment like '//nolint:foo,bar' returns [foo, bar], true. If no +// comment is found, returns nil, false. For 'nolint:all' or 'nolint', returns +// nil, true. +func parseNolint(text string) (Linters, bool) { + text = strings.TrimLeft(text, "/ ") + if !strings.HasPrefix(text, "nolint") { + return nil, false + } + parts := strings.Split(text, ":") + if len(parts) == 1 { + return nil, true + } + linters := strings.Split(parts[1], ",") + for _, linter := range linters { + if strings.EqualFold(linter, "all") { + return nil, true + } + } + return Linters(linters), true +} diff --git a/go/tools/builders/nolint_test.go b/go/tools/builders/nolint_test.go new file mode 100644 index 0000000000..6e1f9e4b7b --- /dev/null +++ b/go/tools/builders/nolint_test.go @@ -0,0 +1,53 @@ +// Copyright 2023 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package main + +import ( + "reflect" + "testing" +) + +func TestParseNolint(t *testing.T) { + assert := func(text string, linters Linters, valid bool) { + result, ok := parseNolint(text) + if valid != ok { + t.Fatalf("parseNolint expect %t got %t", valid, ok) + } + if !reflect.DeepEqual(result, linters) { + t.Fatalf("parseNolint expect %v got %v", linters, result) + } + } + + assert("not a comment", nil, false) + assert("// comment", nil, false) + assert("//nolint", nil, true) + assert("//nolint:all", nil, true) + assert("// nolint:foo", Linters{"foo"}, true) + assert("// nolint:foo,bar,baz", Linters{"foo", "bar", "baz"}, true) +} + +func TestLintersContains(t *testing.T) { + var linters Linters + if !linters.Contains("any") { + t.Fatalf("expected nil to contain any linter") + } + linters = Linters{"bools"} + if linters.Contains("any") { + t.Fatalf("unexpected match of linter") + } + if !linters.Contains("bools") { + t.Fatalf("expected linters to match") + } +} diff --git a/tests/core/nogo/nolint/BUILD.bazel b/tests/core/nogo/nolint/BUILD.bazel new file mode 100644 index 0000000000..c828045c1e --- /dev/null +++ b/tests/core/nogo/nolint/BUILD.bazel @@ -0,0 +1,6 @@ +load("@io_bazel_rules_go//go/tools/bazel_testing:def.bzl", "go_bazel_test") + +go_bazel_test( + name = "nolint_test", + srcs = ["nolint_test.go"], +) diff --git a/tests/core/nogo/nolint/README.rst b/tests/core/nogo/nolint/README.rst new file mode 100644 index 0000000000..8343689838 --- /dev/null +++ b/tests/core/nogo/nolint/README.rst @@ -0,0 +1,14 @@ +Nolint check +========= + +.. _go_library: /docs/go/core/rules.md#_go_library + +Tests to ensure that errors found by nogo and annotated with //nolint are +ignored. + +.. contents:: + +nolint_test +-------- +Verified that errors emitted by ``nogo`` are ignored when `//nolint` appears as +a comment. diff --git a/tests/core/nogo/nolint/nolint_test.go b/tests/core/nogo/nolint/nolint_test.go new file mode 100644 index 0000000000..986484e065 --- /dev/null +++ b/tests/core/nogo/nolint/nolint_test.go @@ -0,0 +1,112 @@ +// Copyright 2019 The Bazel Authors. All rights reserved. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package nolint_test + +import ( + "bytes" + "io/ioutil" + "strings" + "testing" + + "github.com/bazelbuild/rules_go/go/tools/bazel_testing" +) + +func TestMain(m *testing.M) { + bazel_testing.TestMain(m, bazel_testing.Args{ + Main: ` +-- BUILD.bazel -- +load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_tool_library", "nogo") + +nogo( + name = "nogo", + vet = True, + visibility = ["//visibility:public"], +) + +go_library( + name = "has_errors", + srcs = ["has_errors.go"], + importpath = "haserrors", +) + +-- has_errors.go -- +package haserrors + +//nolint:buildtag +// +build build_tags_error + +import ( + "fmt" + "sync/atomic" +) + +func F() {} + +func Foo() bool { + x := uint64(1) + _ = atomic.AddUint64(&x, 1) + if F == nil { //nolint:all + return false + } + fmt.Printf("%b", "hi", true || true) //nolint:printf,bools + return true || true //nolint +} + +func InlineComment() { + s := "hello" // nolint + fmt.Printf("%d", s) +} + +func LinterMatch() bool { + return true || true //nolint:printf +} +`, + }) +} + +func Test(t *testing.T) { + customRegister := `go_register_toolchains(nogo = "@//:nogo")` + if err := replaceInFile("WORKSPACE", "go_register_toolchains()", customRegister); err != nil { + t.Fatal(err) + } + + cmd := bazel_testing.BazelCmd("build", "//:has_errors") + stderr := &bytes.Buffer{} + cmd.Stderr = stderr + if err := cmd.Run(); err == nil { + t.Fatal("unexpected success") + } + + expected := []string{ + "has_errors.go:25:2: fmt.Printf format %d has arg s of wrong type string (printf)", + "has_errors.go:29:9: redundant or: true || true (bools)", + } + + output := stderr.String() + for _, ex := range expected { + if !strings.Contains(output, ex) { + t.Errorf("output did not match expected: %s", ex) + } + } +} + +func replaceInFile(path, old, new string) error { + data, err := ioutil.ReadFile(path) + if err != nil { + return err + } + data = bytes.ReplaceAll(data, []byte(old), []byte(new)) + return ioutil.WriteFile(path, data, 0666) +} From 772261b847d46f75d60e62520093e4926910c857 Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Fri, 19 May 2023 15:09:35 +0000 Subject: [PATCH 2/5] Use a map instead of custom Linters type Inline the new report function and add some comments about nolint ranges. --- go/tools/builders/nogo_main.go | 60 ++++++++++++++++++++------------ go/tools/builders/nolint.go | 25 ++++--------- go/tools/builders/nolint_test.go | 28 ++++++--------- 3 files changed, 54 insertions(+), 59 deletions(-) diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index f04ca33027..c89484235a 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -218,7 +218,7 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil to: pkg.fset.Position(group.End()).Line, } for analyzer, act := range actions { - if linters.Contains(analyzer.Name) { + if linters == nil || linters[analyzer.Name] { act.nolint = append(act.nolint, nl) } } @@ -226,7 +226,15 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil } // For each node in the ast, check if the previous line is covered by a - // range for the linter and expand it. + // range for the linter and expand it. The idea is to find instances of + // + // //nolint + // someExpression(that{ + // may: split, + // multiple: lines, + // }) + // + // We should apply the range to the entire function call expression. ast.Inspect(f, func(n ast.Node) bool { if n == nil { return true @@ -243,7 +251,9 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil continue } // Expand the range to cover this node - rng.to = nodeEnd.Line + if nodeEnd.Line > rng.to { + rng.to = nodeEnd.Line + } } } @@ -328,32 +338,36 @@ func (act *action) execOnce() { inputs[dep.a] = dep.result } + ignoreNolintReporter := func(d analysis.Diagnostic) { + pos := act.pkg.fset.Position(d.Pos) + for _, rng := range act.nolint { + // The list of nolint ranges is built for the entire package. Make sure we + // only apply ranges to the correct file. + if pos.Filename != rng.from.Filename { + continue + } + if pos.Line < rng.from.Line || pos.Line > rng.to { + continue + } + // Found a nolint range. Ignore the issue. + return + } + act.diagnostics = append(act.diagnostics, d) + } + // Run the analysis. factFilter := make(map[reflect.Type]bool) for _, f := range act.a.FactTypes { factFilter[reflect.TypeOf(f)] = true } pass := &analysis.Pass{ - Analyzer: act.a, - Fset: act.pkg.fset, - Files: act.pkg.syntax, - Pkg: act.pkg.types, - TypesInfo: act.pkg.typesInfo, - ResultOf: inputs, - Report: func(d analysis.Diagnostic) { - pos := act.pkg.fset.Position(d.Pos) - for _, rng := range act.nolint { - if pos.Filename != rng.from.Filename { - continue - } - if pos.Line < rng.from.Line || pos.Line > rng.to { - continue - } - // Found a nolint range. Ignore the issue. - return - } - act.diagnostics = append(act.diagnostics, d) - }, + Analyzer: act.a, + Fset: act.pkg.fset, + Files: act.pkg.syntax, + Pkg: act.pkg.types, + TypesInfo: act.pkg.typesInfo, + ResultOf: inputs, + Report: ignoreNolintReporter, ImportPackageFact: act.pkg.facts.ImportPackageFact, ExportPackageFact: act.pkg.facts.ExportPackageFact, ImportObjectFact: act.pkg.facts.ImportObjectFact, diff --git a/go/tools/builders/nolint.go b/go/tools/builders/nolint.go index d3225bce4f..e6e3c0438a 100644 --- a/go/tools/builders/nolint.go +++ b/go/tools/builders/nolint.go @@ -16,24 +16,9 @@ package main import "strings" -type Linters []string - -func (l Linters) Contains(s string) bool { - if len(l) == 0 { - return true - } - for _, name := range l { - if s == name { - return true - } - } - return false -} - -// From a comment like '//nolint:foo,bar' returns [foo, bar], true. If no -// comment is found, returns nil, false. For 'nolint:all' or 'nolint', returns -// nil, true. -func parseNolint(text string) (Linters, bool) { +// Parse nolint directives and return the applicable linters. If all linters +// apply, returns (nil, true). +func parseNolint(text string) (map[string]bool, bool) { text = strings.TrimLeft(text, "/ ") if !strings.HasPrefix(text, "nolint") { return nil, false @@ -43,10 +28,12 @@ func parseNolint(text string) (Linters, bool) { return nil, true } linters := strings.Split(parts[1], ",") + result := map[string]bool{} for _, linter := range linters { if strings.EqualFold(linter, "all") { return nil, true } + result[linter] = true } - return Linters(linters), true + return result, true } diff --git a/go/tools/builders/nolint_test.go b/go/tools/builders/nolint_test.go index 6e1f9e4b7b..c951ac8149 100644 --- a/go/tools/builders/nolint_test.go +++ b/go/tools/builders/nolint_test.go @@ -20,7 +20,7 @@ import ( ) func TestParseNolint(t *testing.T) { - assert := func(text string, linters Linters, valid bool) { + assert := func(text string, linters map[string]bool, valid bool) { result, ok := parseNolint(text) if valid != ok { t.Fatalf("parseNolint expect %t got %t", valid, ok) @@ -34,20 +34,14 @@ func TestParseNolint(t *testing.T) { assert("// comment", nil, false) assert("//nolint", nil, true) assert("//nolint:all", nil, true) - assert("// nolint:foo", Linters{"foo"}, true) - assert("// nolint:foo,bar,baz", Linters{"foo", "bar", "baz"}, true) -} - -func TestLintersContains(t *testing.T) { - var linters Linters - if !linters.Contains("any") { - t.Fatalf("expected nil to contain any linter") - } - linters = Linters{"bools"} - if linters.Contains("any") { - t.Fatalf("unexpected match of linter") - } - if !linters.Contains("bools") { - t.Fatalf("expected linters to match") - } + assert("// nolint:foo", map[string]bool{"foo": true}, true) + assert( + "// nolint:foo,bar,baz", + map[string]bool{ + "foo": true, + "bar": true, + "baz": true, + }, + true, + ) } From da7819e6b765394ebaf26dc2c082145821c2c06e Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Fri, 19 May 2023 15:42:53 +0000 Subject: [PATCH 3/5] Add separate tests for various filters Include a failing test that shows the column issue. Will work on a fix. --- tests/core/nogo/nolint/nolint_test.go | 145 +++++++++++++++++++------- 1 file changed, 105 insertions(+), 40 deletions(-) diff --git a/tests/core/nogo/nolint/nolint_test.go b/tests/core/nogo/nolint/nolint_test.go index 986484e065..ba6bad34b1 100644 --- a/tests/core/nogo/nolint/nolint_test.go +++ b/tests/core/nogo/nolint/nolint_test.go @@ -32,74 +32,139 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_tool_library", "nogo") nogo( name = "nogo", vet = True, + deps = ["@org_golang_x_tools//go/analysis/passes/nilness"], visibility = ["//visibility:public"], ) go_library( - name = "has_errors", - srcs = ["has_errors.go"], - importpath = "haserrors", + name = "inline", + srcs = ["inline.go"], + importpath = "inline", ) --- has_errors.go -- -package haserrors +go_library( + name = "inline-filter", + srcs = ["inline_filter.go"], + importpath = "inlinefilter", +) -//nolint:buildtag -// +build build_tags_error +go_library( + name = "block", + srcs = ["block.go"], + importpath = "block", +) -import ( - "fmt" - "sync/atomic" +go_library( + name = "block-multiline", + srcs = ["block_multiline.go"], + importpath = "blockmultiline", ) -func F() {} +go_library( + name = "inline-errors", + srcs = ["inline_errors.go"], + importpath = "inlineerrors", +) -func Foo() bool { - x := uint64(1) - _ = atomic.AddUint64(&x, 1) - if F == nil { //nolint:all - return false - } - fmt.Printf("%b", "hi", true || true) //nolint:printf,bools - return true || true //nolint +go_library( + name = "inline-column", + srcs = ["inline_column.go"], + importpath = "inlinecolumn", +) +-- inline.go -- +package inline + +func F() { + s := "hello" + fmt.Printf("%d", s) //nolint +} + +-- inline_filter.go -- +package inlinefilter + +func F() bool { + return true || true //nolint:bools +} + +-- block.go -- +package block + +import "fmt" + +func F() { + //nolint + fmt.Printf("%d", "hello") +} + +-- block_multiline.go -- +package blockmultiline + +func F() bool { + var i *int + //nolint + return true && + i != nil } -func InlineComment() { - s := "hello" // nolint - fmt.Printf("%d", s) +-- inline_errors.go -- +package inlineerrors + +import "fmt" + +func F() { + var i *int + if i == nil { + fmt.Printf("%d", "hello") //nolint + fmt.Println(*i) // Keep nil deref error + } } -func LinterMatch() bool { - return true || true //nolint:printf +-- inline_column.go -- +package inlinecolumn + +import "fmt" + +func F() { + // Purposely used 'helo' to align the column + fmt.Printf("%d", "helo") //nolint + superLongVariableName := true || true + var _ = superLongVariableName } `, }) } -func Test(t *testing.T) { - customRegister := `go_register_toolchains(nogo = "@//:nogo")` - if err := replaceInFile("WORKSPACE", "go_register_toolchains()", customRegister); err != nil { - t.Fatal(err) - } +func runTest(t *testing.T, target string, expected string) { + t.Helper() - cmd := bazel_testing.BazelCmd("build", "//:has_errors") + cmd := bazel_testing.BazelCmd("build", target) stderr := &bytes.Buffer{} cmd.Stderr = stderr - if err := cmd.Run(); err == nil { + err := cmd.Run() + if expected != "" && err == nil { t.Fatal("unexpected success") } - - expected := []string{ - "has_errors.go:25:2: fmt.Printf format %d has arg s of wrong type string (printf)", - "has_errors.go:29:9: redundant or: true || true (bools)", + output := stderr.String() + if !strings.Contains(output, expected) { + t.Errorf("output did not contain expected: %s\n%s", expected, output) } +} - output := stderr.String() - for _, ex := range expected { - if !strings.Contains(output, ex) { - t.Errorf("output did not match expected: %s", ex) - } +func Test(t *testing.T) { + customRegister := `go_register_toolchains(nogo = "@//:nogo")` + if err := replaceInFile("WORKSPACE", "go_register_toolchains()", customRegister); err != nil { + t.Fatal(err) } + + // Execute tests that should filter out errors + runTest(t, "//:inline", "") + runTest(t, "//:inline-filter", "") + runTest(t, "//:block", "") + runTest(t, "//:block-multiline", "") + + // Execute tests that should return unfiltered errors + runTest(t, "//:inline-errors", "inline_errors.go:9:15: nil dereference in load (nilness)") + runTest(t, "//:inline-column", "inline_column.go:8:27: redundant or: true || true (bools)") } func replaceInFile(path, old, new string) error { From 8dfd83e0a51add9c330e279338390bdc24439484 Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Fri, 19 May 2023 16:46:17 +0000 Subject: [PATCH 4/5] Use ast.CommentMap to associate comments to nodes This is better than the logic used by golangci-lint in that it does not incorrectly attribute an inline comment with the next line if it contains an ast.Node with a matching column (see inline_column test). --- go/tools/builders/nogo_main.go | 66 +++++++------------------ tests/core/nogo/nolint/nolint_test.go | 69 ++++++++++++++++++--------- 2 files changed, 64 insertions(+), 71 deletions(-) diff --git a/go/tools/builders/nogo_main.go b/go/tools/builders/nogo_main.go index c89484235a..c6156e1dab 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -204,61 +204,29 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil // Process nolint directives similar to golangci-lint. for _, f := range pkg.syntax { - // First, gather all comments and apply a range to the entire comment block. - // This will cover inline comments and allow us to detect comments above a - // node later. - for _, group := range f.Comments { - for _, comm := range group.List { - linters, ok := parseNolint(comm.Text) - if !ok { - continue - } - nl := &Range{ - from: pkg.fset.Position(group.Pos()), - to: pkg.fset.Position(group.End()).Line, - } - for analyzer, act := range actions { - if linters == nil || linters[analyzer.Name] { - act.nolint = append(act.nolint, nl) - } - } - } - } - - // For each node in the ast, check if the previous line is covered by a - // range for the linter and expand it. The idea is to find instances of - // - // //nolint - // someExpression(that{ - // may: split, - // multiple: lines, - // }) - // - // We should apply the range to the entire function call expression. - ast.Inspect(f, func(n ast.Node) bool { - if n == nil { - return true + // CommentMap will correctly associate comments to the largest node group + // applicable. This handles inline comments that might trail a large + // assignment and will apply the comment to the entire assignment. + commentMap := ast.NewCommentMap(pkg.fset, f, f.Comments) + for node, groups := range commentMap { + rng := &Range{ + from: pkg.fset.Position(node.Pos()), + to: pkg.fset.Position(node.End()).Line, } - - nodeStart := pkg.fset.Position(n.Pos()) - nodeEnd := pkg.fset.Position(n.End()) - for _, act := range actions { - for _, rng := range act.nolint { - if rng.from.Filename != nodeStart.Filename { - continue - } - if rng.to != nodeStart.Line-1 || rng.from.Column != nodeStart.Column { + for _, group := range groups { + for _, comm := range group.List { + linters, ok := parseNolint(comm.Text) + if !ok { continue } - // Expand the range to cover this node - if nodeEnd.Line > rng.to { - rng.to = nodeEnd.Line + for analyzer, act := range actions { + if linters == nil || linters[analyzer.Name] { + act.nolint = append(act.nolint, rng) + } } } } - - return true - }) + } } // Execute the analyzers. diff --git a/tests/core/nogo/nolint/nolint_test.go b/tests/core/nogo/nolint/nolint_test.go index ba6bad34b1..6e15637709 100644 --- a/tests/core/nogo/nolint/nolint_test.go +++ b/tests/core/nogo/nolint/nolint_test.go @@ -39,40 +39,48 @@ nogo( go_library( name = "inline", srcs = ["inline.go"], - importpath = "inline", + importpath = "test", ) go_library( - name = "inline-filter", + name = "inline_filter", srcs = ["inline_filter.go"], - importpath = "inlinefilter", + importpath = "test", ) go_library( name = "block", srcs = ["block.go"], - importpath = "block", + importpath = "test", ) go_library( - name = "block-multiline", + name = "block_multiline", srcs = ["block_multiline.go"], - importpath = "blockmultiline", + importpath = "test", ) go_library( - name = "inline-errors", + name = "inline_errors", srcs = ["inline_errors.go"], - importpath = "inlineerrors", + importpath = "test", ) go_library( - name = "inline-column", + name = "inline_column", srcs = ["inline_column.go"], - importpath = "inlinecolumn", + importpath = "test", +) + +go_library( + name = "large_block", + srcs = ["large_block.go"], + importpath = "test", ) -- inline.go -- -package inline +package test + +import "fmt" func F() { s := "hello" @@ -80,14 +88,14 @@ func F() { } -- inline_filter.go -- -package inlinefilter +package test func F() bool { return true || true //nolint:bools } -- block.go -- -package block +package test import "fmt" @@ -97,7 +105,7 @@ func F() { } -- block_multiline.go -- -package blockmultiline +package test func F() bool { var i *int @@ -107,7 +115,7 @@ func F() bool { } -- inline_errors.go -- -package inlineerrors +package test import "fmt" @@ -120,7 +128,7 @@ func F() { } -- inline_column.go -- -package inlinecolumn +package test import "fmt" @@ -130,6 +138,19 @@ func F() { superLongVariableName := true || true var _ = superLongVariableName } + +-- large_block.go -- +package test + +import "fmt" + +var V = struct { + S string + B bool +} { + S: fmt.Sprintf("%d", "hello"), //nolint + B: true || true, +} `, }) } @@ -141,10 +162,13 @@ func runTest(t *testing.T, target string, expected string) { stderr := &bytes.Buffer{} cmd.Stderr = stderr err := cmd.Run() + output := stderr.String() if expected != "" && err == nil { - t.Fatal("unexpected success") + t.Fatal("unexpected success", output) + } + if expected == "" && err != nil { + t.Fatal("unexpected failure", output) } - output := stderr.String() if !strings.Contains(output, expected) { t.Errorf("output did not contain expected: %s\n%s", expected, output) } @@ -158,13 +182,14 @@ func Test(t *testing.T) { // Execute tests that should filter out errors runTest(t, "//:inline", "") - runTest(t, "//:inline-filter", "") + runTest(t, "//:inline_filter", "") runTest(t, "//:block", "") - runTest(t, "//:block-multiline", "") + runTest(t, "//:block_multiline", "") // Execute tests that should return unfiltered errors - runTest(t, "//:inline-errors", "inline_errors.go:9:15: nil dereference in load (nilness)") - runTest(t, "//:inline-column", "inline_column.go:8:27: redundant or: true || true (bools)") + runTest(t, "//:inline_errors", "inline_errors.go:9:15: nil dereference in load (nilness)") + runTest(t, "//:inline_column", "inline_column.go:8:27: redundant or: true || true (bools)") + runTest(t, "//:large_block", "large_block.go:10:5: redundant or: true || true (bools)") } func replaceInFile(path, old, new string) error { From a66f2f3d1502e817c23e3e4d7bdd7044d7e675b0 Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Mon, 5 Jun 2023 16:55:37 +0000 Subject: [PATCH 5/5] Address PR feedback Use table driven tests and `CombinedOutput`. --- go/tools/builders/nolint_test.go | 76 +++++++++++++++++-------- tests/core/nogo/nolint/nolint_test.go | 81 ++++++++++++++++++--------- 2 files changed, 107 insertions(+), 50 deletions(-) diff --git a/go/tools/builders/nolint_test.go b/go/tools/builders/nolint_test.go index c951ac8149..2870eaaff6 100644 --- a/go/tools/builders/nolint_test.go +++ b/go/tools/builders/nolint_test.go @@ -20,28 +20,60 @@ import ( ) func TestParseNolint(t *testing.T) { - assert := func(text string, linters map[string]bool, valid bool) { - result, ok := parseNolint(text) - if valid != ok { - t.Fatalf("parseNolint expect %t got %t", valid, ok) - } - if !reflect.DeepEqual(result, linters) { - t.Fatalf("parseNolint expect %v got %v", linters, result) - } + tests := []struct { + Name string + Comment string + Valid bool + Linters []string + }{ + { + Name: "Invalid", + Comment: "not a comment", + }, + { + Name: "No match", + Comment: "// comment", + }, + { + Name: "All linters", + Comment: "//nolint", + Valid: true, + }, + { + Name: "All linters (explicit)", + Comment: "//nolint:all", + Valid: true, + }, + { + Name: "Single linter", + Comment: "// nolint:foo", + Valid: true, + Linters: []string{"foo"}, + }, + { + Name: "Multiple linters", + Comment: "// nolint:a,b,c", + Valid: true, + Linters: []string{"a", "b", "c"}, + }, } - assert("not a comment", nil, false) - assert("// comment", nil, false) - assert("//nolint", nil, true) - assert("//nolint:all", nil, true) - assert("// nolint:foo", map[string]bool{"foo": true}, true) - assert( - "// nolint:foo,bar,baz", - map[string]bool{ - "foo": true, - "bar": true, - "baz": true, - }, - true, - ) + for _, tc := range tests { + t.Run(tc.Name, func(t *testing.T) { + result, ok := parseNolint(tc.Comment) + if tc.Valid != ok { + t.Fatalf("parseNolint expect %t got %t", tc.Valid, ok) + } + var linters map[string]bool + if len(tc.Linters) != 0 { + linters = make(map[string]bool) + for _, l := range tc.Linters { + linters[l] = true + } + } + if !reflect.DeepEqual(result, linters) { + t.Fatalf("parseNolint expect %v got %v", linters, result) + } + }) + } } diff --git a/tests/core/nogo/nolint/nolint_test.go b/tests/core/nogo/nolint/nolint_test.go index 6e15637709..78ecbbcf38 100644 --- a/tests/core/nogo/nolint/nolint_test.go +++ b/tests/core/nogo/nolint/nolint_test.go @@ -155,41 +155,66 @@ var V = struct { }) } -func runTest(t *testing.T, target string, expected string) { - t.Helper() - - cmd := bazel_testing.BazelCmd("build", target) - stderr := &bytes.Buffer{} - cmd.Stderr = stderr - err := cmd.Run() - output := stderr.String() - if expected != "" && err == nil { - t.Fatal("unexpected success", output) - } - if expected == "" && err != nil { - t.Fatal("unexpected failure", output) - } - if !strings.Contains(output, expected) { - t.Errorf("output did not contain expected: %s\n%s", expected, output) - } -} - func Test(t *testing.T) { customRegister := `go_register_toolchains(nogo = "@//:nogo")` if err := replaceInFile("WORKSPACE", "go_register_toolchains()", customRegister); err != nil { t.Fatal(err) } - // Execute tests that should filter out errors - runTest(t, "//:inline", "") - runTest(t, "//:inline_filter", "") - runTest(t, "//:block", "") - runTest(t, "//:block_multiline", "") + tests := []struct { + Name string + Target string + Expected string + }{ + { + Name: "Inline comment", + Target: "//:inline", + }, + { + Name: "Inline with lint filter", + Target: "//:inline_filter", + }, + { + Name: "Block comment", + Target: "//:block", + }, + { + Name: "Multiline block comment", + Target: "//:block_multiline", + }, + { + Name: "Inline with errors", + Target: "//:inline_errors", + Expected: "inline_errors.go:9:15: nil dereference in load (nilness)", + }, + { + Name: "Inline comment on same column does not apply", + Target: "//:inline_column", + Expected: "inline_column.go:8:27: redundant or: true || true (bools)", + }, + { + Name: "Inline comment does not apply to larger block", + Target: "//:large_block", + Expected: "large_block.go:10:5: redundant or: true || true (bools)", + }, + } - // Execute tests that should return unfiltered errors - runTest(t, "//:inline_errors", "inline_errors.go:9:15: nil dereference in load (nilness)") - runTest(t, "//:inline_column", "inline_column.go:8:27: redundant or: true || true (bools)") - runTest(t, "//:large_block", "large_block.go:10:5: redundant or: true || true (bools)") + for _, tc := range tests { + t.Run(tc.Name, func(t *testing.T) { + cmd := bazel_testing.BazelCmd("build", tc.Target) + b, err := cmd.CombinedOutput() + output := string(b) + if tc.Expected != "" && err == nil { + t.Fatal("unexpected success", output) + } + if tc.Expected == "" && err != nil { + t.Fatal("unexpected failure", output) + } + if !strings.Contains(output, tc.Expected) { + t.Errorf("output did not contain expected: %s\n%s", tc.Expected, output) + } + }) + } } func replaceInFile(path, old, new string) error {