Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,12 @@ The full pattern struct has the following fields:
* `package`: a regular expression for the full package import path. The package
path includes the package version if the package has a version >= 2. This is
only supported when `analyze_types` is enabled.
* `ignore`: a list of [file glob
strings](https://github.com/bmatcuk/doublestar#patterns). If a glob string
matches `<package>/<file name>`, the pattern is ignored for the file that is
being analyzed. A glob string that starts with `!` reverts that. All glob
strings are checked one-by-one and the end result is then used to decide
whether the pattern applies.

To distinguish such patterns from traditional regular expression patterns, the
encoding must start with a `{` or contain line breaks. When using just JSON
Expand All @@ -121,6 +127,7 @@ isn't necessary. The following pattern strings are equivalent:
A larger set of interesting patterns might include:

-* `^fmt\.Print.*$` -- forbid use of Print statements because they are likely just for debugging
-* `{pattern: ^fmt\.Print.*$ ignore: ["**/main.go"]}` -- forbid use of Print statements in all files except main.go
-* `^fmt\.Errorf$` -- forbid Errorf in favor of using github.com/pkg/errors
-* `^ginkgo\.F[A-Z].*$` -- forbid ginkgo focused commands (used for debug issues)
-* `^spew\.Dump$` -- forbid dumping detailed data to stdout
Expand Down
14 changes: 13 additions & 1 deletion forbidigo/forbidigo.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import (
"go/token"
"go/types"
"log"
"path"
"regexp"
"strings"

Expand Down Expand Up @@ -119,6 +120,10 @@ type RunConfig struct {
// Nil disables that step, i.e. patterns match the literal source code.
TypesInfo *types.Info

// Pkg is needed for "ignore" glob patterns. Not providing it
// causes file globs to be ignore.
Pkg *types.Package

// DebugLog is used to print debug messages. May be nil.
DebugLog func(format string, args ...interface{})
}
Expand Down Expand Up @@ -200,10 +205,17 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor {
// use that. It's used for matching unless usage of type information
// is enabled.
srcText := v.textFor(node)
filename := ""
if v.runConfig.Pkg != nil {
pkgPath := v.runConfig.Pkg.Path()
pos := v.runConfig.Fset.Position(node.Pos())
filename = pkgPath + "/" + path.Base(pos.Filename)
}
matchTexts, pkgText := v.expandMatchText(node, srcText)
v.runConfig.DebugLog("%s: match %v, package %q", v.runConfig.Fset.Position(node.Pos()), matchTexts, pkgText)
v.runConfig.DebugLog("%s: match %v, package %q, filename %q", v.runConfig.Fset.Position(node.Pos()), matchTexts, pkgText, filename)
for _, p := range v.linter.patterns {
if p.matches(matchTexts) &&
(filename == "" || !p.ignoreFile(filename)) &&
(p.Package == "" || p.pkgRe.MatchString(pkgText)) &&
!v.permit(node) {
v.issues = append(v.issues, UsedIssue{
Expand Down
30 changes: 29 additions & 1 deletion forbidigo/forbidigo_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,34 @@ func TestFoo() {
fmt.Printf("here i am")
}

func Example() {
}`)
assert.NotEmpty(t, issues)
})

t.Run("Skip file based on glob", func(t *testing.T) {
linter, _ := NewLinter([]string{`{p: fmt\.Printf, ignore: ["**/file.go"]}`})
issues := parseFile(t, linter, false, "file.go", `
package bar

func TestFoo() {
fmt.Printf("here i am")
}

func Example() {
}`)
assert.Empty(t, issues)
})

t.Run("Check file based on glob", func(t *testing.T) {
linter, _ := NewLinter([]string{`{p: fmt\.Printf, ignore: ["!**/file.go"]}`})
issues := parseFile(t, linter, false, "file.go", `
package bar

func TestFoo() {
fmt.Printf("here i am")
}

func Example() {
}`)
assert.NotEmpty(t, issues)
Expand Down Expand Up @@ -230,7 +258,7 @@ func parseFile(t *testing.T, linter *Linter, expand bool, fileName, contents str
for _, n := range p.Syntax {
nodes = append(nodes, n)
}
newIssues, err := linter.RunWithConfig(RunConfig{Fset: p.Fset, TypesInfo: p.TypesInfo, DebugLog: t.Logf}, nodes...)
newIssues, err := linter.RunWithConfig(RunConfig{Fset: p.Fset, TypesInfo: p.TypesInfo, DebugLog: t.Logf, Pkg: p.Types}, nodes...)
if err != nil {
t.Fatalf("failed: %s", err)
}
Expand Down
47 changes: 47 additions & 0 deletions forbidigo/patterns.go
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,13 @@ import (
"strings"

"gopkg.in/yaml.v2"

// The standard library does not support ** for matching slashes,
// something that we need to support matching any file. doublestar was
// mentioned in
// https://github.com/golang/go/issues/11862#issuecomment-1207510648 as
// an alternative.
"github.com/bmatcuk/doublestar/v4"
)

// pattern matches code that is not supposed to be used.
Expand All @@ -26,6 +33,14 @@ type pattern struct {
// Msg gets printed in addition to the normal message if a match is
// found.
Msg string `yaml:"msg,omitempty"`

// Ignore determines which source code files this pattern applies to.
// If a glob string matches `<package>/<file name>`, the pattern is
// ignored for the file that is being analyzed. A glob string that
// starts with `!` reverts that. All glob string are checked one-by-one
// and the end result is then used to decide whether the pattern
// applies.
Ignore []string `yaml:"ignore,omitempty"`
}

// A yamlPattern pattern in a YAML string may be represented either by a string
Expand Down Expand Up @@ -97,6 +112,12 @@ func (p *pattern) validate() error {
p.pkgRe = pkgRe
}

for i, glob := range p.Ignore {
if !doublestar.ValidatePattern(glob) {
return fmt.Errorf("file glob pattern #%d is invalid: %q", i, glob)
}
}

return nil
}

Expand Down Expand Up @@ -125,3 +146,29 @@ func extractComment(re *syntax.Regexp) string {
}
return ""
}

func (p *pattern) ignoreFile(filename string) bool {
ignore := false
for _, glob := range p.Ignore {
if strings.HasPrefix(glob, "!") {
if !ignore {
// No need to match, nothing would change.
continue
}
// The glob was validated, matching cannot fail.
if ok, _ := doublestar.Match(glob[1:], filename); ok {
ignore = false
}
} else {
if ignore {
// No need to match, nothing would change.
continue
}
// The glob was validated, matching cannot fail.
if ok, _ := doublestar.Match(glob, filename); ok {
ignore = true
}
}
}
return ignore
}
74 changes: 74 additions & 0 deletions forbidigo/patterns_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package forbidigo

import (
"strings"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -15,6 +16,7 @@ func TestParseValidPatterns(t *testing.T) {
expectedComment string
expectedPattern string
expectedPackage string
expectedFile []string
}{
{
name: "simple expression, no comment",
Expand Down Expand Up @@ -68,6 +70,12 @@ p: ^fmt\.Println$
`,
expectedPattern: `^fmt\.Println$`,
},
{
name: "match import with YAML and file filter",
ptrn: `{p: ^fmt\.Println$, ignore: ["**", "!**/main.go"]}`,
expectedPattern: `^fmt\.Println$`,
expectedFile: []string{`**`, `!**/main.go`},
},
} {
t.Run(tc.name, func(t *testing.T) {
ptrn, err := parse(tc.ptrn)
Expand Down Expand Up @@ -156,3 +164,69 @@ func TestUnmarshalYAML(t *testing.T) {
})
}
}

func TestGlob(t *testing.T) {
testcases := map[string]struct {
ignore []string
expectError bool
expectIgnoreFile map[string]bool
}{
"disable": {
ignore: []string{"main.go"},
expectIgnoreFile: map[string]bool{
"main.go": true,
"hello.go": false,
},
},
"enable": {
ignore: []string{"*", "!main.go"},
expectIgnoreFile: map[string]bool{
"main.go": false,
"hello.go": true,
},
},
"all": {
ignore: []string{},
expectIgnoreFile: map[string]bool{
"main.go": false,
"hello.go": false,
},
},
"double": {
ignore: []string{"**/main.go"},
expectIgnoreFile: map[string]bool{
"example.com/hello/main.go": true,
},
},
}

for name, tc := range testcases {
t.Run(name, func(t *testing.T) {
p := pattern{
Pattern: "unused",
Ignore: tc.ignore,
}
if err := p.validate(); err != nil {
if !tc.expectError {
t.Fatalf("unexpected validation error: %v", err)
}
} else {
if tc.expectError {
t.Fatal("validation unexpectedly succeeded")
}
}

for file, expectIgnore := range tc.expectIgnoreFile {
t.Run(strings.ReplaceAll(file, "/", "-"), func(t *testing.T) {
ignore := p.ignoreFile(file)
if expectIgnore && !ignore {
t.Fatal("expected file to be ignored")
}
if !expectIgnore && ignore {
t.Fatal("expected file not to be ignored")
}
})
}
})
}
}
1 change: 1 addition & 0 deletions go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module github.com/ashanbrown/forbidigo
go 1.12

require (
github.com/bmatcuk/doublestar/v4 v4.6.0 // indirect
github.com/pkg/errors v0.9.1
github.com/stretchr/testify v1.4.0
golang.org/x/tools v0.3.0
Expand Down
2 changes: 2 additions & 0 deletions go.sum
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
github.com/bmatcuk/doublestar/v4 v4.6.0 h1:HTuxyug8GyFbRkrffIpzNCSK4luc0TY3wzXvzIZhEXc=
github.com/bmatcuk/doublestar/v4 v4.6.0/go.mod h1:xBQ8jztBU6kakFMg+8WGxn0c6z1fTSPVIjEY1Wr7jzc=
github.com/davecgh/go-spew v1.1.0 h1:ZDRjVQ15GmhC3fiQ8ni8+OwkZQO4DARzQgrnXU1Liz8=
github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4=
Expand Down
2 changes: 1 addition & 1 deletion main.go
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ func main() {
for _, n := range p.Syntax {
nodes = append(nodes, n)
}
newIssues, err := linter.RunWithConfig(forbidigo.RunConfig{Fset: p.Fset, TypesInfo: p.TypesInfo}, nodes...)
newIssues, err := linter.RunWithConfig(forbidigo.RunConfig{Fset: p.Fset, TypesInfo: p.TypesInfo, Pkg: p.Types}, nodes...)
if err != nil {
log.Fatalf("failed: %s", err)
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ func (a *analyzer) runAnalysis(pass *analysis.Pass) (interface{}, error) {
for _, f := range pass.Files {
nodes = append(nodes, f)
}
config := forbidigo.RunConfig{Fset: pass.Fset, DebugLog: a.debugLog}
config := forbidigo.RunConfig{Fset: pass.Fset, DebugLog: a.debugLog, Pkg: pass.Pkg}
if a.expand {
config.TypesInfo = pass.TypesInfo
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/analyzer/analyzer_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ func TestLiteralAnalyzer(t *testing.T) {
testdata := analysistest.TestData()
patterns := append(forbidigo.DefaultPatterns(),
`^pkg\.Forbidden$`,
`^Shiny`,
`{p: ^Shiny, ignore: ["**/ignored.go"]}`,
`^AlsoShiny`,
`^renamed\.Forbidden`,
)
Expand Down
7 changes: 7 additions & 0 deletions pkg/analyzer/testdata/src/matchtext/ignored.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
package testdata

import (
. "example.com/some/thing"
)

var _ = Shiny