From 510c38d9d970b265c0e288d2ef71512f79a64f48 Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Fri, 20 Oct 2023 08:50:11 +0300 Subject: [PATCH 1/8] gocritic: support of enable-all and disable-all options --- .golangci.reference.yml | 20 +- pkg/config/linters_settings.go | 2 + pkg/golinters/gocritic.go | 480 +++++++++++++++------------------ pkg/golinters/gocritic_test.go | 448 +++++++++++++++++++++++++++--- pkg/lint/lintersdb/manager.go | 2 +- 5 files changed, 657 insertions(+), 295 deletions(-) diff --git a/.golangci.reference.yml b/.golangci.reference.yml index 3f87428e6c7f..47ba398fd394 100644 --- a/.golangci.reference.yml +++ b/.golangci.reference.yml @@ -525,22 +525,32 @@ linters-settings: ignore-strings: 'foo.+' gocritic: - # 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. + # Disable all checks. + # Default: false + disable-all: true + # Which checks should be enabled in addition to default checks; can't be combined with 'disabled-checks'. + # By default, list of stable checks is used (https://go-critic.github.io/overview#checks-overview): + # appendAssign, argOrder, assignOp, badCall, badCond, captLocal, caseOrder, codegenComment, commentFormatting, + # defaultCaseOrder, deprecatedComment, dupArg, dupBranchBody, dupCase, dupSubExpr, elseif, exitAfterDefer, + # flagDeref, flagName, ifElseChain, mapKey, newDeref, offBy1, regexpMust, singleCaseSwitch, sloppyLen, + # sloppyTypeAssert, switchTrue, typeSwitchVar, underef, unlambda, unslice, valSwap, wrapperFunc + # To see which checks are enabled run `GL_DEBUG=gocritic golangci-lint run --enable=gocritic`. enabled-checks: - nestingReduce - unnamedResult - ruleguard - truncateCmp + # Enable all checks. + # Default: false + enable-all: true # Which checks should be disabled; can't be combined with 'enabled-checks'. # Default: [] disabled-checks: - regexpMust - # Enable multiple checks by tags, run `GL_DEBUG=gocritic golangci-lint run` to see all tags and checks. + # Enable multiple checks by tags in addition to default checks. + # Run `GL_DEBUG=gocritic golangci-lint run --enable=gocritic` to see all tags and checks. # See https://github.com/go-critic/go-critic#usage -> section "Tags". # Default: [] enabled-tags: diff --git a/pkg/config/linters_settings.go b/pkg/config/linters_settings.go index a3206f597824..532c3d0c3fd0 100644 --- a/pkg/config/linters_settings.go +++ b/pkg/config/linters_settings.go @@ -459,7 +459,9 @@ type GoConstSettings struct { type GoCriticSettings struct { Go string `mapstructure:"-"` + DisableAll bool `mapstructure:"disable-all"` EnabledChecks []string `mapstructure:"enabled-checks"` + EnableAll bool `mapstructure:"enable-all"` DisabledChecks []string `mapstructure:"disabled-checks"` EnabledTags []string `mapstructure:"enabled-tags"` DisabledTags []string `mapstructure:"disabled-tags"` diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index 3cf43afc6225..1f6f71c5872a 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -15,6 +15,7 @@ import ( "github.com/go-critic/go-critic/checkers" gocriticlinter "github.com/go-critic/go-critic/linter" "golang.org/x/exp/maps" + "golang.org/x/exp/slices" "golang.org/x/tools/go/analysis" "github.com/golangci/golangci-lint/pkg/config" @@ -31,13 +32,13 @@ var ( isGoCriticDebug = logutils.HaveDebugTag(logutils.DebugKeyGoCritic) ) -func NewGoCritic(settings *config.GoCriticSettings, cfg *config.Config) *goanalysis.Linter { +func NewGoCritic(settings *config.GoCriticSettings, lintConfigDir string) *goanalysis.Linter { var mu sync.Mutex var resIssues []goanalysis.Issue wrapper := &goCriticWrapper{ - cfg: cfg, - sizes: types.SizesFor("gc", runtime.GOARCH), + lintConfigDir: lintConfigDir, + sizes: types.SizesFor("gc", runtime.GOARCH), } analyzer := &analysis.Analyzer{ @@ -74,12 +75,13 @@ Dynamic rules are written declaratively with AST patterns, filters, report messa }). WithIssuesReporter(func(*linter.Context) []goanalysis.Issue { return resIssues - }).WithLoadMode(goanalysis.LoadModeTypesInfo) + }). + WithLoadMode(goanalysis.LoadModeTypesInfo) } type goCriticWrapper struct { settingsWrapper *goCriticSettingsWrapper - cfg *config.Config + lintConfigDir string sizes types.Sizes once sync.Once } @@ -92,15 +94,15 @@ func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutil w.once.Do(func() { err := checkers.InitEmbeddedRules() if err != nil { - logger.Fatalf("%s: %v: setting an explicit GOROOT can fix this problem.", goCriticName, err) + logger.Fatalf("%s: %v: setting an explicit GOROOT can fix this problem", goCriticName, err) } }) settingsWrapper := newGoCriticSettingsWrapper(settings, logger) - - settingsWrapper.inferEnabledChecks() - - if err := settingsWrapper.validate(); err != nil { + settingsWrapper.InferEnabledChecks() + // NOTE(a.telyshev): Validate must be after InferEnabledChecks, not before. + // Because it uses gathered information about tags set and finally enabled checks. + if err := settingsWrapper.Validate(); err != nil { logger.Fatalf("%s: invalid settings: %s", goCriticName, err) } @@ -109,7 +111,7 @@ func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutil func (w *goCriticWrapper) run(pass *analysis.Pass) ([]goanalysis.Issue, error) { if w.settingsWrapper == nil { - return nil, fmt.Errorf("the settings wrapper is nil") + return nil, errors.New("the settings wrapper is nil") } linterCtx := gocriticlinter.NewContext(pass.Fset, w.sizes) @@ -134,11 +136,11 @@ func (w *goCriticWrapper) run(pass *analysis.Pass) ([]goanalysis.Issue, error) { } func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context) ([]*gocriticlinter.Checker, error) { - allParams := w.settingsWrapper.getLowerCasedParams() + allParams := w.settingsWrapper.GetLowerCasedParams() var enabledCheckers []*gocriticlinter.Checker for _, info := range gocriticlinter.GetCheckersInfo() { - if !w.settingsWrapper.isCheckEnabled(info.Name) { + if !w.settingsWrapper.IsCheckEnabled(info.Name) { continue } @@ -156,8 +158,11 @@ func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context return enabledCheckers, nil } -func runGocriticOnPackage(linterCtx *gocriticlinter.Context, checks []*gocriticlinter.Checker, - files []*ast.File) []result.Issue { +func runGocriticOnPackage( + linterCtx *gocriticlinter.Context, + checks []*gocriticlinter.Checker, + files []*ast.File, +) []result.Issue { var res []result.Issue for _, f := range files { filename := filepath.Base(linterCtx.FileSet.Position(f.Pos()).Filename) @@ -254,377 +259,336 @@ func (w *goCriticWrapper) normalizeCheckerParamsValue(p any) any { return rv.Bool() case reflect.String: // Perform variable substitution. - return strings.ReplaceAll(rv.String(), "${configDir}", w.cfg.GetConfigDir()) + return strings.ReplaceAll(rv.String(), "${configDir}", w.lintConfigDir) default: return p } } -// TODO(ldez): rewrite and simplify goCriticSettingsWrapper. - type goCriticSettingsWrapper struct { *config.GoCriticSettings logger logutils.Log - allCheckers []*gocriticlinter.CheckerInfo - allCheckerMap map[string]*gocriticlinter.CheckerInfo + allCheckers []*gocriticlinter.CheckerInfo + allCheckersByName map[string]*gocriticlinter.CheckerInfo + + allTagsSorted []string + allChecksByTag map[string][]string - inferredEnabledChecks map[string]bool + inferredEnabledChecks map[string]struct{} } func newGoCriticSettingsWrapper(settings *config.GoCriticSettings, logger logutils.Log) *goCriticSettingsWrapper { allCheckers := gocriticlinter.GetCheckersInfo() - - allCheckerMap := make(map[string]*gocriticlinter.CheckerInfo) + allCheckersByName := make(map[string]*gocriticlinter.CheckerInfo, len(allCheckers)) for _, checkInfo := range allCheckers { - allCheckerMap[checkInfo.Name] = checkInfo - } - - return &goCriticSettingsWrapper{ - GoCriticSettings: settings, - logger: logger, - allCheckers: allCheckers, - allCheckerMap: allCheckerMap, - inferredEnabledChecks: map[string]bool{}, + allCheckersByName[checkInfo.Name] = checkInfo } -} -func (s *goCriticSettingsWrapper) buildTagToCheckersMap() map[string][]string { - tagToCheckers := map[string][]string{} - - for _, checker := range s.allCheckers { + allChecksByTag := make(map[string][]string) + for _, checker := range allCheckers { for _, tag := range checker.Tags { - tagToCheckers[tag] = append(tagToCheckers[tag], checker.Name) + allChecksByTag[tag] = append(allChecksByTag[tag], checker.Name) } } - return tagToCheckers -} - -func (s *goCriticSettingsWrapper) checkerTagsDebugf() { - if !isGoCriticDebug { - return + allTagsSorted := make([]string, 0, len(allChecksByTag)) + for t := range allChecksByTag { + allTagsSorted = append(allTagsSorted, t) } + sort.Strings(allTagsSorted) - tagToCheckers := s.buildTagToCheckersMap() - - allTags := maps.Keys(tagToCheckers) - sort.Strings(allTags) - - goCriticDebugf("All gocritic existing tags and checks:") - for _, tag := range allTags { - debugChecksListf(tagToCheckers[tag], " tag %q", tag) + return &goCriticSettingsWrapper{ + GoCriticSettings: settings, + logger: logger, + allCheckers: allCheckers, + allCheckersByName: allCheckersByName, + allTagsSorted: allTagsSorted, + allChecksByTag: allChecksByTag, + inferredEnabledChecks: make(map[string]struct{}), } } -func (s *goCriticSettingsWrapper) disabledCheckersDebugf() { - if !isGoCriticDebug { - return - } - - var disabledCheckers []string - for _, checker := range s.allCheckers { - if s.inferredEnabledChecks[strings.ToLower(checker.Name)] { - continue - } +func (s *goCriticSettingsWrapper) GetLowerCasedParams() map[string]config.GoCriticCheckSettings { + ret := make(map[string]config.GoCriticCheckSettings, len(s.SettingsPerCheck)) - disabledCheckers = append(disabledCheckers, checker.Name) + for checker, params := range s.SettingsPerCheck { + ret[strings.ToLower(checker)] = params } - if len(disabledCheckers) == 0 { - goCriticDebugf("All checks are enabled") - } else { - debugChecksListf(disabledCheckers, "Final not used") - } + return ret } -func (s *goCriticSettingsWrapper) inferEnabledChecks() { - s.checkerTagsDebugf() +// InferEnabledChecks tries to be consistent with (lintersdb.EnabledSet).build. +func (s *goCriticSettingsWrapper) InferEnabledChecks() { + s.debugChecksInitialState() - enabledByDefaultChecks := s.getDefaultEnabledCheckersNames() + enabledByDefaultChecks, disabledByDefaultChecks := s.buildEnabledAndDisabledByDefaultChecks() debugChecksListf(enabledByDefaultChecks, "Enabled by default") - - disabledByDefaultChecks := s.getDefaultDisabledCheckersNames() debugChecksListf(disabledByDefaultChecks, "Disabled by default") - enabledChecks := make([]string, 0, len(s.EnabledTags)+len(enabledByDefaultChecks)) + enabledChecks := make(map[string]struct{}) - // EnabledTags - if len(s.EnabledTags) != 0 { - tagToCheckers := s.buildTagToCheckersMap() - for _, tag := range s.EnabledTags { - enabledChecks = append(enabledChecks, tagToCheckers[tag]...) + if s.EnableAll { + enabledChecks = make(map[string]struct{}, len(s.allCheckers)) + for _, info := range s.allCheckers { + enabledChecks[info.Name] = struct{}{} + } + } else if !s.DisableAll { + // NOTE(a.telyshev): enable-all/disable-all revokes the default settings. + enabledChecks = make(map[string]struct{}, len(enabledByDefaultChecks)) + for _, check := range enabledByDefaultChecks { + enabledChecks[check] = struct{}{} } - - 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.EnabledTags) != 0 { + enabledFromTags := s.expandTagsToChecks(s.EnabledTags) + debugChecksListf(enabledFromTags, "Enabled by config tags %s", sprintStrings(s.EnabledTags)) - // DisabledTags - if len(s.DisabledTags) != 0 { - enabledChecks = s.filterByDisableTags(enabledChecks, s.DisabledTags) + for _, check := range enabledFromTags { + enabledChecks[check] = struct{}{} + } } - // EnabledChecks if len(s.EnabledChecks) != 0 { debugChecksListf(s.EnabledChecks, "Enabled by config") - alreadyEnabledChecksSet := stringsSliceToSet(enabledChecks) - for _, enabledCheck := range s.EnabledChecks { - if alreadyEnabledChecksSet[enabledCheck] { - s.logger.Warnf("%s: no need to enable check %q: it's already enabled", goCriticName, enabledCheck) + for _, check := range s.EnabledChecks { + if _, ok := enabledChecks[check]; ok { + s.logger.Warnf("%s: no need to enable check %q: it's already enabled", goCriticName, check) continue } - enabledChecks = append(enabledChecks, enabledCheck) + enabledChecks[check] = struct{}{} + } + } + + if len(s.DisabledTags) != 0 { + disabledFromTags := s.expandTagsToChecks(s.DisabledTags) + debugChecksListf(disabledFromTags, "Disabled by config tags %s", sprintStrings(s.DisabledTags)) + + for _, check := range disabledFromTags { + delete(enabledChecks, check) } } - // DisabledChecks if len(s.DisabledChecks) != 0 { debugChecksListf(s.DisabledChecks, "Disabled by config") - enabledChecksSet := stringsSliceToSet(enabledChecks) - for _, disabledCheck := range s.DisabledChecks { - if !enabledChecksSet[disabledCheck] { - s.logger.Warnf("%s: 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.", goCriticName, disabledCheck) + for _, check := range s.DisabledChecks { + if _, ok := enabledChecks[check]; !ok { + s.logger.Warnf("%s: no need to disable check %q: it's already disabled", goCriticName, check) continue } - delete(enabledChecksSet, disabledCheck) - } - - enabledChecks = nil - for enabledCheck := range enabledChecksSet { - enabledChecks = append(enabledChecks, enabledCheck) + delete(enabledChecks, check) } } - s.inferredEnabledChecks = map[string]bool{} - for _, check := range enabledChecks { - s.inferredEnabledChecks[strings.ToLower(check)] = true - } - - debugChecksListf(enabledChecks, "Final used") - - s.disabledCheckersDebugf() + s.inferredEnabledChecks = enabledChecks + s.debugChecksFinalState() } -func (s *goCriticSettingsWrapper) validate() 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 fmt.Errorf("validate enabled tags: %w", err) - } - - tagToCheckers := s.buildTagToCheckersMap() - - for _, tag := range s.EnabledTags { - if _, ok := tagToCheckers[tag]; !ok { - return fmt.Errorf("gocritic [enabled]tag %q doesn't exist", tag) - } +func (s *goCriticSettingsWrapper) buildEnabledAndDisabledByDefaultChecks() (enabled []string, disabled []string) { + for _, info := range s.allCheckers { + if enable := isEnabledByDefaultGoCriticChecker(info); enable { + enabled = append(enabled, info.Name) + } else { + disabled = append(disabled, info.Name) } } + return enabled, disabled +} - if len(s.DisabledTags) > 0 { - tagToCheckers := s.buildTagToCheckersMap() - for _, tag := range s.EnabledTags { - if _, ok := tagToCheckers[tag]; !ok { - return fmt.Errorf("gocritic [disabled]tag %q doesn't exist", tag) - } - } - } +func isEnabledByDefaultGoCriticChecker(info *gocriticlinter.CheckerInfo) bool { + // https://github.com/go-critic/go-critic/blob/5b67cfd487ae9fe058b4b19321901b3131810f65/cmd/gocritic/check.go#L342-L345 + return !info.HasTag(gocriticlinter.ExperimentalTag) && + !info.HasTag(gocriticlinter.OpinionatedTag) && + !info.HasTag(gocriticlinter.PerformanceTag) && + !info.HasTag(gocriticlinter.SecurityTag) +} - if err := validateStringsUniq(s.EnabledChecks); err != nil { - return fmt.Errorf("validate enabled checks: %w", err) +func (s *goCriticSettingsWrapper) expandTagsToChecks(tags []string) []string { + var checks []string + for _, t := range tags { + checks = append(checks, s.allChecksByTag[t]...) } + return checks +} - if err := validateStringsUniq(s.DisabledChecks); err != nil { - return fmt.Errorf("validate disabled checks: %w", err) +func (s *goCriticSettingsWrapper) debugChecksInitialState() { + if !isGoCriticDebug { + return } - if err := s.validateCheckerNames(); err != nil { - return fmt.Errorf("validation failed: %w", err) + goCriticDebugf("All gocritic existing tags and checks:") + for _, tag := range s.allTagsSorted { + debugChecksListf(s.allChecksByTag[tag], " tag %q", tag) } - - return nil } -func (s *goCriticSettingsWrapper) isCheckEnabled(name string) bool { - return s.inferredEnabledChecks[strings.ToLower(name)] -} +func (s *goCriticSettingsWrapper) debugChecksFinalState() { + if !isGoCriticDebug { + return + } -// 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.allCheckers)) + var enabledChecks []string + var disabledChecks []string for _, checker := range s.allCheckers { - allCheckerNames[strings.ToLower(checker.Name)] = true + name := checker.Name + if _, ok := s.inferredEnabledChecks[name]; ok { + enabledChecks = append(enabledChecks, name) + } else { + disabledChecks = append(disabledChecks, name) + } } - return allCheckerNames -} - -func (s *goCriticSettingsWrapper) getDefaultEnabledCheckersNames() []string { - var enabled []string + debugChecksListf(enabledChecks, "Final used") - for _, info := range s.allCheckers { - enable := s.isEnabledByDefaultCheck(info) - if enable { - enabled = append(enabled, info.Name) - } + if len(disabledChecks) == 0 { + goCriticDebugf("All checks are enabled") + } else { + debugChecksListf(disabledChecks, "Final not used") } - - return enabled } -func (s *goCriticSettingsWrapper) getDefaultDisabledCheckersNames() []string { - var disabled []string - - for _, info := range s.allCheckers { - enable := s.isEnabledByDefaultCheck(info) - if !enable { - disabled = append(disabled, info.Name) - } +func debugChecksListf(checks []string, format string, args ...any) { + if !isGoCriticDebug { + return } - return disabled + goCriticDebugf("%s checks (%d): %s", fmt.Sprintf(format, args...), len(checks), sprintStrings(checks)) } -func (s *goCriticSettingsWrapper) validateCheckerNames() error { - allowedNames := s.getAllCheckerNames() +func sprintStrings(ss []string) string { + sort.Strings(ss) + return fmt.Sprint(ss) +} - 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)) +// Validate tries to be consistent with (lintersdb.Validator).validateEnabledDisabledLintersConfig. +func (s *goCriticSettingsWrapper) Validate() error { + for _, v := range []func() error{ + s.validateOptionsCombinations, + s.validateCheckerTags, + s.validateCheckerNames, + s.validateDisabledAndEnabledAtOneMoment, + s.validateAtLeastOneCheckerEnabled, + } { + if err := v(); err != nil { + return err } } + return nil +} - 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)) +func (s *goCriticSettingsWrapper) validateOptionsCombinations() error { + if s.EnableAll { + if s.DisableAll { + return errors.New("enable-all and disable-all options must not be combined") } - } - 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 len(s.EnabledTags) != 0 { + return errors.New("enable-all and enabled-tags options must not be combined") } - if !s.isCheckEnabled(checkName) { - s.logger.Warnf("%s: settings were provided for not enabled check %q", goCriticName, checkName) + if len(s.EnabledChecks) != 0 { + return errors.New("enable-all and enabled-checks options must not be combined") } } - return nil -} + if s.DisableAll { + if len(s.DisabledTags) != 0 { + return errors.New("disable-all and disabled-tags options must not be combined") + } -func (s *goCriticSettingsWrapper) getLowerCasedParams() map[string]config.GoCriticCheckSettings { - ret := make(map[string]config.GoCriticCheckSettings, len(s.SettingsPerCheck)) + if len(s.DisabledChecks) != 0 { + return errors.New("disable-all and disabled-checks options must not be combined") + } - for checker, params := range s.SettingsPerCheck { - ret[strings.ToLower(checker)] = params + if len(s.EnabledTags) == 0 && len(s.EnabledChecks) == 0 { + return errors.New("all checks were disabled, but no one check was enabled: at least one must be enabled") + } } - return ret + return nil } -func (s *goCriticSettingsWrapper) filterByDisableTags(enabledChecks, disableTags []string) []string { - enabledChecksSet := stringsSliceToSet(enabledChecks) - - for _, enabledCheck := range enabledChecks { - checkInfo, checkInfoExists := s.allCheckerMap[enabledCheck] - if !checkInfoExists { - s.logger.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) +func (s *goCriticSettingsWrapper) validateCheckerTags() error { + for _, tag := range s.EnabledTags { + if !s.isKnownTag(tag) { + return fmt.Errorf("enabled tag %q doesn't exist, see %s's documentation", tag, goCriticName) } } - debugChecksListf(enabledChecks, "Disabled by config tags %s", sprintStrings(disableTags)) - - enabledChecks = nil - for enabledCheck := range enabledChecksSet { - enabledChecks = append(enabledChecks, enabledCheck) + for _, tag := range s.DisabledTags { + if !s.isKnownTag(tag) { + return fmt.Errorf("disabled tag %q doesn't exist, see %s's documentation", tag, goCriticName) + } } - return enabledChecks + return nil } -func (s *goCriticSettingsWrapper) isEnabledByDefaultCheck(info *gocriticlinter.CheckerInfo) bool { - return !info.HasTag("experimental") && - !info.HasTag("opinionated") && - !info.HasTag("performance") +func (s *goCriticSettingsWrapper) isKnownTag(tag string) bool { + _, ok := s.allChecksByTag[tag] + return ok } -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) +func (s *goCriticSettingsWrapper) validateCheckerNames() error { + for _, name := range s.EnabledChecks { + if !s.isKnownCheck(name) { + return fmt.Errorf("enabled check %q doesn't exist, see %s's documentation", name, goCriticName) } - 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{}{} + for _, name := range s.DisabledChecks { + if !s.isKnownCheck(name) { + return fmt.Errorf("disabled check %q doesn't exist, see %s documentation", name, goCriticName) + } } - results := make([]string, 0) - for _, s := range s2 { - if _, exists := s1Map[s]; exists { - results = append(results, s) + for name := range s.SettingsPerCheck { + if !s.isKnownCheck(name) { + return fmt.Errorf("invalid settings, check %q doesn't exist, see %s documentation", name, goCriticName) + } + if !s.IsCheckEnabled(name) { + s.logger.Warnf("%s: settings were provided for disabled check %q", goCriticName, name) } } - return results + return nil } -func sprintAllowedCheckerNames(allowedNames map[string]bool) string { - namesSlice := maps.Keys(allowedNames) - return sprintStrings(namesSlice) +func (s *goCriticSettingsWrapper) isKnownCheck(name string) bool { + _, ok := s.allCheckersByName[name] + return ok } -func sprintStrings(ss []string) string { - sort.Strings(ss) - return fmt.Sprint(ss) -} +func (s *goCriticSettingsWrapper) validateDisabledAndEnabledAtOneMoment() error { + for _, tag := range s.DisabledTags { + if slices.Contains(s.EnabledTags, tag) { + return fmt.Errorf("tag %q disabled and enabled at one moment", tag) + } + } -func debugChecksListf(checks []string, format string, args ...any) { - if !isGoCriticDebug { - return + for _, check := range s.DisabledChecks { + if slices.Contains(s.EnabledChecks, check) { + return fmt.Errorf("check %q disabled and enabled at one moment", check) + } } - goCriticDebugf("%s checks (%d): %s", fmt.Sprintf(format, args...), len(checks), sprintStrings(checks)) + return nil } -func stringsSliceToSet(ss []string) map[string]bool { - ret := make(map[string]bool, len(ss)) - for _, s := range ss { - ret[s] = true +func (s *goCriticSettingsWrapper) validateAtLeastOneCheckerEnabled() error { + if len(s.inferredEnabledChecks) == 0 { + return errors.New("eventually all checks were disabled: at least one must be enabled") } + return nil +} - return ret +func (s *goCriticSettingsWrapper) IsCheckEnabled(name string) bool { + _, ok := s.inferredEnabledChecks[name] + return ok } diff --git a/pkg/golinters/gocritic_test.go b/pkg/golinters/gocritic_test.go index 02b458bbc2e3..aea421eed117 100644 --- a/pkg/golinters/gocritic_test.go +++ b/pkg/golinters/gocritic_test.go @@ -1,56 +1,442 @@ package golinters import ( - "log" + "strings" "testing" + "github.com/go-critic/go-critic/checkers" + gocriticlinter "github.com/go-critic/go-critic/linter" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "golang.org/x/exp/maps" + "golang.org/x/exp/slices" + "github.com/golangci/golangci-lint/pkg/config" "github.com/golangci/golangci-lint/pkg/logutils" ) -func Test_intersectStringSlice(t *testing.T) { - s1 := []string{"diagnostic", "experimental", "opinionated"} - s2 := []string{"opinionated", "experimental"} +// https://go-critic.com/overview.html +func Test_goCriticSettingsWrapper_InferEnabledChecks(t *testing.T) { + err := checkers.InitEmbeddedRules() + require.NoError(t, err) - s3 := intersectStringSlice(s1, s2) + allCheckersInfo := gocriticlinter.GetCheckersInfo() - assert.ElementsMatch(t, []string{"experimental", "opinionated"}, s3) -} + allChecksByTag := make(map[string][]string) + allChecks := make([]string, 0, len(allCheckersInfo)) + for _, checker := range allCheckersInfo { + allChecks = append(allChecks, checker.Name) + for _, tag := range checker.Tags { + allChecksByTag[tag] = append(allChecksByTag[tag], checker.Name) + } + } -func Test_filterByDisableTags(t *testing.T) { - disabledTags := []string{"experimental", "opinionated"} - enabledChecks := []string{"appendAssign", "sortSlice", "caseOrder", "dupImport"} + enabledByDefaultChecks := make([]string, 0, len(allCheckersInfo)) + for _, info := range allCheckersInfo { + if isEnabledByDefaultGoCriticChecker(info) { + enabledByDefaultChecks = append(enabledByDefaultChecks, info.Name) + } + } + t.Logf("enabled by default checks:\n%s", strings.Join(enabledByDefaultChecks, "\n")) - settingsWrapper := newGoCriticSettingsWrapper(nil, &tLog{}) + insert := func(in []string, toInsert ...string) []string { + return append(slices.Clone(in), toInsert...) + } - filterEnabledChecks := settingsWrapper.filterByDisableTags(enabledChecks, disabledTags) + remove := func(in []string, toRemove ...string) []string { + result := slices.Clone(in) + for _, v := range toRemove { + if i := slices.Index(result, v); i != -1 { + result = slices.Delete(result, i, i+1) + } + } + return result + } - assert.ElementsMatch(t, filterEnabledChecks, []string{"appendAssign", "caseOrder"}) -} + uniq := func(in []string) []string { + result := slices.Clone(in) + slices.Sort(result) + return slices.Compact(result) + } -type tLog struct{} + cases := []struct { + name string + sett *config.GoCriticSettings + expectedEnabledChecks []string + }{ + { + name: "no configuration", + sett: &config.GoCriticSettings{}, + expectedEnabledChecks: enabledByDefaultChecks, + }, + { + name: "enable checks", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"assignOp", "badCall", "emptyDecl"}, + }, + expectedEnabledChecks: insert(enabledByDefaultChecks, "emptyDecl"), + }, + { + name: "disable checks", + sett: &config.GoCriticSettings{ + DisabledChecks: []string{"assignOp", "emptyDecl"}, + }, + expectedEnabledChecks: remove(enabledByDefaultChecks, "assignOp"), + }, + { + name: "enable tags", + sett: &config.GoCriticSettings{ + EnabledTags: []string{"style", "experimental"}, + }, + expectedEnabledChecks: uniq(insert(insert( + enabledByDefaultChecks, + allChecksByTag["style"]...), + allChecksByTag["experimental"]...)), + }, + { + name: "disable tags", + sett: &config.GoCriticSettings{ + DisabledTags: []string{"diagnostic"}, + }, + expectedEnabledChecks: remove(enabledByDefaultChecks, allChecksByTag["diagnostic"]...), + }, + { + name: "enable checks disable checks", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"badCall", "badLock"}, + DisabledChecks: []string{"assignOp", "badSorting"}, + }, + expectedEnabledChecks: insert(remove(enabledByDefaultChecks, "assignOp"), "badLock"), + }, + { + name: "enable checks enable tags", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"badCall", "badLock", "hugeParam"}, + EnabledTags: []string{"diagnostic"}, + }, + expectedEnabledChecks: uniq(insert(insert(enabledByDefaultChecks, + allChecksByTag["diagnostic"]...), + "hugeParam")), + }, + { + name: "enable checks disable tags", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"badCall", "badLock", "boolExprSimplify", "hugeParam"}, + DisabledTags: []string{"style", "diagnostic"}, + }, + expectedEnabledChecks: insert(remove(remove(enabledByDefaultChecks, + allChecksByTag["style"]...), + allChecksByTag["diagnostic"]...), + "hugeParam"), + }, + { + name: "enable all checks via tags", + sett: &config.GoCriticSettings{ + EnabledTags: []string{"diagnostic", "experimental", "opinionated", "performance", "style"}, + }, + expectedEnabledChecks: allChecks, + }, + { + name: "disable checks enable tags", + sett: &config.GoCriticSettings{ + DisabledChecks: []string{"assignOp", "badCall", "badLock", "hugeParam"}, + EnabledTags: []string{"style", "diagnostic"}, + }, + expectedEnabledChecks: remove(uniq(insert(insert(enabledByDefaultChecks, + allChecksByTag["style"]...), + allChecksByTag["diagnostic"]...)), + "assignOp", "badCall", "badLock"), + }, + { + name: "disable checks disable tags", + sett: &config.GoCriticSettings{ + DisabledChecks: []string{"badCall", "badLock", "codegenComment", "hugeParam"}, + DisabledTags: []string{"style"}, + }, + expectedEnabledChecks: remove(remove(enabledByDefaultChecks, + allChecksByTag["style"]...), + "badCall", "codegenComment"), + }, + { + name: "enable tags disable tags", + sett: &config.GoCriticSettings{ + EnabledTags: []string{"experimental"}, + DisabledTags: []string{"style"}, + }, + expectedEnabledChecks: remove(uniq(insert(enabledByDefaultChecks, + allChecksByTag["experimental"]...)), + allChecksByTag["style"]...), + }, + { + name: "enable checks disable checks enable tags", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"badCall", "badLock", "boolExprSimplify", "indexAlloc", "hugeParam"}, + DisabledChecks: []string{"deprecatedComment", "typeSwitchVar"}, + EnabledTags: []string{"experimental"}, + }, + expectedEnabledChecks: remove(uniq(insert(insert(enabledByDefaultChecks, + allChecksByTag["experimental"]...), + "indexAlloc", "hugeParam")), + "deprecatedComment", "typeSwitchVar"), + }, + { + name: "enable checks disable checks enable tags disable tags", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"badCall", "badCond", "badLock", "indexAlloc", "hugeParam"}, + DisabledChecks: []string{"deprecatedComment", "typeSwitchVar"}, + EnabledTags: []string{"experimental"}, + DisabledTags: []string{"performance"}, + }, + expectedEnabledChecks: remove(remove(uniq(insert(insert(enabledByDefaultChecks, + allChecksByTag["experimental"]...), + "badCond")), + allChecksByTag["performance"]...), + "deprecatedComment", "typeSwitchVar"), + }, + { + name: "enable single tag only", + sett: &config.GoCriticSettings{ + DisableAll: true, + EnabledTags: []string{"experimental"}, + }, + expectedEnabledChecks: allChecksByTag["experimental"], + }, + { + name: "enable two tags only", + sett: &config.GoCriticSettings{ + DisableAll: true, + EnabledTags: []string{"experimental", "performance"}, + }, + expectedEnabledChecks: uniq(insert(allChecksByTag["experimental"], allChecksByTag["performance"]...)), + }, + { + name: "disable single tag only", + sett: &config.GoCriticSettings{ + EnableAll: true, + DisabledTags: []string{"style"}, + }, + expectedEnabledChecks: remove(allChecks, allChecksByTag["style"]...), + }, + { + name: "disable two tags only", + sett: &config.GoCriticSettings{ + EnableAll: true, + DisabledTags: []string{"style", "diagnostic"}, + }, + expectedEnabledChecks: remove(remove(allChecks, allChecksByTag["style"]...), allChecksByTag["diagnostic"]...), + }, + { + name: "enable some checks only", + sett: &config.GoCriticSettings{ + DisableAll: true, + EnabledChecks: []string{"deferInLoop", "dupImport", "ifElseChain", "mapKey"}, + }, + expectedEnabledChecks: []string{"deferInLoop", "dupImport", "ifElseChain", "mapKey"}, + }, + { + name: "disable some checks only", + sett: &config.GoCriticSettings{ + EnableAll: true, + DisabledChecks: []string{"deferInLoop", "dupImport", "ifElseChain", "mapKey"}, + }, + expectedEnabledChecks: remove(allChecks, "deferInLoop", "dupImport", "ifElseChain", "mapKey"), + }, + { + name: "enable single tag and some checks from another tag only", + sett: &config.GoCriticSettings{ + DisableAll: true, + EnabledTags: []string{"experimental"}, + EnabledChecks: []string{"importShadow"}, + }, + expectedEnabledChecks: insert(allChecksByTag["experimental"], "importShadow"), + }, + { + name: "disable single tag and some checks from another tag only", + sett: &config.GoCriticSettings{ + EnableAll: true, + DisabledTags: []string{"experimental"}, + DisabledChecks: []string{"importShadow"}, + }, + expectedEnabledChecks: remove(remove(allChecks, allChecksByTag["experimental"]...), "importShadow"), + }, + } -func (l *tLog) Fatalf(format string, args ...any) { - log.Printf(format, args...) -} + for _, tt := range cases { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() -func (l *tLog) Panicf(format string, args ...any) { - log.Printf(format, args...) -} + lg := logutils.NewStderrLog("Test_goCriticSettingsWrapper_InferEnabledChecks") + wr := newGoCriticSettingsWrapper(tt.sett, lg) -func (l *tLog) Errorf(format string, args ...any) { - log.Printf(format, args...) + wr.InferEnabledChecks() + assert.ElementsMatch(t, tt.expectedEnabledChecks, maps.Keys(wr.inferredEnabledChecks)) + assert.NoError(t, wr.Validate()) + }) + } } -func (l *tLog) Warnf(format string, args ...any) { - log.Printf(format, args...) -} +func Test_goCriticSettingsWrapper_Validate(t *testing.T) { + cases := []struct { + name string + sett *config.GoCriticSettings + expectedErr bool + }{ + { + name: "combine enable-all and disable-all", + sett: &config.GoCriticSettings{ + EnableAll: true, + DisableAll: true, + }, + expectedErr: true, + }, + { + name: "combine enable-all and enabled-tags", + sett: &config.GoCriticSettings{ + EnableAll: true, + EnabledTags: []string{"experimental"}, + }, + expectedErr: true, + }, + { + name: "combine enable-all and enabled-checks", + sett: &config.GoCriticSettings{ + EnableAll: true, + EnabledChecks: []string{"dupImport"}, + }, + expectedErr: true, + }, + { + name: "combine disable-all and disabled-tags", + sett: &config.GoCriticSettings{ + DisableAll: true, + DisabledTags: []string{"style"}, + }, + expectedErr: true, + }, + { + name: "combine disable-all and disable-checks", + sett: &config.GoCriticSettings{ + DisableAll: true, + DisabledChecks: []string{"assignOp"}, + }, + expectedErr: true, + }, + { + name: "disable-all and no one check enabled", + sett: &config.GoCriticSettings{ + DisableAll: true, + }, + expectedErr: true, + }, + { + name: "unknown enabled tag", + sett: &config.GoCriticSettings{ + EnabledTags: []string{"diagnostic", "go-proverbs"}, + }, + expectedErr: true, + }, + { + name: "unknown disabled tag", + sett: &config.GoCriticSettings{ + DisabledTags: []string{"style", "go-proverbs"}, + }, + expectedErr: true, + }, + { + name: "unknown enabled check", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"assignOp", "indexAlloc", "noExitAfterDefer"}, + }, + expectedErr: true, + }, + { + name: "unknown disabled check", + sett: &config.GoCriticSettings{ + DisabledChecks: []string{"dupSubExpr", "noExitAfterDefer", "returnAfterHttpError"}, + }, + expectedErr: true, + }, + { + name: "settings for unknown check", + sett: &config.GoCriticSettings{ + SettingsPerCheck: map[string]config.GoCriticCheckSettings{ + "captLocall": {"paramsOnly": false}, + }, + }, + expectedErr: true, + }, + { + name: "settings for disabled check", + sett: &config.GoCriticSettings{ + DisabledChecks: []string{"elseif"}, + SettingsPerCheck: map[string]config.GoCriticCheckSettings{ + "elseif": {"skipBalanced": true}, + }, + }, + expectedErr: false, // Just logging. + }, + { + name: "enabled and disabled at one moment check", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"badCall", "badLock", "codegenComment", "hugeParam"}, + DisabledChecks: []string{"elseif", "badLock"}, + }, + expectedErr: true, + }, + { + name: "enabled and disabled at one moment tag", + sett: &config.GoCriticSettings{ + EnabledTags: []string{"performance", "style"}, + DisabledTags: []string{"style", "diagnostic"}, + }, + expectedErr: true, + }, + { + name: "disable all checks via tags", + sett: &config.GoCriticSettings{ + DisabledTags: []string{"diagnostic", "experimental", "opinionated", "performance", "style"}, + }, + expectedErr: true, + }, + { + name: "enable-all and disable all checks via tags", + sett: &config.GoCriticSettings{ + EnableAll: true, + DisabledTags: []string{"diagnostic", "experimental", "opinionated", "performance", "style"}, + }, + expectedErr: true, + }, + { + name: "valid configuration", + sett: &config.GoCriticSettings{ + EnabledTags: []string{"performance"}, + DisabledChecks: []string{"dupImport", "ifElseChain", "octalLiteral", "whyNoLint"}, + SettingsPerCheck: map[string]config.GoCriticCheckSettings{ + "hugeParam": {"sizeThreshold": 100}, + "rangeValCopy": {"skipTestFuncs": true}, + }, + }, + expectedErr: false, + }, + } -func (l *tLog) Infof(format string, args ...any) { - log.Printf(format, args...) -} + for _, tt := range cases { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() -func (l *tLog) Child(_ string) logutils.Log { return nil } + lg := logutils.NewStderrLog("Test_goCriticSettingsWrapper_Validate") + wr := newGoCriticSettingsWrapper(tt.sett, lg) -func (l *tLog) SetLevel(_ logutils.LogLevel) {} + wr.InferEnabledChecks() + + err := wr.Validate() + if tt.expectedErr { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 4a3c1b4d64f4..e6e39f936e75 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -464,7 +464,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.GetConfigDir())). WithSince("v1.12.0"). WithPresets(linter.PresetStyle, linter.PresetMetaLinter). WithLoadForGoAnalysis(). From b1120649a8044d05caeee56de69f42d817e23811 Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Fri, 26 Jan 2024 09:28:25 +0200 Subject: [PATCH 2/8] self-review fixes --- pkg/golinters/gocritic.go | 114 ++++++++++++++++------------- pkg/golinters/gocritic_test.go | 26 +++++-- pkg/lint/lintersdb/manager.go | 2 +- test/testdata/configs/gocritic.yml | 4 +- 4 files changed, 85 insertions(+), 61 deletions(-) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index 1f6f71c5872a..4f0494c07249 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -32,13 +32,13 @@ var ( isGoCriticDebug = logutils.HaveDebugTag(logutils.DebugKeyGoCritic) ) -func NewGoCritic(settings *config.GoCriticSettings, lintConfigDir string) *goanalysis.Linter { +func NewGoCritic(settings *config.GoCriticSettings, lintConfigDirGetter func() string) *goanalysis.Linter { var mu sync.Mutex var resIssues []goanalysis.Issue wrapper := &goCriticWrapper{ - lintConfigDir: lintConfigDir, - sizes: types.SizesFor("gc", runtime.GOARCH), + lintConfigDirGetter: lintConfigDirGetter, + sizes: types.SizesFor("gc", runtime.GOARCH), } analyzer := &analysis.Analyzer{ @@ -80,10 +80,10 @@ Dynamic rules are written declaratively with AST patterns, filters, report messa } type goCriticWrapper struct { - settingsWrapper *goCriticSettingsWrapper - lintConfigDir string - sizes types.Sizes - once sync.Once + settingsWrapper *goCriticSettingsWrapper + lintConfigDirGetter func() string + sizes types.Sizes + once sync.Once } func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutils.Log) { @@ -100,7 +100,7 @@ func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutil settingsWrapper := newGoCriticSettingsWrapper(settings, logger) settingsWrapper.InferEnabledChecks() - // NOTE(a.telyshev): Validate must be after InferEnabledChecks, not before. + // NOTE(Antonboom): Validate must be after InferEnabledChecks, not before. // Because it uses gathered information about tags set and finally enabled checks. if err := settingsWrapper.Validate(); err != nil { logger.Fatalf("%s: invalid settings: %s", goCriticName, err) @@ -136,7 +136,7 @@ func (w *goCriticWrapper) run(pass *analysis.Pass) ([]goanalysis.Issue, error) { } func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context) ([]*gocriticlinter.Checker, error) { - allParams := w.settingsWrapper.GetLowerCasedParams() + allLowerCasedParams := w.settingsWrapper.GetLowerCasedParams() var enabledCheckers []*gocriticlinter.Checker for _, info := range gocriticlinter.GetCheckersInfo() { @@ -144,7 +144,7 @@ func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context continue } - if err := w.configureCheckerInfo(info, allParams); err != nil { + if err := w.configureCheckerInfo(info, allLowerCasedParams); err != nil { return nil, err } @@ -205,13 +205,17 @@ func runGocriticOnFile(linterCtx *gocriticlinter.Context, f *ast.File, checks [] return res } -func (w *goCriticWrapper) configureCheckerInfo(info *gocriticlinter.CheckerInfo, allParams map[string]config.GoCriticCheckSettings) error { - params := allParams[strings.ToLower(info.Name)] +func (w *goCriticWrapper) configureCheckerInfo( + info *gocriticlinter.CheckerInfo, + allLowerCasedParams map[string]config.GoCriticCheckSettings, +) error { + params := allLowerCasedParams[strings.ToLower(info.Name)] if params == nil { // no config for this checker return nil } - infoParams := normalizeCheckerInfoParams(info) + // NOTE(ldez): lowercase info param keys here because golangci-lint's config parser lowercases all strings. + infoParams := normalizeMap(info.Params) for k, p := range params { v, ok := infoParams[k] if ok { @@ -235,13 +239,11 @@ func (w *goCriticWrapper) configureCheckerInfo(info *gocriticlinter.CheckerInfo, return nil } -func normalizeCheckerInfoParams(info *gocriticlinter.CheckerInfo) gocriticlinter.CheckerParams { - // lowercase info param keys here because golangci-lint's config parser lowercases all strings - ret := gocriticlinter.CheckerParams{} - for k, v := range info.Params { +func normalizeMap[ValueT any](in map[string]ValueT) map[string]ValueT { + ret := make(map[string]ValueT, len(in)) + for k, v := range in { ret[strings.ToLower(k)] = v } - return ret } @@ -259,7 +261,7 @@ func (w *goCriticWrapper) normalizeCheckerParamsValue(p any) any { return rv.Bool() case reflect.String: // Perform variable substitution. - return strings.ReplaceAll(rv.String(), "${configDir}", w.lintConfigDir) + return strings.ReplaceAll(rv.String(), "${configDir}", w.lintConfigDirGetter()) default: return p } @@ -270,54 +272,50 @@ type goCriticSettingsWrapper struct { logger logutils.Log - allCheckers []*gocriticlinter.CheckerInfo - allCheckersByName map[string]*gocriticlinter.CheckerInfo + allCheckers []*gocriticlinter.CheckerInfo - allTagsSorted []string - allChecksByTag map[string][]string + allChecks map[string]struct{} + allChecksLowerCased map[string]struct{} + allChecksByTag map[string][]string + allTagsSorted []string - inferredEnabledChecks map[string]struct{} + inferredEnabledChecks map[string]struct{} + inferredEnabledLowerCasedChecks map[string]struct{} } func newGoCriticSettingsWrapper(settings *config.GoCriticSettings, logger logutils.Log) *goCriticSettingsWrapper { allCheckers := gocriticlinter.GetCheckersInfo() - allCheckersByName := make(map[string]*gocriticlinter.CheckerInfo, len(allCheckers)) - for _, checkInfo := range allCheckers { - allCheckersByName[checkInfo.Name] = checkInfo - } + allChecks := make(map[string]struct{}, len(allCheckers)) + allChecksLowerCased := make(map[string]struct{}, len(allCheckers)) allChecksByTag := make(map[string][]string) for _, checker := range allCheckers { + allChecks[checker.Name] = struct{}{} + allChecksLowerCased[strings.ToLower(checker.Name)] = struct{}{} + for _, tag := range checker.Tags { allChecksByTag[tag] = append(allChecksByTag[tag], checker.Name) } } - allTagsSorted := make([]string, 0, len(allChecksByTag)) - for t := range allChecksByTag { - allTagsSorted = append(allTagsSorted, t) - } + allTagsSorted := maps.Keys(allChecksByTag) sort.Strings(allTagsSorted) return &goCriticSettingsWrapper{ - GoCriticSettings: settings, - logger: logger, - allCheckers: allCheckers, - allCheckersByName: allCheckersByName, - allTagsSorted: allTagsSorted, - allChecksByTag: allChecksByTag, - inferredEnabledChecks: make(map[string]struct{}), + GoCriticSettings: settings, + logger: logger, + allCheckers: allCheckers, + allChecks: allChecks, + allChecksLowerCased: allChecksLowerCased, + allChecksByTag: allChecksByTag, + allTagsSorted: allTagsSorted, + inferredEnabledChecks: make(map[string]struct{}), + inferredEnabledLowerCasedChecks: make(map[string]struct{}), } } 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 + return normalizeMap(s.SettingsPerCheck) } // InferEnabledChecks tries to be consistent with (lintersdb.EnabledSet).build. @@ -336,7 +334,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { enabledChecks[info.Name] = struct{}{} } } else if !s.DisableAll { - // NOTE(a.telyshev): enable-all/disable-all revokes the default settings. + // NOTE(Antonboom): enable-all/disable-all revokes the default settings. enabledChecks = make(map[string]struct{}, len(enabledByDefaultChecks)) for _, check := range enabledByDefaultChecks { enabledChecks[check] = struct{}{} @@ -386,10 +384,11 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { } s.inferredEnabledChecks = enabledChecks + s.inferredEnabledLowerCasedChecks = normalizeMap(s.inferredEnabledChecks) s.debugChecksFinalState() } -func (s *goCriticSettingsWrapper) buildEnabledAndDisabledByDefaultChecks() (enabled []string, disabled []string) { +func (s *goCriticSettingsWrapper) buildEnabledAndDisabledByDefaultChecks() (enabled, disabled []string) { for _, info := range s.allCheckers { if enable := isEnabledByDefaultGoCriticChecker(info); enable { enabled = append(enabled, info.Name) @@ -549,10 +548,11 @@ func (s *goCriticSettingsWrapper) validateCheckerNames() error { } for name := range s.SettingsPerCheck { - if !s.isKnownCheck(name) { - return fmt.Errorf("invalid settings, check %q doesn't exist, see %s documentation", name, goCriticName) + lcName := strings.ToLower(name) + if !s.isKnownLowerCasedCheck(lcName) { + return fmt.Errorf("invalid check settings: check %q doesn't exist, see %s documentation", name, goCriticName) } - if !s.IsCheckEnabled(name) { + if !s.isLowerCasedCheckEnabled(lcName) { s.logger.Warnf("%s: settings were provided for disabled check %q", goCriticName, name) } } @@ -561,7 +561,12 @@ func (s *goCriticSettingsWrapper) validateCheckerNames() error { } func (s *goCriticSettingsWrapper) isKnownCheck(name string) bool { - _, ok := s.allCheckersByName[name] + _, ok := s.allChecks[name] + return ok +} + +func (s *goCriticSettingsWrapper) isKnownLowerCasedCheck(name string) bool { + _, ok := s.allChecksLowerCased[name] return ok } @@ -588,6 +593,11 @@ func (s *goCriticSettingsWrapper) validateAtLeastOneCheckerEnabled() error { return nil } +func (s *goCriticSettingsWrapper) isLowerCasedCheckEnabled(name string) bool { + _, ok := s.inferredEnabledLowerCasedChecks[name] + return ok +} + func (s *goCriticSettingsWrapper) IsCheckEnabled(name string) bool { _, ok := s.inferredEnabledChecks[name] return ok diff --git a/pkg/golinters/gocritic_test.go b/pkg/golinters/gocritic_test.go index aea421eed117..957b34b695da 100644 --- a/pkg/golinters/gocritic_test.go +++ b/pkg/golinters/gocritic_test.go @@ -318,7 +318,7 @@ func Test_goCriticSettingsWrapper_Validate(t *testing.T) { name: "combine disable-all and disable-checks", sett: &config.GoCriticSettings{ DisableAll: true, - DisabledChecks: []string{"assignOp"}, + DisabledChecks: []string{"appendAssign"}, }, expectedErr: true, }, @@ -346,7 +346,7 @@ func Test_goCriticSettingsWrapper_Validate(t *testing.T) { { name: "unknown enabled check", sett: &config.GoCriticSettings{ - EnabledChecks: []string{"assignOp", "indexAlloc", "noExitAfterDefer"}, + EnabledChecks: []string{"appendAssign", "noExitAfterDefer", "underef"}, }, expectedErr: true, }, @@ -361,7 +361,8 @@ func Test_goCriticSettingsWrapper_Validate(t *testing.T) { name: "settings for unknown check", sett: &config.GoCriticSettings{ SettingsPerCheck: map[string]config.GoCriticCheckSettings{ - "captLocall": {"paramsOnly": false}, + "captLocall": {"paramsOnly": false}, + "unnamedResult": {"checkExported": true}, }, }, expectedErr: true, @@ -376,11 +377,22 @@ func Test_goCriticSettingsWrapper_Validate(t *testing.T) { }, expectedErr: false, // Just logging. }, + { + name: "settings by lower-cased checker name", + sett: &config.GoCriticSettings{ + EnabledChecks: []string{"tooManyResultsChecker"}, + SettingsPerCheck: map[string]config.GoCriticCheckSettings{ + "toomanyresultschecker": {"maxResults": 3}, + "unnamedResult": {"checkExported": true}, + }, + }, + expectedErr: false, + }, { name: "enabled and disabled at one moment check", sett: &config.GoCriticSettings{ - EnabledChecks: []string{"badCall", "badLock", "codegenComment", "hugeParam"}, - DisabledChecks: []string{"elseif", "badLock"}, + EnabledChecks: []string{"appendAssign", "codegenComment", "underef"}, + DisabledChecks: []string{"elseif", "underef"}, }, expectedErr: true, }, @@ -433,7 +445,9 @@ func Test_goCriticSettingsWrapper_Validate(t *testing.T) { err := wr.Validate() if tt.expectedErr { - assert.Error(t, err) + if assert.Error(t, err) { + t.Log(err) + } } else { assert.NoError(t, err) } diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index e6e39f936e75..c7348d6e3bc8 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -464,7 +464,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { WithPresets(linter.PresetStyle). WithURL("https://github.com/jgautheron/goconst"), - linter.NewConfig(golinters.NewGoCritic(gocriticCfg, m.cfg.GetConfigDir())). + linter.NewConfig(golinters.NewGoCritic(gocriticCfg, m.cfg.GetConfigDir)). 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 019e6afb83fa..178518274a06 100644 --- a/test/testdata/configs/gocritic.yml +++ b/test/testdata/configs/gocritic.yml @@ -2,9 +2,9 @@ linters-settings: gocritic: enabled-checks: - rangeValCopy - - flagDeref - - wrapperFunc - ruleguard + disabled-checks: + - dupSubExpr # So as not to overlap `ruleguard`. settings: rangeValCopy: sizeThreshold: 2 From 8911693ef57e0cd9169a9c7265151aca2dbbdb1a Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Tue, 30 Jan 2024 09:30:31 +0200 Subject: [PATCH 3/8] review fixes & go-critic tests improving --- pkg/golinters/gocritic.go | 24 ++++----- test/ruleguard/README.md | 15 +++++- test/ruleguard/dup.go | 23 -------- test/ruleguard/preferWriteString.go | 12 +++++ test/ruleguard/rangeExprCopy.go | 2 +- ...strings_simplify.go => stringsSimplify.go} | 2 +- test/testdata/configs/gocritic-fix.yml | 3 +- test/testdata/configs/gocritic.yml | 16 +++--- test/testdata/gocritic.go | 52 ++++++++++++++----- 9 files changed, 86 insertions(+), 63 deletions(-) delete mode 100644 test/ruleguard/dup.go create mode 100644 test/ruleguard/preferWriteString.go rename test/ruleguard/{strings_simplify.go => stringsSimplify.go} (93%) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index 4f0494c07249..f1073ec63c4c 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -100,7 +100,7 @@ func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutil settingsWrapper := newGoCriticSettingsWrapper(settings, logger) settingsWrapper.InferEnabledChecks() - // NOTE(Antonboom): Validate must be after InferEnabledChecks, not before. + // Validate must be after InferEnabledChecks, not before. // Because it uses gathered information about tags set and finally enabled checks. if err := settingsWrapper.Validate(); err != nil { logger.Fatalf("%s: invalid settings: %s", goCriticName, err) @@ -158,11 +158,7 @@ func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context return enabledCheckers, nil } -func runGocriticOnPackage( - linterCtx *gocriticlinter.Context, - checks []*gocriticlinter.Checker, - files []*ast.File, -) []result.Issue { +func runGocriticOnPackage(linterCtx *gocriticlinter.Context, checks []*gocriticlinter.Checker, files []*ast.File) []result.Issue { var res []result.Issue for _, f := range files { filename := filepath.Base(linterCtx.FileSet.Position(f.Pos()).Filename) @@ -214,7 +210,7 @@ func (w *goCriticWrapper) configureCheckerInfo( return nil } - // NOTE(ldez): lowercase info param keys here because golangci-lint's config parser lowercases all strings. + // To lowercase info param keys here because golangci-lint's config parser lowercases all strings. infoParams := normalizeMap(info.Params) for k, p := range params { v, ok := infoParams[k] @@ -274,12 +270,14 @@ type goCriticSettingsWrapper struct { allCheckers []*gocriticlinter.CheckerInfo - allChecks map[string]struct{} - allChecksLowerCased map[string]struct{} - allChecksByTag map[string][]string - allTagsSorted []string + allChecks map[string]struct{} + allChecksByTag map[string][]string + allTagsSorted []string + inferredEnabledChecks map[string]struct{} - inferredEnabledChecks map[string]struct{} + // *LowerCased* fields are used for GoCriticSettings.SettingsPerCheck validation only. + + allChecksLowerCased map[string]struct{} inferredEnabledLowerCasedChecks map[string]struct{} } @@ -334,7 +332,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { enabledChecks[info.Name] = struct{}{} } } else if !s.DisableAll { - // NOTE(Antonboom): enable-all/disable-all revokes the default settings. + // enable-all/disable-all revokes the default settings. enabledChecks = make(map[string]struct{}, len(enabledByDefaultChecks)) for _, check := range enabledByDefaultChecks { enabledChecks[check] = struct{}{} diff --git a/test/ruleguard/README.md b/test/ruleguard/README.md index 2e2441698750..35797144a3ee 100644 --- a/test/ruleguard/README.md +++ b/test/ruleguard/README.md @@ -1 +1,14 @@ -This directory contains ruleguard files that are used in functional tests. +This directory contains ruleguard files that are used in functional tests: + +```bash +T=gocritic.go make test_linters +``` + +```bash +T=gocritic.go make test_fix +``` + +Helpful: + +- https://go-critic.com/overview.html +- https://github.com/go-critic/go-critic/blob/master/checkers/rules/rules.go diff --git a/test/ruleguard/dup.go b/test/ruleguard/dup.go deleted file mode 100644 index c808a34229ec..000000000000 --- a/test/ruleguard/dup.go +++ /dev/null @@ -1,23 +0,0 @@ -//go:build ruleguard - -package ruleguard - -import "github.com/quasilyte/go-ruleguard/dsl" - -// Suppose that we want to report the duplicated left and right operands of binary operations. -// -// But if the operand has some side effects, this rule can cause false positives: -// `f() && f()` can make sense (although it's not the best piece of code). -// -// This is where *filters* come to the rescue. -func DupSubExpr(m dsl.Matcher) { - // All filters are written as a Where() argument. - // In our case, we need to assert that $x is "pure". - // It can be achieved by checking the m["x"] member Pure field. - m.Match(`$x || $x`, - `$x && $x`, - `$x | $x`, - `$x & $x`). - Where(m["x"].Pure). - Report(`suspicious identical LHS and RHS`) -} diff --git a/test/ruleguard/preferWriteString.go b/test/ruleguard/preferWriteString.go new file mode 100644 index 000000000000..c3057ba45118 --- /dev/null +++ b/test/ruleguard/preferWriteString.go @@ -0,0 +1,12 @@ +//go:build ruleguard + +package ruleguard + +import "github.com/quasilyte/go-ruleguard/dsl" + +func preferWriteString(m dsl.Matcher) { + m.Match(`$w.Write([]byte($s))`). + Where(m["w"].Type.Implements("io.StringWriter")). + Suggest("$w.WriteString($s)"). + Report(`$w.WriteString($s) should be preferred to the $$`) +} diff --git a/test/ruleguard/rangeExprCopy.go b/test/ruleguard/rangeExprCopy.go index 443d15c9bfa5..042ba14d4236 100644 --- a/test/ruleguard/rangeExprCopy.go +++ b/test/ruleguard/rangeExprCopy.go @@ -6,7 +6,7 @@ import ( "github.com/quasilyte/go-ruleguard/dsl" ) -func RangeExprVal(m dsl.Matcher) { +func rangeExprCopy(m dsl.Matcher) { m.Match(`for _, $_ := range $x { $*_ }`, `for _, $_ = range $x { $*_ }`). Where(m["x"].Addressable && m["x"].Type.Size >= 512). Report(`$x copy can be avoided with &$x`). diff --git a/test/ruleguard/strings_simplify.go b/test/ruleguard/stringsSimplify.go similarity index 93% rename from test/ruleguard/strings_simplify.go rename to test/ruleguard/stringsSimplify.go index a9c550b1fe83..3371436a7e83 100644 --- a/test/ruleguard/strings_simplify.go +++ b/test/ruleguard/stringsSimplify.go @@ -4,7 +4,7 @@ package ruleguard import "github.com/quasilyte/go-ruleguard/dsl" -func StringsSimplify(m dsl.Matcher) { +func stringsSimplify(m dsl.Matcher) { // Some issues have simple fixes that can be expressed as // a replacement pattern. Rules can use Suggest() function // to add a quickfix action for such issues. diff --git a/test/testdata/configs/gocritic-fix.yml b/test/testdata/configs/gocritic-fix.yml index 5ea41749a5de..7587a761a884 100644 --- a/test/testdata/configs/gocritic-fix.yml +++ b/test/testdata/configs/gocritic-fix.yml @@ -4,5 +4,4 @@ linters-settings: - ruleguard settings: ruleguard: - rules: 'ruleguard/rangeExprCopy.go,ruleguard/strings_simplify.go' - + rules: 'ruleguard/rangeExprCopy.go,ruleguard/stringsSimplify.go' diff --git a/test/testdata/configs/gocritic.yml b/test/testdata/configs/gocritic.yml index 178518274a06..d424cb1798b3 100644 --- a/test/testdata/configs/gocritic.yml +++ b/test/testdata/configs/gocritic.yml @@ -1,20 +1,20 @@ linters-settings: gocritic: + disabled-checks: + - appendAssign + - switchTrue enabled-checks: - - rangeValCopy + - hugeParam - ruleguard - disabled-checks: - - dupSubExpr # So as not to overlap `ruleguard`. settings: - rangeValCopy: - sizeThreshold: 2 + hugeParam: + sizeThreshold: 24 ruleguard: - debug: dupSubExpr failOn: dsl,import - # comma-separated paths to ruleguard files. + # Comma-separated paths to ruleguard files. # The ${configDir} is substituted by the directory containing the golangci-lint config file. # Note about the directory structure for functional tests: # The ruleguard files used in functional tests cannot be under the 'testdata' directory. # This is because they import the 'github.com/quasilyte/go-ruleguard/dsl' package, # which needs to be added to go.mod. The testdata directory is ignored by go mod. - rules: '${configDir}/../../ruleguard/strings_simplify.go,${configDir}/../../ruleguard/dup.go' + rules: '${configDir}/../../ruleguard/preferWriteString.go,${configDir}/../../ruleguard/stringsSimplify.go' diff --git a/test/testdata/gocritic.go b/test/testdata/gocritic.go index afca6f3c479c..a87a193d075f 100644 --- a/test/testdata/gocritic.go +++ b/test/testdata/gocritic.go @@ -4,6 +4,7 @@ package testdata import ( "flag" + "io" "log" "strings" ) @@ -11,38 +12,61 @@ import ( var _ = *flag.Bool("global1", false, "") // want `flagDeref: immediate deref in \*flag.Bool\(.global1., false, ..\) is most likely an error; consider using flag\.BoolVar` type size1 struct { - a bool + a [12]bool } type size2 struct { size1 - b bool + b [12]bool } -func gocriticRangeValCopySize1(ss []size1) { - for _, s := range ss { - log.Print(s) +func gocriticAppendAssign() { + var positives, negatives []int + positives = append(negatives, 1) + negatives = append(negatives, -1) + log.Print(positives, negatives) +} + +func gocriticDupSubExpr(x bool) { + if x && x { // want "dupSubExpr: suspicious identical LHS and RHS.*" + log.Print("x is true") } } -func gocriticRangeValCopySize2(ss []size2) { - for _, s := range ss { // want "rangeValCopy: each iteration copies 2 bytes.*" - log.Print(s) +func gocriticHugeParamSize1(ss size1) { + log.Print(ss) +} + +func gocriticHugeParamSize2(ss size2) { // want "hugeParam: ss is heavy \\(24 bytes\\); consider passing it by pointer" + log.Print(ss) +} + +func gocriticHugeParamSize2Ptr(ss *size2) { + log.Print(*ss) +} + +func gocriticSwitchTrue() { + switch true { + case false: + log.Print("false") + default: + log.Print("default") } } +func goCriticPreferStringWriter(w interface { + io.Writer + io.StringWriter +}) { + w.Write([]byte("test")) // want "ruleguard: w\\.WriteString\\(\"test\"\\) should be preferred.*" +} + func gocriticStringSimplify() { s := "Most of the time, travellers worry about their luggage." s = strings.Replace(s, ",", "", -1) // want "ruleguard: this Replace call can be simplified.*" log.Print(s) } -func gocriticDup(x bool) { - if x && x { // want "ruleguard: suspicious identical LHS and RHS.*" - log.Print("x is true") - } -} - func gocriticRuleWrapperFunc() { strings.Replace("abcabc", "a", "d", -1) // want "ruleguard: this Replace call can be simplified.*" } From e7154294bc3ceea025565442d154cfe1eedccabe Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Mon, 19 Feb 2024 09:24:30 +0300 Subject: [PATCH 4/8] Update pkg/golinters/gocritic.go Co-authored-by: Ludovic Fernandez --- pkg/golinters/gocritic.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index f1073ec63c4c..32844abae232 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -407,8 +407,8 @@ func isEnabledByDefaultGoCriticChecker(info *gocriticlinter.CheckerInfo) bool { func (s *goCriticSettingsWrapper) expandTagsToChecks(tags []string) []string { var checks []string - for _, t := range tags { - checks = append(checks, s.allChecksByTag[t]...) + for _, tag := range tags { + checks = append(checks, s.allChecksByTag[tag]...) } return checks } From 3943500c53d537368847d0e28bd2e6aa5418ca6f Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Mon, 19 Feb 2024 09:24:36 +0300 Subject: [PATCH 5/8] Update pkg/golinters/gocritic.go Co-authored-by: Ludovic Fernandez --- pkg/golinters/gocritic.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index 32844abae232..64d3e7575786 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -458,9 +458,9 @@ func debugChecksListf(checks []string, format string, args ...any) { goCriticDebugf("%s checks (%d): %s", fmt.Sprintf(format, args...), len(checks), sprintStrings(checks)) } -func sprintStrings(ss []string) string { - sort.Strings(ss) - return fmt.Sprint(ss) +func sprintStrings(v []string) string { + sort.Strings(v) + return fmt.Sprint(v) } // Validate tries to be consistent with (lintersdb.Validator).validateEnabledDisabledLintersConfig. From d1344398b2937de619704b8fe147bb802aaf65b5 Mon Sep 17 00:00:00 2001 From: Anton Telyshev Date: Mon, 19 Feb 2024 10:20:59 +0300 Subject: [PATCH 6/8] review fixes --- pkg/golinters/gocritic.go | 90 +++++++++++++++-------------------- pkg/lint/lintersdb/manager.go | 2 +- 2 files changed, 39 insertions(+), 53 deletions(-) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index 64d3e7575786..be15be307a0b 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -32,12 +32,12 @@ var ( isGoCriticDebug = logutils.HaveDebugTag(logutils.DebugKeyGoCritic) ) -func NewGoCritic(settings *config.GoCriticSettings, lintConfigDirGetter func() string) *goanalysis.Linter { +func NewGoCritic(settings *config.GoCriticSettings, cfg *config.Config) *goanalysis.Linter { var mu sync.Mutex var resIssues []goanalysis.Issue wrapper := &goCriticWrapper{ - lintConfigDirGetter: lintConfigDirGetter, + lintConfigDirGetter: cfg.GetConfigDir, // Config directory is filled after calling this constructor. sizes: types.SizesFor("gc", runtime.GOARCH), } @@ -270,23 +270,30 @@ type goCriticSettingsWrapper struct { allCheckers []*gocriticlinter.CheckerInfo - allChecks map[string]struct{} - allChecksByTag map[string][]string + allChecks goCriticChecks[struct{}] + allChecksByTag goCriticChecks[[]string] allTagsSorted []string - inferredEnabledChecks map[string]struct{} + inferredEnabledChecks goCriticChecks[struct{}] - // *LowerCased* fields are used for GoCriticSettings.SettingsPerCheck validation only. + // *LowerCased fields are used for GoCriticSettings.SettingsPerCheck validation only. - allChecksLowerCased map[string]struct{} - inferredEnabledLowerCasedChecks map[string]struct{} + allChecksLowerCased goCriticChecks[struct{}] + inferredEnabledChecksLowerCased goCriticChecks[struct{}] +} + +type goCriticChecks[T any] map[string]T + +func (m goCriticChecks[T]) has(name string) bool { + _, ok := m[name] + return ok } func newGoCriticSettingsWrapper(settings *config.GoCriticSettings, logger logutils.Log) *goCriticSettingsWrapper { allCheckers := gocriticlinter.GetCheckersInfo() - allChecks := make(map[string]struct{}, len(allCheckers)) - allChecksLowerCased := make(map[string]struct{}, len(allCheckers)) - allChecksByTag := make(map[string][]string) + allChecks := make(goCriticChecks[struct{}], len(allCheckers)) + allChecksLowerCased := make(goCriticChecks[struct{}], len(allCheckers)) + allChecksByTag := make(goCriticChecks[[]string]) for _, checker := range allCheckers { allChecks[checker.Name] = struct{}{} allChecksLowerCased[strings.ToLower(checker.Name)] = struct{}{} @@ -307,11 +314,15 @@ func newGoCriticSettingsWrapper(settings *config.GoCriticSettings, logger loguti allChecksLowerCased: allChecksLowerCased, allChecksByTag: allChecksByTag, allTagsSorted: allTagsSorted, - inferredEnabledChecks: make(map[string]struct{}), - inferredEnabledLowerCasedChecks: make(map[string]struct{}), + inferredEnabledChecks: make(goCriticChecks[struct{}]), + inferredEnabledChecksLowerCased: make(goCriticChecks[struct{}]), } } +func (s *goCriticSettingsWrapper) IsCheckEnabled(name string) bool { + return s.inferredEnabledChecks.has(name) +} + func (s *goCriticSettingsWrapper) GetLowerCasedParams() map[string]config.GoCriticCheckSettings { return normalizeMap(s.SettingsPerCheck) } @@ -324,16 +335,16 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { debugChecksListf(enabledByDefaultChecks, "Enabled by default") debugChecksListf(disabledByDefaultChecks, "Disabled by default") - enabledChecks := make(map[string]struct{}) + enabledChecks := make(goCriticChecks[struct{}]) if s.EnableAll { - enabledChecks = make(map[string]struct{}, len(s.allCheckers)) + enabledChecks = make(goCriticChecks[struct{}], len(s.allCheckers)) for _, info := range s.allCheckers { enabledChecks[info.Name] = struct{}{} } } else if !s.DisableAll { // enable-all/disable-all revokes the default settings. - enabledChecks = make(map[string]struct{}, len(enabledByDefaultChecks)) + enabledChecks = make(goCriticChecks[struct{}], len(enabledByDefaultChecks)) for _, check := range enabledByDefaultChecks { enabledChecks[check] = struct{}{} } @@ -352,7 +363,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { debugChecksListf(s.EnabledChecks, "Enabled by config") for _, check := range s.EnabledChecks { - if _, ok := enabledChecks[check]; ok { + if enabledChecks.has(check) { s.logger.Warnf("%s: no need to enable check %q: it's already enabled", goCriticName, check) continue } @@ -373,7 +384,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { debugChecksListf(s.DisabledChecks, "Disabled by config") for _, check := range s.DisabledChecks { - if _, ok := enabledChecks[check]; !ok { + if !enabledChecks.has(check) { s.logger.Warnf("%s: no need to disable check %q: it's already disabled", goCriticName, check) continue } @@ -382,13 +393,13 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { } s.inferredEnabledChecks = enabledChecks - s.inferredEnabledLowerCasedChecks = normalizeMap(s.inferredEnabledChecks) + s.inferredEnabledChecksLowerCased = normalizeMap(s.inferredEnabledChecks) s.debugChecksFinalState() } func (s *goCriticSettingsWrapper) buildEnabledAndDisabledByDefaultChecks() (enabled, disabled []string) { for _, info := range s.allCheckers { - if enable := isEnabledByDefaultGoCriticChecker(info); enable { + if enabledByDef := isEnabledByDefaultGoCriticChecker(info); enabledByDef { enabled = append(enabled, info.Name) } else { disabled = append(disabled, info.Name) @@ -434,7 +445,7 @@ func (s *goCriticSettingsWrapper) debugChecksFinalState() { for _, checker := range s.allCheckers { name := checker.Name - if _, ok := s.inferredEnabledChecks[name]; ok { + if s.inferredEnabledChecks.has(name) { enabledChecks = append(enabledChecks, name) } else { disabledChecks = append(disabledChecks, name) @@ -513,13 +524,13 @@ func (s *goCriticSettingsWrapper) validateOptionsCombinations() error { func (s *goCriticSettingsWrapper) validateCheckerTags() error { for _, tag := range s.EnabledTags { - if !s.isKnownTag(tag) { + if !s.allChecksByTag.has(tag) { return fmt.Errorf("enabled tag %q doesn't exist, see %s's documentation", tag, goCriticName) } } for _, tag := range s.DisabledTags { - if !s.isKnownTag(tag) { + if !s.allChecksByTag.has(tag) { return fmt.Errorf("disabled tag %q doesn't exist, see %s's documentation", tag, goCriticName) } } @@ -527,30 +538,25 @@ func (s *goCriticSettingsWrapper) validateCheckerTags() error { return nil } -func (s *goCriticSettingsWrapper) isKnownTag(tag string) bool { - _, ok := s.allChecksByTag[tag] - return ok -} - func (s *goCriticSettingsWrapper) validateCheckerNames() error { for _, name := range s.EnabledChecks { - if !s.isKnownCheck(name) { + if !s.allChecks.has(name) { return fmt.Errorf("enabled check %q doesn't exist, see %s's documentation", name, goCriticName) } } for _, name := range s.DisabledChecks { - if !s.isKnownCheck(name) { + if !s.allChecks.has(name) { return fmt.Errorf("disabled check %q doesn't exist, see %s documentation", name, goCriticName) } } for name := range s.SettingsPerCheck { lcName := strings.ToLower(name) - if !s.isKnownLowerCasedCheck(lcName) { + if !s.allChecksLowerCased.has(lcName) { return fmt.Errorf("invalid check settings: check %q doesn't exist, see %s documentation", name, goCriticName) } - if !s.isLowerCasedCheckEnabled(lcName) { + if !s.inferredEnabledChecksLowerCased.has(lcName) { s.logger.Warnf("%s: settings were provided for disabled check %q", goCriticName, name) } } @@ -558,16 +564,6 @@ func (s *goCriticSettingsWrapper) validateCheckerNames() error { return nil } -func (s *goCriticSettingsWrapper) isKnownCheck(name string) bool { - _, ok := s.allChecks[name] - return ok -} - -func (s *goCriticSettingsWrapper) isKnownLowerCasedCheck(name string) bool { - _, ok := s.allChecksLowerCased[name] - return ok -} - func (s *goCriticSettingsWrapper) validateDisabledAndEnabledAtOneMoment() error { for _, tag := range s.DisabledTags { if slices.Contains(s.EnabledTags, tag) { @@ -590,13 +586,3 @@ func (s *goCriticSettingsWrapper) validateAtLeastOneCheckerEnabled() error { } return nil } - -func (s *goCriticSettingsWrapper) isLowerCasedCheckEnabled(name string) bool { - _, ok := s.inferredEnabledLowerCasedChecks[name] - return ok -} - -func (s *goCriticSettingsWrapper) IsCheckEnabled(name string) bool { - _, ok := s.inferredEnabledChecks[name] - return ok -} diff --git a/pkg/lint/lintersdb/manager.go b/pkg/lint/lintersdb/manager.go index 41b3fe0e4b42..977ef0d1c61d 100644 --- a/pkg/lint/lintersdb/manager.go +++ b/pkg/lint/lintersdb/manager.go @@ -468,7 +468,7 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config { WithPresets(linter.PresetStyle). WithURL("https://github.com/jgautheron/goconst"), - linter.NewConfig(golinters.NewGoCritic(gocriticCfg, m.cfg.GetConfigDir)). + linter.NewConfig(golinters.NewGoCritic(gocriticCfg, m.cfg)). WithSince("v1.12.0"). WithPresets(linter.PresetStyle, linter.PresetMetaLinter). WithLoadForGoAnalysis(). From 9599db6563861423a47a25f99c39a5519aacbf1a Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Mon, 19 Feb 2024 14:38:11 +0100 Subject: [PATCH 7/8] review: organize --- pkg/golinters/gocritic.go | 164 +++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 82 deletions(-) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index be15be307a0b..ee72921dddad 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -125,7 +125,7 @@ func (w *goCriticWrapper) run(pass *analysis.Pass) ([]goanalysis.Issue, error) { linterCtx.SetPackageInfo(pass.TypesInfo, pass.Pkg) - pkgIssues := runGocriticOnPackage(linterCtx, enabledCheckers, pass.Files) + pkgIssues := runGoCriticOnPackage(linterCtx, enabledCheckers, pass.Files) issues := make([]goanalysis.Issue, 0, len(pkgIssues)) for i := range pkgIssues { @@ -158,49 +158,6 @@ func (w *goCriticWrapper) buildEnabledCheckers(linterCtx *gocriticlinter.Context return enabledCheckers, nil } -func runGocriticOnPackage(linterCtx *gocriticlinter.Context, checks []*gocriticlinter.Checker, files []*ast.File) []result.Issue { - var res []result.Issue - for _, f := range files { - filename := filepath.Base(linterCtx.FileSet.Position(f.Pos()).Filename) - linterCtx.SetFileInfo(filename, f) - - issues := runGocriticOnFile(linterCtx, f, checks) - res = append(res, issues...) - } - return res -} - -func runGocriticOnFile(linterCtx *gocriticlinter.Context, f *ast.File, checks []*gocriticlinter.Checker) []result.Issue { - var res []result.Issue - - for _, c := range checks { - // All checkers are expected to use *lint.Context - // as read-only structure, so no copying is required. - for _, warn := range c.Check(f) { - pos := linterCtx.FileSet.Position(warn.Pos) - issue := result.Issue{ - Pos: pos, - Text: fmt.Sprintf("%s: %s", c.Info.Name, warn.Text), - FromLinter: goCriticName, - } - - if warn.HasQuickFix() { - issue.Replacement = &result.Replacement{ - Inline: &result.InlineFix{ - StartCol: pos.Column - 1, - Length: int(warn.Suggestion.To - warn.Suggestion.From), - NewString: string(warn.Suggestion.Replacement), - }, - } - } - - res = append(res, issue) - } - } - - return res -} - func (w *goCriticWrapper) configureCheckerInfo( info *gocriticlinter.CheckerInfo, allLowerCasedParams map[string]config.GoCriticCheckSettings, @@ -235,14 +192,6 @@ func (w *goCriticWrapper) configureCheckerInfo( return nil } -func normalizeMap[ValueT any](in map[string]ValueT) map[string]ValueT { - ret := make(map[string]ValueT, len(in)) - for k, v := range in { - ret[strings.ToLower(k)] = v - } - return ret -} - // normalizeCheckerParamsValue normalizes value types. // go-critic asserts that CheckerParam.Value has some specific types, // but the file parsers (TOML, YAML, JSON) don't create the same representation for raw type. @@ -263,6 +212,56 @@ func (w *goCriticWrapper) normalizeCheckerParamsValue(p any) any { } } +func runGoCriticOnPackage(linterCtx *gocriticlinter.Context, checks []*gocriticlinter.Checker, files []*ast.File) []result.Issue { + var res []result.Issue + for _, f := range files { + filename := filepath.Base(linterCtx.FileSet.Position(f.Pos()).Filename) + linterCtx.SetFileInfo(filename, f) + + issues := runGoCriticOnFile(linterCtx, f, checks) + res = append(res, issues...) + } + return res +} + +func runGoCriticOnFile(linterCtx *gocriticlinter.Context, f *ast.File, checks []*gocriticlinter.Checker) []result.Issue { + var res []result.Issue + + for _, c := range checks { + // All checkers are expected to use *lint.Context + // as read-only structure, so no copying is required. + for _, warn := range c.Check(f) { + pos := linterCtx.FileSet.Position(warn.Pos) + issue := result.Issue{ + Pos: pos, + Text: fmt.Sprintf("%s: %s", c.Info.Name, warn.Text), + FromLinter: goCriticName, + } + + if warn.HasQuickFix() { + issue.Replacement = &result.Replacement{ + Inline: &result.InlineFix{ + StartCol: pos.Column - 1, + Length: int(warn.Suggestion.To - warn.Suggestion.From), + NewString: string(warn.Suggestion.Replacement), + }, + } + } + + res = append(res, issue) + } + } + + return res +} + +type goCriticChecks[T any] map[string]T + +func (m goCriticChecks[T]) has(name string) bool { + _, ok := m[name] + return ok +} + type goCriticSettingsWrapper struct { *config.GoCriticSettings @@ -281,13 +280,6 @@ type goCriticSettingsWrapper struct { inferredEnabledChecksLowerCased goCriticChecks[struct{}] } -type goCriticChecks[T any] map[string]T - -func (m goCriticChecks[T]) has(name string) bool { - _, ok := m[name] - return ok -} - func newGoCriticSettingsWrapper(settings *config.GoCriticSettings, logger logutils.Log) *goCriticSettingsWrapper { allCheckers := gocriticlinter.GetCheckersInfo() @@ -352,7 +344,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { if len(s.EnabledTags) != 0 { enabledFromTags := s.expandTagsToChecks(s.EnabledTags) - debugChecksListf(enabledFromTags, "Enabled by config tags %s", sprintStrings(s.EnabledTags)) + debugChecksListf(enabledFromTags, "Enabled by config tags %s", sprintSortedStrings(s.EnabledTags)) for _, check := range enabledFromTags { enabledChecks[check] = struct{}{} @@ -373,7 +365,7 @@ func (s *goCriticSettingsWrapper) InferEnabledChecks() { if len(s.DisabledTags) != 0 { disabledFromTags := s.expandTagsToChecks(s.DisabledTags) - debugChecksListf(disabledFromTags, "Disabled by config tags %s", sprintStrings(s.DisabledTags)) + debugChecksListf(disabledFromTags, "Disabled by config tags %s", sprintSortedStrings(s.DisabledTags)) for _, check := range disabledFromTags { delete(enabledChecks, check) @@ -408,14 +400,6 @@ func (s *goCriticSettingsWrapper) buildEnabledAndDisabledByDefaultChecks() (enab return enabled, disabled } -func isEnabledByDefaultGoCriticChecker(info *gocriticlinter.CheckerInfo) bool { - // https://github.com/go-critic/go-critic/blob/5b67cfd487ae9fe058b4b19321901b3131810f65/cmd/gocritic/check.go#L342-L345 - return !info.HasTag(gocriticlinter.ExperimentalTag) && - !info.HasTag(gocriticlinter.OpinionatedTag) && - !info.HasTag(gocriticlinter.PerformanceTag) && - !info.HasTag(gocriticlinter.SecurityTag) -} - func (s *goCriticSettingsWrapper) expandTagsToChecks(tags []string) []string { var checks []string for _, tag := range tags { @@ -461,19 +445,6 @@ func (s *goCriticSettingsWrapper) debugChecksFinalState() { } } -func debugChecksListf(checks []string, format string, args ...any) { - if !isGoCriticDebug { - return - } - - goCriticDebugf("%s checks (%d): %s", fmt.Sprintf(format, args...), len(checks), sprintStrings(checks)) -} - -func sprintStrings(v []string) string { - sort.Strings(v) - return fmt.Sprint(v) -} - // Validate tries to be consistent with (lintersdb.Validator).validateEnabledDisabledLintersConfig. func (s *goCriticSettingsWrapper) Validate() error { for _, v := range []func() error{ @@ -586,3 +557,32 @@ func (s *goCriticSettingsWrapper) validateAtLeastOneCheckerEnabled() error { } return nil } + +func normalizeMap[ValueT any](in map[string]ValueT) map[string]ValueT { + ret := make(map[string]ValueT, len(in)) + for k, v := range in { + ret[strings.ToLower(k)] = v + } + return ret +} + +func isEnabledByDefaultGoCriticChecker(info *gocriticlinter.CheckerInfo) bool { + // https://github.com/go-critic/go-critic/blob/5b67cfd487ae9fe058b4b19321901b3131810f65/cmd/gocritic/check.go#L342-L345 + return !info.HasTag(gocriticlinter.ExperimentalTag) && + !info.HasTag(gocriticlinter.OpinionatedTag) && + !info.HasTag(gocriticlinter.PerformanceTag) && + !info.HasTag(gocriticlinter.SecurityTag) +} + +func debugChecksListf(checks []string, format string, args ...any) { + if !isGoCriticDebug { + return + } + + goCriticDebugf("%s checks (%d): %s", fmt.Sprintf(format, args...), len(checks), sprintSortedStrings(checks)) +} + +func sprintSortedStrings(v []string) string { + sort.Strings(slices.Clone(v)) + return fmt.Sprint(v) +} From cd4e02f49fb37a044f35d53b6b3087d938d6c8f4 Mon Sep 17 00:00:00 2001 From: Fernandez Ludovic Date: Mon, 19 Feb 2024 14:39:32 +0100 Subject: [PATCH 8/8] review: rename lintConfigDirGetter --- pkg/golinters/gocritic.go | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/pkg/golinters/gocritic.go b/pkg/golinters/gocritic.go index ee72921dddad..7fc791db3f02 100644 --- a/pkg/golinters/gocritic.go +++ b/pkg/golinters/gocritic.go @@ -37,8 +37,8 @@ func NewGoCritic(settings *config.GoCriticSettings, cfg *config.Config) *goanaly var resIssues []goanalysis.Issue wrapper := &goCriticWrapper{ - lintConfigDirGetter: cfg.GetConfigDir, // Config directory is filled after calling this constructor. - sizes: types.SizesFor("gc", runtime.GOARCH), + getConfigDir: cfg.GetConfigDir, // Config directory is filled after calling this constructor. + sizes: types.SizesFor("gc", runtime.GOARCH), } analyzer := &analysis.Analyzer{ @@ -80,10 +80,10 @@ Dynamic rules are written declaratively with AST patterns, filters, report messa } type goCriticWrapper struct { - settingsWrapper *goCriticSettingsWrapper - lintConfigDirGetter func() string - sizes types.Sizes - once sync.Once + settingsWrapper *goCriticSettingsWrapper + getConfigDir func() string + sizes types.Sizes + once sync.Once } func (w *goCriticWrapper) init(settings *config.GoCriticSettings, logger logutils.Log) { @@ -206,7 +206,7 @@ func (w *goCriticWrapper) normalizeCheckerParamsValue(p any) any { return rv.Bool() case reflect.String: // Perform variable substitution. - return strings.ReplaceAll(rv.String(), "${configDir}", w.lintConfigDirGetter()) + return strings.ReplaceAll(rv.String(), "${configDir}", w.getConfigDir()) default: return p }