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) +}