Prevent the need for a version bump and fix a few test cases

README.md

@@ -12,19 +12,31 @@
 
     forbidigo [flags...] patterns... -- packages...
 
-If no patterns are specified, the default pattern of `^(fmt\.Print.*|print|println)$` is used to eliminate debug statements.  By default,
+If no patterns are specified, the default patterns of `^(P<pkg>fmt)\.Print(|f|ln)$` and `^(print|println)$` are used to eliminate debug statements.  By default,
 functions (and whole files), that are identifies as Godoc examples (https://blog.golang.org/examples) are excluded from 
 checking.
 
+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, a
+package that contains a subgroup literally called `pkg` (`(?P<pkg>...)`) will be
+matched against text where the import name got replaced with the full package
+name (e.g. `example.com/some/pkg`) if such a substitution is possible for the
+current expression. Otherwise such a pattern is ignored.
+
 A larger set of interesting patterns might include:
 
-* `^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
+* `^(?P<pkg>fmt)\.Print.*$` -- forbid use of Print statements because they are likely just for debugging
+* `^(?P<pkg>fmt)\.Errorf$` -- forbid Errorf in favor of using github.com/pkg/errors
+* `^(?P<pkg>ginkgo)\.F[A-Z].*$` -- forbid ginkgo focused commands (used for debug issues)
+* `^(?P<pkg>spew)\.Dump$` -- forbid dumping detailed data to stdout
+* `^(?P<pkg>fmt)\.Errorf(# please use github\.com/pkg/errors)?$` -- forbid Errorf, with a custom message
 
-Note that the linter has no knowledge of what packages were actually imported, so aliased imports will match these patterns.
 
 ### Flags
 - **-set_exit_status** (default false) - Set exit status to 1 if any issues are found.

examples/expected_results.txt

@@ -1,3 +1,3 @@
-use of `fmt.Println` forbidden by pattern `^(fmt\.Print(|f|ln)|print|println)$` at CURDIR/examples/example.go:6:2
-use of `print` forbidden by pattern `^(fmt\.Print(|f|ln)|print|println)$` at CURDIR/examples/example.go:8:2
-use of `println` forbidden by pattern `^(fmt\.Print(|f|ln)|print|println)$` at CURDIR/examples/example.go:9:2
+use of `fmt.Println` forbidden by pattern `^(?P<pkg>fmt)\.Print(|f|ln)$` at CURDIR/examples/example.go:6:2
+use of `print` forbidden by pattern `^(print|println)$` at CURDIR/examples/example.go:8:2
+use of `println` forbidden by pattern `^(print|println)$` at CURDIR/examples/example.go:9:2

forbidigo/forbidigo.go

@@ -7,6 +7,7 @@ import (
 	"go/ast"
 	"go/printer"
 	"go/token"
+	"go/types"
 	"log"
 	"regexp"
 	"strings"
@@ -57,7 +58,7 @@ type Linter struct {
 }
 
 func DefaultPatterns() []string {
-	return []string{`^(fmt\.Print(|f|ln)|print|println)$`}
+	return []string{`^(?P<pkg>fmt)\.Print(|f|ln)$`, `^(print|println)$`}
 }
 
 //go:generate go-options config
@@ -97,12 +98,19 @@ type visitor struct {
 	linter   *Linter
 	comments []*ast.CommentGroup
 
-	fset   *token.FileSet
-	issues []Issue
+	fset      *token.FileSet
+	typesInfo *types.Info
+	issues    []Issue
 }
 
+// Deprecated: Run was the original entrypoint before RunWithTypes was introduced to support matching
+// full package names.
 func (l *Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
-	var issues []Issue 
+	return l.RunWithTypes(fset, nil, nodes...)
+}
+
+func (l *Linter) RunWithTypes(fset *token.FileSet, typesInfo *types.Info, nodes ...ast.Node) ([]Issue, error) {
+	var issues []Issue
 	for _, node := range nodes {
 		var comments []*ast.CommentGroup
 		isTestFile := false
@@ -146,6 +154,7 @@ func (l *Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
 			isTestFile: isTestFile,
 			linter:     l,
 			fset:       fset,
+			typesInfo:  typesInfo,
 			comments:   comments,
 		}
 		ast.Walk(&visitor, node)
@@ -155,6 +164,7 @@ func (l *Linter) Run(fset *token.FileSet, nodes ...ast.Node) ([]Issue, error) {
 }
 
 func (v *visitor) Visit(node ast.Node) ast.Visitor {
+	var selectorExpr *ast.SelectorExpr
 	switch node := node.(type) {
 	case *ast.FuncDecl:
 		// don't descend into godoc examples if we are ignoring them
@@ -164,14 +174,57 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor {
 		}
 		return v
 	case *ast.SelectorExpr:
+		selectorExpr = node
 	case *ast.Ident:
 	default:
 		return v
 	}
+
+	// The text as it appears in the source is always used because issues
+	// use that. The other texts to match against are extracted when needed
+	// by a pattern.
+	srcText := v.textFor(node)
+	pkgText := ""
 	for _, p := range v.linter.patterns {
-		if p.pattern.MatchString(v.textFor(node)) && !v.permit(node) {
+		if p.matchPackage && pkgText == "" {
+			if v.typesInfo == nil {
+				continue
+			}
+			if selectorExpr == nil {
+				// Not a selector at all.
+				continue
+			}
+			selector := selectorExpr.X
+			ident, ok := selector.(*ast.Ident)
+			if !ok {
+				// Not an identifier.
+				continue
+			}
+			object, ok := v.typesInfo.Uses[ident]
+			if !ok {
+				// No information about the identifier. Should
+				// not happen, but perhaps there were compile
+				// errors?
+				continue
+			}
+			pkgName, ok := object.(*types.PkgName)
+			if !ok {
+				// No package name, cannot match.
+				continue
+			}
+			pkgText = pkgName.Imported().Path() + "." + selectorExpr.Sel.Name
+		}
+
+		matchText := ""
+		switch {
+		case p.matchPackage:
+			matchText = pkgText
+		default:
+			matchText = srcText
+		}
+		if p.pattern.MatchString(matchText) && !v.permit(node) {
 			v.issues = append(v.issues, UsedIssue{
-				identifier: v.textFor(node),
+				identifier: srcText, // Always report the expression as it appears in the source code.
 				pattern:    p.pattern.String(),
 				pos:        node.Pos(),
 				position:   v.fset.Position(node.Pos()),
@@ -182,6 +235,7 @@ func (v *visitor) Visit(node ast.Node) ast.Visitor {
 	return nil
 }
 
+// textFor returns the function as it appears in the source code (= <importname>.<function name>).
 func (v *visitor) textFor(node ast.Node) string {
 	buf := new(bytes.Buffer)
 	if err := printer.Fprint(buf, v.fset, node); err != nil {

forbidigo/forbidigo_test.go

@@ -1,11 +1,16 @@
 package forbidigo
 
 import (
-	"go/parser"
-	"go/token"
+	"go/ast"
+	"log"
+	"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) {
@@ -19,6 +24,18 @@ 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, `
+package bar
+
+import renamed "fmt"
+
+func foo() {
+	renamed.Printf("here i am")
+}` /* only detected inside golanci-lint */)
+	})
+
 	t.Run("displays custom messages", func(t *testing.T) {
 		linter, _ := NewLinter([]string{`^fmt\.Printf(# a custom message)?$`})
 		expectIssues(t, linter, `
@@ -134,24 +151,86 @@ func ExampleFoo() {
 }`)
 		assert.NotEmpty(t, issues)
 	})
+
+	t.Run("import renames not detected by simple pattern", func(t *testing.T) {
+		linter, _ := NewLinter([]string{`fmt\.Printf`}, OptionExcludeGodocExamples(false))
+		issues := parseFile(t, linter, "file.go", `
+package bar
+
+import fmt2 "fmt"
+
+func ExampleFoo() {
+	fmt2.Printf("here i am")
+}`)
+		assert.Empty(t, issues)
+	})
+
+	t.Run("import renames detected by package pattern", func(t *testing.T) {
+		linter, _ := NewLinter([]string{`(?P<pkg>fmt)\.Printf`}, OptionExcludeGodocExamples(false))
+		expectIssues(t, linter, `
+package bar
+
+import fmt2 "fmt"
+
+func ExampleFoo() {
+	fmt2.Printf("here i am")
+}`, "use of `fmt2.Printf` forbidden by pattern `(?P<pkg>fmt)\\.Printf` at testing.go:7:2")
+	})
+
 }
 
+// sourcePath matches "at /tmp/TestForbiddenIdentifiersdisplays_custom_messages4260088387/001/testing.go".
+var sourcePath = regexp.MustCompile(`at .*/([[:alnum:]]+.go)`)
+
 func expectIssues(t *testing.T, linter *Linter, contents string, issues ...string) {
 	actualIssues := parseFile(t, linter, "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)
+	// 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 | packages.NeedTypesInfo | packages.NeedDeps,
+		Env:   env,
+		Tests: true,
+	}
+	pwd, err := os.Getwd()
+	require.NoError(t, err)
+	defer 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.RunWithTypes(p.Fset, p.TypesInfo, nodes...)
+		if err != nil {
+			log.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)
 	}

forbidigo/patterns.go

@@ -8,8 +8,9 @@ import (
 )
 
 type pattern struct {
-	pattern *regexp.Regexp
-	msg     string
+	pattern      *regexp.Regexp
+	msg          string
+	matchPackage bool
 }
 
 func parse(ptrn string) (*pattern, error) {
@@ -22,7 +23,14 @@ func parse(ptrn string) (*pattern, error) {
 		return nil, fmt.Errorf("unable to parse pattern `%s`: %s", ptrn, err)
 	}
 	msg := extractComment(re)
-	return &pattern{pattern: ptrnRe, msg: msg}, nil
+	matchPackage := false
+	for _, groupName := range ptrnRe.SubexpNames() {
+		switch groupName {
+		case "pkg":
+			matchPackage = true
+		}
+	}
+	return &pattern{pattern: ptrnRe, msg: msg, matchPackage: matchPackage}, nil
 }
 
 // Traverse the leaf submatches in the regex tree and extract a comment, if any

forbidigo/patterns_test.go

@@ -9,9 +9,10 @@ import (
 
 func TestParseValidPatterns(t *testing.T) {
 	for _, tc := range []struct {
-		name            string
-		ptrn            string
-		expectedComment string
+		name                 string
+		ptrn                 string
+		expectedComment      string
+		expectedMatchPackage bool
 	}{
 		{
 			name: "simple expression, no comment",
@@ -41,12 +42,18 @@ func TestParseValidPatterns(t *testing.T) {
 			ptrn:            `^fmt\.Println(# Please don't use this!)?$`,
 			expectedComment: "Please don't use this!",
 		},
+		{
+			name:                 "match package with non-empty group",
+			ptrn:                 `^(?P<pkg>fmt).Println$`,
+			expectedMatchPackage: true,
+		},
 	} {
 		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)
+			assert.Equal(t, tc.expectedMatchPackage, ptrn.matchPackage, "match pattern")
 		})
 	}
 }

go.mod

@@ -5,5 +5,5 @@ 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
 )