Update gocritic

Fix #324, relates #314

1. Update gocritic to the latest version
2. Use proper gocritic checkers repo, old repo was archived
3. Get enabled by default gocritic checks in sync with go-critic: don't
enable performance, experimental and opinionated checks by default
4. Support of `enabled-tags` options for gocritic
5. Enable almost all gocritic checks for the project
6. Make rich debugging for gocritic
7. Meticulously validate gocritic checks config
This commit is contained in:
Denis Isaev 2019-01-09 00:11:34 +03:00 committed by Isaev Denis
parent 7705f82591
commit 87aae77943
86 changed files with 1155 additions and 467 deletions

View File

@ -150,17 +150,22 @@ linters-settings:
range-loops: true # Report preallocation suggestions on range loops, true by default
for-loops: false # Report preallocation suggestions on for loops, false by default
gocritic:
# which checks should be enabled; can't be combined with 'disabled-checks';
# default are: [appendAssign appendCombine assignOp builtinShadow captLocal caseOrder defaultCaseOrder
# dupArg dupBranchBody dupCase elseif flagDeref ifElseChain importShadow indexAlloc paramTypeCombine
# rangeExprCopy rangeValCopy regexpMust singleCaseSwitch sloppyLen switchTrue typeSwitchVar typeUnparen
# underef unlambda unslice dupSubExpr hugeParam];
# all checks list: https://github.com/go-critic/checkers
# Which checks should be enabled; can't be combined with 'disabled-checks';
# See https://go-critic.github.io/overview#checks-overview
# To check which checks are enabled run `GL_DEBUG=gocritic golangci-lint run`
# By default list of stable checks is used.
enabled-checks:
- rangeValCopy
# which checks should be disabled; can't be combined with 'enabled-checks'; default is empty
# Which checks should be disabled; can't be combined with 'enabled-checks'; default is empty
disabled-checks:
- regexpMust
# Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks.
# Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags".
enabled-tags:
- performance
settings: # settings passed to gocritic
captLocal: # must be valid enabled check name
checkLocals: true

View File

@ -24,6 +24,14 @@ linters-settings:
line-length: 140
goimports:
local-prefixes: github.com/golangci/golangci-lint
gocritic:
enabled-tags:
- performance
- style
- experimental
disabled-checks:
- wrapperFunc
- commentFormatting # https://github.com/go-critic/go-critic/issues/755
linters:
enable-all: true
@ -35,4 +43,4 @@ linters:
run:
skip-dirs:
- test/testdata_etc
- test/testdata_etc

View File

@ -28,4 +28,10 @@ release:
rm -rf dist
curl -sL https://git.io/goreleaser | bash
update_deps:
GO111MODULE=on go mod verify
GO111MODULE=on go mod tidy
rm -rf vendor
GO111MODULE=on go mod vendor
.PHONY: test

View File

@ -667,17 +667,22 @@ linters-settings:
range-loops: true # Report preallocation suggestions on range loops, true by default
for-loops: false # Report preallocation suggestions on for loops, false by default
gocritic:
# which checks should be enabled; can't be combined with 'disabled-checks';
# default are: [appendAssign appendCombine assignOp builtinShadow captLocal caseOrder defaultCaseOrder
# dupArg dupBranchBody dupCase elseif flagDeref ifElseChain importShadow indexAlloc paramTypeCombine
# rangeExprCopy rangeValCopy regexpMust singleCaseSwitch sloppyLen switchTrue typeSwitchVar typeUnparen
# underef unlambda unslice dupSubExpr hugeParam];
# all checks list: https://github.com/go-critic/checkers
# Which checks should be enabled; can't be combined with 'disabled-checks';
# See https://go-critic.github.io/overview#checks-overview
# To check which checks are enabled run `GL_DEBUG=gocritic golangci-lint run`
# By default list of stable checks is used.
enabled-checks:
- rangeValCopy
# which checks should be disabled; can't be combined with 'enabled-checks'; default is empty
# Which checks should be disabled; can't be combined with 'enabled-checks'; default is empty
disabled-checks:
- regexpMust
# Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint` run to see all tags and checks.
# Empty list by default. See https://github.com/go-critic/go-critic#usage -> section "Tags".
enabled-tags:
- performance
settings: # settings passed to gocritic
captLocal: # must be valid enabled check name
checkLocals: true
@ -764,6 +769,14 @@ linters-settings:
line-length: 140
goimports:
local-prefixes: github.com/golangci/golangci-lint
gocritic:
enabled-tags:
- performance
- style
- experimental
disabled-checks:
- wrapperFunc
- commentFormatting # https://github.com/go-critic/go-critic/issues/755
linters:
enable-all: true

8
go.mod
View File

@ -6,14 +6,10 @@ require (
github.com/StackExchange/wmi v0.0.0-20180116203802-5d049714c4a6 // indirect
github.com/davecgh/go-spew v1.1.0 // indirect
github.com/fatih/color v1.6.0
github.com/go-critic/checkers v0.0.0-20181204210945-97246d3b3c67
github.com/go-lintpack/lintpack v0.5.1
github.com/go-critic/go-critic v0.0.0-20181204210945-0af0999fabfb
github.com/go-lintpack/lintpack v0.5.2
github.com/go-ole/go-ole v1.2.1 // indirect
github.com/go-toolsmith/astcast v0.0.0-20181028201508-b7a89ed70af1 // indirect
github.com/go-toolsmith/astcopy v0.0.0-20180903214859-79b422d080c4 // indirect
github.com/go-toolsmith/pkgload v0.0.0-20181120203407-5122569a890b // indirect
github.com/go-toolsmith/strparse v0.0.0-20180903215201-830b6daa1241 // indirect
github.com/go-toolsmith/typep v0.0.0-20181030061450-d63dc7650676 // indirect
github.com/gobwas/glob v0.2.3 // indirect
github.com/gogo/protobuf v1.0.0 // indirect
github.com/golang/mock v1.1.1

9
go.sum
View File

@ -10,10 +10,10 @@ github.com/fatih/color v1.6.0 h1:66qjqZk8kalYAvDRtM1AdAJQI0tj4Wrue3Eq3B3pmFU=
github.com/fatih/color v1.6.0/go.mod h1:Zm6kSWBoL9eyXnKyktHP6abPY2pDugNf5KwzbycvMj4=
github.com/fsnotify/fsnotify v1.4.7 h1:IXs+QLmnXW2CcXuY+8Mzv/fWEsPGWxqefPtCP5CnV9I=
github.com/fsnotify/fsnotify v1.4.7/go.mod h1:jwhsz4b93w/PPRr/qN1Yymfu8t87LnFCMoQvtojpjFo=
github.com/go-critic/checkers v0.0.0-20181204210945-97246d3b3c67 h1:AhL5n4pH/qzefJ64+0RbymXZSBsvgbBaVJQCcjFaJPw=
github.com/go-critic/checkers v0.0.0-20181204210945-97246d3b3c67/go.mod h1:Cg5JCP9M6m93z6fecpRcVgD2lZf2RvPtb85ldjiShZc=
github.com/go-lintpack/lintpack v0.5.1 h1:v5D/csM90cu5PANqkj1JcNZGX/mrr3Z2Wu7Q8KuFd9M=
github.com/go-lintpack/lintpack v0.5.1/go.mod h1:NwZuYi2nUHho8XEIZ6SIxihrnPoqBTDqfpXvXAN0sXM=
github.com/go-critic/go-critic v0.0.0-20181204210945-0af0999fabfb h1:faOtDYqSVJsFEJAW+SwEMvh7alhYsb42fER6tt8yXfA=
github.com/go-critic/go-critic v0.0.0-20181204210945-0af0999fabfb/go.mod h1:PSww+HOJZQ3TN2hi6sphNiW1PhwELxbsK8+Jy1sjML8=
github.com/go-lintpack/lintpack v0.5.2 h1:DI5mA3+eKdWeJ40nU4d6Wc26qmdG8RCi/btYq0TuRN0=
github.com/go-lintpack/lintpack v0.5.2/go.mod h1:NwZuYi2nUHho8XEIZ6SIxihrnPoqBTDqfpXvXAN0sXM=
github.com/go-ole/go-ole v1.2.1 h1:2lOsA72HgjxAuMlKpFiCbHTvu44PIVkZ5hqm3RSdI/E=
github.com/go-ole/go-ole v1.2.1/go.mod h1:7FAglXiTm7HKlQRDeOQ6ZNUHidzCWXuZWq/1dTyBNF8=
github.com/go-toolsmith/astcast v0.0.0-20181028201508-b7a89ed70af1 h1:h+1eMw+tZAlgTVclcVN0/rdPaBI/RUzG0peblT6df+Q=
@ -147,6 +147,7 @@ golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
golang.org/x/tools v0.0.0-20180221164845-07fd8470d635/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20181117154741-2ddaf7f79a09/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20181201035826-d0ca3933b724/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20181205014116-22934f0fdb62/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
golang.org/x/tools v0.0.0-20181220024903-92cdcd90bf52 h1:oOIe9Zzq27JsS/3ACpGF1HwWnWNflZWT/3EvM7mtcEk=
golang.org/x/tools v0.0.0-20181220024903-92cdcd90bf52/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
gopkg.in/airbrake/gobrake.v2 v2.0.9 h1:7z2uVWwn7oVeeugY1DtlPAy5H+KYgB1KeKTnqjNatLo=

View File

@ -1,11 +1,7 @@
package config
import (
"errors"
"strings"
"time"
"github.com/golangci/golangci-lint/pkg/logutils"
)
const (
@ -198,99 +194,6 @@ type PreallocSettings struct {
ForLoops bool `mapstructure:"for-loops"`
}
type GocriticCheckSettings map[string]interface{}
type GocriticSettings struct {
EnabledChecks []string `mapstructure:"enabled-checks"`
DisabledChecks []string `mapstructure:"disabled-checks"`
SettingsPerCheck map[string]GocriticCheckSettings `mapstructure:"settings"`
inferredEnabledChecks map[string]bool
}
func (s *GocriticSettings) InferEnabledChecks(log logutils.Log) {
enabledChecks := s.EnabledChecks
if len(enabledChecks) == 0 {
if len(s.DisabledChecks) != 0 {
for _, defaultCheck := range defaultGocriticEnabledChecks {
if !s.isCheckDisabled(defaultCheck) {
enabledChecks = append(enabledChecks, defaultCheck)
}
}
} else {
enabledChecks = defaultGocriticEnabledChecks
}
}
s.inferredEnabledChecks = map[string]bool{}
for _, check := range enabledChecks {
s.inferredEnabledChecks[strings.ToLower(check)] = true
}
log.Infof("Gocritic enabled checks: %s", enabledChecks)
}
func (s GocriticSettings) isCheckDisabled(name string) bool {
for _, disabledCheck := range s.DisabledChecks {
if disabledCheck == name {
return true
}
}
return false
}
func (s GocriticSettings) Validate(log logutils.Log) error {
if len(s.EnabledChecks) != 0 && len(s.DisabledChecks) != 0 {
return errors.New("both enabled and disabled check aren't allowed for gocritic")
}
for checkName := range s.SettingsPerCheck {
if !s.IsCheckEnabled(checkName) {
log.Warnf("Gocritic settings were provided for not enabled check %q", checkName)
}
}
return nil
}
func (s GocriticSettings) IsCheckEnabled(name string) bool {
return s.inferredEnabledChecks[strings.ToLower(name)]
}
// Its a good idea to keep this list in sync with the gocritic stable checks list in:
// https://github.com/go-critic/go-critic/blob/master/checkers/checkers_test.go#L63
var defaultGocriticEnabledChecks = []string{
"appendAssign",
"appendCombine",
"assignOp",
"builtinShadow",
"captLocal",
"caseOrder",
"defaultCaseOrder",
"dupArg",
"dupBranchBody",
"dupCase",
"elseif",
"flagDeref",
"ifElseChain",
"importShadow",
"indexAlloc",
"paramTypeCombine",
"rangeExprCopy",
"rangeValCopy",
"regexpMust",
"singleCaseSwitch",
"sloppyLen",
"switchTrue",
"typeSwitchVar",
"typeUnparen",
"underef",
"unlambda",
"unslice",
"dupSubExpr",
"hugeParam",
}
var defaultLintersSettings = LintersSettings{
Lll: LllSettings{
LineLength: 120,

View File

@ -0,0 +1,297 @@
package config
import (
"fmt"
"sort"
"strings"
"github.com/go-lintpack/lintpack"
"github.com/pkg/errors"
_ "github.com/go-critic/go-critic/checkers" // this import register checkers
"github.com/golangci/golangci-lint/pkg/logutils"
)
const gocriticDebugKey = "gocritic"
var gocriticDebugf = logutils.Debug(gocriticDebugKey)
var isGocriticDebug = logutils.HaveDebugTag(gocriticDebugKey)
var allGocriticCheckers = lintpack.GetCheckersInfo()
type GocriticCheckSettings map[string]interface{}
type GocriticSettings struct {
EnabledChecks []string `mapstructure:"enabled-checks"`
DisabledChecks []string `mapstructure:"disabled-checks"`
EnabledTags []string `mapstructure:"enabled-tags"`
SettingsPerCheck map[string]GocriticCheckSettings `mapstructure:"settings"`
inferredEnabledChecks map[string]bool
}
func debugChecksListf(checks []string, format string, args ...interface{}) {
if isGocriticDebug {
prefix := fmt.Sprintf(format, args...)
gocriticDebugf(prefix+" checks (%d): %s", len(checks), sprintStrings(checks))
}
}
func stringsSliceToSet(ss []string) map[string]bool {
ret := map[string]bool{}
for _, s := range ss {
ret[s] = true
}
return ret
}
func buildGocriticTagToCheckersMap() map[string][]string {
tagToCheckers := map[string][]string{}
for _, checker := range allGocriticCheckers {
for _, tag := range checker.Tags {
tagToCheckers[tag] = append(tagToCheckers[tag], checker.Name)
}
}
return tagToCheckers
}
func gocriticCheckerTagsDebugf() {
if !isGocriticDebug {
return
}
tagToCheckers := buildGocriticTagToCheckersMap()
var allTags []string
for tag := range tagToCheckers {
allTags = append(allTags, tag)
}
sort.Strings(allTags)
gocriticDebugf("All gocritic existing tags and checks:")
for _, tag := range allTags {
debugChecksListf(tagToCheckers[tag], " tag %q", tag)
}
}
func (s *GocriticSettings) gocriticDisabledCheckersDebugf() {
if !isGocriticDebug {
return
}
var disabledCheckers []string
for _, checker := range allGocriticCheckers {
if s.inferredEnabledChecks[strings.ToLower(checker.Name)] {
continue
}
disabledCheckers = append(disabledCheckers, checker.Name)
}
if len(disabledCheckers) == 0 {
gocriticDebugf("All checks are enabled")
} else {
debugChecksListf(disabledCheckers, "Final not used")
}
}
//nolint:gocyclo
func (s *GocriticSettings) InferEnabledChecks(log logutils.Log) {
gocriticCheckerTagsDebugf()
enabledByDefaultChecks := getDefaultEnabledGocriticCheckersNames()
debugChecksListf(enabledByDefaultChecks, "Enabled by default")
disabledByDefaultChecks := getDefaultDisabledGocriticCheckersNames()
debugChecksListf(disabledByDefaultChecks, "Disabled by default")
var enabledChecks []string
if len(s.EnabledTags) != 0 {
tagToCheckers := buildGocriticTagToCheckersMap()
for _, tag := range s.EnabledTags {
enabledChecks = append(enabledChecks, tagToCheckers[tag]...)
}
debugChecksListf(enabledChecks, "Enabled by config tags %s", sprintStrings(s.EnabledTags))
}
if !(len(s.EnabledTags) == 0 && len(s.EnabledChecks) != 0) {
// don't use default checks only if we have no enabled tags and enable some checks manually
enabledChecks = append(enabledChecks, enabledByDefaultChecks...)
}
if len(s.EnabledChecks) != 0 {
debugChecksListf(s.EnabledChecks, "Enabled by config")
alreadyEnabledChecksSet := stringsSliceToSet(enabledChecks)
for _, enabledCheck := range s.EnabledChecks {
if alreadyEnabledChecksSet[enabledCheck] {
log.Warnf("No need to enable check %q: it's already enabled", enabledCheck)
continue
}
enabledChecks = append(enabledChecks, enabledCheck)
}
}
if len(s.DisabledChecks) != 0 {
debugChecksListf(s.DisabledChecks, "Disabled by config")
enabledChecksSet := stringsSliceToSet(enabledChecks)
for _, disabledCheck := range s.DisabledChecks {
if !enabledChecksSet[disabledCheck] {
log.Warnf("Gocritic check %q was disabled by config, was it's not enabled, no need to disable it",
disabledCheck)
continue
}
delete(enabledChecksSet, disabledCheck)
}
enabledChecks = nil
for enabledCheck := range enabledChecksSet {
enabledChecks = append(enabledChecks, enabledCheck)
}
}
s.inferredEnabledChecks = map[string]bool{}
for _, check := range enabledChecks {
s.inferredEnabledChecks[strings.ToLower(check)] = true
}
debugChecksListf(enabledChecks, "Final used")
s.gocriticDisabledCheckersDebugf()
}
func validateStringsUniq(ss []string) error {
set := map[string]bool{}
for _, s := range ss {
_, ok := set[s]
if ok {
return fmt.Errorf("%q occurs multiple times in list", s)
}
set[s] = true
}
return nil
}
//nolint:gocyclo
func (s *GocriticSettings) Validate(log logutils.Log) error {
if len(s.EnabledTags) == 0 {
if len(s.EnabledChecks) != 0 && len(s.DisabledChecks) != 0 {
return errors.New("both enabled and disabled check aren't allowed for gocritic")
}
} else {
if err := validateStringsUniq(s.EnabledTags); err != nil {
return errors.Wrap(err, "validate enabled tags")
}
tagToCheckers := buildGocriticTagToCheckersMap()
for _, tag := range s.EnabledTags {
if _, ok := tagToCheckers[tag]; !ok {
return fmt.Errorf("gocritic tag %q doesn't exist", tag)
}
}
}
if err := validateStringsUniq(s.EnabledChecks); err != nil {
return errors.Wrap(err, "validate enabled checks")
}
if err := validateStringsUniq(s.DisabledChecks); err != nil {
return errors.Wrap(err, "validate disabled checks")
}
for checkName := range s.SettingsPerCheck {
if !s.IsCheckEnabled(checkName) {
log.Warnf("Gocritic settings were provided for not enabled check %q", checkName)
}
}
if err := s.validateCheckerNames(); err != nil {
return errors.Wrap(err, "validation failed")
}
return nil
}
func (s *GocriticSettings) IsCheckEnabled(name string) bool {
return s.inferredEnabledChecks[strings.ToLower(name)]
}
func sprintAllowedCheckerNames(allowedNames map[string]bool) string {
var namesSlice []string
for name := range allowedNames {
namesSlice = append(namesSlice, name)
}
return sprintStrings(namesSlice)
}
func sprintStrings(ss []string) string {
sort.Strings(ss)
return fmt.Sprint(ss)
}
func getAllCheckerNames() map[string]bool {
allCheckerNames := map[string]bool{}
for _, checker := range allGocriticCheckers {
allCheckerNames[strings.ToLower(checker.Name)] = true
}
return allCheckerNames
}
func isEnabledByDefaultGocriticCheck(info *lintpack.CheckerInfo) bool {
return !info.HasTag("experimental") &&
!info.HasTag("opinionated") &&
!info.HasTag("performance")
}
func getDefaultEnabledGocriticCheckersNames() []string {
var enabled []string
for _, info := range allGocriticCheckers {
// get in sync with lintpack behavior in bindDefaultEnabledList
// in https://github.com/go-lintpack/lintpack/blob/master/linter/lintmain/internal/check/check.go#L317
enable := isEnabledByDefaultGocriticCheck(info)
if enable {
enabled = append(enabled, info.Name)
}
}
return enabled
}
func getDefaultDisabledGocriticCheckersNames() []string {
var disabled []string
for _, info := range allGocriticCheckers {
// get in sync with lintpack behavior in bindDefaultEnabledList
// in https://github.com/go-lintpack/lintpack/blob/master/linter/lintmain/internal/check/check.go#L317
enable := isEnabledByDefaultGocriticCheck(info)
if !enable {
disabled = append(disabled, info.Name)
}
}
return disabled
}
func (s *GocriticSettings) validateCheckerNames() error {
allowedNames := getAllCheckerNames()
for _, name := range s.EnabledChecks {
if !allowedNames[strings.ToLower(name)] {
return fmt.Errorf("enabled checker %s doesn't exist, all existing checkers: %s",
name, sprintAllowedCheckerNames(allowedNames))
}
}
for _, name := range s.DisabledChecks {
if !allowedNames[strings.ToLower(name)] {
return fmt.Errorf("disabled checker %s doesn't exist, all existing checkers: %s",
name, sprintAllowedCheckerNames(allowedNames))
}
}
return nil
}

View File

@ -10,8 +10,6 @@ import (
"runtime/debug"
"sync"
_ "github.com/go-critic/checkers" // this import register checkers
"github.com/go-lintpack/lintpack"
"golang.org/x/tools/go/loader"
@ -34,6 +32,7 @@ func (lint Gocritic) Run(ctx context.Context, lintCtx *linter.Context) ([]result
lintpackCtx := lintpack.NewContext(lintCtx.Program.Fset, sizes)
s := lintCtx.Settings().Gocritic
var enabledCheckers []*lintpack.Checker
for _, info := range lintpack.GetCheckersInfo() {
if !s.IsCheckEnabled(info.Name) {

View File

@ -37,7 +37,7 @@ func (g Gocyclo) Run(ctx context.Context, lintCtx *linter.Context) ([]result.Iss
res := make([]result.Issue, 0, len(stats))
for _, s := range stats {
if s.Complexity <= lintCtx.Settings().Gocyclo.MinComplexity {
break //Break as the stats is already sorted from greatest to least
break // Break as the stats is already sorted from greatest to least
}
res = append(res, result.Issue{

View File

@ -37,7 +37,7 @@ func (g Gofmt) Desc() string {
"this tool runs with -s option to check for code simplification"
}
func getFirstDeletedAndAddedLineNumberInHunk(h *diffpkg.Hunk) (int, int, error) {
func getFirstDeletedAndAddedLineNumberInHunk(h *diffpkg.Hunk) (firstDeleted, firstAdded int, err error) {
lines := bytes.Split(h.Body, []byte{'\n'})
lineNumber := int(h.OrigStartLine - 1)
firstAddedLineNumber := -1

View File

@ -305,7 +305,7 @@ func (m megacheck) runMegacheck(workingPkgs []*packages.Package, checkExportedUn
// parseIgnore is a copy from megacheck code just to not fork megacheck
func parseIgnore(s string) ([]lint.Ignore, error) {
var out []lint.Ignore
if len(s) == 0 {
if s == "" {
return nil, nil
}
for _, part := range strings.Fields(s) {

View File

@ -1,8 +0,0 @@
# checkers
gocritic linter main checkers collection.
Type of the checkers:
- diagnostic
- style
- performance
- experimental

View File

@ -1,49 +0,0 @@
package checkers
import (
"go/ast"
"regexp"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "docStub"
info.Tags = []string{"style", "experimental"}
info.Summary = "Detects comments that silence go lint complaints about doc-comment"
info.Before = `
// Foo ...
func Foo() {
}`
info.After = `
// (A) - remove the doc-comment stub
func Foo() {}
// (B) - replace it with meaningful comment
// Foo is a demonstration-only function.
func Foo() {}`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
c := &docStubChecker{ctx: ctx}
c.badCommentRE = regexp.MustCompile(`//\s?\w+([^a-zA-Z]+|( XXX.?))$`)
return astwalk.WalkerForFuncDecl(c)
})
}
type docStubChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
badCommentRE *regexp.Regexp
}
func (c *docStubChecker) VisitFuncDecl(decl *ast.FuncDecl) {
if decl.Name.IsExported() && decl.Doc != nil && c.badCommentRE.MatchString(decl.Doc.List[0].Text) {
c.warn(decl)
}
}
func (c *docStubChecker) warn(decl *ast.FuncDecl) {
c.ctx.Warn(decl, "silencing go lint doc-comment warnings is unadvised")
}

View File

@ -1,7 +1,7 @@
MIT License
Copyright (c) 2018 Iskander Sharipov
Copyright (c) 2018 Oleg Kovalov
Copyright (c) 2018 Alekseev Artem
Copyright (c) 2018 Ravil Bikbulatov
Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal

View File

@ -4,7 +4,7 @@ import (
"go/ast"
"go/token"
"github.com/go-critic/checkers/internal/lintutil"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astequal"

View File

@ -0,0 +1,147 @@
package checkers
import (
"go/ast"
"go/constant"
"go/token"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astcast"
"github.com/go-toolsmith/astcopy"
"github.com/go-toolsmith/astequal"
"github.com/go-toolsmith/typep"
"golang.org/x/tools/go/ast/astutil"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "badCond"
info.Tags = []string{"diagnostic", "experimental"}
info.Summary = "Detects suspicious condition expressions"
info.Before = `
for i := 0; i > n; i++ {
xs[i] = 0
}`
info.After = `
for i := 0; i < n; i++ {
xs[i] = 0
}`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
return astwalk.WalkerForFuncDecl(&badCondChecker{ctx: ctx})
})
}
type badCondChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
}
func (c *badCondChecker) VisitFuncDecl(decl *ast.FuncDecl) {
ast.Inspect(decl.Body, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.ForStmt:
c.checkForStmt(n)
case ast.Expr:
c.checkExpr(n)
}
return true
})
}
func (c *badCondChecker) checkExpr(expr ast.Expr) {
// TODO(Quasilyte): recognize more patterns.
cond := astcast.ToBinaryExpr(expr)
lhs := astcast.ToBinaryExpr(astutil.Unparen(cond.X))
rhs := astcast.ToBinaryExpr(astutil.Unparen(cond.Y))
if cond.Op != token.LAND {
return
}
// Notes:
// `x != a || x != b` handled by go vet.
// Pattern 1.
// `x < a && x > b`; Where `a` is less than `b`.
if c.lessAndGreater(lhs, rhs) {
c.warnCond(cond, "always false")
return
}
// Pattern 2.
// `x == a && x == b`
//
// Valid when `b == a` is intended, but still reported.
// We can disable "just suspicious" warnings by default
// is users are upset with the current behavior.
if c.equalToBoth(lhs, rhs) {
c.warnCond(cond, "suspicious")
return
}
}
func (c *badCondChecker) equalToBoth(lhs, rhs *ast.BinaryExpr) bool {
return lhs.Op == token.EQL && rhs.Op == token.EQL &&
astequal.Expr(lhs.X, rhs.X)
}
func (c *badCondChecker) lessAndGreater(lhs, rhs *ast.BinaryExpr) bool {
if lhs.Op != token.LSS || rhs.Op != token.GTR {
return false
}
if !astequal.Expr(lhs.X, rhs.X) {
return false
}
a := c.ctx.TypesInfo.Types[lhs.Y].Value
b := c.ctx.TypesInfo.Types[rhs.Y].Value
return a != nil && b != nil && constant.Compare(a, token.LSS, b)
}
func (c *badCondChecker) checkForStmt(stmt *ast.ForStmt) {
// TODO(Quasilyte): handle other kinds of bad conditionals.
init := astcast.ToAssignStmt(stmt.Init)
if init.Tok != token.DEFINE || len(init.Lhs) != 1 || len(init.Rhs) != 1 {
return
}
if astcast.ToBasicLit(init.Rhs[0]).Value != "0" {
return
}
iter := astcast.ToIdent(init.Lhs[0])
cond := astcast.ToBinaryExpr(stmt.Cond)
if cond.Op != token.GTR || !astequal.Expr(iter, cond.X) {
return
}
if !typep.SideEffectFree(c.ctx.TypesInfo, cond.Y) {
return
}
post := astcast.ToIncDecStmt(stmt.Post)
if post.Tok != token.INC || !astequal.Expr(iter, post.X) {
return
}
mutated := lintutil.CouldBeMutated(c.ctx.TypesInfo, stmt.Body, cond.Y) ||
lintutil.CouldBeMutated(c.ctx.TypesInfo, stmt.Body, iter)
if mutated {
return
}
c.warnForStmt(stmt, cond)
}
func (c *badCondChecker) warnForStmt(cause ast.Node, cond *ast.BinaryExpr) {
suggest := astcopy.BinaryExpr(cond)
suggest.Op = token.LSS
c.ctx.Warn(cause, "`%s` in loop; probably meant `%s`?",
cond, suggest)
}
func (c *badCondChecker) warnCond(cond *ast.BinaryExpr, tag string) {
c.ctx.Warn(cond, "`%s` condition is %s", cond, tag)
}

View File

@ -4,7 +4,7 @@ import (
"go/ast"
"go/token"
"github.com/go-critic/checkers/internal/lintutil"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astcopy"
@ -32,12 +32,27 @@ b := (x) == (y)`
type boolExprSimplifyChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
ctx *lintpack.CheckerContext
hasFloats bool
}
func (c *boolExprSimplifyChecker) VisitExpr(x ast.Expr) {
// TODO: avoid eager copy?
// Can't be stable until wasted copying is fixed.
// Throw away non-bool expressions and avoid redundant
// AST copying below.
if typ := c.ctx.TypesInfo.TypeOf(x); typ == nil || !typep.HasBoolKind(typ.Underlying()) {
return
}
// We'll loose all types info after a copy,
// this is why we record valuable info before doing it.
c.hasFloats = lintutil.ContainsNode(x, func(n ast.Node) bool {
if x, ok := n.(*ast.BinaryExpr); ok {
return typep.HasFloatProp(c.ctx.TypesInfo.TypeOf(x.X).Underlying()) ||
typep.HasFloatProp(c.ctx.TypesInfo.TypeOf(x.Y).Underlying())
}
return false
})
y := c.simplifyBool(astcopy.Expr(x))
if !astequal.Expr(x, y) {
c.warn(x, y)
@ -80,6 +95,10 @@ func (c *boolExprSimplifyChecker) negatedEquals(cur *astutil.Cursor) bool {
}
func (c *boolExprSimplifyChecker) invertComparison(cur *astutil.Cursor) bool {
if c.hasFloats { // See #673
return false
}
neg := lintutil.AsUnaryExprOp(cur.Node(), token.NOT)
cmp := lintutil.AsBinaryExpr(astutil.Unparen(neg.X))
if lintutil.IsNil(neg) || lintutil.IsNil(cmp) {

View File

@ -32,8 +32,7 @@ type captLocalChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
upcaseNames map[string]bool
paramsOnly bool
paramsOnly bool
}
func (c *captLocalChecker) VisitLocalDef(def astwalk.Name, _ ast.Expr) {

View File

@ -0,0 +1,71 @@
package checkers
import (
"go/ast"
"regexp"
"strings"
"unicode"
"unicode/utf8"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "commentFormatting"
info.Tags = []string{"style", "experimental"}
info.Summary = "Detects comments with non-idiomatic formatting"
info.Before = `//This is a comment`
info.After = `// This is a comment`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
pragmaRE := regexp.MustCompile(`(?m)^//\w+:.*$`)
return astwalk.WalkerForComment(&commentFormattingChecker{
ctx: ctx,
pragmaRE: pragmaRE,
})
})
}
type commentFormattingChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
pragmaRE *regexp.Regexp
}
func (c *commentFormattingChecker) VisitComment(cg *ast.CommentGroup) {
if strings.HasPrefix(cg.List[0].Text, "/*") {
return
}
for _, comment := range cg.List {
if len(comment.Text) <= len("// ") {
continue
}
if c.pragmaRE.MatchString(comment.Text) {
continue
}
// Make a decision based on a first comment text rune.
r, _ := utf8.DecodeRuneInString(comment.Text[len("//"):])
if !c.specialChar(r) && !unicode.IsSpace(r) {
c.warn(cg)
return
}
}
}
func (c *commentFormattingChecker) specialChar(r rune) bool {
// Permitted list to avoid false-positives.
switch r {
case '+', '-', '#', '!':
return true
default:
return false
}
}
func (c *commentFormattingChecker) warn(cg *ast.CommentGroup) {
c.ctx.Warn(cg, "put a space between `//` and comment text")
}

View File

@ -0,0 +1,95 @@
package checkers
import (
"go/ast"
"go/token"
"regexp"
"strings"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "docStub"
info.Tags = []string{"style", "experimental"}
info.Summary = "Detects comments that silence go lint complaints about doc-comment"
info.Before = `
// Foo ...
func Foo() {
}`
info.After = `
// (A) - remove the doc-comment stub
func Foo() {}
// (B) - replace it with meaningful comment
// Foo is a demonstration-only function.
func Foo() {}`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
re := `(?i)^\.\.\.$|^\.$|^xxx\.?$|^whatever\.?$`
c := &docStubChecker{
ctx: ctx,
stubCommentRE: regexp.MustCompile(re),
}
return c
})
}
type docStubChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
stubCommentRE *regexp.Regexp
}
func (c *docStubChecker) WalkFile(f *ast.File) {
for _, decl := range f.Decls {
switch decl := decl.(type) {
case *ast.FuncDecl:
c.visitDoc(decl, decl.Name, decl.Doc, false)
case *ast.GenDecl:
if decl.Tok != token.TYPE {
continue
}
if len(decl.Specs) == 1 {
spec := decl.Specs[0].(*ast.TypeSpec)
// Only 1 spec, use doc from the decl itself.
c.visitDoc(spec, spec.Name, decl.Doc, true)
}
// N specs, use per-spec doc.
for _, spec := range decl.Specs {
spec := spec.(*ast.TypeSpec)
c.visitDoc(spec, spec.Name, spec.Doc, true)
}
}
}
}
func (c *docStubChecker) visitDoc(decl ast.Node, sym *ast.Ident, doc *ast.CommentGroup, article bool) {
if !sym.IsExported() || doc == nil {
return
}
line := strings.TrimSpace(doc.List[0].Text[len("//"):])
if article {
// Skip optional article.
for _, a := range []string{"The ", "An ", "A "} {
if strings.HasPrefix(line, a) {
line = line[len(a):]
break
}
}
}
if !strings.HasPrefix(line, sym.Name) {
return
}
line = strings.TrimSpace(line[len(sym.Name):])
// Now try to detect the "stub" part.
if c.stubCommentRE.MatchString(line) {
c.warn(decl)
}
}
func (c *docStubChecker) warn(cause ast.Node) {
c.ctx.Warn(cause, "silencing go lint doc-comment warnings is unadvised")
}

View File

@ -3,7 +3,7 @@ package checkers
import (
"go/ast"
"github.com/go-critic/checkers/internal/lintutil"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
)

View File

@ -0,0 +1,82 @@
package checkers
import (
"go/ast"
"go/token"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astcast"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "equalFold"
info.Tags = []string{"performance", "experimental"}
info.Summary = "Detects unoptimal strings/bytes case-insensitive comparison"
info.Before = `strings.ToLower(x) == strings.ToLower(y)`
info.After = `strings.EqualFold(x, y)`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
return astwalk.WalkerForExpr(&equalFoldChecker{ctx: ctx})
})
}
type equalFoldChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
}
func (c *equalFoldChecker) VisitExpr(e ast.Expr) {
switch e := e.(type) {
case *ast.CallExpr:
c.checkBytes(e)
case *ast.BinaryExpr:
c.checkStrings(e)
}
}
// uncaseCall simplifies lower(x) or upper(x) to x.
// If no simplification is applied, second return value is false.
func (c *equalFoldChecker) uncaseCall(x ast.Expr, lower, upper string) (ast.Expr, bool) {
call := astcast.ToCallExpr(x)
name := qualifiedName(call.Fun)
if name != lower && name != upper {
return x, false
}
return call.Args[0], true
}
func (c *equalFoldChecker) checkBytes(expr *ast.CallExpr) {
if qualifiedName(expr.Fun) != "bytes.Equal" {
return
}
x, ok1 := c.uncaseCall(expr.Args[0], "bytes.ToLower", "bytes.ToUpper")
y, ok2 := c.uncaseCall(expr.Args[1], "bytes.ToLower", "bytes.ToUpper")
if !ok1 && !ok2 {
return
}
c.warnBytes(expr, x, y)
}
func (c *equalFoldChecker) checkStrings(expr *ast.BinaryExpr) {
if expr.Op != token.EQL && expr.Op != token.NEQ {
return
}
x, ok1 := c.uncaseCall(expr.X, "strings.ToLower", "strings.ToUpper")
y, ok2 := c.uncaseCall(expr.Y, "strings.ToLower", "strings.ToUpper")
if !ok1 && !ok2 {
return
}
c.warnStrings(expr, x, y)
}
func (c *equalFoldChecker) warnStrings(cause ast.Node, x, y ast.Expr) {
c.ctx.Warn(cause, "consider replacing with strings.EqualFold(%s, %s)", x, y)
}
func (c *equalFoldChecker) warnBytes(cause ast.Node, x, y ast.Expr) {
c.ctx.Warn(cause, "consider replacing with bytes.EqualFold(%s, %s)", x, y)
}

View File

@ -0,0 +1,78 @@
package checkers
import (
"go/ast"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astfmt"
"github.com/go-toolsmith/astp"
"golang.org/x/tools/go/ast/astutil"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "exitAfterDefer"
info.Tags = []string{"diagnostic", "experimental"}
info.Summary = "Detects calls to exit/fatal inside functions that use defer"
info.Before = `
defer os.Remove(filename)
if bad {
log.Fatalf("something bad happened")
}`
info.After = `
defer os.Remove(filename)
if bad {
log.Printf("something bad happened")
return
}`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
return astwalk.WalkerForFuncDecl(&exitAfterDeferChecker{ctx: ctx})
})
}
type exitAfterDeferChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
}
func (c *exitAfterDeferChecker) VisitFuncDecl(fn *ast.FuncDecl) {
// TODO(Quasilyte): handle goto and other kinds of flow that break
// the algorithm below that expects the latter statement to be
// executed after the ones that come before it.
var deferStmt *ast.DeferStmt
pre := func(cur *astutil.Cursor) bool {
// Don't recurse into local anonymous functions.
return !astp.IsFuncLit(cur.Node())
}
post := func(cur *astutil.Cursor) bool {
switch n := cur.Node().(type) {
case *ast.DeferStmt:
deferStmt = n
case *ast.CallExpr:
if deferStmt != nil {
switch qualifiedName(n.Fun) {
case "log.Fatal", "log.Fatalf", "log.Fatalln", "os.Exit":
c.warn(n, deferStmt)
return false
}
}
}
return true
}
astutil.Apply(fn.Body, pre, post)
}
func (c *exitAfterDeferChecker) warn(cause *ast.CallExpr, deferStmt *ast.DeferStmt) {
var s string
if fnlit, ok := deferStmt.Call.Fun.(*ast.FuncLit); ok {
// To avoid long and multi-line warning messages,
// collapse the function literals.
s = "defer " + astfmt.Sprint(fnlit.Type) + "{...}(...)"
} else {
s = astfmt.Sprint(deferStmt)
}
c.ctx.Warn(cause, "%s clutters `%s`", cause.Fun, s)
}

View File

@ -30,12 +30,22 @@ type flagNameChecker struct {
func (c *flagNameChecker) VisitExpr(expr ast.Expr) {
call := astcast.ToCallExpr(expr)
switch qualifiedName(call.Fun) {
case "flag.Bool", "flag.Duration", "flag.Float64", "flag.String",
"flag.Int", "flag.Int64", "flag.Uint", "flag.Uint64":
sym := astcast.ToIdent(astcast.ToSelectorExpr(call.Fun).Sel)
obj := c.ctx.TypesInfo.ObjectOf(sym)
if obj == nil {
return
}
pkg := obj.Pkg()
if !isStdlibPkg(pkg) || pkg.Name() != "flag" {
return
}
switch sym.Name {
case "Bool", "Duration", "Float64", "String",
"Int", "Int64", "Uint", "Uint64":
c.checkFlagName(call, call.Args[0])
case "flag.BoolVar", "flag.DurationVar", "flag.Float64Var", "flag.StringVar",
"flag.IntVar", "flag.Int64Var", "flag.UintVar", "flag.Uint64Var":
case "BoolVar", "DurationVar", "Float64Var", "StringVar",
"IntVar", "Int64Var", "UintVar", "Uint64Var":
c.checkFlagName(call, call.Args[1])
}
}

View File

@ -39,8 +39,7 @@ func (c *importShadowChecker) VisitLocalDef(def astwalk.Name, _ ast.Expr) {
}
func (c *importShadowChecker) warn(id ast.Node, importedName string, pkg *types.Package) {
if pkg.Path() == pkg.Name() {
// Сheck for standart library packages.
if isStdlibPkg(pkg) {
c.ctx.Warn(id, "shadow of imported package '%s'", importedName)
} else {
c.ctx.Warn(id, "shadow of imported from '%s' package '%s'", pkg.Path(), importedName)

View File

@ -0,0 +1,27 @@
package lintutil
import (
"go/ast"
"golang.org/x/tools/go/ast/astutil"
)
// FindNode applies pred for root and all it's childs until it returns true.
// Matched node is returned.
// If none of the nodes matched predicate, nil is returned.
func FindNode(root ast.Node, pred func(ast.Node) bool) ast.Node {
var found ast.Node
astutil.Apply(root, nil, func(cur *astutil.Cursor) bool {
if pred(cur.Node()) {
found = cur.Node()
return false
}
return true
})
return found
}
// ContainsNode reports whether `FindNode(root, pred)!=nil`.
func ContainsNode(root ast.Node, pred func(ast.Node) bool) bool {
return FindNode(root, pred) != nil
}

View File

@ -0,0 +1,86 @@
package lintutil
import (
"go/ast"
"go/token"
"go/types"
"github.com/go-toolsmith/astequal"
"github.com/go-toolsmith/astp"
"github.com/go-toolsmith/typep"
)
// Different utilities to make simple analysis over typed ast values flow.
//
// It's primitive and can't replace SSA, but the bright side is that
// it does not require building an additional IR eagerly.
// Expected to be used sparingly inside a few checkers.
//
// If proven really useful, can be moved to go-toolsmith library.
// IsImmutable reports whether n can be midified through any operation.
func IsImmutable(info *types.Info, n ast.Expr) bool {
if astp.IsBasicLit(n) {
return true
}
tv, ok := info.Types[n]
return ok && !tv.Assignable() && !tv.Addressable()
}
// CouldBeMutated reports whether dst can be modified inside body.
//
// Note that it does not take already existing pointers to dst.
// An example of safe and correct usage is checking of something
// that was just defined, so the dst is a result of that definition.
func CouldBeMutated(info *types.Info, body ast.Node, dst ast.Expr) bool {
if IsImmutable(info, dst) { // Fast path.
return false
}
// We don't track pass-by-value.
// If it's already a pointer, passing it by value
// means that there can be a potential indirect modification.
//
// It's possible to be less conservative here and find at least
// one such value pass before giving up.
if typep.IsPointer(info.TypeOf(dst)) {
return true
}
var isDst func(x ast.Expr) bool
if dst, ok := dst.(*ast.Ident); ok {
// Identifier can be shadowed,
// so we need to check the object as well.
obj := info.ObjectOf(dst)
if obj == nil {
return true // Being conservative
}
isDst = func(x ast.Expr) bool {
id, ok := x.(*ast.Ident)
return ok && id.Name == dst.Name && info.ObjectOf(id) == obj
}
} else {
isDst = func(x ast.Expr) bool {
return astequal.Expr(dst, x)
}
}
return ContainsNode(body, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.UnaryExpr:
if n.Op == token.AND && isDst(n.X) {
return true // Address taken
}
case *ast.AssignStmt:
for _, lhs := range n.Lhs {
if isDst(lhs) {
return true
}
}
case *ast.IncDecStmt:
// Incremented or decremented.
return isDst(n.X)
}
return false
})
}

View File

@ -2,12 +2,13 @@ package checkers
import (
"go/ast"
"go/types"
"go/token"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astcast"
"github.com/go-toolsmith/astcopy"
"github.com/go-toolsmith/typep"
)
func init() {
@ -33,10 +34,12 @@ type methodExprCallChecker struct {
func (c *methodExprCallChecker) VisitExpr(x ast.Expr) {
call := astcast.ToCallExpr(x)
s := astcast.ToSelectorExpr(call.Fun)
id := astcast.ToIdent(s.X)
obj := c.ctx.TypesInfo.ObjectOf(id)
if _, ok := obj.(*types.TypeName); ok {
if len(call.Args) < 1 || astcast.ToIdent(call.Args[0]).Name == "nil" {
return
}
if typep.IsTypeExpr(c.ctx.TypesInfo, s.X) {
c.warn(call, s)
}
}
@ -45,5 +48,10 @@ func (c *methodExprCallChecker) warn(cause *ast.CallExpr, s *ast.SelectorExpr) {
selector := astcopy.SelectorExpr(s)
selector.X = cause.Args[0]
// Remove "&" from the receiver (if any).
if u, ok := selector.X.(*ast.UnaryExpr); ok && u.Op == token.AND {
selector.X = u.X
}
c.ctx.Warn(cause, "consider to change `%s` to `%s`", cause.Fun, selector)
}

View File

@ -3,6 +3,7 @@ package checkers
import (
"go/ast"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astequal"
@ -71,7 +72,7 @@ func (c *typeSwitchVarChecker) checkTypeSwitch(root *ast.TypeSwitchStmt) {
// Create artificial node just for matching.
assert1 := ast.TypeAssertExpr{X: expr, Type: clause.List[0]}
for _, stmt := range clause.Body {
assert2 := findNode(stmt, func(x ast.Node) bool {
assert2 := lintutil.FindNode(stmt, func(x ast.Node) bool {
return astequal.Node(&assert1, x)
})
if object == c.ctx.TypesInfo.ObjectOf(identOf(assert2)) {

View File

@ -3,6 +3,7 @@ package checkers
import (
"go/ast"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astcopy"
@ -66,7 +67,7 @@ func (c *typeUnparenChecker) checkTypeExpr(x ast.Expr) {
}
func (c *typeUnparenChecker) hasParens(x ast.Expr) bool {
return containsNode(x, astp.IsParenExpr)
return lintutil.ContainsNode(x, astp.IsParenExpr)
}
func (c *typeUnparenChecker) unparenExpr(x ast.Expr) ast.Expr {

View File

@ -4,7 +4,7 @@ import (
"go/ast"
"go/types"
"github.com/go-critic/checkers/internal/lintutil"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astp"

View File

@ -4,6 +4,7 @@ import (
"go/ast"
"go/token"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
)
@ -43,7 +44,7 @@ func (c *unlabelStmtChecker) EnterFunc(fn *ast.FuncDecl) bool {
}
// TODO(Quasilyte): should not do additional traversal here.
// For now, skip all functions that contain goto statement.
return !containsNode(fn.Body, func(n ast.Node) bool {
return !lintutil.ContainsNode(fn.Body, func(n ast.Node) bool {
br, ok := n.(*ast.BranchStmt)
return ok && br.Tok == token.GOTO
})
@ -78,7 +79,7 @@ func (c *unlabelStmtChecker) VisitStmt(stmt ast.Stmt) {
matchUsage := func(n ast.Node) bool {
return c.canBreakFrom(n) && c.usesLabel(c.blockStmtOf(n), name)
}
if !containsNode(c.blockStmtOf(labeled.Stmt), matchUsage) {
if !lintutil.ContainsNode(c.blockStmtOf(labeled.Stmt), matchUsage) {
c.warnRedundant(labeled)
return
}
@ -95,7 +96,7 @@ func (c *unlabelStmtChecker) VisitStmt(stmt ast.Stmt) {
if !c.isLoop(last) {
return
}
br := findNode(c.blockStmtOf(last), func(n ast.Node) bool {
br := lintutil.FindNode(c.blockStmtOf(last), func(n ast.Node) bool {
br, ok := n.(*ast.BranchStmt)
return ok && br.Label != nil &&
br.Label.Name == name && br.Tok == token.CONTINUE
@ -152,7 +153,7 @@ func (c *unlabelStmtChecker) blockStmtOf(n ast.Node) *ast.BlockStmt {
// usesLabel reports whether n contains a usage of label.
func (c *unlabelStmtChecker) usesLabel(n *ast.BlockStmt, label string) bool {
return containsNode(n, func(n ast.Node) bool {
return lintutil.ContainsNode(n, func(n ast.Node) bool {
branch, ok := n.(*ast.BranchStmt)
return ok && branch.Label != nil &&
branch.Label.Name == label &&

View File

@ -4,7 +4,7 @@ import (
"go/ast"
"go/types"
"github.com/go-critic/checkers/internal/lintutil"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astequal"
@ -60,8 +60,9 @@ func (c *unlambdaChecker) VisitExpr(x ast.Expr) {
}
}
c.warn(fn, callable)
if len(result.Args) == n {
c.warn(fn, callable)
}
}
func (c *unlambdaChecker) warn(cause ast.Node, suggestion string) {

View File

@ -54,6 +54,6 @@ func (c *unsliceChecker) unslice(expr ast.Expr) ast.Expr {
return expr
}
func (c *unsliceChecker) warn(cause ast.Expr, unsliced ast.Expr) {
func (c *unsliceChecker) warn(cause, unsliced ast.Expr) {
c.ctx.Warn(cause, "could simplify %s to %s", cause, unsliced)
}

View File

@ -6,9 +6,13 @@ import (
"strings"
"github.com/go-lintpack/lintpack"
"golang.org/x/tools/go/ast/astutil"
)
// isStdlibPkg reports whether pkg is a package from the Go standard library.
func isStdlibPkg(pkg *types.Package) bool {
return pkg != nil && pkg.Path() == pkg.Name()
}
// isUnitTestFunc reports whether FuncDecl declares testing function.
func isUnitTestFunc(ctx *lintpack.CheckerContext, fn *ast.FuncDecl) bool {
if !strings.HasPrefix(fn.Name.Name, "Test") {
@ -70,23 +74,3 @@ func identOf(x ast.Node) *ast.Ident {
return nil
}
}
// findNode applies pred for root and all it's childs until it returns true.
// Matched node is returned.
// If none of the nodes matched predicate, nil is returned.
func findNode(root ast.Node, pred func(ast.Node) bool) ast.Node {
var found ast.Node
astutil.Apply(root, nil, func(cur *astutil.Cursor) bool {
if pred(cur.Node()) {
found = cur.Node()
return false
}
return true
})
return found
}
// containsNode reports whether `findNode(root, pred)!=nil`.
func containsNode(root ast.Node, pred func(ast.Node) bool) bool {
return findNode(root, pred) != nil
}

View File

@ -0,0 +1,77 @@
package checkers
import (
"go/ast"
"go/token"
"github.com/go-critic/go-critic/checkers/internal/lintutil"
"github.com/go-lintpack/lintpack"
"github.com/go-lintpack/lintpack/astwalk"
"github.com/go-toolsmith/astcast"
"github.com/go-toolsmith/astequal"
"github.com/go-toolsmith/typep"
"golang.org/x/tools/go/ast/astutil"
)
func init() {
var info lintpack.CheckerInfo
info.Name = "weakCond"
info.Tags = []string{"diagnostic", "experimental"}
info.Summary = "Detects conditions that are unsafe due to not being exhaustive"
info.Before = `xs != nil && xs[0] != nil`
info.After = `len(xs) != 0 && xs[0] != nil`
collection.AddChecker(&info, func(ctx *lintpack.CheckerContext) lintpack.FileWalker {
return astwalk.WalkerForExpr(&weakCondChecker{ctx: ctx})
})
}
type weakCondChecker struct {
astwalk.WalkHandler
ctx *lintpack.CheckerContext
}
func (c *weakCondChecker) VisitExpr(expr ast.Expr) {
// TODO(Quasilyte): more patterns.
// TODO(Quasilyte): analyze and fix false positives.
cond := astcast.ToBinaryExpr(expr)
lhs := astcast.ToBinaryExpr(astutil.Unparen(cond.X))
rhs := astutil.Unparen(cond.Y)
// Pattern 1.
// `x != nil && usageOf(x[i])`
// Pattern 2.
// `x == nil || usageOf(x[i])`
// lhs is `x <op> nil`
x := lhs.X
if !typep.IsSlice(c.ctx.TypesInfo.TypeOf(x)) {
return
}
if astcast.ToIdent(lhs.Y).Name != "nil" {
return
}
pat1prefix := cond.Op == token.LAND && lhs.Op == token.NEQ
pat2prefix := cond.Op == token.LOR && lhs.Op == token.EQL
if !pat1prefix && !pat2prefix {
return
}
if c.isIndexed(rhs, x) {
c.warn(expr, "nil check may not be enough, check for len")
}
}
// isIndexed reports whether x is indexed inside given expr tree.
func (c *weakCondChecker) isIndexed(tree, x ast.Expr) bool {
return lintutil.ContainsNode(tree, func(n ast.Node) bool {
indexing := astcast.ToIndexExpr(n)
return astequal.Expr(x, indexing.X)
})
}
func (c *weakCondChecker) warn(cause ast.Node, suggest string) {
c.ctx.Warn(cause, "suspicious `%s`; %s", cause, suggest)
}

View File

@ -0,0 +1,41 @@
package astwalk
import (
"go/ast"
"strings"
)
type commentWalker struct {
visitor CommentVisitor
}
func (w *commentWalker) WalkFile(f *ast.File) {
if !w.visitor.EnterFile(f) {
return
}
for _, cg := range f.Comments {
visitCommentGroups(cg, w.visitor.VisitComment)
}
}
func visitCommentGroups(cg *ast.CommentGroup, visit func(*ast.CommentGroup)) {
var group []*ast.Comment
visitGroup := func(list []*ast.Comment) {
if len(list) == 0 {
return
}
cg := &ast.CommentGroup{List: list}
visit(cg)
}
for _, comment := range cg.List {
if strings.HasPrefix(comment.Text, "/*") {
visitGroup(group)
group = group[:0]
visitGroup([]*ast.Comment{comment})
} else {
group = append(group, comment)
}
}
visitGroup(group)
}

View File

@ -2,7 +2,6 @@ package astwalk
import (
"go/ast"
"strings"
)
type localCommentWalker struct {
@ -27,24 +26,7 @@ func (w *localCommentWalker) WalkFile(f *ast.File) {
continue
}
var group []*ast.Comment
visitGroup := func(list []*ast.Comment) {
if len(list) == 0 {
return
}
cg := &ast.CommentGroup{List: list}
w.visitor.VisitLocalComment(cg)
}
for _, comment := range cg.List {
if strings.HasPrefix(comment.Text, "/*") {
visitGroup(group)
group = group[:0]
visitGroup([]*ast.Comment{comment})
} else {
group = append(group, comment)
}
}
visitGroup(group)
visitCommentGroups(cg, w.visitor.VisitLocalComment)
}
}
}

View File

@ -5,8 +5,8 @@ import (
"go/token"
"go/types"
"github.com/go-lintpack/lintpack/internal/lintutil"
"github.com/go-toolsmith/astp"
"github.com/go-toolsmith/typep"
)
type typeExprWalker struct {
@ -49,7 +49,7 @@ func (w *typeExprWalker) visit(x ast.Expr) bool {
func (w *typeExprWalker) walk(x ast.Node) bool {
switch x := x.(type) {
case *ast.ParenExpr:
if lintutil.IsTypeExpr(w.info, x.X) {
if typep.IsTypeExpr(w.info, x.X) {
return w.visit(x)
}
return true
@ -63,7 +63,7 @@ func (w *typeExprWalker) walk(x ast.Node) bool {
// Like with conversions, method expressions are another special.
return w.inspectInner(x.X)
case *ast.StarExpr:
if lintutil.IsTypeExpr(w.info, x.X) {
if typep.IsTypeExpr(w.info, x.X) {
return w.visit(x)
}
return true
@ -95,7 +95,7 @@ func (w *typeExprWalker) walk(x ast.Node) bool {
func (w *typeExprWalker) inspectInner(x ast.Expr) bool {
parens, ok := x.(*ast.ParenExpr)
if ok && lintutil.IsTypeExpr(w.info, parens.X) && astp.IsStarExpr(parens.X) {
if ok && typep.IsTypeExpr(w.info, parens.X) && astp.IsStarExpr(parens.X) {
ast.Inspect(parens.X, w.walk)
return false
}

View File

@ -58,6 +58,12 @@ type (
walkerEvents
VisitLocalComment(*ast.CommentGroup)
}
// CommentVisitor visits every comment.
CommentVisitor interface {
walkerEvents
VisitComment(*ast.CommentGroup)
}
)
// walkerEvents describes common hooks available for most visitor types.

View File

@ -41,6 +41,11 @@ func WalkerForLocalComment(v LocalCommentVisitor) lintpack.FileWalker {
return &localCommentWalker{visitor: v}
}
// WalkerForComment returns file walker implementation for CommentVisitor.
func WalkerForComment(v CommentVisitor) lintpack.FileWalker {
return &commentWalker{visitor: v}
}
// WalkerForDocComment returns file walker implementation for DocCommentVisitor.
func WalkerForDocComment(v DocCommentVisitor) lintpack.FileWalker {
return &docCommentWalker{visitor: v}

View File

@ -1,44 +0,0 @@
package lintutil
import (
"go/ast"
"github.com/go-toolsmith/astequal"
)
// AstSet is a simple ast.Node set.
// Zero value is ready to use set.
// Can be reused after Clear call.
type AstSet struct {
items []ast.Node
}
// Contains reports whether s contains x.
func (s *AstSet) Contains(x ast.Node) bool {
for i := range s.items {
if astequal.Node(s.items[i], x) {
return true
}
}
return false
}
// Insert pushes x in s if it's not already there.
// Returns true if element was inserted.
func (s *AstSet) Insert(x ast.Node) bool {
if s.Contains(x) {
return false
}
s.items = append(s.items, x)
return true
}
// Clear removes all element from set.
func (s *AstSet) Clear() {
s.items = s.items[:0]
}
// Len returns the number of elements contained inside s.
func (s *AstSet) Len() int {
return len(s.items)
}

View File

@ -1,121 +0,0 @@
package lintutil
import (
"go/ast"
"go/token"
)
var (
nilIdent = &ast.Ident{}
nilSelectorExpr = &ast.SelectorExpr{}
nilUnaryExpr = &ast.UnaryExpr{}
nilBinaryExpr = &ast.BinaryExpr{}
nilCallExpr = &ast.CallExpr{}
nilParenExpr = &ast.ParenExpr{}
nilAssignStmt = &ast.AssignStmt{}
)
// IsNil reports whether x is nil.
// Unlike simple nil check, also detects nil AST sentinels.
func IsNil(x ast.Node) bool {
switch x := x.(type) {
case *ast.Ident:
return x == nilIdent || x == nil
case *ast.SelectorExpr:
return x == nilSelectorExpr || x == nil
case *ast.UnaryExpr:
return x == nilUnaryExpr || x == nil
case *ast.BinaryExpr:
return x == nilBinaryExpr || x == nil
case *ast.CallExpr:
return x == nilCallExpr || x == nil
case *ast.ParenExpr:
return x == nilParenExpr || x == nil
case *ast.AssignStmt:
return x == nilAssignStmt || x == nil
default:
return x == nil
}
}
// AsIdent coerces x into non-nil ident.
func AsIdent(x ast.Node) *ast.Ident {
e, ok := x.(*ast.Ident)
if !ok {
return nilIdent
}
return e
}
// AsSelectorExpr coerces x into non-nil selector expr.
func AsSelectorExpr(x ast.Node) *ast.SelectorExpr {
e, ok := x.(*ast.SelectorExpr)
if !ok {
return nilSelectorExpr
}
return e
}
// AsUnaryExpr coerces x into non-nil unary expr.
func AsUnaryExpr(x ast.Node) *ast.UnaryExpr {
e, ok := x.(*ast.UnaryExpr)
if !ok {
return nilUnaryExpr
}
return e
}
// AsUnaryExprOp is like AsUnaryExpr, but also checks for op token.
func AsUnaryExprOp(x ast.Node, op token.Token) *ast.UnaryExpr {
e, ok := x.(*ast.UnaryExpr)
if !ok || e.Op != op {
return nilUnaryExpr
}
return e
}
// AsBinaryExpr coerces x into non-nil binary expr.
func AsBinaryExpr(x ast.Node) *ast.BinaryExpr {
e, ok := x.(*ast.BinaryExpr)
if !ok {
return nilBinaryExpr
}
return e
}
// AsBinaryExprOp is like AsBinaryExpr, but also checks for op token.
func AsBinaryExprOp(x ast.Node, op token.Token) *ast.BinaryExpr {
e, ok := x.(*ast.BinaryExpr)
if !ok || e.Op != op {
return nilBinaryExpr
}
return e
}
// AsCallExpr coerces x into non-nil call expr.
func AsCallExpr(x ast.Node) *ast.CallExpr {
e, ok := x.(*ast.CallExpr)
if !ok {
return nilCallExpr
}
return e
}
// AsParenExpr coerces x into non-nil paren expr.
func AsParenExpr(x ast.Node) *ast.ParenExpr {
e, ok := x.(*ast.ParenExpr)
if !ok {
return nilParenExpr
}
return e
}
// AsAssignStmt coerces x into non-nil assign stmt.
func AsAssignStmt(x ast.Node) *ast.AssignStmt {
stmt, ok := x.(*ast.AssignStmt)
if !ok {
return nilAssignStmt
}
return stmt
}

View File

@ -1,37 +0,0 @@
package lintutil
import (
"go/ast"
"go/types"
)
// TODO: this package is a way to reuse code between lint and astwalk.
// Would be good to find it a better name.
// IsTypeExpr reports whether x represents type expression.
//
// Type expression does not evaluate to any run time value,
// but rather describes type that is used inside Go expression.
// For example, (*T)(v) is a CallExpr that "calls" (*T).
// (*T) is a type expression that tells Go compiler type v should be converted to.
func IsTypeExpr(info *types.Info, x ast.Expr) bool {
switch x := x.(type) {
case *ast.StarExpr:
return IsTypeExpr(info, x.X)
case *ast.ParenExpr:
return IsTypeExpr(info, x.X)
case *ast.SelectorExpr:
return IsTypeExpr(info, x.Sel)
case *ast.Ident:
// Identifier may be a type expression if object
// it reffers to is a type name.
_, ok := info.ObjectOf(x).(*types.TypeName)
return ok
case *ast.FuncType, *ast.StructType, *ast.InterfaceType, *ast.ArrayType, *ast.MapType:
return true
default:
return false
}
}

9
vendor/modules.txt vendored
View File

@ -10,13 +10,12 @@ github.com/davecgh/go-spew/spew
github.com/fatih/color
# github.com/fsnotify/fsnotify v1.4.7
github.com/fsnotify/fsnotify
# github.com/go-critic/checkers v0.0.0-20181204210945-97246d3b3c67
github.com/go-critic/checkers
github.com/go-critic/checkers/internal/lintutil
# github.com/go-lintpack/lintpack v0.5.1
# github.com/go-critic/go-critic v0.0.0-20181204210945-0af0999fabfb
github.com/go-critic/go-critic/checkers
github.com/go-critic/go-critic/checkers/internal/lintutil
# github.com/go-lintpack/lintpack v0.5.2
github.com/go-lintpack/lintpack
github.com/go-lintpack/lintpack/astwalk
github.com/go-lintpack/lintpack/internal/lintutil
# github.com/go-ole/go-ole v1.2.1
github.com/go-ole/go-ole
github.com/go-ole/go-ole/oleutil