Merge pull request #198 from dtcaciuc/analyzer

Provide go/analysis analyzer instance

Author: dtcaciuc

README.md

@@ -36,6 +36,14 @@ takes no arguments.
 The `-blank` flag enables checking for assignments of errors to the
 blank identifier. It takes no arguments.
 
+### go/analysis
+
+The package provides `Analyzer` instance that can be used with
+[go/analysis](https://pkg.go.dev/golang.org/x/tools/go/analysis) API.
+
+Currently supported flags are `blank`, `assert`, `exclude`, and `excludeonly`.
+Just as the API itself, the analyzer is exprimental and may change in the
+future.
 
 ## Excluding functions
 

errcheck/analyzer.go

@@ -0,0 +1,78 @@
+package errcheck
+
+import (
+	"fmt"
+	"go/ast"
+	"go/token"
+	"reflect"
+	"regexp"
+
+	"golang.org/x/tools/go/analysis"
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name:       "errcheck",
+	Doc:        "check for unchecked errors",
+	Run:        runAnalyzer,
+	ResultType: reflect.TypeOf(Result{}),
+}
+
+var (
+	argBlank       bool
+	argAsserts     bool
+	argExcludeFile string
+	argExcludeOnly bool
+)
+
+func init() {
+	Analyzer.Flags.BoolVar(&argBlank, "blank", false, "if true, check for errors assigned to blank identifier")
+	Analyzer.Flags.BoolVar(&argAsserts, "assert", false, "if true, check for ignored type assertion results")
+	Analyzer.Flags.StringVar(&argExcludeFile, "exclude", "", "Path to a file containing a list of functions to exclude from checking")
+	Analyzer.Flags.BoolVar(&argExcludeOnly, "excludeonly", false, "Use only excludes from exclude file")
+}
+
+func runAnalyzer(pass *analysis.Pass) (interface{}, error) {
+
+	exclude := map[string]bool{}
+	if !argExcludeOnly {
+		for _, name := range DefaultExcludedSymbols {
+			exclude[name] = true
+		}
+	}
+	if argExcludeFile != "" {
+		excludes, err := ReadExcludes(argExcludeFile)
+		if err != nil {
+			return nil, fmt.Errorf("Could not read exclude file: %v\n", err)
+		}
+		for _, name := range excludes {
+			exclude[name] = true
+		}
+	}
+
+	var allErrors []UncheckedError
+	for _, f := range pass.Files {
+		v := &visitor{
+			typesInfo: pass.TypesInfo,
+			fset:      pass.Fset,
+			blank:     argBlank,
+			asserts:   argAsserts,
+			exclude:   exclude,
+			ignore:    map[string]*regexp.Regexp{}, // deprecated & not used
+			lines:     make(map[string][]string),
+			errors:    nil,
+		}
+
+		ast.Walk(v, f)
+
+		for _, err := range v.errors {
+			pass.Report(analysis.Diagnostic{
+				Pos:     token.Pos(int(f.Pos()) + err.Pos.Offset),
+				Message: "unchecked error",
+			})
+		}
+
+		allErrors = append(allErrors, v.errors...)
+	}
+
+	return Result{UncheckedErrors: allErrors}, nil
+}

errcheck/errcheck.go

@@ -25,49 +25,6 @@ func init() {
 var (
 	// ErrNoGoFiles is returned when CheckPackage is run on a package with no Go source files
 	ErrNoGoFiles = errors.New("package contains no go source files")
-
-	// DefaultExcludedSymbols is a list of symbol names that are usually excluded from checks by default.
-	//
-	// Note, that they still need to be explicitly copied to Checker.Exclusions.Symbols
-	DefaultExcludedSymbols = []string{
-		// bytes
-		"(*bytes.Buffer).Write",
-		"(*bytes.Buffer).WriteByte",
-		"(*bytes.Buffer).WriteRune",
-		"(*bytes.Buffer).WriteString",
-
-		// fmt
-		"fmt.Errorf",
-		"fmt.Print",
-		"fmt.Printf",
-		"fmt.Println",
-		"fmt.Fprint(*bytes.Buffer)",
-		"fmt.Fprintf(*bytes.Buffer)",
-		"fmt.Fprintln(*bytes.Buffer)",
-		"fmt.Fprint(*strings.Builder)",
-		"fmt.Fprintf(*strings.Builder)",
-		"fmt.Fprintln(*strings.Builder)",
-		"fmt.Fprint(os.Stderr)",
-		"fmt.Fprintf(os.Stderr)",
-		"fmt.Fprintln(os.Stderr)",
-
-		// io
-		"(*io.PipeReader).CloseWithError",
-		"(*io.PipeWriter).CloseWithError",
-
-		// math/rand
-		"math/rand.Read",
-		"(*math/rand.Rand).Read",
-
-		// strings
-		"(*strings.Builder).Write",
-		"(*strings.Builder).WriteByte",
-		"(*strings.Builder).WriteRune",
-		"(*strings.Builder).WriteString",
-
-		// hash
-		"(hash.Hash).Write",
-	}
 )
 
 // UncheckedError indicates the position of an unchecked error return.
@@ -261,16 +218,17 @@ func (c *Checker) CheckPackage(pkg *packages.Package) Result {
 	}
 
 	v := &visitor{
-		pkg:     pkg,
-		ignore:  ignore,
-		blank:   !c.Exclusions.BlankAssignments,
-		asserts: !c.Exclusions.TypeAssertions,
-		lines:   make(map[string][]string),
-		exclude: excludedSymbols,
-		errors:  []UncheckedError{},
+		typesInfo: pkg.TypesInfo,
+		fset:      pkg.Fset,
+		ignore:    ignore,
+		blank:     !c.Exclusions.BlankAssignments,
+		asserts:   !c.Exclusions.TypeAssertions,
+		lines:     make(map[string][]string),
+		exclude:   excludedSymbols,
+		errors:    []UncheckedError{},
 	}
 
-	for _, astFile := range v.pkg.Syntax {
+	for _, astFile := range pkg.Syntax {
 		if c.shouldSkipFile(astFile) {
 			continue
 		}
@@ -281,12 +239,13 @@ func (c *Checker) CheckPackage(pkg *packages.Package) Result {
 
 // visitor implements the errcheck algorithm
 type visitor struct {
-	pkg     *packages.Package
-	ignore  map[string]*regexp.Regexp
-	blank   bool
-	asserts bool
-	lines   map[string][]string
-	exclude map[string]bool
+	typesInfo *types.Info
+	fset      *token.FileSet
+	ignore    map[string]*regexp.Regexp
+	blank     bool
+	asserts   bool
+	lines     map[string][]string
+	exclude   map[string]bool
 
 	errors []UncheckedError
 }
@@ -306,7 +265,7 @@ func (v *visitor) selectorAndFunc(call *ast.CallExpr) (*ast.SelectorExpr, *types
 		return nil, nil, false
 	}
 
-	fn, ok := v.pkg.TypesInfo.ObjectOf(sel.Sel).(*types.Func)
+	fn, ok := v.typesInfo.ObjectOf(sel.Sel).(*types.Func)
 	if !ok {
 		// Shouldn't happen, but be paranoid
 		return nil, nil, false
@@ -393,7 +352,7 @@ func (v *visitor) namesForExcludeCheck(call *ast.CallExpr) []string {
 
 	// This will be missing for functions without a receiver (like fmt.Printf),
 	// so just fall back to the the function's fullName in that case.
-	selection, ok := v.pkg.TypesInfo.Selections[sel]
+	selection, ok := v.typesInfo.Selections[sel]
 	if !ok {
 		return []string{name}
 	}
@@ -420,14 +379,14 @@ func (v *visitor) namesForExcludeCheck(call *ast.CallExpr) []string {
 func (v *visitor) argName(expr ast.Expr) string {
 	// Special-case literal "os.Stdout" and "os.Stderr"
 	if sel, ok := expr.(*ast.SelectorExpr); ok {
-		if obj := v.pkg.TypesInfo.ObjectOf(sel.Sel); obj != nil {
+		if obj := v.typesInfo.ObjectOf(sel.Sel); obj != nil {
 			vr, ok := obj.(*types.Var)
 			if ok && vr.Pkg() != nil && vr.Pkg().Name() == "os" && (vr.Name() == "Stderr" || vr.Name() == "Stdout") {
 				return "os." + vr.Name()
 			}
 		}
 	}
-	t := v.pkg.TypesInfo.TypeOf(expr)
+	t := v.typesInfo.TypeOf(expr)
 	if t == nil {
 		return ""
 	}
@@ -478,7 +437,7 @@ func (v *visitor) ignoreCall(call *ast.CallExpr) bool {
 		return true
 	}
 
-	if obj := v.pkg.TypesInfo.Uses[id]; obj != nil {
+	if obj := v.typesInfo.Uses[id]; obj != nil {
 		if pkg := obj.Pkg(); pkg != nil {
 			if re, ok := v.ignore[nonVendoredPkgPath(pkg.Path())]; ok {
 				return re.MatchString(id.Name)
@@ -504,7 +463,7 @@ func nonVendoredPkgPath(pkgPath string) string {
 // len(s) == number of return types of call
 // s[i] == true iff return type at position i from left is an error type
 func (v *visitor) errorsByArg(call *ast.CallExpr) []bool {
-	switch t := v.pkg.TypesInfo.Types[call].Type.(type) {
+	switch t := v.typesInfo.Types[call].Type.(type) {
 	case *types.Named:
 		// Single return
 		return []bool{isErrorType(t)}
@@ -546,15 +505,18 @@ func (v *visitor) callReturnsError(call *ast.CallExpr) bool {
 // isRecover returns true if the given CallExpr is a call to the built-in recover() function.
 func (v *visitor) isRecover(call *ast.CallExpr) bool {
 	if fun, ok := call.Fun.(*ast.Ident); ok {
-		if _, ok := v.pkg.TypesInfo.Uses[fun].(*types.Builtin); ok {
+		if _, ok := v.typesInfo.Uses[fun].(*types.Builtin); ok {
 			return fun.Name == "recover"
 		}
 	}
 	return false
 }
 
+// TODO (dtcaciuc) collect token.Pos and then convert them to UncheckedErrors
+// after visitor is done running. This will allow to integrate more cleanly
+// with analyzer so that we don't have to convert Position back to Pos.
 func (v *visitor) addErrorAtPosition(position token.Pos, call *ast.CallExpr) {
-	pos := v.pkg.Fset.Position(position)
+	pos := v.fset.Position(position)
 	lines, ok := v.lines[pos.Filename]
 	if !ok {
 		lines = readfile(pos.Filename)

errcheck/excludes.go

@@ -0,0 +1,83 @@
+package errcheck
+
+import (
+	"bufio"
+	"bytes"
+	"io/ioutil"
+	"strings"
+)
+
+var (
+	// DefaultExcludedSymbols is a list of symbol names that are usually excluded from checks by default.
+	//
+	// Note, that they still need to be explicitly copied to Checker.Exclusions.Symbols
+	DefaultExcludedSymbols = []string{
+		// bytes
+		"(*bytes.Buffer).Write",
+		"(*bytes.Buffer).WriteByte",
+		"(*bytes.Buffer).WriteRune",
+		"(*bytes.Buffer).WriteString",
+
+		// fmt
+		"fmt.Errorf",
+		"fmt.Print",
+		"fmt.Printf",
+		"fmt.Println",
+		"fmt.Fprint(*bytes.Buffer)",
+		"fmt.Fprintf(*bytes.Buffer)",
+		"fmt.Fprintln(*bytes.Buffer)",
+		"fmt.Fprint(*strings.Builder)",
+		"fmt.Fprintf(*strings.Builder)",
+		"fmt.Fprintln(*strings.Builder)",
+		"fmt.Fprint(os.Stderr)",
+		"fmt.Fprintf(os.Stderr)",
+		"fmt.Fprintln(os.Stderr)",
+
+		// io
+		"(*io.PipeReader).CloseWithError",
+		"(*io.PipeWriter).CloseWithError",
+
+		// math/rand
+		"math/rand.Read",
+		"(*math/rand.Rand).Read",
+
+		// strings
+		"(*strings.Builder).Write",
+		"(*strings.Builder).WriteByte",
+		"(*strings.Builder).WriteRune",
+		"(*strings.Builder).WriteString",
+
+		// hash
+		"(hash.Hash).Write",
+	}
+)
+
+// ReadExcludes reads an excludes file, a newline delimited file that lists
+// patterns for which to allow unchecked errors.
+//
+// Lines that start with two forward slashes are considered comments and are ignored.
+//
+func ReadExcludes(path string) ([]string, error) {
+	var excludes []string
+
+	buf, err := ioutil.ReadFile(path)
+	if err != nil {
+		return nil, err
+	}
+
+	scanner := bufio.NewScanner(bytes.NewReader(buf))
+
+	for scanner.Scan() {
+		name := scanner.Text()
+		// Skip comments and empty lines.
+		if strings.HasPrefix(name, "//") || name == "" {
+			continue
+		}
+		excludes = append(excludes, name)
+	}
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+
+	return excludes, nil
+}

errcheck/excludes_test.go

@@ -0,0 +1,39 @@
+package errcheck
+
+import (
+	"reflect"
+	"testing"
+)
+
+func TestReadExcludes(t *testing.T) {
+	expectedExcludes := []string{
+		"hello()",
+		"world()",
+	}
+	t.Logf("expectedExcludes: %#v", expectedExcludes)
+	excludes, err := ReadExcludes("testdata/excludes.txt")
+	if err != nil {
+		t.Fatal(err)
+	}
+	t.Logf("excludes: %#v", excludes)
+	if !reflect.DeepEqual(expectedExcludes, excludes) {
+		t.Fatal("excludes did not match expectedExcludes")
+	}
+}
+
+func TestReadEmptyExcludes(t *testing.T) {
+	excludes, err := ReadExcludes("testdata/empty_excludes.txt")
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(excludes) != 0 {
+		t.Fatalf("expected empty excludes, got %#v", excludes)
+	}
+}
+
+func TestReadExcludesMissingFile(t *testing.T) {
+	_, err := ReadExcludes("testdata/missing_file")
+	if err == nil {
+		t.Fatal("expected non-nil err, got nil")
+	}
+}

errcheck/testdata/custom_exclude.txt

@@ -0,0 +1 @@
+(custom_exclude.ErrorMakerInterfaceWrapper).MakeNilError