var-name checks each package name only once (#1319)

rule/var_naming.go

@@ -4,8 +4,10 @@ import (
 	"fmt"
 	"go/ast"
 	"go/token"
+	"path/filepath"
 	"regexp"
 	"strings"
+	"sync"
 
 	"github.com/mgechev/revive/lint"
 )
@@ -42,12 +44,15 @@ type VarNamingRule struct {
 	allowUpperCaseConst   bool                // if true - allows to use UPPER_SOME_NAMES for constants
 	skipPackageNameChecks bool                // check for meaningless and user-defined bad package names
 	extraBadPackageNames  map[string]struct{} // inactive if skipPackageNameChecks is false
+	pkgNameAlreadyChecked syncSet             // set of packages names already checked
 }
 
 // Configure validates the rule configuration, and configures the rule accordingly.
 //
 // Configuration implements the [lint.ConfigurableRule] interface.
 func (r *VarNamingRule) Configure(arguments lint.Arguments) error {
+	r.pkgNameAlreadyChecked = syncSet{elements: map[string]struct{}{}}
+
 	if len(arguments) >= 1 {
 		list, err := getList(arguments[0], "allowlist")
 		if err != nil {
@@ -101,69 +106,25 @@ func (r *VarNamingRule) Configure(arguments lint.Arguments) error {
 	return nil
 }
 
-func (r *VarNamingRule) applyPackageCheckRules(walker *lintNames) {
-	node := walker.fileAst.Name
-	packageName := node.Name
-	lowerPackageName := strings.ToLower(packageName)
-
-	if _, ok := r.extraBadPackageNames[lowerPackageName]; ok {
-		walker.onFailure(lint.Failure{
-			Failure:    "avoid bad package names",
-			Confidence: 1,
-			Node:       node,
-			Category:   lint.FailureCategoryNaming,
-		})
-		return
-	}
-
-	if _, ok := defaultBadPackageNames[lowerPackageName]; ok {
-		walker.onFailure(lint.Failure{
-			Failure:    "avoid meaningless package names",
-			Confidence: 1,
-			Node:       node,
-			Category:   lint.FailureCategoryNaming,
-		})
-		return
-	}
-
-	// Package names need slightly different handling than other names.
-	if strings.Contains(packageName, "_") && !strings.HasSuffix(packageName, "_test") {
-		walker.onFailure(lint.Failure{
-			Failure:    "don't use an underscore in package name",
-			Confidence: 1,
-			Node:       node,
-			Category:   lint.FailureCategoryNaming,
-		})
-	}
-	if anyCapsRE.MatchString(packageName) {
-		walker.onFailure(lint.Failure{
-			Failure:    fmt.Sprintf("don't use MixedCaps in package name; %s should be %s", packageName, lowerPackageName),
-			Confidence: 1,
-			Node:       node,
-			Category:   lint.FailureCategoryNaming,
-		})
-	}
-}
-
 // Apply applies the rule to given file.
 func (r *VarNamingRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
 	var failures []lint.Failure
-
-	fileAst := file.AST
-
-	walker := lintNames{
-		file:      file,
-		fileAst:   fileAst,
-		allowList: r.allowList,
-		blockList: r.blockList,
-		onFailure: func(failure lint.Failure) {
-			failures = append(failures, failure)
-		},
-		upperCaseConst: r.allowUpperCaseConst,
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
 	}
 
 	if !r.skipPackageNameChecks {
-		r.applyPackageCheckRules(&walker)
+		r.applyPackageCheckRules(file, onFailure)
+	}
+
+	fileAst := file.AST
+	walker := lintNames{
+		file:           file,
+		fileAst:        fileAst,
+		allowList:      r.allowList,
+		blockList:      r.blockList,
+		onFailure:      onFailure,
+		upperCaseConst: r.allowUpperCaseConst,
 	}
 
 	ast.Walk(&walker, fileAst)
@@ -176,6 +137,48 @@ func (*VarNamingRule) Name() string {
 	return "var-naming"
 }
 
+func (r *VarNamingRule) applyPackageCheckRules(file *lint.File, onFailure func(failure lint.Failure)) {
+	fileDir := filepath.Dir(file.Name)
+
+	// Protect pkgsWithNameFailure from concurrent modifications
+	r.pkgNameAlreadyChecked.Lock()
+	defer r.pkgNameAlreadyChecked.Unlock()
+	if r.pkgNameAlreadyChecked.has(fileDir) {
+		return
+	}
+	r.pkgNameAlreadyChecked.add(fileDir) // mark this package as already checked
+
+	pkgNameNode := file.AST.Name
+	pkgName := pkgNameNode.Name
+	pkgNameLower := strings.ToLower(pkgName)
+	if _, ok := r.extraBadPackageNames[pkgNameLower]; ok {
+		onFailure(r.pkgNameFailure(pkgNameNode, "avoid bad package names"))
+		return
+	}
+
+	if _, ok := defaultBadPackageNames[pkgNameLower]; ok {
+		onFailure(r.pkgNameFailure(pkgNameNode, "avoid meaningless package names"))
+		return
+	}
+
+	// Package names need slightly different handling than other names.
+	if strings.Contains(pkgName, "_") && !strings.HasSuffix(pkgName, "_test") {
+		onFailure(r.pkgNameFailure(pkgNameNode, "don't use an underscore in package name"))
+	}
+	if anyCapsRE.MatchString(pkgName) {
+		onFailure(r.pkgNameFailure(pkgNameNode, "don't use MixedCaps in package names; %s should be %s", pkgName, pkgNameLower))
+	}
+}
+
+func (*VarNamingRule) pkgNameFailure(node ast.Node, msg string, args ...any) lint.Failure {
+	return lint.Failure{
+		Failure:    fmt.Sprintf(msg, args...),
+		Confidence: 1,
+		Node:       node,
+		Category:   lint.FailureCategoryNaming,
+	}
+}
+
 func (w *lintNames) checkList(fl *ast.FieldList, thing string) {
 	if fl == nil {
 		return
@@ -340,3 +343,17 @@ func getList(arg any, argName string) ([]string, error) {
 	}
 	return list, nil
 }
+
+type syncSet struct {
+	sync.Mutex
+	elements map[string]struct{}
+}
+
+func (sm *syncSet) has(s string) bool {
+	_, result := sm.elements[s]
+	return result
+}
+
+func (sm *syncSet) add(s string) {
+	sm.elements[s] = struct{}{}
+}

test/utils_test.go

@@ -90,9 +90,6 @@ func assertFailures(t *testing.T, baseDir string, fi os.FileInfo, src []byte, ru
 	l := lint.New(os.ReadFile, 0)
 
 	ins := parseInstructions(t, filepath.Join(baseDir, fi.Name()), src)
-	if ins == nil {
-		return fmt.Errorf("Test file %v does not have instructions", fi.Name())
-	}
 
 	ps, err := l.Lint([][]string{{filepath.Join(baseDir, fi.Name())}}, rules, lint.Config{
 		Rules: config,

testdata/golint/a_pkg_with_caps.go

@@ -0,0 +1,5 @@
+// MixedCaps package name
+// This file must be the first (by lexicographical order) file in the directory
+
+// Package PkgName ...
+package PkgName // MATCH /don't use MixedCaps in package names; PkgName should be pkgname/

testdata/golint/another_pkg_with_caps.go

@@ -0,0 +1,6 @@
+// MixedCaps package name
+
+// Package PkgName ...
+package PkgName // var-naming doesn't match because the package name was already checked in a_pkg_with_caps.go
+
+func pkgName() {}

testdata/golint/pkg_caps.go

@@ -1,4 +0,0 @@
-// MixedCaps package name
-
-// Package PkgName ...
-package PkgName // MATCH /don't use MixedCaps in package name; PkgName should be pkgname/

testdata/golint/var_naming.go

@@ -1,7 +1,7 @@
 // Test for name linting.
 
-// Package pkg_with_underscores ...
-package pkg_with_underscores // MATCH /don't use an underscore in package name/
+// Package pkg ...
+package pkg // package names checks are tested elesewhere
 
 import (
 	"io"