Repository: mgechev/revive
Pull request #1319 (2025-04-17 19:51:14)

var-name checks each package name only once
This PR is related to #1317, more specifically to 
[Generate a single failure per package (instead of one for each file in a package)]( https://github.com/mgechev/revive/pull/1312#pullrequestreview-2770906632)

Here, "check" must be understood as "check package name"

Remarks:
1. The proposed implementation checks only once each package name. This is accomplished by checking source file and keeping a reference to the directory it belongs to; after, no other file in the same directory will be checked.
2. Testing required a trick. Standard test method allows checking only one file at the time thus it is inappropriate to test package-wide checks. On the other side, `var-naming` rule is tested with other `golint`-related rules. These rules are tested differently, by executing all of them in a package-wide environment (defined under `testdata\golint`). This allows to test `var-naming` only fails once per package name. Testsdata files `a_pkg_with_caps.go`  and `another_pkg_with_caps.go` are named in such a way that they are the first and the second files to be analysed: `var-naming` will raise a failure for the first but not for the second even if both have the same package name. The testing machinery required a modification to do not raise an error when a testdata file has no _testing instructions_ (i.e. annotations of the form `// MATCH /.../`) Testing the functionality implemented by this PR requires a test file (`another_pkg_with_caps.go`) without testing instructions.

BTW: the testing machinery might deserve a refactoring to remove unused code (JSON test code for example) and to make it more Go idiomatic.



Patch:
diff --git a/rule/var_naming.go b/rule/var_naming.go
index 503ef5b..818797d 100644
--- a/rule/var_naming.go
+++ b/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,71 +106,27 @@ 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
+	onFailure := func(failure lint.Failure) {
+		failures = append(failures, failure)
+	}
 
-	fileAst := file.AST
+	if !r.skipPackageNameChecks {
+		r.applyPackageCheckRules(file, onFailure)
+	}
 
+	fileAst := file.AST
 	walker := lintNames{
-		file:      file,
-		fileAst:   fileAst,
-		allowList: r.allowList,
-		blockList: r.blockList,
-		onFailure: func(failure lint.Failure) {
-			failures = append(failures, failure)
-		},
+		file:           file,
+		fileAst:        fileAst,
+		allowList:      r.allowList,
+		blockList:      r.blockList,
+		onFailure:      onFailure,
 		upperCaseConst: r.allowUpperCaseConst,
 	}
 
-	if !r.skipPackageNameChecks {
-		r.applyPackageCheckRules(&walker)
-	}
-
 	ast.Walk(&walker, fileAst)
 
 	return failures
@@ -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 patch:
diff --git a/test/utils_test.go b/test/utils_test.go
index 9686c48..20502eb 100644
--- a/test/utils_test.go
+++ b/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,
diff --git a/testdata/golint/a_pkg_with_caps.go b/testdata/golint/a_pkg_with_caps.go
new file mode 100644
index 0000000..fce2b54
--- /dev/null
+++ b/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/
diff --git a/testdata/golint/another_pkg_with_caps.go b/testdata/golint/another_pkg_with_caps.go
new file mode 100644
index 0000000..d56a21d
--- /dev/null
+++ b/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() {}
diff --git a/testdata/golint/pkg_caps.go b/testdata/golint/pkg_caps.go
deleted file mode 100644
index e8ca0b7..0000000
--- a/testdata/golint/pkg_caps.go
+++ /dev/null
@@ -1,4 +0,0 @@
-// MixedCaps package name
-
-// Package PkgName ...
-package PkgName // MATCH /don't use MixedCaps in package name; PkgName should be pkgname/
diff --git a/testdata/golint/var_naming.go b/testdata/golint/var_naming.go
index 4d26acd..9f16884 100644
--- a/testdata/golint/var_naming.go
+++ b/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"