refactor: enable gocritic linter; fix lint issues (#1375)

.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

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)
 			}
 		}

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()

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)
 				}
 			}

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 {

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
 		}

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)
 }

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)
 		}
 	}

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 {

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.

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,
 		},
 		{

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)
 		}

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,
 			})

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())

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)
 }
 

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)

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)