govet: implement analyzers config (#697)

.golangci.example.yml

@@ -95,6 +95,14 @@ linters-settings:
           - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
           - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
           - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf
+
+    # enable or disable analyzers by name
+    enable:
+      - atomicalign
+    enable-all: false
+    disable:
+      - shadow
+    disable-all: false
   golint:
     # minimal confidence for issues, default is 0.8
     min-confidence: 0.8

README.md

@@ -677,6 +677,14 @@ linters-settings:
           - (github.com/golangci/golangci-lint/pkg/logutils.Log).Warnf
           - (github.com/golangci/golangci-lint/pkg/logutils.Log).Errorf
           - (github.com/golangci/golangci-lint/pkg/logutils.Log).Fatalf
+
+    # enable or disable analyzers by name
+    enable:
+      - atomicalign
+    enable-all: false
+    disable:
+      - shadow
+    disable-all: false
   golint:
     # minimal confidence for issues, default is 0.8
     min-confidence: 0.8

pkg/config/config.go

@@ -183,6 +183,24 @@ type LintersSettings struct {
 type GovetSettings struct {
 	CheckShadowing bool `mapstructure:"check-shadowing"`
 	Settings       map[string]map[string]interface{}
+
+	Enable     []string
+	Disable    []string
+	EnableAll  bool `mapstructure:"enable-all"`
+	DisableAll bool `mapstructure:"disable-all"`
+}
+
+func (cfg GovetSettings) Validate() error {
+	if cfg.EnableAll && cfg.DisableAll {
+		return errors.New("enable-all and disable-all can't be combined")
+	}
+	if cfg.EnableAll && len(cfg.Enable) != 0 {
+		return errors.New("enable-all and enable can't be combined")
+	}
+	if cfg.DisableAll && len(cfg.Disable) != 0 {
+		return errors.New("disable-all and disable can't be combined")
+	}
+	return nil
 }
 
 type ErrcheckSettings struct {

pkg/config/reader.go

@@ -110,7 +110,9 @@ func (r *FileReader) validateConfig() error {
 			return fmt.Errorf("error in exclude rule #%d: %v", i, err)
 		}
 	}
-
+	if err := c.LintersSettings.Govet.Validate(); err != nil {
+		return fmt.Errorf("error in govet config: %v", err)
+	}
 	return nil
 }
 

pkg/golinters/govet.go

@@ -11,6 +11,7 @@ import (
 	"golang.org/x/tools/go/analysis/passes/asmdecl"
 	"golang.org/x/tools/go/analysis/passes/assign"
 	"golang.org/x/tools/go/analysis/passes/atomic"
+	"golang.org/x/tools/go/analysis/passes/atomicalign"
 	"golang.org/x/tools/go/analysis/passes/bools"
 	"golang.org/x/tools/go/analysis/passes/buildtag"
 	"golang.org/x/tools/go/analysis/passes/cgocall"
@@ -33,9 +34,37 @@ import (
 	"golang.org/x/tools/go/analysis/passes/unusedresult"
 )
 
-func NewGovet(cfg *config.GovetSettings) *goanalysis.Linter {
-	analyzers := []*analysis.Analyzer{
-		// the traditional vet suite:
+func getAllAnalyzers() []*analysis.Analyzer {
+	return []*analysis.Analyzer{
+		asmdecl.Analyzer,
+		assign.Analyzer,
+		atomic.Analyzer,
+		atomicalign.Analyzer,
+		bools.Analyzer,
+		buildtag.Analyzer,
+		cgocall.Analyzer,
+		composite.Analyzer,
+		copylock.Analyzer,
+		errorsas.Analyzer,
+		httpresponse.Analyzer,
+		loopclosure.Analyzer,
+		lostcancel.Analyzer,
+		nilfunc.Analyzer,
+		printf.Analyzer,
+		shadow.Analyzer,
+		shift.Analyzer,
+		stdmethods.Analyzer,
+		structtag.Analyzer,
+		tests.Analyzer,
+		unmarshal.Analyzer,
+		unreachable.Analyzer,
+		unsafeptr.Analyzer,
+		unusedresult.Analyzer,
+	}
+}
+
+func getDefaultAnalyzers() []*analysis.Analyzer {
+	return []*analysis.Analyzer{
 		asmdecl.Analyzer,
 		assign.Analyzer,
 		atomic.Analyzer,
@@ -59,20 +88,64 @@ func NewGovet(cfg *config.GovetSettings) *goanalysis.Linter {
 		unsafeptr.Analyzer,
 		unusedresult.Analyzer,
 	}
+}
 
+func isAnalyzerEnabled(name string, cfg *config.GovetSettings, defaultAnalyzers []*analysis.Analyzer) bool {
+	if cfg.EnableAll {
+		return true
+	}
+	// Raw for loops should be OK on small slice lengths.
+	for _, n := range cfg.Enable {
+		if n == name {
+			return true
+		}
+	}
+	for _, n := range cfg.Disable {
+		if n == name {
+			return false
+		}
+	}
+	if cfg.DisableAll {
+		return false
+	}
+	for _, a := range defaultAnalyzers {
+		if a.Name == name {
+			return true
+		}
+	}
+	return false
+}
+
+func analyzersFromConfig(cfg *config.GovetSettings) []*analysis.Analyzer {
+	if cfg == nil {
+		return getDefaultAnalyzers()
+	}
+	if cfg.CheckShadowing {
+		// Keeping for backward compatibility.
+		cfg.Enable = append(cfg.Enable, shadow.Analyzer.Name)
+	}
+
+	var enabledAnalyzers []*analysis.Analyzer
+	defaultAnalyzers := getDefaultAnalyzers()
+	for _, a := range getAllAnalyzers() {
+		if isAnalyzerEnabled(a.Name, cfg, defaultAnalyzers) {
+			enabledAnalyzers = append(enabledAnalyzers, a)
+		}
+	}
+
+	return enabledAnalyzers
+}
+
+func NewGovet(cfg *config.GovetSettings) *goanalysis.Linter {
 	var settings map[string]map[string]interface{}
 	if cfg != nil {
-		if cfg.CheckShadowing {
-			analyzers = append(analyzers, shadow.Analyzer)
-		}
 		settings = cfg.Settings
 	}
-
 	return goanalysis.NewLinter(
 		"govet",
 		"Vet examines Go source code and reports suspicious constructs, "+
 			"such as Printf calls whose arguments do not align with the format string",
-		analyzers,
+		analyzersFromConfig(cfg),
 		settings,
 	)
 }

pkg/golinters/govet_test.go

@@ -0,0 +1,94 @@
+package golinters
+
+import (
+	"sort"
+	"testing"
+
+	"github.com/golangci/golangci-lint/pkg/config"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/asmdecl"
+	"golang.org/x/tools/go/analysis/passes/assign"
+	"golang.org/x/tools/go/analysis/passes/atomic"
+	"golang.org/x/tools/go/analysis/passes/bools"
+	"golang.org/x/tools/go/analysis/passes/buildtag"
+	"golang.org/x/tools/go/analysis/passes/cgocall"
+	"golang.org/x/tools/go/analysis/passes/shadow"
+)
+
+func TestGovet(t *testing.T) {
+	// Checking that every default analyzer is in "all analyzers" list.
+	allAnalyzers := getAllAnalyzers()
+	checkList := append(getDefaultAnalyzers(),
+		shadow.Analyzer, // special case, used in analyzersFromConfig
+	)
+	for _, defaultAnalyzer := range checkList {
+		found := false
+		for _, a := range allAnalyzers {
+			if a.Name == defaultAnalyzer.Name {
+				found = true
+				break
+			}
+		}
+		if !found {
+			t.Errorf("%s is not in allAnalyzers", defaultAnalyzer.Name)
+		}
+	}
+}
+
+type sortedAnalyzers []*analysis.Analyzer
+
+func (p sortedAnalyzers) Len() int           { return len(p) }
+func (p sortedAnalyzers) Less(i, j int) bool { return p[i].Name < p[j].Name }
+func (p sortedAnalyzers) Swap(i, j int)      { p[i].Name, p[j].Name = p[j].Name, p[i].Name }
+
+func TestGovetSorted(t *testing.T) {
+	// Keeping analyzers sorted so their order match the import order.
+	t.Run("All", func(t *testing.T) {
+		if !sort.IsSorted(sortedAnalyzers(getAllAnalyzers())) {
+			t.Error("please keep all analyzers list sorted by name")
+		}
+	})
+	t.Run("Default", func(t *testing.T) {
+		if !sort.IsSorted(sortedAnalyzers(getDefaultAnalyzers())) {
+			t.Error("please keep default analyzers list sorted by name")
+		}
+	})
+}
+
+func TestGovetAnalyzerIsEnabled(t *testing.T) {
+	defaultAnalyzers := []*analysis.Analyzer{
+		asmdecl.Analyzer,
+		assign.Analyzer,
+		atomic.Analyzer,
+		bools.Analyzer,
+		buildtag.Analyzer,
+		cgocall.Analyzer,
+	}
+	for _, tc := range []struct {
+		Enable     []string
+		Disable    []string
+		EnableAll  bool
+		DisableAll bool
+
+		Name    string
+		Enabled bool
+	}{
+		{Name: "assign", Enabled: true},
+		{Name: "cgocall", Enabled: false, DisableAll: true},
+		{Name: "errorsas", Enabled: false},
+		{Name: "bools", Enabled: false, Disable: []string{"bools"}},
+		{Name: "unsafeptr", Enabled: true, Enable: []string{"unsafeptr"}},
+		{Name: "shift", Enabled: true, EnableAll: true},
+	} {
+		cfg := &config.GovetSettings{
+			Enable:     tc.Enable,
+			Disable:    tc.Disable,
+			EnableAll:  tc.EnableAll,
+			DisableAll: tc.DisableAll,
+		}
+		if enabled := isAnalyzerEnabled(tc.Name, cfg, defaultAnalyzers); enabled != tc.Enabled {
+			t.Errorf("%+v", tc)
+		}
+	}
+}

vendor/golang.org/x/tools/go/analysis/passes/atomicalign/atomicalign.go

@@ -0,0 +1,126 @@
+// Copyright 2019 The Go Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style
+// license that can be found in the LICENSE file.
+
+// Package atomicalign defines an Analyzer that checks for non-64-bit-aligned
+// arguments to sync/atomic functions. On non-32-bit platforms, those functions
+// panic if their argument variables are not 64-bit aligned. It is therefore
+// the caller's responsibility to arrange for 64-bit alignment of such variables.
+// See https://golang.org/pkg/sync/atomic/#pkg-note-BUG
+package atomicalign
+
+import (
+	"go/ast"
+	"go/token"
+	"go/types"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name:     "atomicalign",
+	Doc:      "check for non-64-bits-aligned arguments to sync/atomic functions",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	if 8*pass.TypesSizes.Sizeof(types.Typ[types.Uintptr]) == 64 {
+		return nil, nil // 64-bit platform
+	}
+	if imports(pass.Pkg, "sync/atomic") == nil {
+		return nil, nil // doesn't directly import sync/atomic
+	}
+
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	nodeFilter := []ast.Node{
+		(*ast.CallExpr)(nil),
+	}
+
+	inspect.Preorder(nodeFilter, func(node ast.Node) {
+		call := node.(*ast.CallExpr)
+		sel, ok := call.Fun.(*ast.SelectorExpr)
+		if !ok {
+			return
+		}
+		pkgIdent, ok := sel.X.(*ast.Ident)
+		if !ok {
+			return
+		}
+		pkgName, ok := pass.TypesInfo.Uses[pkgIdent].(*types.PkgName)
+		if !ok || pkgName.Imported().Path() != "sync/atomic" {
+			return
+		}
+
+		switch sel.Sel.Name {
+		case "AddInt64", "AddUint64",
+			"LoadInt64", "LoadUint64",
+			"StoreInt64", "StoreUint64",
+			"SwapInt64", "SwapUint64",
+			"CompareAndSwapInt64", "CompareAndSwapUint64":
+
+			// For all the listed functions, the expression to check is always the first function argument.
+			check64BitAlignment(pass, sel.Sel.Name, call.Args[0])
+		}
+	})
+
+	return nil, nil
+}
+
+func check64BitAlignment(pass *analysis.Pass, funcName string, arg ast.Expr) {
+	// Checks the argument is made of the address operator (&) applied to
+	// to a struct field (as opposed to a variable as the first word of
+	// uint64 and int64 variables can be relied upon to be 64-bit aligned.
+	unary, ok := arg.(*ast.UnaryExpr)
+	if !ok || unary.Op != token.AND {
+		return
+	}
+
+	// Retrieve the types.Struct in order to get the offset of the
+	// atomically accessed field.
+	sel, ok := unary.X.(*ast.SelectorExpr)
+	if !ok {
+		return
+	}
+	tvar, ok := pass.TypesInfo.Selections[sel].Obj().(*types.Var)
+	if !ok || !tvar.IsField() {
+		return
+	}
+
+	stype, ok := pass.TypesInfo.Types[sel.X].Type.Underlying().(*types.Struct)
+	if !ok {
+		return
+	}
+
+	var offset int64
+	var fields []*types.Var
+	for i := 0; i < stype.NumFields(); i++ {
+		f := stype.Field(i)
+		fields = append(fields, f)
+		if f == tvar {
+			// We're done, this is the field we were looking for,
+			// no need to fill the fields slice further.
+			offset = pass.TypesSizes.Offsetsof(fields)[i]
+			break
+		}
+	}
+	if offset&7 == 0 {
+		return // 64-bit aligned
+	}
+
+	pass.Reportf(arg.Pos(), "address of non 64-bit aligned field .%s passed to atomic.%s", tvar.Name(), funcName)
+}
+
+// imports reports whether pkg has path among its direct imports.
+// It returns the imported package if so, or nil if not.
+// copied from passes/cgocall.
+func imports(pkg *types.Package, path string) *types.Package {
+	for _, imp := range pkg.Imports() {
+		if imp.Path() == path {
+			return imp
+		}
+	}
+	return nil
+}

vendor/modules.txt

@@ -207,6 +207,7 @@ golang.org/x/tools/go/analysis
 golang.org/x/tools/go/analysis/passes/asmdecl
 golang.org/x/tools/go/analysis/passes/assign
 golang.org/x/tools/go/analysis/passes/atomic
+golang.org/x/tools/go/analysis/passes/atomicalign
 golang.org/x/tools/go/analysis/passes/bools
 golang.org/x/tools/go/analysis/passes/buildssa
 golang.org/x/tools/go/analysis/passes/buildtag