From 98f6d333c286e02dd513bfb221faa2ce3ece8936 Mon Sep 17 00:00:00 2001 From: Patrick Scott Date: Wed, 7 Jun 2023 14:02:48 -0400 Subject: [PATCH] Implement //nolint parsing similar to golangci-lint (#3562) * 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. * Use a map instead of custom Linters type Inline the new report function and add some comments about nolint ranges. * Add separate tests for various filters Include a failing test that shows the column issue. Will work on a fix. * 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). * Address PR feedback Use table driven tests and `CombinedOutput`. --------- Co-authored-by: Patrick Scott --- go/tools/builders/BUILD.bazel | 10 ++ go/tools/builders/nogo_main.go | 52 +++++- go/tools/builders/nolint.go | 39 +++++ go/tools/builders/nolint_test.go | 79 +++++++++ tests/core/nogo/nolint/BUILD.bazel | 6 + tests/core/nogo/nolint/README.rst | 14 ++ tests/core/nogo/nolint/nolint_test.go | 227 ++++++++++++++++++++++++++ 7 files changed, 426 insertions(+), 1 deletion(-) 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..c6156e1dab 100644 --- a/go/tools/builders/nogo_main.go +++ b/go/tools/builders/nogo_main.go @@ -202,6 +202,33 @@ func checkPackage(analyzers []*analysis.Analyzer, packagePath string, packageFil act.pkg = pkg } + // Process nolint directives similar to golangci-lint. + for _, f := range pkg.syntax { + // 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, + } + for _, group := range groups { + for _, comm := range group.List { + linters, ok := parseNolint(comm.Text) + if !ok { + continue + } + for analyzer, act := range actions { + if linters == nil || linters[analyzer.Name] { + act.nolint = append(act.nolint, rng) + } + } + } + } + } + } + // Execute the analyzers. execAll(roots) @@ -211,6 +238,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 +258,7 @@ type action struct { diagnostics []analysis.Diagnostic usesFacts bool err error + nolint []*Range } func (act *action) String() string { @@ -273,6 +306,23 @@ 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 { @@ -285,7 +335,7 @@ func (act *action) execOnce() { Pkg: act.pkg.types, TypesInfo: act.pkg.typesInfo, ResultOf: inputs, - Report: func(d analysis.Diagnostic) { act.diagnostics = append(act.diagnostics, d) }, + 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 new file mode 100644 index 0000000000..e6e3c0438a --- /dev/null +++ b/go/tools/builders/nolint.go @@ -0,0 +1,39 @@ +// 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" + +// 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 + } + parts := strings.Split(text, ":") + if len(parts) == 1 { + 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 result, true +} diff --git a/go/tools/builders/nolint_test.go b/go/tools/builders/nolint_test.go new file mode 100644 index 0000000000..2870eaaff6 --- /dev/null +++ b/go/tools/builders/nolint_test.go @@ -0,0 +1,79 @@ +// 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) { + 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"}, + }, + } + + 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/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..78ecbbcf38 --- /dev/null +++ b/tests/core/nogo/nolint/nolint_test.go @@ -0,0 +1,227 @@ +// 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, + deps = ["@org_golang_x_tools//go/analysis/passes/nilness"], + visibility = ["//visibility:public"], +) + +go_library( + name = "inline", + srcs = ["inline.go"], + importpath = "test", +) + +go_library( + name = "inline_filter", + srcs = ["inline_filter.go"], + importpath = "test", +) + +go_library( + name = "block", + srcs = ["block.go"], + importpath = "test", +) + +go_library( + name = "block_multiline", + srcs = ["block_multiline.go"], + importpath = "test", +) + +go_library( + name = "inline_errors", + srcs = ["inline_errors.go"], + importpath = "test", +) + +go_library( + name = "inline_column", + srcs = ["inline_column.go"], + importpath = "test", +) + +go_library( + name = "large_block", + srcs = ["large_block.go"], + importpath = "test", +) +-- inline.go -- +package test + +import "fmt" + +func F() { + s := "hello" + fmt.Printf("%d", s) //nolint +} + +-- inline_filter.go -- +package test + +func F() bool { + return true || true //nolint:bools +} + +-- block.go -- +package test + +import "fmt" + +func F() { + //nolint + fmt.Printf("%d", "hello") +} + +-- block_multiline.go -- +package test + +func F() bool { + var i *int + //nolint + return true && + i != nil +} + +-- inline_errors.go -- +package test + +import "fmt" + +func F() { + var i *int + if i == nil { + fmt.Printf("%d", "hello") //nolint + fmt.Println(*i) // Keep nil deref error + } +} + +-- inline_column.go -- +package test + +import "fmt" + +func F() { + // Purposely used 'helo' to align the column + fmt.Printf("%d", "helo") //nolint + 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, +} +`, + }) +} + +func Test(t *testing.T) { + customRegister := `go_register_toolchains(nogo = "@//:nogo")` + if err := replaceInFile("WORKSPACE", "go_register_toolchains()", customRegister); err != nil { + t.Fatal(err) + } + + 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)", + }, + } + + 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 { + data, err := ioutil.ReadFile(path) + if err != nil { + return err + } + data = bytes.ReplaceAll(data, []byte(old), []byte(new)) + return ioutil.WriteFile(path, data, 0666) +}