diff --git a/.golangci.example.yml b/.golangci.example.yml index d19b4dd6e147..49e1d9a56462 100644 --- a/.golangci.example.yml +++ b/.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 diff --git a/README.md b/README.md index 3b6d2137b80d..fe76e671b30a 100644 --- a/README.md +++ b/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 diff --git a/pkg/config/config.go b/pkg/config/config.go index b92b2428da38..4edccfa3b0ca 100644 --- a/pkg/config/config.go +++ b/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 { diff --git a/pkg/config/reader.go b/pkg/config/reader.go index df72632fa3cb..9ca928bdd037 100644 --- a/pkg/config/reader.go +++ b/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 } diff --git a/pkg/golinters/govet.go b/pkg/golinters/govet.go index 1aae17dc9fbc..b3b7b61a2a03 100644 --- a/pkg/golinters/govet.go +++ b/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, ) } diff --git a/pkg/golinters/govet_test.go b/pkg/golinters/govet_test.go new file mode 100644 index 000000000000..65c6ca32b093 --- /dev/null +++ b/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) + } + } +} diff --git a/pkg/lint/runner.go b/pkg/lint/runner.go index 157d2e4ed751..77a70e5af34f 100644 --- a/pkg/lint/runner.go +++ b/pkg/lint/runner.go @@ -92,8 +92,6 @@ func NewRunner(astCache *astcache.Cache, cfg *config.Config, log logutils.Log, g }, nil } -func someUnusedFunc() {} - type lintRes struct { linter *linter.Config err error diff --git a/vendor/golang.org/x/tools/go/analysis/passes/atomicalign/atomicalign.go b/vendor/golang.org/x/tools/go/analysis/passes/atomicalign/atomicalign.go new file mode 100644 index 000000000000..d3fc3e2d731a --- /dev/null +++ b/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 +} diff --git a/vendor/modules.txt b/vendor/modules.txt index c355340e4def..d809a9d0c91b 100644 --- a/vendor/modules.txt +++ b/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