From e8ed57373920405361935054554b6dbe383e3e23 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Mon, 26 May 2025 12:40:17 +0300 Subject: [PATCH] refactor: enable gocritic linter; fix lint issues (#1375) --- .golangci.yml | 14 +++++++++++ cli/main.go | 2 +- cli/main_test.go | 16 ++++++------ lint/file.go | 2 +- lint/filefilter.go | 2 +- lint/name.go | 7 +++--- lint/package.go | 43 ++++++++++++++++---------------- revivelib/core.go | 17 ++++--------- rule/dot_imports.go | 3 ++- rule/import_alias_naming.go | 1 + rule/import_alias_naming_test.go | 2 +- rule/imports_blocklist.go | 2 +- rule/redundant_import_alias.go | 2 +- rule/string_format.go | 10 ++++---- rule/struct_tag.go | 4 +-- rule/var_naming.go | 20 +++++++-------- test/utils_test.go | 2 +- 17 files changed, 79 insertions(+), 70 deletions(-) diff --git a/.golangci.yml b/.golangci.yml index d265a4c..30993c5 100644 --- a/.golangci.yml +++ b/.golangci.yml @@ -6,6 +6,7 @@ version: "2" linters: default: none enable: + - gocritic - govet - ineffassign - misspell @@ -15,6 +16,11 @@ linters: - unused settings: + gocritic: + enable-all: true + disabled-checks: + - hugeParam + - rangeValCopy govet: enable-all: true disable: @@ -73,3 +79,11 @@ linters: - name: use-any - name: var-declaration - name: var-naming + +issues: + # Show all issues from a linter. + max-issues-per-linter: 0 + # Show all issues with the same text. + max-same-issues: 0 + # Show all issues for a line. + uniq-by-line: false diff --git a/cli/main.go b/cli/main.go index c703643..fd4876f 100644 --- a/cli/main.go +++ b/cli/main.go @@ -190,7 +190,7 @@ func getVersion(builtBy, date, commit, version string) string { bi, ok := debug.ReadBuildInfo() if ok { version = strings.TrimPrefix(bi.Main.Version, "v") - if len(buildInfo) == 0 { + if buildInfo == "" { return fmt.Sprintf("version %s\n", version) } } diff --git a/cli/main_test.go b/cli/main_test.go index cb913dc..7e9c404 100644 --- a/cli/main_test.go +++ b/cli/main_test.go @@ -25,13 +25,13 @@ func TestXDGConfigDirIsPreferredFirst(t *testing.T) { xdgDirPath := filepath.FromSlash("/tmp-iofs/xdg/config") homeDirPath := filepath.FromSlash("/tmp-iofs/home/tester") - AppFs.MkdirAll(xdgDirPath, 0755) - AppFs.MkdirAll(homeDirPath, 0755) + AppFs.MkdirAll(xdgDirPath, 0o755) + AppFs.MkdirAll(homeDirPath, 0o755) - afero.WriteFile(AppFs, filepath.Join(xdgDirPath, "revive.toml"), []byte("\n"), 0644) + afero.WriteFile(AppFs, filepath.Join(xdgDirPath, "revive.toml"), []byte("\n"), 0o644) t.Setenv("XDG_CONFIG_HOME", xdgDirPath) - afero.WriteFile(AppFs, filepath.Join(homeDirPath, "revive.toml"), []byte("\n"), 0644) + afero.WriteFile(AppFs, filepath.Join(homeDirPath, "revive.toml"), []byte("\n"), 0o644) setHome(t, homeDirPath) got := buildDefaultConfigPath() @@ -45,9 +45,9 @@ func TestXDGConfigDirIsPreferredFirst(t *testing.T) { func TestHomeConfigDir(t *testing.T) { t.Cleanup(func() { AppFs = afero.NewMemMapFs() }) homeDirPath := filepath.FromSlash("/tmp-iofs/home/tester") - AppFs.MkdirAll(homeDirPath, 0755) + AppFs.MkdirAll(homeDirPath, 0o755) - afero.WriteFile(AppFs, filepath.Join(homeDirPath, "revive.toml"), []byte("\n"), 0644) + afero.WriteFile(AppFs, filepath.Join(homeDirPath, "revive.toml"), []byte("\n"), 0o644) setHome(t, homeDirPath) got := buildDefaultConfigPath() @@ -69,9 +69,9 @@ func setHome(t *testing.T, dir string) { func TestXDGConfigDir(t *testing.T) { t.Cleanup(func() { AppFs = afero.NewMemMapFs() }) xdgDirPath := filepath.FromSlash("/tmp-iofs/xdg/config") - AppFs.MkdirAll(xdgDirPath, 0755) + AppFs.MkdirAll(xdgDirPath, 0o755) - afero.WriteFile(AppFs, filepath.Join(xdgDirPath, "revive.toml"), []byte("\n"), 0644) + afero.WriteFile(AppFs, filepath.Join(xdgDirPath, "revive.toml"), []byte("\n"), 0o644) t.Setenv("XDG_CONFIG_HOME", xdgDirPath) got := buildDefaultConfigPath() diff --git a/lint/file.go b/lint/file.go index 950d756..60060ff 100644 --- a/lint/file.go +++ b/lint/file.go @@ -222,7 +222,7 @@ func (f *File) disabledIntervals(rules []Rule, mustSpecifyDisableReason bool, fa for _, name := range tempNames { name = strings.Trim(name, "\n") - if len(name) > 0 { + if name != "" { ruleNames = append(ruleNames, name) } } diff --git a/lint/filefilter.go b/lint/filefilter.go index 0e81fed..6aed2a0 100644 --- a/lint/filefilter.go +++ b/lint/filefilter.go @@ -31,7 +31,7 @@ func ParseFileFilter(rawFilter string) (*FileFilter, error) { rawFilter = strings.TrimSpace(rawFilter) result := new(FileFilter) result.raw = rawFilter - result.matchesNothing = len(result.raw) == 0 + result.matchesNothing = result.raw == "" result.matchesAll = result.raw == "*" || result.raw == "~" if !result.matchesAll && !result.matchesNothing { if err := result.prepareRegexp(); err != nil { diff --git a/lint/name.go b/lint/name.go index 6ccfb0e..4631cec 100644 --- a/lint/name.go +++ b/lint/name.go @@ -28,9 +28,10 @@ func Name(name string, allowlist, blocklist []string) (should string) { w, i := 0, 0 // index of start of word, scan for i+1 <= len(runes) { eow := false // whether we hit the end of a word - if i+1 == len(runes) { + switch { + case i+1 == len(runes): eow = true - } else if runes[i+1] == '_' { + case runes[i+1] == '_': // underscore; shift the remainder forward over any run of underscores eow = true n := 1 @@ -45,7 +46,7 @@ func Name(name string, allowlist, blocklist []string) (should string) { copy(runes[i+1:], runes[i+n+1:]) runes = runes[:len(runes)-n] - } else if unicode.IsLower(runes[i]) && !unicode.IsLower(runes[i+1]) { + case unicode.IsLower(runes[i]) && !unicode.IsLower(runes[i+1]): // lower->non-lower eow = true } diff --git a/lint/package.go b/lint/package.go index c0bcf49..5b25de3 100644 --- a/lint/package.go +++ b/lint/package.go @@ -17,18 +17,17 @@ import ( // Package represents a package in the project. type Package struct { - fset *token.FileSet + fset *token.FileSet + + mu sync.RWMutex files map[string]*File goVersion *goversion.Version - typesPkg *types.Package typesInfo *types.Info - // sortable is the set of types in the package that implement sort.Interface. sortable map[string]bool // main is whether this is a "main" package. main int - sync.RWMutex } var ( @@ -47,16 +46,16 @@ var ( // Files return package's files. func (p *Package) Files() map[string]*File { - p.RLock() - defer p.RUnlock() + p.mu.RLock() + defer p.mu.RUnlock() return p.files } // IsMain returns if that's the main package. func (p *Package) IsMain() bool { - p.Lock() - defer p.Unlock() + p.mu.Lock() + defer p.mu.Unlock() switch p.main { case trueValue: @@ -76,32 +75,32 @@ func (p *Package) IsMain() bool { // TypesPkg yields information on this package func (p *Package) TypesPkg() *types.Package { - p.RLock() - defer p.RUnlock() + p.mu.RLock() + defer p.mu.RUnlock() return p.typesPkg } // TypesInfo yields type information of this package identifiers func (p *Package) TypesInfo() *types.Info { - p.RLock() - defer p.RUnlock() + p.mu.RLock() + defer p.mu.RUnlock() return p.typesInfo } // Sortable yields a map of sortable types in this package func (p *Package) Sortable() map[string]bool { - p.RLock() - defer p.RUnlock() + p.mu.RLock() + defer p.mu.RUnlock() return p.sortable } // TypeCheck performs type checking for given package. func (p *Package) TypeCheck() error { - p.Lock() - defer p.Unlock() + p.mu.Lock() + defer p.mu.Unlock() alreadyTypeChecked := p.typesInfo != nil || p.typesPkg != nil if alreadyTypeChecked { @@ -157,8 +156,8 @@ func check(config *types.Config, n string, fset *token.FileSet, astFiles []*ast. // TypeOf returns the type of expression. func (p *Package) TypeOf(expr ast.Expr) types.Type { - p.RLock() - defer p.RUnlock() + p.mu.RLock() + defer p.mu.RUnlock() if p.typesInfo == nil { return nil @@ -177,8 +176,8 @@ const ( ) func (p *Package) scanSortable() { - p.Lock() - defer p.Unlock() + p.mu.Lock() + defer p.mu.Unlock() sortableFlags := map[string]sortableMethodsFlags{} for _, f := range p.files { @@ -216,8 +215,8 @@ func (p *Package) lint(rules []Rule, config Config, failures chan Failure) error // IsAtLeastGoVersion returns true if the Go version for this package is v or higher, false otherwise func (p *Package) IsAtLeastGoVersion(v *goversion.Version) bool { - p.RLock() - defer p.RUnlock() + p.mu.RLock() + defer p.mu.RUnlock() return p.goVersion.GreaterThanOrEqual(v) } diff --git a/revivelib/core.go b/revivelib/core.go index 0e237f4..fa5c64a 100755 --- a/revivelib/core.go +++ b/revivelib/core.go @@ -116,7 +116,7 @@ func (r *Revive) Lint(patterns ...*LintPattern) (<-chan lint.Failure, error) { func (r *Revive) Format( formatterName string, failuresChan <-chan lint.Failure, -) (string, int, error) { +) (output string, exitCode int, err error) { conf := r.config formatChan := make(chan lint.Failure) exitChan := make(chan bool) @@ -126,19 +126,12 @@ func (r *Revive) Format( return "", 0, fmt.Errorf("formatting - getting formatter: %w", err) } - var ( - output string - formatErr error - ) - go func() { - output, formatErr = formatter.Format(formatChan, *conf) + output, err = formatter.Format(formatChan, *conf) exitChan <- true }() - exitCode := 0 - for failure := range failuresChan { if failure.Confidence < conf.Confidence { continue @@ -162,8 +155,8 @@ func (r *Revive) Format( close(formatChan) <-exitChan - if formatErr != nil { - return "", exitCode, fmt.Errorf("formatting: %w", formatErr) + if err != nil { + return "", exitCode, fmt.Errorf("formatting: %w", err) } return output, exitCode, nil @@ -188,7 +181,7 @@ func normalizeSplit(strs []string) []string { for _, s := range strs { t := strings.Trim(s, " \t") - if len(t) > 0 { + if t != "" { res = append(res, t) } } diff --git a/rule/dot_imports.go b/rule/dot_imports.go index bb86a73..a5f2210 100644 --- a/rule/dot_imports.go +++ b/rule/dot_imports.go @@ -3,6 +3,7 @@ package rule import ( "fmt" "go/ast" + "strconv" "github.com/mgechev/revive/lint" ) @@ -94,7 +95,7 @@ func (w lintImports) Visit(_ ast.Node) ast.Visitor { type allowPackages map[string]struct{} func (ap allowPackages) add(pkg string) { - ap[fmt.Sprintf(`"%s"`, pkg)] = struct{}{} // import path strings are with double quotes + ap[strconv.Quote(pkg)] = struct{}{} // import path strings are with double quotes } func (ap allowPackages) isAllowedPackage(pkg string) bool { diff --git a/rule/import_alias_naming.go b/rule/import_alias_naming.go index 94cea78..a46c331 100644 --- a/rule/import_alias_naming.go +++ b/rule/import_alias_naming.go @@ -15,6 +15,7 @@ type ImportAliasNamingRule struct { const defaultImportAliasNamingAllowRule = "^[a-z][a-z0-9]{0,}$" +//nolint:gocritic // regexpSimplify: backward compatibility var defaultImportAliasNamingAllowRegexp = regexp.MustCompile(defaultImportAliasNamingAllowRule) // Configure validates the rule configuration, and configures the rule accordingly. diff --git a/rule/import_alias_naming_test.go b/rule/import_alias_naming_test.go index 69aa60c..88e0bba 100644 --- a/rule/import_alias_naming_test.go +++ b/rule/import_alias_naming_test.go @@ -21,7 +21,7 @@ func TestImportAliasNamingRule_Configure(t *testing.T) { name: "no arguments", arguments: lint.Arguments{}, wantErr: nil, - wantAllowRegex: regexp.MustCompile("^[a-z][a-z0-9]{0,}$"), + wantAllowRegex: regexp.MustCompile("^[a-z][a-z0-9]{0,}$"), //nolint:gocritic // regexpSimplify: backward compatibility wantDenyRegex: nil, }, { diff --git a/rule/imports_blocklist.go b/rule/imports_blocklist.go index c96382d..8d3b086 100644 --- a/rule/imports_blocklist.go +++ b/rule/imports_blocklist.go @@ -24,7 +24,7 @@ func (r *ImportsBlocklistRule) Configure(arguments lint.Arguments) error { if !ok { return fmt.Errorf("invalid argument to the imports-blocklist rule. Expecting a string, got %T", arg) } - regStr, err := regexp.Compile(fmt.Sprintf(`(?m)"%s"$`, replaceImportRegexp.ReplaceAllString(argStr, `(\W|\w)*`))) + regStr, err := regexp.Compile(fmt.Sprintf(`(?m)"%s"$`, replaceImportRegexp.ReplaceAllString(argStr, `(\W|\w)*`))) //nolint:gocritic // regexpSimplify: false positive if err != nil { return fmt.Errorf("invalid argument to the imports-blocklist rule. Expecting %q to be a valid regular expression, got: %w", argStr, err) } diff --git a/rule/redundant_import_alias.go b/rule/redundant_import_alias.go index 692507a..9c2edb0 100644 --- a/rule/redundant_import_alias.go +++ b/rule/redundant_import_alias.go @@ -23,7 +23,7 @@ func (*RedundantImportAlias) Apply(file *lint.File, _ lint.Arguments) []lint.Fai if getImportPackageName(imp) == imp.Name.Name { failures = append(failures, lint.Failure{ Confidence: 1, - Failure: fmt.Sprintf("Import alias \"%s\" is redundant", imp.Name.Name), + Failure: fmt.Sprintf("Import alias %q is redundant", imp.Name.Name), Node: imp, Category: lint.FailureCategoryImports, }) diff --git a/rule/string_format.go b/rule/string_format.go index d539577..80e43af 100644 --- a/rule/string_format.go +++ b/rule/string_format.go @@ -118,7 +118,7 @@ func (r *StringFormatRule) parseArgument(argument any, ruleNum int) (scopes stri for scopeNum, rawScope := range rawScopes { rawScope = strings.TrimSpace(rawScope) - if len(rawScope) == 0 { + if rawScope == "" { return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("empty scope in rule scopes:", ruleNum, 0, scopeNum) } @@ -133,14 +133,14 @@ func (r *StringFormatRule) parseArgument(argument any, ruleNum int) (scopes stri r.parseScopeError(fmt.Sprintf("unexpected number of submatches when parsing scope: %d, expected 4", len(matches)), ruleNum, 0, scopeNum) } scope.funcName = matches[1] - if len(matches[2]) > 0 { + if matches[2] != "" { var err error scope.argument, err = strconv.Atoi(matches[2]) if err != nil { return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("unable to parse argument number in rule scope", ruleNum, 0, scopeNum) } } - if len(matches[3]) > 0 { + if matches[3] != "" { scope.field = matches[3] } @@ -235,7 +235,7 @@ func (r *stringFormatSubrule) apply(call *ast.CallExpr, scope *stringFormatSubru arg := call.Args[scope.argument] var lit *ast.BasicLit - if len(scope.field) > 0 { + if scope.field != "" { // Try finding the scope's Field, treating arg as a composite literal composite, ok := arg.(*ast.CompositeLit) if !ok { @@ -292,7 +292,7 @@ func (r *stringFormatSubrule) stringIsOK(s string) bool { func (r *stringFormatSubrule) generateFailure(node ast.Node) { var failure string switch { - case len(r.errorMessage) > 0: + case r.errorMessage != "": failure = r.errorMessage case r.negated: failure = fmt.Sprintf("string literal matches user defined regex /%s/", r.regexp.String()) diff --git a/rule/struct_tag.go b/rule/struct_tag.go index aa2a8ab..d9ef017 100644 --- a/rule/struct_tag.go +++ b/rule/struct_tag.go @@ -235,7 +235,7 @@ func (lintStructTagRule) getTagName(tag *structtag.Tag) string { } func checkASN1Tag(checkCtx *checkContext, tag *structtag.Tag, fieldType ast.Expr) (message string, succeeded bool) { - checkList := append(tag.Options, tag.Name) + checkList := slices.Concat(tag.Options, []string{tag.Name}) for _, opt := range checkList { switch opt { case "application", "explicit", "generalized", "ia5", "omitempty", "optional", "set", "utf8": @@ -605,7 +605,7 @@ func typeValueMatch(t ast.Expr, val string) bool { return typeMatches } -func (w lintStructTagRule) addFailureWithTagKey(n ast.Node, msg string, tagKey string) { +func (w lintStructTagRule) addFailureWithTagKey(n ast.Node, msg, tagKey string) { w.addFailuref(n, "%s in %s tag", msg, tagKey) } diff --git a/rule/var_naming.go b/rule/var_naming.go index 710d141..fec72fe 100644 --- a/rule/var_naming.go +++ b/rule/var_naming.go @@ -171,6 +171,15 @@ func (*VarNamingRule) pkgNameFailure(node ast.Node, msg string, args ...any) lin } } +type lintNames struct { + file *lint.File + fileAst *ast.File + onFailure func(lint.Failure) + allowList []string + blockList []string + upperCaseConst bool +} + func (w *lintNames) checkList(fl *ast.FieldList, thing string) { if fl == nil { return @@ -229,15 +238,6 @@ func (w *lintNames) check(id *ast.Ident, thing string) { }) } -type lintNames struct { - file *lint.File - fileAst *ast.File - onFailure func(lint.Failure) - allowList []string - blockList []string - upperCaseConst bool -} - func (w *lintNames) Visit(n ast.Node) ast.Visitor { switch v := n.(type) { case *ast.AssignStmt: @@ -323,7 +323,7 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor { // isUpperCaseConst checks if a string is in constant name format like `SOME_CONST`, `SOME_CONST_2`, `X123_3`, `_SOME_PRIVATE_CONST`. // See #851, #865. func isUpperCaseConst(s string) bool { - if len(s) == 0 { + if s == "" { return false } r := []rune(s) diff --git a/test/utils_test.go b/test/utils_test.go index 63b5b2c..c30aea1 100644 --- a/test/utils_test.go +++ b/test/utils_test.go @@ -197,7 +197,7 @@ func parseInstructions(t *testing.T, filename string, src []byte) []instruction if i := strings.Index(line, "MATCH:"); i >= 0 { // This is a match for a different line. lns := strings.TrimPrefix(line[i:], "MATCH:") - lns = lns[:strings.Index(lns, " ")] + lns = lns[:strings.Index(lns, " ")] //nolint:gocritic // offBy1: false positive matchLine, err = strconv.Atoi(lns) if err != nil { t.Fatalf("Bad match line number %q at %v:%d: %v", lns, filename, ln, err)