Skip to content

Commit

Permalink
Implement //nolint parsing similar to golangci-lint
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
obs-gh-patrickscott committed May 18, 2023
1 parent c403db6 commit a7a4d63
Show file tree
Hide file tree
Showing 7 changed files with 322 additions and 7 deletions.
10 changes: 10 additions & 0 deletions go/tools/builders/BUILD.bazel
Original file line number Diff line number Diff line change
Expand Up @@ -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 = [
Expand Down Expand Up @@ -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
Expand Down
82 changes: 75 additions & 7 deletions go/tools/builders/nogo_main.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand All @@ -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
Expand All @@ -226,6 +280,7 @@ type action struct {
diagnostics []analysis.Diagnostic
usesFacts bool
err error
nolint []*Range
}

func (act *action) String() string {
Expand Down Expand Up @@ -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,
Expand Down
52 changes: 52 additions & 0 deletions go/tools/builders/nolint.go
Original file line number Diff line number Diff line change
@@ -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
}
53 changes: 53 additions & 0 deletions go/tools/builders/nolint_test.go
Original file line number Diff line number Diff line change
@@ -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")
}
}
6 changes: 6 additions & 0 deletions tests/core/nogo/nolint/BUILD.bazel
Original file line number Diff line number Diff line change
@@ -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"],
)
14 changes: 14 additions & 0 deletions tests/core/nogo/nolint/README.rst
Original file line number Diff line number Diff line change
@@ -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.
112 changes: 112 additions & 0 deletions tests/core/nogo/nolint/nolint_test.go
Original file line number Diff line number Diff line change
@@ -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)
}

0 comments on commit a7a4d63

Please sign in to comment.