Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

govet: implement analyzers config #697

Merged
merged 4 commits into from
Sep 15, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .golangci.example.yml
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
8 changes: 8 additions & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
18 changes: 18 additions & 0 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
4 changes: 3 additions & 1 deletion pkg/config/reader.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
89 changes: 81 additions & 8 deletions pkg/golinters/govet.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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,
Expand All @@ -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 {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please, add minimal validation to (r *FileReader) validateConfig() by calling something like:

func (cfg *config.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
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, thank you.
We probably want to validate that cfg.Enable contains valid analyzer names, but I'm not sure how to do it in that scope.

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,
)
}
94 changes: 94 additions & 0 deletions pkg/golinters/govet_test.go
Original file line number Diff line number Diff line change
@@ -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)
}
}
}
2 changes: 0 additions & 2 deletions pkg/lint/runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading