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 <patrick.scott@observeinc.com>

7 files changed, 426 insertions, 1 deletions

diff --git a/go/tools/builders/BUILD.bazel b/go/tools/builders/BUILD.bazel
index 5bbfbd67..28724714 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 b9423fb4..c6156e1d 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 00000000..e6e3c043
--- /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 00000000..2870eaaf
--- /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 00000000..c828045c
--- /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 00000000..83436898
--- /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 00000000..78ecbbcf
--- /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)
+}