From aaf6ad74d4a14df3abaeb0b068ef9b4f24c5cf85 Mon Sep 17 00:00:00 2001 From: "Andrew S. Brown" Date: Sun, 22 Jan 2023 13:50:53 -0800 Subject: [PATCH 1/4] Fix lint --- .golangci.yml | 1 + .pre-commit-config.yaml | 2 +- Makefile | 8 ++++++++ 3 files changed, 10 insertions(+), 1 deletion(-) diff --git a/.golangci.yml b/.golangci.yml index 2d89202..9688026 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -1,3 +1,4 @@ linters: enable: - prealloc + - nakedret diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 925f136..45dcf46 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -1,5 +1,5 @@ repos: - repo: https://github.com/golangci/golangci-lint - rev: v1.44.2 + rev: v1.50.1 hooks: - id: golangci-lint diff --git a/Makefile b/Makefile index 8d17ac8..191c886 100644 --- a/Makefile +++ b/Makefile @@ -1,4 +1,12 @@ SHELL=bash +setup: + pre-commit install + test: cd examples && diff <(sed 's|CURDIR|$(CURDIR)|' expected_results.txt) <(go run .. 2>&1 | sed '/^go: downloading/d') + +lint: + pre-commit run --all-files + +.PHONY: lint test From 0e094790aa3e9271f23b32faf4755694773bd536 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Tue, 25 Oct 2022 20:10:43 +0200 Subject: [PATCH 2/4] support import aliases, methods and fields It was possible to use fmt.Print by importing "fmt" under a different name. Now a pattern can get matched against the full package path instead of the import name. This is also useful to ban items from packages that may get imported under various different names. It was possible to forbid a function in a package, but not a method or field of a certain type in a package. The same approach for replacing the match text can now also be used for those. The only way of debugging was to single-step with a debugger and manually printing variables. This gets tiresome because in particular for the analyzer test, inner functions get called a lot. Debug log output during unit testing helps. Informing the user about text expansion may also be useful, but is not addressed yet. --- README.md | 115 ++++++++++- forbidigo/forbidigo.go | 181 ++++++++++++++++-- forbidigo/forbidigo_test.go | 126 ++++++++++-- forbidigo/patterns.go | 98 +++++++++- forbidigo/patterns_test.go | 105 +++++++++- go.mod | 3 +- go.sum | 37 +++- main.go | 22 ++- pkg/analyzer/analyzer.go | 15 +- pkg/analyzer/analyzer_test.go | 46 ++++- pkg/analyzer/testdata/forbidden.go | 10 - .../src/example.com/another/pkg/pkg2.go | 5 + .../testdata/src/example.com/some/pkg/pkg.go | 18 ++ .../example.com/some/renamedpkg/renamed.go | 11 ++ .../src/example.com/some/thing/thing.go | 4 + .../testdata/src/expandtext/expandtext.go | 78 ++++++++ .../testdata/src/matchtext/matchtext.go | 78 ++++++++ 17 files changed, 867 insertions(+), 85 deletions(-) delete mode 100644 pkg/analyzer/testdata/forbidden.go create mode 100644 pkg/analyzer/testdata/src/example.com/another/pkg/pkg2.go create mode 100644 pkg/analyzer/testdata/src/example.com/some/pkg/pkg.go create mode 100644 pkg/analyzer/testdata/src/example.com/some/renamedpkg/renamed.go create mode 100644 pkg/analyzer/testdata/src/example.com/some/thing/thing.go create mode 100644 pkg/analyzer/testdata/src/expandtext/expandtext.go create mode 100644 pkg/analyzer/testdata/src/matchtext/matchtext.go diff --git a/README.md b/README.md index b4bbf58..ca50a2c 100644 --- a/README.md +++ b/README.md @@ -16,20 +16,121 @@ If no patterns are specified, the default pattern of `^(fmt\.Print.*|print|print functions (and whole files), that are identifies as Godoc examples (https://blog.golang.org/examples) are excluded from checking. -A larger set of interesting patterns might include: +By default, patterns get matched against the actual expression as it appears in +the source code. The effect is that ``^fmt\.Print.*$` will not match when that +package gets imported with `import fmt2 "fmt"` and then the function gets +called with `fmt2.Print`. + +This makes it hard to match packages that may get imported under a variety of +different names, for example because there is no established convention or the +name is so generic that import aliases have to be used. To solve this, +forbidigo also supports a more advanced mode where it uses type information to +identify what an expression references. This needs to be enabled through the +`analyze_types` command line parameter. Beware this may have a performance +impact because additional information is required for the analysis. + +Replacing the literal source code works for items in a package as in the +`fmt2.Print` example above and also for struct fields and methods. For those, +`..` replaces the source code +text. `` is what the package declares in its `package` statement, +which may be different from last part of the import path: + + import "example.com/some/pkg" // pkg uses `package somepkg` + s := somepkg.SomeStruct{} + s.SomeMethod() // -> somepkg.SomeStruct.SomeMethod + +Pointers are treated like the type they point to: + + var cf *spew.ConfigState = ... + cf.Dump() // -> spew.ConfigState.Dump + +When a type is an alias for a type in some other package, the name of that +other package will be used. + +An imported identifier gets replaced as if it had been imported without `import .` +*and* also gets matched literally, so in this example both `^ginkgo.FIt$` +and `^FIt$` would catch the usage of `FIt`: + + import . "github.com/onsi/ginkgo/v2" + FIt(...) // -> ginkgo.FIt, FIt + +Beware that looking up the package name has limitations. When a struct embeds +some other type, references to the inherited fields or methods get resolved +with the outer struct as type: + + package foo + + type InnerStruct { + SomeField int + } + + func (i innerStruct) SomeMethod() {} + + type OuterStruct { + InnerStruct + } -* `^fmt\.Print.*$` -- forbid use of Print statements because they are likely just for debugging -* `^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 -* `^fmt\.Errorf(# please use github\.com/pkg/errors)?$` -- forbid Errorf, with a custom message + s := OuterStruct{} + s.SomeMethod() // -> foo.OuterStruct.SomeMethod + i := s.SomeField // -> foo.OuterStruct.SomeField + +When a method gets called via some interface, that invocation also only +gets resolved to the interface, not the underlying implementation: + + // innerStruct as above + + type myInterface interface { + SomeMethod() + } + + var i myInterface = InnerStruct{} + i.SomeMethod() // -> foo.myInterface.SomeMethod + +Using the package name is simple, but the name is not necessarily unique. For +more advanced cases, it is possible to specify more complex patterns. Such +patterns are strings that contain JSON or YAML for a struct. + +The full pattern struct has the following fields: + +* `msg`: an additional comment that gets added to the error message when a + pattern matches. +* `pattern`: the regular expression that matches the source code or expanded + expression, depending on the global flag. +* `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. + +To distinguish such patterns from traditional regular expression patterns, the +encoding must start with a `{` or contain line breaks. When using just JSON +encoding, backslashes must get quoted inside strings. When using YAML, this +isn't necessary. The following pattern strings are equivalent: + + {p: "^fmt\\.Println$", msg: "do not write to stdout"} + + {p: ^fmt\.Println$, + msg: do not write to stdout, + } + + {p: ^fmt\.Println$, msg: do not write to stdout} + + p: ^fmt\.Println$ + msg: do not write to stdout + +A larger set of interesting patterns might include: -Note that the linter has no knowledge of what packages were actually imported, so aliased imports will match these patterns. +-* `^fmt\.Print.*$` -- forbid use of Print statements because they are likely just for debugging +-* `^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 +-* `^spew.ConfigState\.Dump$` -- also forbid it via a `ConfigState` +-* `^fmt\.Errorf(# please use github\.com/pkg/errors)?$` -- forbid Errorf, with a custom message +-* `{p: ^fmt\.Errorf$, msg: please use github.com/pkg/errors}` -- the same with separate msg field ### Flags - **-set_exit_status** (default false) - Set exit status to 1 if any issues are found. - **-exclude_godoc_examples** (default true) - Controls whether godoc examples are identified and excluded - **-tests** (default true) - Controls whether tests are included +- **-analyze_types** (default false) - Replace literal source code before matching ## Purpose diff --git a/forbidigo/forbidigo.go b/forbidigo/forbidigo.go index 9b7d46b..9b37654 100644 --- a/forbidigo/forbidigo.go +++ b/forbidigo/forbidigo.go @@ -7,6 +7,7 @@ import ( "go/ast" "go/printer" "go/token" + "go/types" "log" "regexp" "strings" @@ -97,19 +98,43 @@ type visitor struct { linter *Linter comments []*ast.CommentGroup - fset *token.FileSet - issues []Issue + runConfig RunConfig + issues []Issue } +// Deprecated: Run was the original entrypoint before RunWithConfig was introduced to support +// additional match patterns that need additional information. func (l *Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { - var issues []Issue + return l.RunWithConfig(RunConfig{Fset: fset}, nodes...) +} + +// RunConfig provides information that the linter needs for different kinds +// of match patterns. Ideally, all fields should get set. More fields may get +// added in the future as needed. +type RunConfig struct { + // FSet is required. + Fset *token.FileSet + + // TypesInfo is needed for expanding source code expressions. + // Nil disables that step, i.e. patterns match the literal source code. + TypesInfo *types.Info + + // DebugLog is used to print debug messages. May be nil. + DebugLog func(format string, args ...interface{}) +} + +func (l *Linter) RunWithConfig(config RunConfig, nodes ...ast.Node) ([]Issue, error) { + if config.DebugLog == nil { + config.DebugLog = func(format string, args ...interface{}) {} + } + var issues []Issue for _, node := range nodes { var comments []*ast.CommentGroup isTestFile := false isWholeFileExample := false if file, ok := node.(*ast.File); ok { comments = file.Comments - fileName := fset.Position(file.Pos()).Filename + fileName := config.Fset.Position(file.Pos()).Filename isTestFile = strings.HasSuffix(fileName, "_test.go") // From https://blog.golang.org/examples, a "whole file example" is: @@ -145,7 +170,7 @@ func (l *Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) { cfg: l.cfg, isTestFile: isTestFile, linter: l, - fset: fset, + runConfig: config, comments: comments, } ast.Walk(&visitor, node) @@ -163,41 +188,169 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor { return nil } return v + // The following two are handled below. case *ast.SelectorExpr: case *ast.Ident: + // Everything else isn't. default: return v } + + // The text as it appears in the source is always used because issues + // use that. It's used for matching unless usage of type information + // is enabled. + srcText := v.textFor(node) + matchTexts, pkgText := v.expandMatchText(node, srcText) + v.runConfig.DebugLog("%s: match %v, package %q", v.runConfig.Fset.Position(node.Pos()), matchTexts, pkgText) for _, p := range v.linter.patterns { - if p.pattern.MatchString(v.textFor(node)) && !v.permit(node) { + if p.matches(matchTexts) && + (p.Package == "" || p.pkgRe.MatchString(pkgText)) && + !v.permit(node) { v.issues = append(v.issues, UsedIssue{ - identifier: v.textFor(node), - pattern: p.pattern.String(), + identifier: srcText, // Always report the expression as it appears in the source code. + pattern: p.re.String(), pos: node.Pos(), - position: v.fset.Position(node.Pos()), - customMsg: p.msg, + position: v.runConfig.Fset.Position(node.Pos()), + customMsg: p.Msg, }) } } return nil } +// textFor returns the expression as it appears in the source code (for +// example, .). func (v *visitor) textFor(node ast.Node) string { buf := new(bytes.Buffer) - if err := printer.Fprint(buf, v.fset, node); err != nil { - log.Fatalf("ERROR: unable to print node at %s: %s", v.fset.Position(node.Pos()), err) + if err := printer.Fprint(buf, v.runConfig.Fset, node); err != nil { + log.Fatalf("ERROR: unable to print node at %s: %s", v.runConfig.Fset.Position(node.Pos()), err) } return buf.String() } +// expandMatchText expands the selector in a selector expression to the full package +// name and (for variables) the type: +// +// - example.com/some/pkg.Function +// - example.com/some/pkg.CustomType.Method +// +// It updates the text to match against and fills the package string if possible, +// otherwise it just returns. +func (v *visitor) expandMatchText(node ast.Node, srcText string) (matchTexts []string, pkgText string) { + // The text to match against is the literal source code if we cannot + // come up with something different. + matchText := srcText + + if v.runConfig.TypesInfo == nil { + return []string{matchText}, pkgText + } + + location := v.runConfig.Fset.Position(node.Pos()) + + switch node := node.(type) { + case *ast.Ident: + object, ok := v.runConfig.TypesInfo.Uses[node] + if !ok { + // No information about the identifier. Should + // not happen, but perhaps there were compile + // errors? + v.runConfig.DebugLog("%s: unknown identifier %q", location, srcText) + return []string{matchText}, pkgText + } + if pkg := object.Pkg(); pkg != nil { + pkgText = pkg.Path() + v.runConfig.DebugLog("%s: identifier: %q -> %q in package %q", location, srcText, matchText, pkgText) + // match either with or without package name + return []string{pkg.Name() + "." + srcText, srcText}, pkgText + } else { + v.runConfig.DebugLog("%s: identifier: %q -> %q without package", location, srcText, matchText) + } + return []string{matchText}, pkgText + case *ast.SelectorExpr: + selector := node.X + field := node.Sel.Name + + // If we are lucky, the entire selector expression has a known + // type. We don't care about the value. + selectorText := v.textFor(node) + if typeAndValue, ok := v.runConfig.TypesInfo.Types[selector]; ok { + m, p, ok := pkgFromType(typeAndValue.Type) + if !ok { + v.runConfig.DebugLog("%s: selector %q with supported type %T", location, selectorText, typeAndValue.Type) + } + matchText = m + "." + field + pkgText = p + v.runConfig.DebugLog("%s: selector %q with supported type %q: %q -> %q, package %q", location, selectorText, typeAndValue.Type.String(), srcText, matchText, pkgText) + return []string{matchText}, pkgText + } + // Some expressions need special treatment. + switch selector := selector.(type) { + case *ast.Ident: + object, ok := v.runConfig.TypesInfo.Uses[selector] + if !ok { + // No information about the identifier. Should + // not happen, but perhaps there were compile + // errors? + v.runConfig.DebugLog("%s: unknown selector identifier %q", location, selectorText) + return []string{matchText}, pkgText + } + switch object := object.(type) { + case *types.PkgName: + pkgText = object.Imported().Path() + matchText = object.Imported().Name() + "." + field + v.runConfig.DebugLog("%s: selector %q is package: %q -> %q, package %q", location, selectorText, srcText, matchText, pkgText) + return []string{matchText}, pkgText + case *types.Var: + m, p, ok := pkgFromType(object.Type()) + if !ok { + v.runConfig.DebugLog("%s: selector %q is variable with unsupported type %T", location, selectorText, object.Type()) + } + matchText = m + "." + field + pkgText = p + v.runConfig.DebugLog("%s: selector %q is variable of type %q: %q -> %q, package %q", location, selectorText, object.Type().String(), srcText, matchText, pkgText) + default: + // Something else? + v.runConfig.DebugLog("%s: selector %q is identifier with unsupported type %T", location, selectorText, object) + } + default: + v.runConfig.DebugLog("%s: selector %q of unsupported type %T", location, selectorText, selector) + } + return []string{matchText}, pkgText + default: + v.runConfig.DebugLog("%s: unsupported type %T", location, node) + return []string{matchText}, pkgText + } +} + +// pkgFromType tries to determine `.` and the full +// package path. This only needs to work for types of a selector in a selector +// expression. +func pkgFromType(t types.Type) (typeStr, pkgStr string, ok bool) { + if ptr, ok := t.(*types.Pointer); ok { + t = ptr.Elem() + } + + switch t := t.(type) { + case *types.Named: + obj := t.Obj() + pkg := obj.Pkg() + if pkg == nil { + return "", "", false + } + return pkg.Name() + "." + obj.Name(), pkg.Path(), true + default: + return "", "", false + } +} + func (v *visitor) permit(node ast.Node) bool { if v.cfg.IgnorePermitDirectives { return false } - nodePos := v.fset.Position(node.Pos()) + nodePos := v.runConfig.Fset.Position(node.Pos()) nolint := regexp.MustCompile(fmt.Sprintf(`^//\s?permit:%s\b`, regexp.QuoteMeta(v.textFor(node)))) for _, c := range v.comments { - commentPos := v.fset.Position(c.Pos()) + commentPos := v.runConfig.Fset.Position(c.Pos()) if commentPos.Line == nodePos.Line && len(c.List) > 0 && nolint.MatchString(c.List[0].Text) { return true } diff --git a/forbidigo/forbidigo_test.go b/forbidigo/forbidigo_test.go index d8b0f8d..f93132a 100644 --- a/forbidigo/forbidigo_test.go +++ b/forbidigo/forbidigo_test.go @@ -1,17 +1,21 @@ package forbidigo import ( - "go/parser" - "go/token" + "go/ast" + "os" + "path" + "regexp" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/tools/go/packages" ) func TestForbiddenIdentifiers(t *testing.T) { t.Run("it finds forbidden identifiers", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - expectIssues(t, linter, ` + expectIssues(t, linter, false, ` package bar func foo() { @@ -19,9 +23,21 @@ func foo() { }`, "use of `fmt.Printf` forbidden by pattern `fmt\\.Printf` at testing.go:5:2") }) + t.Run("it finds forbidden, renamed identifiers", func(t *testing.T) { + linter, _ := NewLinter([]string{`fmt\.Printf`}) + expectIssues(t, linter, true, ` +package bar + +import renamed "fmt" + +func foo() { + renamed.Printf("here i am") +}`, "use of `renamed.Printf` forbidden by pattern `fmt\\.Printf` at testing.go:7:2") + }) + t.Run("displays custom messages", func(t *testing.T) { linter, _ := NewLinter([]string{`^fmt\.Printf(# a custom message)?$`}) - expectIssues(t, linter, ` + expectIssues(t, linter, false, ` package bar func foo() { @@ -31,7 +47,7 @@ func foo() { t.Run("it doesn't require a package on the identifier", func(t *testing.T) { linter, _ := NewLinter([]string{`Printf`}) - expectIssues(t, linter, ` + expectIssues(t, linter, false, ` package bar func foo() { @@ -41,7 +57,7 @@ func foo() { t.Run("allows explicitly permitting otherwise forbidden identifiers", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - expectIssues(t, linter, ` + expectIssues(t, linter, false, ` package bar func foo() { @@ -51,7 +67,7 @@ func foo() { t.Run("allows old notation for explicitly permitting otherwise forbidden identifiers", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - expectIssues(t, linter, ` + expectIssues(t, linter, false, ` package bar func foo() { @@ -61,7 +77,7 @@ func foo() { t.Run("has option to ignore permit directives", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}, OptionIgnorePermitDirectives(true)) - issues := parseFile(t, linter, "file.go", ` + issues := parseFile(t, linter, false, "file.go", ` package bar func foo() { @@ -72,7 +88,7 @@ func foo() { t.Run("examples are excluded by default in test files", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - issues := parseFile(t, linter, "file_test.go", ` + issues := parseFile(t, linter, false, "file_test.go", ` package bar func ExampleFoo() { @@ -83,7 +99,7 @@ func ExampleFoo() { t.Run("whole file examples are excluded by default", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - issues := parseFile(t, linter, "file_test.go", ` + issues := parseFile(t, linter, false, "file_test.go", ` package bar func Foo() { @@ -98,7 +114,7 @@ func Example() { t.Run("Test functions prevent a file from being considered a whole file example", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - issues := parseFile(t, linter, "file_test.go", ` + issues := parseFile(t, linter, false, "file_test.go", ` package bar func TestFoo() { @@ -112,7 +128,7 @@ func Example() { t.Run("Benchmark functions prevent a file from being considered a whole file example", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}) - issues := parseFile(t, linter, "file_test.go", ` + issues := parseFile(t, linter, false, "file_test.go", ` package bar func BenchmarkFoo() { @@ -126,7 +142,7 @@ func Example() { t.Run("examples can be included", func(t *testing.T) { linter, _ := NewLinter([]string{`fmt\.Printf`}, OptionExcludeGodocExamples(false)) - issues := parseFile(t, linter, "file.go", ` + issues := parseFile(t, linter, false, "file.go", ` package bar func ExampleFoo() { @@ -134,24 +150,92 @@ func ExampleFoo() { }`) assert.NotEmpty(t, issues) }) + + t.Run("import renames not detected without type information", func(t *testing.T) { + linter, _ := NewLinter([]string{`fmt\.Printf`}, OptionExcludeGodocExamples(false)) + issues := parseFile(t, linter, false, "file.go", ` +package bar + +import fmt2 "fmt" + +func ExampleFoo() { + fmt2.Printf("here i am") +}`) + assert.Empty(t, issues) + }) + + t.Run("import renames detected with type information", func(t *testing.T) { + linter, err := NewLinter([]string{`^fmt\.Printf`}, OptionExcludeGodocExamples(false)) + require.NoError(t, err) + expectIssues(t, linter, true, ` +package bar + +import fmt2 "fmt" + +func ExampleFoo() { + fmt2.Printf("here i am") +}`, "use of `fmt2.Printf` forbidden by pattern `^fmt\\.Printf` at testing.go:7:2") + }) + } -func expectIssues(t *testing.T, linter *Linter, contents string, issues ...string) { - actualIssues := parseFile(t, linter, "testing.go", contents) +// sourcePath matches "at /tmp/TestForbiddenIdentifiersdisplays_custom_messages4260088387/001/testing.go". +var sourcePath = regexp.MustCompile(`at .*/([[:alnum:]]+.go)`) + +func expectIssues(t *testing.T, linter *Linter, expand bool, contents string, issues ...string) { + actualIssues := parseFile(t, linter, expand, "testing.go", contents) actualIssueStrs := make([]string, 0, len(actualIssues)) for _, i := range actualIssues { - actualIssueStrs = append(actualIssueStrs, i.String()) + str := i.String() + str = sourcePath.ReplaceAllString(str, "at $1") + actualIssueStrs = append(actualIssueStrs, str) } assert.ElementsMatch(t, issues, actualIssueStrs) } -func parseFile(t *testing.T, linter *Linter, fileName, contents string) []Issue { - fset := token.NewFileSet() - expr, err := parser.ParseFile(fset, fileName, contents, parser.ParseComments) +func parseFile(t *testing.T, linter *Linter, expand bool, fileName, contents string) []Issue { + // We can use packages.Load if we put a single file into a separate + // directory and parse it with Go modules of. We have to be in that + // directory to use "." as pattern, parsing it via the absolute path + // from the forbidigo project doesn't work ("cannot import absolute + // path"). + tmpDir := t.TempDir() + if err := os.WriteFile(path.Join(tmpDir, fileName), []byte(contents), 0644); err != nil { + t.Fatalf("could not write source file: %v", err) + } + env := os.Environ() + env = append(env, "GO111MODULE=off") + cfg := packages.Config{ + Mode: packages.NeedSyntax | packages.NeedName | packages.NeedFiles | packages.NeedTypes, + Env: env, + Tests: true, + } + if expand { + cfg.Mode |= packages.NeedTypesInfo | packages.NeedDeps + } + pwd, err := os.Getwd() + require.NoError(t, err) + defer func() { + _ = os.Chdir(pwd) + }() + err = os.Chdir(tmpDir) + require.NoError(t, err) + pkgs, err := packages.Load(&cfg, ".") if err != nil { - t.Fatalf("unable to parse file contents: %s", err) + t.Fatalf("could not load packages: %v", err) + } + var issues []Issue + for _, p := range pkgs { + nodes := make([]ast.Node, 0, len(p.Syntax)) + for _, n := range p.Syntax { + nodes = append(nodes, n) + } + newIssues, err := linter.RunWithConfig(RunConfig{Fset: p.Fset, TypesInfo: p.TypesInfo, DebugLog: t.Logf}, nodes...) + if err != nil { + t.Fatalf("failed: %s", err) + } + issues = append(issues, newIssues...) } - issues, err := linter.Run(fset, expr) if err != nil { t.Fatalf("unable to parse file: %s", err) } diff --git a/forbidigo/patterns.go b/forbidigo/patterns.go index 9dc70ec..2692dcd 100644 --- a/forbidigo/patterns.go +++ b/forbidigo/patterns.go @@ -5,24 +5,108 @@ import ( "regexp" "regexp/syntax" "strings" + + "gopkg.in/yaml.v2" ) +// pattern matches code that is not supposed to be used. type pattern struct { - pattern *regexp.Regexp - msg string + re, pkgRe *regexp.Regexp + + // Pattern is the regular expression string that is used for matching. + // It gets matched against the literal source code text or the expanded + // text, depending on the mode in which the analyzer runs. + Pattern string `yaml:"p"` + + // Package is a regular expression for the full package path of + // an imported item. Ignored unless the analyzer is configured to + // determine that information. + Package string `yaml:"pkg,omitempty"` + + // Msg gets printed in addition to the normal message if a match is + // found. + Msg string `yaml:"msg,omitempty"` } +// A yamlPattern pattern in a YAML string may be represented either by a string +// (the traditional regular expression syntax) or a struct (for more complex +// patterns). +type yamlPattern pattern + +func (p *yamlPattern) UnmarshalYAML(unmarshal func(interface{}) error) error { + // Try struct first. It's unlikely that a regular expression string + // is valid YAML for a struct. + var ptrn pattern + if err := unmarshal(&ptrn); err != nil { + errStr := err.Error() + // Didn't work, try plain string. + var ptrn string + if err := unmarshal(&ptrn); err != nil { + return fmt.Errorf("pattern is neither a regular expression string (%s) nor a Pattern struct (%s)", err.Error(), errStr) + } + p.Pattern = ptrn + } else { + *p = yamlPattern(ptrn) + } + return ((*pattern)(p)).validate() +} + +var _ yaml.Unmarshaler = &yamlPattern{} + +// parse accepts a regular expression or, if the string starts with { or contains a line break, a +// JSON or YAML representation of a Pattern. func parse(ptrn string) (*pattern, error) { - ptrnRe, err := regexp.Compile(ptrn) + pattern := &pattern{} + + if strings.HasPrefix(strings.TrimSpace(ptrn), "{") || + strings.Contains(ptrn, "\n") { + // Embedded JSON or YAML. We can decode both with the YAML decoder. + if err := yaml.UnmarshalStrict([]byte(ptrn), pattern); err != nil { + return nil, fmt.Errorf("parsing as JSON or YAML failed: %v", err) + } + } else { + pattern.Pattern = ptrn + } + + if err := pattern.validate(); err != nil { + return nil, err + } + return pattern, nil +} + +func (p *pattern) validate() error { + ptrnRe, err := regexp.Compile(p.Pattern) if err != nil { - return nil, fmt.Errorf("unable to compile pattern `%s`: %s", ptrn, err) + return fmt.Errorf("unable to compile source code pattern `%s`: %s", p.Pattern, err) } - re, err := syntax.Parse(ptrn, syntax.Perl) + re, err := syntax.Parse(p.Pattern, syntax.Perl) if err != nil { - return nil, fmt.Errorf("unable to parse pattern `%s`: %s", ptrn, err) + return fmt.Errorf("unable to parse source code pattern `%s`: %s", p.Pattern, err) } msg := extractComment(re) - return &pattern{pattern: ptrnRe, msg: msg}, nil + if msg != "" { + p.Msg = msg + } + p.re = ptrnRe + + if p.Package != "" { + pkgRe, err := regexp.Compile(p.Package) + if err != nil { + return fmt.Errorf("unable to compile package pattern `%s`: %s", p.Package, err) + } + p.pkgRe = pkgRe + } + + return nil +} + +func (p *pattern) matches(matchTexts []string) bool { + for _, text := range matchTexts { + if p.re.MatchString(text) { + return true + } + } + return false } // Traverse the leaf submatches in the regex tree and extract a comment, if any diff --git a/forbidigo/patterns_test.go b/forbidigo/patterns_test.go index ce4e617..dbea9d1 100644 --- a/forbidigo/patterns_test.go +++ b/forbidigo/patterns_test.go @@ -5,6 +5,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "gopkg.in/yaml.v2" ) func TestParseValidPatterns(t *testing.T) { @@ -12,6 +13,8 @@ func TestParseValidPatterns(t *testing.T) { name string ptrn string expectedComment string + expectedPattern string + expectedPackage string }{ { name: "simple expression, no comment", @@ -41,12 +44,43 @@ func TestParseValidPatterns(t *testing.T) { ptrn: `^fmt\.Println(# Please don't use this!)?$`, expectedComment: "Please don't use this!", }, + { + name: "match import", + ptrn: `{p: "^fmt\\.Println$"}`, + expectedPattern: `^fmt\.Println$`, + }, + { + name: "match import with YAML", + ptrn: `{msg: hello world, +p: ^fmt\.Println$ +}`, + expectedComment: "hello world", + expectedPattern: `^fmt\.Println$`, + }, + { + name: "match import with YAML, no line breaks", + ptrn: `{p: ^fmt\.Println$}`, + expectedPattern: `^fmt\.Println$`, + }, + { + name: "simple YAML", + ptrn: `p: ^fmt\.Println$ +`, + expectedPattern: `^fmt\.Println$`, + }, } { t.Run(tc.name, func(t *testing.T) { ptrn, err := parse(tc.ptrn) require.Nil(t, err) - assert.Equal(t, tc.ptrn, ptrn.pattern.String()) - assert.Equal(t, tc.expectedComment, ptrn.msg) + expectedPattern := tc.expectedPattern + if expectedPattern == "" { + expectedPattern = tc.ptrn + } + assert.Equal(t, expectedPattern, ptrn.re.String(), "pattern") + if assert.Equal(t, tc.expectedPackage, ptrn.Package, "package") && tc.expectedPackage != "" { + assert.Equal(t, tc.expectedPackage, ptrn.pkgRe.String(), "package RE") + } + assert.Equal(t, tc.expectedComment, ptrn.Msg, "comment") }) } } @@ -55,3 +89,70 @@ func TestParseInvalidPattern_ReturnsError(t *testing.T) { _, err := parse(`fmt\`) assert.NotNil(t, err) } + +func TestUnmarshalYAML(t *testing.T) { + for _, tc := range []struct { + name string + yaml string + expectedErr string + expectedComment string + expectedPattern string + }{ + { + name: "string: simple expression, no comment", + yaml: `fmt\.Errorf`, + }, + { + name: "string: contains multiple subexpression, with comment", + yaml: `(f)mt\.Errorf(# a comment)?`, + expectedComment: "a comment", + }, + { + name: "struct: simple expression, no comment", + yaml: `p: fmt\.Errorf`, + expectedPattern: `fmt\.Errorf`, + }, + { + name: "match import with YAML", + yaml: `p: ^fmt\.Println$ +`, + expectedPattern: `^fmt\.Println$`, + }, + { + name: "string: invalid regexp", + yaml: `fmt\`, + expectedErr: "unable to compile source code pattern `fmt\\`: error parsing regexp: trailing backslash at end of expression: ``", + }, + { + name: "struct: invalid regexp", + yaml: `p: fmt\ +`, + expectedErr: "unable to compile source code pattern `fmt\\`: error parsing regexp: trailing backslash at end of expression: ``", + }, + { + name: "invalid struct", + yaml: `Foo: bar`, + expectedErr: `pattern is neither a regular expression string (yaml: unmarshal errors: + line 1: cannot unmarshal !!map into string) nor a Pattern struct (yaml: unmarshal errors: + line 1: field Foo not found in type forbidigo.pattern)`, + }, + } { + t.Run(tc.name, func(t *testing.T) { + var p yamlPattern + err := yaml.UnmarshalStrict([]byte(tc.yaml), &p) + if tc.expectedErr == "" { + require.NoError(t, err) + } else { + require.Error(t, err) + require.Equal(t, tc.expectedErr, err.Error()) + return + } + expectedPattern := tc.expectedPattern + if expectedPattern == "" { + expectedPattern = tc.yaml + } + assert.Equal(t, expectedPattern, p.re.String(), "pattern") + assert.Equal(t, tc.expectedComment, p.Msg, "comment") + }) + } +} diff --git a/go.mod b/go.mod index 45bc675..d2f4e16 100644 --- a/go.mod +++ b/go.mod @@ -5,5 +5,6 @@ go 1.12 require ( github.com/pkg/errors v0.9.1 github.com/stretchr/testify v1.4.0 - golang.org/x/tools v0.0.0-20190916130336-e45ffcd953cc + golang.org/x/tools v0.3.0 + gopkg.in/yaml.v2 v2.2.2 ) diff --git a/go.sum b/go.sum index 545861c..306b6f5 100644 --- a/go.sum +++ b/go.sum @@ -4,23 +4,42 @@ github.com/pkg/errors v0.9.1 h1:FEBLx1zS214owpjy7qsBeixbURkuhQAwrK5UwLGTwt4= github.com/pkg/errors v0.9.1/go.mod h1:bwawxfHBFNV+L2hUp1rHADufV3IMtnDRdf1r5NINEl0= github.com/pmezard/go-difflib v1.0.0 h1:4DBwDE0NGyQoBHbLQYPwSUPoCMWR5BEzIk/f1lZbAQM= github.com/pmezard/go-difflib v1.0.0/go.mod h1:iKH77koFhYxTK1pcRnkKkqfTogsbg7gZNVY4sRDYZ/4= -github.com/stretchr/objx v0.1.0 h1:4G4v2dO3VZwixGIRoQ5Lfboy6nUhCyYzaqnIAPPhYs4= github.com/stretchr/objx v0.1.0/go.mod h1:HFkY916IF+rwdDfMAkV7OtwuqBVzrE8GR6GFx+wExME= github.com/stretchr/testify v1.4.0 h1:2E4SXV/wtOkTonXsotYi4li6zVWxYlZuYNCXe9XRJyk= github.com/stretchr/testify v1.4.0/go.mod h1:j7eGeouHqKxXV5pUuKE4zz7dFj8WfuZ+81PSLYec5m4= -golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2 h1:VklqNMn3ovrHsnt90PveolxSbWFaJdECFbxSq0Mqo2M= +github.com/yuin/goldmark v1.4.13/go.mod h1:6yULJ656Px+3vBD8DxQVa3kxgyrAnzto9xy5taEt/CY= golang.org/x/crypto v0.0.0-20190308221718-c2843e01d9a2/go.mod h1:djNgcEr1/C05ACkg1iLfiJU5Ep61QUkGW8qpdssI0+w= -golang.org/x/net v0.0.0-20190620200207-3b0461eec859 h1:R/3boaszxrf1GEUWTVDzSKVwLmSJpwZ1yqXm8j0v2QI= +golang.org/x/crypto v0.0.0-20210921155107-089bfa567519/go.mod h1:GvvjBRRGRdwPK5ydBHafDWAxML/pGHZbMvKqRZ5+Abc= +golang.org/x/mod v0.6.0-dev.0.20220419223038-86c51ed26bb4/go.mod h1:jJ57K6gSWd91VN4djpZkiMVwK6gcyfeH4XE8wZrZaV4= +golang.org/x/mod v0.7.0 h1:LapD9S96VoQRhi/GrNTqeBJFrUjs5UHCAtTlgwA5oZA= +golang.org/x/mod v0.7.0/go.mod h1:iBbtSCu2XBx23ZKBPSOrRkjjQPZFPuis4dIYUhu/chs= golang.org/x/net v0.0.0-20190620200207-3b0461eec859/go.mod h1:z5CRVTTTmAJ677TzLLGU+0bjPO0LkuOLi4/5GtJWs/s= -golang.org/x/sync v0.0.0-20190423024810-112230192c58 h1:8gQV6CLnAEikrhgkHFbMAEhagSSnXWGV915qUMm9mrU= +golang.org/x/net v0.0.0-20210226172049-e18ecbb05110/go.mod h1:m0MpNAwzfU5UDzcl9v0D8zg8gWTRqZa9RBIspLL5mdg= +golang.org/x/net v0.0.0-20220722155237-a158d28d115b/go.mod h1:XRhObCWvk6IyKnWLug+ECip1KBveYUHfp+8e9klMJ9c= +golang.org/x/net v0.2.0/go.mod h1:KqCZLdyyvdV855qA2rE3GC2aiw5xGR5TEjj8smXukLY= golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= -golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a h1:1BGLXjeY4akVXGgbC9HugT3Jv3hCI0z56oJR5vAMgBU= +golang.org/x/sync v0.0.0-20220722155255-886fb9371eb4/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= +golang.org/x/sync v0.1.0 h1:wsuoTGHzEhffawBOhz5CYhcrV4IdKZbEyZjBMuTp12o= +golang.org/x/sync v0.1.0/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM= golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY= -golang.org/x/text v0.3.0 h1:g61tztE5qeGQ89tm6NTjjM9VPIm088od1l6aSorWRWg= +golang.org/x/sys v0.0.0-20201119102817-f84b799fce68/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs= +golang.org/x/sys v0.0.0-20210615035016-665e8c7367d1/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220520151302-bc2c85ada10a/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.0.0-20220722155257-8c9f86f7a55f/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/sys v0.2.0 h1:ljd4t30dBnAvMZaQCevtY0xLLD0A+bRZXbgLMLU1F/A= +golang.org/x/sys v0.2.0/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg= +golang.org/x/term v0.0.0-20201126162022-7de9c90e9dd1/go.mod h1:bj7SfCRtBDWHUb9snDiAeCFNEtKQo2Wmx5Cou7ajbmo= +golang.org/x/term v0.0.0-20210927222741-03fcf44c2211/go.mod h1:jbD1KX2456YbFQfuXm/mYQcufACuNUgVhRMnK/tPxf8= +golang.org/x/term v0.2.0/go.mod h1:TVmDHMZPmdnySmBfhjOoOdhjzdE1h4u1VwSiw2l1Nuc= golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ= -golang.org/x/tools v0.0.0-20190916130336-e45ffcd953cc h1:+GB9/q0gCzmtaIl6WdoJFMS3lPwrR6rpcMyY6jfQHAw= -golang.org/x/tools v0.0.0-20190916130336-e45ffcd953cc/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= -golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7 h1:9zdDQZ7Thm29KFXgAX/+yaf3eVbP7djjWp/dXAppNCc= +golang.org/x/text v0.3.3/go.mod h1:5Zoc/QRtKVWzQhOtBMvqHzDpF6irO9z98xDceosuGiQ= +golang.org/x/text v0.3.7/go.mod h1:u+2+/6zg+i71rQMx5EYifcz6MCKuco9NR6JIITiCfzQ= +golang.org/x/text v0.4.0/go.mod h1:mrYo+phRRbMaCq/xk9113O4dZlRixOauAjOtrjsXDZ8= +golang.org/x/tools v0.0.0-20180917221912-90fa682c2a6e/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ= +golang.org/x/tools v0.0.0-20191119224855-298f0cb1881e/go.mod h1:b+2E5dAYhXwXZwtnZ6UAqBI28+e2cm9otk0dWdXHAEo= +golang.org/x/tools v0.1.12/go.mod h1:hNGJHUnrk76NpqgfD5Aqm5Crs+Hm0VOH/i9J2+nxYbc= +golang.org/x/tools v0.3.0 h1:SrNbZl6ECOS1qFzgTdQfWXZM9XBkiA6tkFrH9YSTPHM= +golang.org/x/tools v0.3.0/go.mod h1:/rWhSS2+zyEVwoJf8YAX6L2f0ntZ7Kn/mGgAWcipA5k= golang.org/x/xerrors v0.0.0-20190717185122-a985d3407aa7/go.mod h1:I/5z698sn9Ka8TeJc9MKroUUfqBBauWjQqLJ2OPfmY0= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405 h1:yhCVgyC4o1eVCa2tZl7eS0r+SDo693bJlVdllGtEeKM= gopkg.in/check.v1 v0.0.0-20161208181325-20d25e280405/go.mod h1:Co6ibVJAznAaIkqp8huTwlJQCZ016jof/cbN4VW5Yz0= diff --git a/main.go b/main.go index 568940b..daaca76 100644 --- a/main.go +++ b/main.go @@ -16,6 +16,7 @@ func main() { setExitStatus := flag.Bool("set_exit_status", false, "Set exit status to 1 if any issues are found") includeTests := flag.Bool("tests", true, "Include tests") excludeGodocExamples := flag.Bool("exclude_godoc_examples", true, "Exclude code in godoc examples") + expand := flag.Bool("analyze_types", false, "Replace the literal source code based on the semantic of the code before matching against patterns") flag.Parse() var patterns = []string(nil) @@ -32,22 +33,27 @@ func main() { if patterns == nil { patterns = forbidigo.DefaultPatterns() } + options := []forbidigo.Option{ + forbidigo.OptionExcludeGodocExamples(*excludeGodocExamples), + } + linter, err := forbidigo.NewLinter(patterns, options...) + if err != nil { + log.Fatalf("Could not create linter: %s", err) + } cfg := packages.Config{ Mode: packages.NeedSyntax | packages.NeedName | packages.NeedFiles | packages.NeedTypes, Tests: *includeTests, } + + if *expand { + cfg.Mode |= packages.NeedTypesInfo | packages.NeedDeps + } + pkgs, err := packages.Load(&cfg, flag.Args()[firstPkg:]...) if err != nil { log.Fatalf("Could not load packages: %s", err) } - options := []forbidigo.Option{ - forbidigo.OptionExcludeGodocExamples(*excludeGodocExamples), - } - linter, err := forbidigo.NewLinter(patterns, options...) - if err != nil { - log.Fatalf("Could not create linter: %s", err) - } var issues []forbidigo.Issue for _, p := range pkgs { @@ -55,7 +61,7 @@ func main() { for _, n := range p.Syntax { nodes = append(nodes, n) } - newIssues, err := linter.Run(p.Fset, nodes...) + newIssues, err := linter.RunWithConfig(forbidigo.RunConfig{Fset: p.Fset, TypesInfo: p.TypesInfo}, nodes...) if err != nil { log.Fatalf("failed: %s", err) } diff --git a/pkg/analyzer/analyzer.go b/pkg/analyzer/analyzer.go index a14ba18..43e557f 100644 --- a/pkg/analyzer/analyzer.go +++ b/pkg/analyzer/analyzer.go @@ -29,6 +29,8 @@ type analyzer struct { patterns []string usePermitDirective bool includeExamples bool + expand bool + debugLog func(format string, args ...interface{}) } // NewAnalyzer returns a go/analysis-compatible analyzer @@ -36,14 +38,21 @@ type analyzer struct { // Set "-examples" to analyze godoc examples // Set "-permit=false" to ignore "//permit:" directives. func NewAnalyzer() *analysis.Analyzer { + return newAnalyzer(nil /* no debug output */) +} + +func newAnalyzer(debugLog func(format string, args ...interface{})) *analysis.Analyzer { var flags flag.FlagSet a := analyzer{ usePermitDirective: true, includeExamples: true, + debugLog: debugLog, } + flags.Var(&listVar{values: &a.patterns}, "p", "pattern") flags.BoolVar(&a.includeExamples, "examples", false, "check godoc examples") flags.BoolVar(&a.usePermitDirective, "permit", true, `when set, lines with "//permit" directives will be ignored`) + flags.BoolVar(&a.expand, "analyze_types", false, `when set, expressions get expanded instead of matching the literal source code`) return &analysis.Analyzer{ Name: "forbidigo", Doc: "forbid identifiers", @@ -67,7 +76,11 @@ func (a *analyzer) runAnalysis(pass *analysis.Pass) (interface{}, error) { for _, f := range pass.Files { nodes = append(nodes, f) } - issues, err := linter.Run(pass.Fset, nodes...) + config := forbidigo.RunConfig{Fset: pass.Fset, DebugLog: a.debugLog} + if a.expand { + config.TypesInfo = pass.TypesInfo + } + issues, err := linter.RunWithConfig(config, nodes...) if err != nil { return nil, err } diff --git a/pkg/analyzer/analyzer_test.go b/pkg/analyzer/analyzer_test.go index 6d521cc..b314f8f 100644 --- a/pkg/analyzer/analyzer_test.go +++ b/pkg/analyzer/analyzer_test.go @@ -1,14 +1,50 @@ -package analyzer_test +package analyzer import ( "testing" - "github.com/ashanbrown/forbidigo/pkg/analyzer" + "github.com/ashanbrown/forbidigo/forbidigo" "golang.org/x/tools/go/analysis/analysistest" ) -func TestAnalyzer(t *testing.T) { +func TestLiteralAnalyzer(t *testing.T) { testdata := analysistest.TestData() - a := analyzer.NewAnalyzer() - analysistest.Run(t, testdata, a, "") + patterns := append(forbidigo.DefaultPatterns(), + `^pkg\.Forbidden$`, + `^Shiny`, + `^AlsoShiny`, + `^renamed\.Forbidden`, + ) + a := newAnalyzer(t.Logf) + for _, pattern := range patterns { + if err := a.Flags.Set("p", pattern); err != nil { + t.Fatalf("unexpected error when setting pattern: %v", err) + } + } + analysistest.Run(t, testdata, a, "matchtext") +} + +func TestExpandAnalyzer(t *testing.T) { + testdata := analysistest.TestData() + patterns := append(forbidigo.DefaultPatterns(), + `{p: ^pkg\.Forbidden$, pkg: ^example.com/some/pkg$}`, + `{p: ^pkg\.CustomType.*Forbidden.*$, pkg: ^example.com/some/pkg$}`, + `{p: ^pkg\.CustomInterface.*Forbidden$, pkg: ^example.com/some/pkg$}`, + `{p: ^Shiny, pkg: ^example.com/some/thing$}`, + `{p: ^AlsoShiny}`, + `{p: myCustomStruct\..*Forbidden, pkg: ^expandtext$}`, + `{p: myCustomInterface\.AlsoForbidden, pkg: ^expandtext$}`, + `{p: renamed\.Forbidden, pkg: ^example.com/some/renamedpkg$}`, + `{p: renamed\.Struct.Forbidden, pkg: ^example.com/some/renamedpkg$}`, + ) + a := newAnalyzer(t.Logf) + for _, pattern := range patterns { + if err := a.Flags.Set("p", pattern); err != nil { + t.Fatalf("unexpected error when setting pattern: %v", err) + } + } + if err := a.Flags.Set("analyze_types", "true"); err != nil { + t.Fatalf("unexpected error when enabling expression expansion: %v", err) + } + analysistest.Run(t, testdata, a, "expandtext") } diff --git a/pkg/analyzer/testdata/forbidden.go b/pkg/analyzer/testdata/forbidden.go deleted file mode 100644 index 7966ae7..0000000 --- a/pkg/analyzer/testdata/forbidden.go +++ /dev/null @@ -1,10 +0,0 @@ -package testdata - -import "fmt" - -func Foo() { - fmt.Println("here I am") // want "forbidden by pattern" - fmt.Printf("this is ok") //permit:fmt.Printf // this is ok - print("not ok") // want "forbidden by pattern" - println("also not ok") // want "forbidden by pattern" -} diff --git a/pkg/analyzer/testdata/src/example.com/another/pkg/pkg2.go b/pkg/analyzer/testdata/src/example.com/another/pkg/pkg2.go new file mode 100644 index 0000000..ff37bc0 --- /dev/null +++ b/pkg/analyzer/testdata/src/example.com/another/pkg/pkg2.go @@ -0,0 +1,5 @@ +package pkg + +import "example.com/some/pkg" + +type CustomTypeAlias = pkg.CustomType diff --git a/pkg/analyzer/testdata/src/example.com/some/pkg/pkg.go b/pkg/analyzer/testdata/src/example.com/some/pkg/pkg.go new file mode 100644 index 0000000..99d20ad --- /dev/null +++ b/pkg/analyzer/testdata/src/example.com/some/pkg/pkg.go @@ -0,0 +1,18 @@ +package pkg + +func Forbidden() { +} + +func NewCustom() CustomType { + return CustomType{} +} + +type CustomType struct { + ForbiddenField int +} + +func (c CustomType) AlsoForbidden() {} + +type CustomInterface interface { + StillForbidden() +} diff --git a/pkg/analyzer/testdata/src/example.com/some/renamedpkg/renamed.go b/pkg/analyzer/testdata/src/example.com/some/renamedpkg/renamed.go new file mode 100644 index 0000000..67078f9 --- /dev/null +++ b/pkg/analyzer/testdata/src/example.com/some/renamedpkg/renamed.go @@ -0,0 +1,11 @@ +// Package renamed intentionally uses a package name that does not match the +// import path to test this situation. +package renamed // import "example.com/some/renamedpkg" + +func ForbiddenFunc() { +} + +type Struct struct{} + +func (s Struct) ForbiddenMethod() { +} diff --git a/pkg/analyzer/testdata/src/example.com/some/thing/thing.go b/pkg/analyzer/testdata/src/example.com/some/thing/thing.go new file mode 100644 index 0000000..0e00312 --- /dev/null +++ b/pkg/analyzer/testdata/src/example.com/some/thing/thing.go @@ -0,0 +1,4 @@ +package thing + +var Shiny int +var AlsoShiny int diff --git a/pkg/analyzer/testdata/src/expandtext/expandtext.go b/pkg/analyzer/testdata/src/expandtext/expandtext.go new file mode 100644 index 0000000..039e39e --- /dev/null +++ b/pkg/analyzer/testdata/src/expandtext/expandtext.go @@ -0,0 +1,78 @@ +package testdata + +import ( + "fmt" + alias "fmt" + + anotherpkg "example.com/another/pkg" + somepkg "example.com/some/pkg" + "example.com/some/renamedpkg" // Package name is "renamed". + . "example.com/some/thing" +) + +func myCustom() somepkg.CustomType { + return somepkg.CustomType{} +} + +var myCustomFunc = myCustom + +type myCustomStruct struct { + somepkg.CustomType +} + +type myCustomInterface interface { + AlsoForbidden() +} + +var forbiddenFunctionRef = somepkg.Forbidden // want "somepkg.Forbidden.*forbidden by pattern .*\\^pkg.*Forbidden" + +var forbiddenVariableRef = Shiny // want "Shiny.*forbidden by pattern.*\\^Shiny" +var forbiddenVariableRef2 = AlsoShiny // want "Shiny.*forbidden by pattern.*\\^AlsoShiny" + +func Foo() { + fmt.Println("here I am") // want "forbidden by pattern" + fmt.Printf("this is ok") //permit:fmt.Printf // this is ok + print("not ok") // want "forbidden by pattern" + println("also not ok") // want "forbidden by pattern" + alias.Println("hello") // want "forbidden by pattern" + somepkg.Forbidden() // want "somepkg.Forbidden.*forbidden by pattern .*\\^pkg.*Forbidden" + + c := somepkg.CustomType{} + c.AlsoForbidden() // want "c.AlsoForbidden.*forbidden by pattern.*\\^pkg..CustomType.*Forbidden" + + // Selector expression with result of function call in package. + somepkg.NewCustom().AlsoForbidden() // want "somepkg.NewCustom...AlsoForbidden.*forbidden by pattern.*\\^pkg..CustomType.*Forbidden" + + // Selector expression with result of normal function call. + myCustom().AlsoForbidden() // want "myCustom...AlsoForbidden.*forbidden by pattern.*\\^pkg..CustomType.*Forbidden" + + // Selector expression with result of normal function call. + myCustomFunc().AlsoForbidden() // want "myCustomFunc...AlsoForbidden.*forbidden by pattern.*\\^pkg..CustomType.*Forbidden" + + // Type alias and pointer. + c2 := &anotherpkg.CustomTypeAlias{} + c2.AlsoForbidden() // want "c2.AlsoForbidden.*forbidden by pattern.*\\^pkg..CustomType.*Forbidden" + + // Interface. + var ci somepkg.CustomInterface + ci.StillForbidden() // want "ci.StillForbidden.*forbidden by pattern.*\\^pkg..CustomInterface..Forbidden" + + // Forbidden embedded inside another: must be forbidden separately! + myCustomStruct{}.AlsoForbidden() // want "myCustomStruct...AlsoForbidden.*forbidden by pattern.*myCustomStruct" + _ = myCustomStruct{}.ForbiddenField // want "myCustomStruct...ForbiddenField.*forbidden by pattern.*myCustomStruct" + + // Forbidden method called via interface: must be forbidden separately! + var ci2 myCustomInterface = somepkg.CustomType{} + ci2.AlsoForbidden() // want "ci2.AlsoForbidden.*forbidden by pattern.*myCustomInterface" + + // Package name != import path. + renamed.ForbiddenFunc() // want "renamed.Forbidden.* by pattern .*renamed..Forbidden" + renamed.Struct{}.ForbiddenMethod() // want "renamed.Struct...ForbiddenMethod.* by pattern .*renamed.*Struct.*Forbidden" +} + +func Bar() string { + fmt := struct { + Println string + }{} + return fmt.Println // not matched because it expands to `struct{Println string}.Println` +} diff --git a/pkg/analyzer/testdata/src/matchtext/matchtext.go b/pkg/analyzer/testdata/src/matchtext/matchtext.go new file mode 100644 index 0000000..58470ae --- /dev/null +++ b/pkg/analyzer/testdata/src/matchtext/matchtext.go @@ -0,0 +1,78 @@ +package testdata + +import ( + "fmt" + alias "fmt" + + anotherpkg "example.com/another/pkg" + somepkg "example.com/some/pkg" + renamed "example.com/some/renamedpkg" // Package name is "renamed". + . "example.com/some/thing" +) + +func myCustom() somepkg.CustomType { + return somepkg.CustomType{} +} + +var myCustomFunc = myCustom + +type myCustomStruct struct { + somepkg.CustomType +} + +type myCustomInterface interface { + AlsoForbidden() +} + +var forbiddenFunctionRef = somepkg.Forbidden + +var forbiddenVariableRef = Shiny // want "Shiny.*forbidden by pattern.*\\^Shiny" +var forbiddenVariableRef2 = AlsoShiny // want "Shiny.*forbidden by pattern.*\\^AlsoShiny" + +func Foo() { + fmt.Println("here I am") // want "forbidden by pattern" + fmt.Printf("this is ok") //permit:fmt.Printf // this is ok + print("not ok") // want "forbidden by pattern" + println("also not ok") // want "forbidden by pattern" + alias.Println("hello") // not matched by default pattern fmt.Println + somepkg.Forbidden() + + c := somepkg.CustomType{} + c.AlsoForbidden() + + // Selector expression with result of function call in package. + somepkg.NewCustom().AlsoForbidden() + + // Selector expression with result of normal function call. + myCustom().AlsoForbidden() + + // Selector expression with result of normal function call. + myCustomFunc().AlsoForbidden() + + // Type alias and pointer. + c2 := &anotherpkg.CustomTypeAlias{} + c2.AlsoForbidden() + + // Interface. + var ci somepkg.CustomInterface + ci.StillForbidden() + + // Struct embedded inside another: must be forbidden separately! + myCustomStruct{}.AlsoForbidden() + _ = myCustomStruct{}.ForbiddenField + + // Forbidden method called via interface: must be forbidden separately! + var ci2 myCustomInterface = somepkg.CustomType{} + ci2.AlsoForbidden() + + // Package name != import path. + renamed.ForbiddenFunc() // want "renamed.Forbidden.* by pattern .*renamed..Forbidden" + renamed.Struct{}.ForbiddenMethod() +} + +func Bar() string { + fmt := struct { + Println string + }{} + return fmt.Println // want "forbidden by pattern" +} From 3d3f790a850f7d87c49a46c42f691372d285a875 Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 29 Jan 2023 11:19:54 +0100 Subject: [PATCH 3/4] CI: bump Go image to 1.19.5 Otherwise "go install" fails: # github.com/golangci/golangci-lint/pkg/golinters/goanalysis pkg/golinters/goanalysis/runner_loadingpackage.go:126:3: unknown field 'Instances' in struct literal of type types.Info note: module requires Go 1.19 --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 49aacbd..e33ed15 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -3,7 +3,7 @@ version: 2 jobs: build: docker: - - image: cimg/go:1.17.7 + - image: cimg/go:1.19.5 steps: - checkout - run: sudo apt-get update && sudo apt-get -y -qq install python pip From cac3c7d91da90dc9e19749b1d4c4f0578e6faadb Mon Sep 17 00:00:00 2001 From: Patrick Ohly Date: Sun, 29 Jan 2023 11:29:10 +0100 Subject: [PATCH 4/4] CI: use python3 Python 2 is no longer available: W: https://download.docker.com/linux/ubuntu/dists/jammy/InRelease: Key is stored in legacy trusted.gpg keyring (/etc/apt/trusted.gpg), see the DEPRECATION section in apt-key(8) for details. E: Package 'python' has no installation candidate --- .circleci/config.yml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index e33ed15..4eec796 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -6,8 +6,8 @@ jobs: - image: cimg/go:1.19.5 steps: - checkout - - run: sudo apt-get update && sudo apt-get -y -qq install python pip - - run: pip install pre-commit + - run: sudo apt-get update && sudo apt-get -y -qq install python3 python3-pip + - run: pip3 install pre-commit - run: SKIP=no-commit-to-branch pre-commit run -a - run: go test ./... - run: make test