From 5a32c620b6947455dcf909c65927ba4db2a085c4 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Sun, 14 Aug 2022 16:26:54 +0200 Subject: [PATCH] fix: refactor the configuration loading and parsing system It's only a first step, must be rewrite in the future. --- .golangci.reference.yml | 2 +- pkg/commands/executor.go | 5 - pkg/config/linters_settings.go | 16 +- pkg/config/linters_settings_gocritic.go | 366 --------------- pkg/golinters/gocritic.go | 427 +++++++++++++++++- .../gocritic_test.go} | 6 +- pkg/lint/lintersdb/manager.go | 4 +- test/testdata/configs/gocritic.yml | 5 +- test/testdata/gocritic.go | 4 + 9 files changed, 435 insertions(+), 400 deletions(-) delete mode 100644 pkg/config/linters_settings_gocritic.go rename pkg/{config/linters_settings_gocritic_test.go => golinters/gocritic_test.go} (88%) diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 9f419d14685c..46fd2df5adea 100644 --- a/.golangci.reference.yml +++ b/.golangci.reference.yml @@ -1113,7 +1113,7 @@ linters-settings: makezero: # Allow only slices initialized with a length of zero. # Default: false - always: false + always: true maligned: # Print struct with more effective memory layout or not. diff --git a/pkg/commands/executor.go b/pkg/commands/executor.go index d9c747f6f1b5..2723f5c0a92a 100644 --- a/pkg/commands/executor.go +++ b/pkg/commands/executor.go @@ -117,11 +117,6 @@ func NewExecutor(version, commit, date string) *Executor { // recreate after getting config e.DBManager = lintersdb.NewManager(e.cfg, e.log).WithCustomLinters() - e.cfg.LintersSettings.Gocritic.InferEnabledChecks(e.log) - if err = e.cfg.LintersSettings.Gocritic.Validate(e.log); err != nil { - e.log.Fatalf("Invalid gocritic settings: %s", err) - } - // Slice options must be explicitly set for proper merging of config and command-line options. fixSlicesFlags(e.runCmd.Flags()) fixSlicesFlags(e.lintersCmd.Flags()) diff --git a/pkg/config/linters_settings.go b/pkg/config/linters_settings.go index 3c356b40eff5..1e923409a05d 100644 --- a/pkg/config/linters_settings.go +++ b/pkg/config/linters_settings.go @@ -40,8 +40,8 @@ var defaultLintersSettings = LintersSettings{ Gocognit: GocognitSettings{ MinComplexity: 30, }, - Gocritic: GocriticSettings{ - SettingsPerCheck: map[string]GocriticCheckSettings{}, + Gocritic: GoCriticSettings{ + SettingsPerCheck: map[string]GoCriticCheckSettings{}, }, Godox: GodoxSettings{ Keywords: []string{}, @@ -133,7 +133,7 @@ type LintersSettings struct { Gci GciSettings Gocognit GocognitSettings Goconst GoConstSettings - Gocritic GocriticSettings + Gocritic GoCriticSettings Gocyclo GoCycloSettings Godot GodotSettings Godox GodoxSettings @@ -306,6 +306,16 @@ type GoConstSettings struct { IgnoreCalls bool `mapstructure:"ignore-calls"` } +type GoCriticSettings struct { + EnabledChecks []string `mapstructure:"enabled-checks"` + DisabledChecks []string `mapstructure:"disabled-checks"` + EnabledTags []string `mapstructure:"enabled-tags"` + DisabledTags []string `mapstructure:"disabled-tags"` + SettingsPerCheck map[string]GoCriticCheckSettings `mapstructure:"settings"` +} + +type GoCriticCheckSettings map[string]interface{} + type GoCycloSettings struct { MinComplexity int `mapstructure:"min-complexity"` } diff --git a/pkg/config/linters_settings_gocritic.go b/pkg/config/linters_settings_gocritic.go deleted file mode 100644 index 7f6905f5f952..000000000000 --- a/pkg/config/linters_settings_gocritic.go +++ /dev/null @@ -1,366 +0,0 @@ -package config - -import ( - "fmt" - "sort" - "strings" - - _ "github.com/go-critic/go-critic/checkers" // this import register checkers - "github.com/go-critic/go-critic/framework/linter" - "github.com/pkg/errors" - - "github.com/golangci/golangci-lint/pkg/logutils" -) - -const gocriticDebugKey = "gocritic" - -var ( - gocriticDebugf = logutils.Debug(gocriticDebugKey) - isGocriticDebug = logutils.HaveDebugTag(gocriticDebugKey) - allGocriticCheckers = linter.GetCheckersInfo() - allGocriticCheckerMap = func() map[string]*linter.CheckerInfo { - checkInfoMap := make(map[string]*linter.CheckerInfo) - for _, checkInfo := range allGocriticCheckers { - checkInfoMap[checkInfo.Name] = checkInfo - } - return checkInfoMap - }() -) - -type GocriticCheckSettings map[string]interface{} - -type GocriticSettings struct { - EnabledChecks []string `mapstructure:"enabled-checks"` - DisabledChecks []string `mapstructure:"disabled-checks"` - EnabledTags []string `mapstructure:"enabled-tags"` - DisabledTags []string `mapstructure:"disabled-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 := make(map[string]bool, len(ss)) - 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() - - allTags := make([]string, 0, len(tagToCheckers)) - 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") - } -} - -func (s *GocriticSettings) InferEnabledChecks(log logutils.Log) { - gocriticCheckerTagsDebugf() - - enabledByDefaultChecks := getDefaultEnabledGocriticCheckersNames() - debugChecksListf(enabledByDefaultChecks, "Enabled by default") - - disabledByDefaultChecks := getDefaultDisabledGocriticCheckersNames() - debugChecksListf(disabledByDefaultChecks, "Disabled by default") - - enabledChecks := make([]string, 0, len(s.EnabledTags)+len(enabledByDefaultChecks)) - - // EnabledTags - 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...) - } - - // DisabledTags - if len(s.DisabledTags) != 0 { - enabledChecks = filterByDisableTags(enabledChecks, s.DisabledTags, log) - } - - // EnabledChecks - 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) - } - } - - // DisabledChecks - 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 explicitly disabled via config. However, as this check "+ - "is disabled by default, there is no need to explicitly disable it via config.", 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 -} - -func intersectStringSlice(s1, s2 []string) []string { - s1Map := make(map[string]struct{}, len(s1)) - for _, s := range s1 { - s1Map[s] = struct{}{} - } - - result := make([]string, 0) - for _, s := range s2 { - if _, exists := s1Map[s]; exists { - result = append(result, s) - } - } - - return result -} - -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 [enabled]tag %q doesn't exist", tag) - } - } - } - - if len(s.DisabledTags) > 0 { - tagToCheckers := buildGocriticTagToCheckersMap() - for _, tag := range s.EnabledTags { - if _, ok := tagToCheckers[tag]; !ok { - return fmt.Errorf("gocritic [disabled]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") - } - - if err := s.validateCheckerNames(log); 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 { - namesSlice := make([]string, 0, len(allowedNames)) - for name := range allowedNames { - namesSlice = append(namesSlice, name) - } - return sprintStrings(namesSlice) -} - -func sprintStrings(ss []string) string { - sort.Strings(ss) - return fmt.Sprint(ss) -} - -// getAllCheckerNames returns a map containing all checker names supported by gocritic. -func getAllCheckerNames() map[string]bool { - allCheckerNames := make(map[string]bool, len(allGocriticCheckers)) - for _, checker := range allGocriticCheckers { - allCheckerNames[strings.ToLower(checker.Name)] = true - } - - return allCheckerNames -} - -func isEnabledByDefaultGocriticCheck(info *linter.CheckerInfo) bool { - return !info.HasTag("experimental") && - !info.HasTag("opinionated") && - !info.HasTag("performance") -} - -func getDefaultEnabledGocriticCheckersNames() []string { - var enabled []string - for _, info := range allGocriticCheckers { - enable := isEnabledByDefaultGocriticCheck(info) - if enable { - enabled = append(enabled, info.Name) - } - } - - return enabled -} - -func getDefaultDisabledGocriticCheckersNames() []string { - var disabled []string - for _, info := range allGocriticCheckers { - enable := isEnabledByDefaultGocriticCheck(info) - if !enable { - disabled = append(disabled, info.Name) - } - } - - return disabled -} - -func (s *GocriticSettings) validateCheckerNames(log logutils.Log) 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)) - } - } - - for checkName := range s.SettingsPerCheck { - if _, ok := allowedNames[checkName]; !ok { - return fmt.Errorf("invalid setting, checker %s doesn't exist, all existing checkers: %s", - checkName, sprintAllowedCheckerNames(allowedNames)) - } - if !s.IsCheckEnabled(checkName) { - log.Warnf("Gocritic settings were provided for not enabled check %q", checkName) - } - } - - return nil -} - -func (s *GocriticSettings) GetLowercasedParams() map[string]GocriticCheckSettings { - ret := make(map[string]GocriticCheckSettings, len(s.SettingsPerCheck)) - for checker, params := range s.SettingsPerCheck { - ret[strings.ToLower(checker)] = params - } - return ret -} - -func filterByDisableTags(enabledChecks, disableTags []string, log logutils.Log) []string { - enabledChecksSet := stringsSliceToSet(enabledChecks) - for _, enabledCheck := range enabledChecks { - checkInfo, checkInfoExists := allGocriticCheckerMap[enabledCheck] - if !checkInfoExists { - log.Warnf("Gocritic check %q was not exists via filtering disabled tags", enabledCheck) - continue - } - hitTags := intersectStringSlice(checkInfo.Tags, disableTags) - if len(hitTags) != 0 { - delete(enabledChecksSet, enabledCheck) - } - } - debugChecksListf(enabledChecks, "Disabled by config tags %s", sprintStrings(disableTags)) - - enabledChecks = nil - for enabledCheck := range enabledChecksSet { - enabledChecks = append(enabledChecks, enabledCheck) - } - return enabledChecks -} diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index ce50179db523..9af28ce8cb5f 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -13,30 +13,37 @@ import ( "github.com/go-critic/go-critic/checkers" gocriticlinter "github.com/go-critic/go-critic/framework/linter" + "github.com/pkg/errors" "golang.org/x/tools/go/analysis" "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/golinters/goanalysis" "github.com/golangci/golangci-lint/pkg/lint/linter" + "github.com/golangci/golangci-lint/pkg/logutils" "github.com/golangci/golangci-lint/pkg/result" ) -const gocriticName = "gocritic" +const goCriticName = "gocritic" -func NewGocritic(settings *config.GocriticSettings, cfg *config.Config) *goanalysis.Linter { +const goCriticDebugKey = "gocritic" + +var ( + goCriticDebugf = logutils.Debug(goCriticDebugKey) + isGoCriticDebug = logutils.HaveDebugTag(goCriticDebugKey) +) + +func NewGoCritic(settings *config.GoCriticSettings, cfg *config.Config) *goanalysis.Linter { var mu sync.Mutex var resIssues []goanalysis.Issue - sizes := types.SizesFor("gc", runtime.GOARCH) - wrapper := &goCriticWrapper{ settings: settings, cfg: cfg, - sizes: sizes, + sizes: types.SizesFor("gc", runtime.GOARCH), } analyzer := &analysis.Analyzer{ - Name: gocriticName, + Name: goCriticName, Doc: goanalysis.TheOnlyanalyzerDoc, Run: func(pass *analysis.Pass) (interface{}, error) { issues, err := wrapper.run(pass) @@ -57,26 +64,55 @@ func NewGocritic(settings *config.GocriticSettings, cfg *config.Config) *goanaly } return goanalysis.NewLinter( - gocriticName, + goCriticName, `Provides diagnostics that check for bugs, performance and style issues. Extensible without recompilation through dynamic rules. Dynamic rules are written declaratively with AST patterns, filters, report message and optional suggestion.`, []*analysis.Analyzer{analyzer}, nil, - ).WithIssuesReporter(func(*linter.Context) []goanalysis.Issue { - return resIssues - }).WithLoadMode(goanalysis.LoadModeTypesInfo) + ). + WithContextSetter(func(context *linter.Context) { + wrapper.init() + }). + WithIssuesReporter(func(*linter.Context) []goanalysis.Issue { + return resIssues + }).WithLoadMode(goanalysis.LoadModeTypesInfo) } type goCriticWrapper struct { - settings *config.GocriticSettings - cfg *config.Config - sizes types.Sizes - once sync.Once + settings *config.GoCriticSettings + settingsWrapper *goCriticSettingsWrapper + cfg *config.Config + sizes types.Sizes + once sync.Once +} + +func (w *goCriticWrapper) init() { + if w.settings != nil { + // the 'defer' is here to catch the panic from checkers.InitEmbeddedRules() + defer func() { + if err := recover(); err != nil { + linterLogger.Fatalf("%s: %v: setting an explicit GOROOT can fix this problem.", goCriticName, err) + return + } + }() + w.once.Do(checkers.InitEmbeddedRules) + + settingsWrapper := newGoCriticSettingsWrapper(w.settings) + + settingsWrapper.InferEnabledChecks(linterLogger) + if err := settingsWrapper.Validate(linterLogger); err != nil { + linterLogger.Fatalf("%s: invalid settings: %s", goCriticName, err) + } + + w.settingsWrapper = settingsWrapper + } } func (w *goCriticWrapper) run(pass *analysis.Pass) ([]goanalysis.Issue, error) { - w.once.Do(checkers.InitEmbeddedRules) + if w.settingsWrapper == nil { + return nil, fmt.Errorf("the settings wrapper is nil") + } linterCtx := gocriticlinter.NewContext(pass.Fset, w.sizes) @@ -98,11 +134,11 @@ func (w *goCriticWrapper) run(pass *analysis.Pass) ([]goanalysis.Issue, error) { } func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context) ([]*gocriticlinter.Checker, error) { - allParams := w.settings.GetLowercasedParams() + allParams := w.settingsWrapper.GetLowercasedParams() var enabledCheckers []*gocriticlinter.Checker for _, info := range gocriticlinter.GetCheckersInfo() { - if !w.settings.IsCheckEnabled(info.Name) { + if !w.settingsWrapper.IsCheckEnabled(info.Name) { continue } @@ -144,7 +180,7 @@ func runGocriticOnFile(linterCtx *gocriticlinter.Context, f *ast.File, checks [] issue := result.Issue{ Pos: pos, Text: fmt.Sprintf("%s: %s", c.Info.Name, warn.Text), - FromLinter: gocriticName, + FromLinter: goCriticName, } if warn.HasQuickFix() { @@ -164,7 +200,7 @@ func runGocriticOnFile(linterCtx *gocriticlinter.Context, f *ast.File, checks [] return res } -func (w *goCriticWrapper) configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string]config.GocriticCheckSettings) error { +func (w *goCriticWrapper) configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string]config.GoCriticCheckSettings) error { params := allParams[strings.ToLower(info.Name)] if params == nil { // no config for this checker return nil @@ -226,3 +262,356 @@ func (w *goCriticWrapper) normalizeCheckerParamsValue(p interface{}) interface{} return p } } + +// TODO(ldez): rewrite and simplify goCriticSettingsWrapper. + +type goCriticSettingsWrapper struct { + *config.GoCriticSettings + + allGocriticCheckers []*gocriticlinter.CheckerInfo + allGocriticCheckerMap map[string]*gocriticlinter.CheckerInfo + + inferredEnabledChecks map[string]bool +} + +func newGoCriticSettingsWrapper(settings *config.GoCriticSettings) *goCriticSettingsWrapper { + allGocriticCheckers := gocriticlinter.GetCheckersInfo() + + allGocriticCheckerMap := make(map[string]*gocriticlinter.CheckerInfo) + for _, checkInfo := range allGocriticCheckers { + allGocriticCheckerMap[checkInfo.Name] = checkInfo + } + + return &goCriticSettingsWrapper{ + GoCriticSettings: settings, + allGocriticCheckers: allGocriticCheckers, + allGocriticCheckerMap: allGocriticCheckerMap, + 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 := make(map[string]bool, len(ss)) + for _, s := range ss { + ret[s] = true + } + + return ret +} + +func (s *goCriticSettingsWrapper) buildGocriticTagToCheckersMap() map[string][]string { + tagToCheckers := map[string][]string{} + for _, checker := range s.allGocriticCheckers { + for _, tag := range checker.Tags { + tagToCheckers[tag] = append(tagToCheckers[tag], checker.Name) + } + } + return tagToCheckers +} + +func (s *goCriticSettingsWrapper) gocriticCheckerTagsDebugf() { + if !isGoCriticDebug { + return + } + + tagToCheckers := s.buildGocriticTagToCheckersMap() + + allTags := make([]string, 0, len(tagToCheckers)) + 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 *goCriticSettingsWrapper) gocriticDisabledCheckersDebugf() { + if !isGoCriticDebug { + return + } + + var disabledCheckers []string + for _, checker := range s.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") + } +} + +func (s *goCriticSettingsWrapper) InferEnabledChecks(log logutils.Log) { + s.gocriticCheckerTagsDebugf() + + enabledByDefaultChecks := s.getDefaultEnabledGocriticCheckersNames() + debugChecksListf(enabledByDefaultChecks, "Enabled by default") + + disabledByDefaultChecks := s.getDefaultDisabledGocriticCheckersNames() + debugChecksListf(disabledByDefaultChecks, "Disabled by default") + + enabledChecks := make([]string, 0, len(s.EnabledTags)+len(enabledByDefaultChecks)) + + // EnabledTags + if len(s.EnabledTags) != 0 { + tagToCheckers := s.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...) + } + + // DisabledTags + if len(s.DisabledTags) != 0 { + enabledChecks = s.filterByDisableTags(enabledChecks, s.DisabledTags, log) + } + + // EnabledChecks + 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) + } + } + + // DisabledChecks + 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 explicitly disabled via config. However, as this check "+ + "is disabled by default, there is no need to explicitly disable it via config.", 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 +} + +func intersectStringSlice(s1, s2 []string) []string { + s1Map := make(map[string]struct{}, len(s1)) + for _, s := range s1 { + s1Map[s] = struct{}{} + } + + results := make([]string, 0) + for _, s := range s2 { + if _, exists := s1Map[s]; exists { + results = append(results, s) + } + } + + return results +} + +func (s *goCriticSettingsWrapper) 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 := s.buildGocriticTagToCheckersMap() + for _, tag := range s.EnabledTags { + if _, ok := tagToCheckers[tag]; !ok { + return fmt.Errorf("gocritic [enabled]tag %q doesn't exist", tag) + } + } + } + + if len(s.DisabledTags) > 0 { + tagToCheckers := s.buildGocriticTagToCheckersMap() + for _, tag := range s.EnabledTags { + if _, ok := tagToCheckers[tag]; !ok { + return fmt.Errorf("gocritic [disabled]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") + } + + if err := s.validateCheckerNames(log); err != nil { + return errors.Wrap(err, "validation failed") + } + + return nil +} + +func (s *goCriticSettingsWrapper) IsCheckEnabled(name string) bool { + return s.inferredEnabledChecks[strings.ToLower(name)] +} + +func sprintAllowedCheckerNames(allowedNames map[string]bool) string { + namesSlice := make([]string, 0, len(allowedNames)) + for name := range allowedNames { + namesSlice = append(namesSlice, name) + } + return sprintStrings(namesSlice) +} + +func sprintStrings(ss []string) string { + sort.Strings(ss) + return fmt.Sprint(ss) +} + +// getAllCheckerNames returns a map containing all checker names supported by gocritic. +func (s *goCriticSettingsWrapper) getAllCheckerNames() map[string]bool { + allCheckerNames := make(map[string]bool, len(s.allGocriticCheckers)) + for _, checker := range s.allGocriticCheckers { + allCheckerNames[strings.ToLower(checker.Name)] = true + } + + return allCheckerNames +} + +func isEnabledByDefaultGocriticCheck(info *gocriticlinter.CheckerInfo) bool { + return !info.HasTag("experimental") && + !info.HasTag("opinionated") && + !info.HasTag("performance") +} + +func (s *goCriticSettingsWrapper) getDefaultEnabledGocriticCheckersNames() []string { + var enabled []string + for _, info := range s.allGocriticCheckers { + enable := isEnabledByDefaultGocriticCheck(info) + if enable { + enabled = append(enabled, info.Name) + } + } + + return enabled +} + +func (s *goCriticSettingsWrapper) getDefaultDisabledGocriticCheckersNames() []string { + var disabled []string + for _, info := range s.allGocriticCheckers { + enable := isEnabledByDefaultGocriticCheck(info) + if !enable { + disabled = append(disabled, info.Name) + } + } + + return disabled +} + +func (s *goCriticSettingsWrapper) validateCheckerNames(log logutils.Log) error { + allowedNames := s.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)) + } + } + + for checkName := range s.SettingsPerCheck { + if _, ok := allowedNames[checkName]; !ok { + return fmt.Errorf("invalid setting, checker %s doesn't exist, all existing checkers: %s", + checkName, sprintAllowedCheckerNames(allowedNames)) + } + if !s.IsCheckEnabled(checkName) { + log.Warnf("%s: settings were provided for not enabled check %q", goCriticName, checkName) + } + } + + return nil +} + +func (s *goCriticSettingsWrapper) GetLowercasedParams() map[string]config.GoCriticCheckSettings { + ret := make(map[string]config.GoCriticCheckSettings, len(s.SettingsPerCheck)) + for checker, params := range s.SettingsPerCheck { + ret[strings.ToLower(checker)] = params + } + return ret +} + +func (s *goCriticSettingsWrapper) filterByDisableTags(enabledChecks, disableTags []string, log logutils.Log) []string { + enabledChecksSet := stringsSliceToSet(enabledChecks) + for _, enabledCheck := range enabledChecks { + checkInfo, checkInfoExists := s.allGocriticCheckerMap[enabledCheck] + if !checkInfoExists { + log.Warnf("%s: check %q was not exists via filtering disabled tags", goCriticName, enabledCheck) + continue + } + hitTags := intersectStringSlice(checkInfo.Tags, disableTags) + if len(hitTags) != 0 { + delete(enabledChecksSet, enabledCheck) + } + } + debugChecksListf(enabledChecks, "Disabled by config tags %s", sprintStrings(disableTags)) + + enabledChecks = nil + for enabledCheck := range enabledChecksSet { + enabledChecks = append(enabledChecks, enabledCheck) + } + return enabledChecks +} diff --git a/pkg/config/linters_settings_gocritic_test.go b/pkg/golinters/gocritic_test.go similarity index 88% rename from pkg/config/linters_settings_gocritic_test.go rename to pkg/golinters/gocritic_test.go index 6b961a7379dd..cb6dcbe25b39 100644 --- a/pkg/config/linters_settings_gocritic_test.go +++ b/pkg/golinters/gocritic_test.go @@ -1,4 +1,4 @@ -package config +package golinters import ( "log" @@ -25,7 +25,9 @@ func Test_filterByDisableTags(t *testing.T) { disabledTags := []string{"experimental", "opinionated"} enabledChecks := []string{"appendAssign", "sortSlice", "caseOrder", "dupImport"} - filterEnabledChecks := filterByDisableTags(enabledChecks, disabledTags, &tLog{}) + settingsWrapper := newGoCriticSettingsWrapper(nil) + + filterEnabledChecks := settingsWrapper.filterByDisableTags(enabledChecks, disabledTags, &tLog{}) sort.Strings(filterEnabledChecks) diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index ff19634c90f5..4552153c69bd 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -119,7 +119,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { gciCfg *config.GciSettings gocognitCfg *config.GocognitSettings goconstCfg *config.GoConstSettings - gocriticCfg *config.GocriticSettings + gocriticCfg *config.GoCriticSettings gocycloCfg *config.GoCycloSettings godotCfg *config.GodotSettings godoxCfg *config.GodoxSettings @@ -436,7 +436,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { WithPresets(linter.PresetStyle). WithURL("https://github.com/jgautheron/goconst"), - linter.NewConfig(golinters.NewGocritic(gocriticCfg, m.cfg)). + linter.NewConfig(golinters.NewGoCritic(gocriticCfg, m.cfg)). WithSince("v1.12.0"). WithPresets(linter.PresetStyle, linter.PresetMetaLinter). WithLoadForGoAnalysis(). diff --git a/test/testdata/configs/gocritic.yml b/test/testdata/configs/gocritic.yml index 5cdda3736af5..019e6afb83fa 100644 --- a/test/testdata/configs/gocritic.yml +++ b/test/testdata/configs/gocritic.yml @@ -3,10 +3,11 @@ linters-settings: enabled-checks: - rangeValCopy - flagDeref + - wrapperFunc - ruleguard settings: - rangevalcopy: - sizethreshold: 2 + rangeValCopy: + sizeThreshold: 2 ruleguard: debug: dupSubExpr failOn: dsl,import diff --git a/test/testdata/gocritic.go b/test/testdata/gocritic.go index d3b59fce3f22..d59bc48ebbeb 100644 --- a/test/testdata/gocritic.go +++ b/test/testdata/gocritic.go @@ -42,3 +42,7 @@ func gocriticDup(x bool) { log.Print("x is true") } } + +func gocriticRuleWrapperFunc() { + strings.Replace("abcabc", "a", "d", -1) // ERROR "ruleguard: this Replace call can be simplified.*" +}