var-naming: detect meaningless package names (#1312)

RULES_DESCRIPTIONS.md

@@ -1118,12 +1118,14 @@ _Configuration_: N/A
 
 _Description_: This rule warns when [initialism](https://go.dev/wiki/CodeReviewComments#initialisms), [variable](https://go.dev/wiki/CodeReviewComments#variable-names) or [package](https://go.dev/wiki/CodeReviewComments#package-names) naming conventions are not followed. It ignores functions starting with `Example`, `Test`, `Benchmark`, and `Fuzz` in test files, preserving `golint` original behavior.
 
-_Configuration_: This rule accepts two slices of strings and one optional slice with single map with named parameters.
+_Configuration_: This rule accepts two slices of strings and one optional slice containing a single map with named parameters.
-(it's due to TOML hasn't "slice of any" and we keep backward compatibility with previous config version)
+(This is because TOML does not support "slice of any," and we maintain backward compatibility with the previous configuration version).
-First slice is an allowlist and second one is a blocklist of initialisms.
+The first slice is an allowlist, and the second one is a blocklist of initialisms.
-In map, you can add boolean `upperCaseConst` (`uppercaseconst`, `upper-case-const`) parameter to allow `UPPER_CASE` for `const`
+In the map, you can add a boolean `upperCaseConst` (`uppercaseconst`, `upper-case-const`) parameter to allow `UPPER_CASE` for `const`.
-You can also add boolean `skipPackageNameChecks` (`skippackagenamechecks`, `skip-package-name-checks`) to skip package name checks.
+You can also add a boolean `skipPackageNameChecks` (`skippackagenamechecks`, `skip-package-name-checks`) to skip package name checks.
-By default, the rule behaves exactly as the alternative in `golint` but optionally, you can relax it (see [golint/lint/issues/89](https://github.com/golang/lint/issues/89))
+When `skipPackageNameChecks` is false (the default), you can configure `extraBadPackageNames` (`extrabadpackagenames`, `extra-bad-package-names`) to forbid using the values from the list as package names additionally to the standard meaningless ones: "common", "interfaces", "misc", "types", "util", "utils".
+
+By default, the rule behaves exactly as the alternative in `golint` but optionally, you can relax it (see [golint/lint/issues/89](https://github.com/golang/lint/issues/89)).
 
 Examples:
 
@@ -1137,6 +1139,11 @@ Examples:
   arguments = [[], [], [{skipPackageNameChecks=true}]]
 ```
 
+```toml
+[rule.var-naming]
+  arguments = [[], [], [{extraBadPackageNames=["helpers", "models"]}]]
+```
+
 ```toml
 [rule.var-naming]
   arguments = [["ID"], ["VM"], [{upper-case-const=true}]]
@@ -1147,6 +1154,11 @@ Examples:
   arguments = [[], [], [{skip-package-name-checks=true}]]
 ```
 
+```toml
+[rule.var-naming]
+  arguments = [[], [], [{extra-bad-package-names=["helpers", "models"]}]]
+```
+
 ## waitgroup-by-value
 
 _Description_: Function parameters that are passed by value, are in fact a copy of the original argument. Passing a copy of a `sync.WaitGroup` is usually not what the developer wants to do.

rule/var_naming.go

@@ -22,12 +22,26 @@ var knownNameExceptions = map[string]bool{
 	"kWh":          true,
 }
 
+// defaultBadPackageNames is the list of "bad" package names from https://go.dev/wiki/CodeReviewComments#package-names
+// and https://go.dev/blog/package-names#bad-package-names.
+// The rule warns about the usage of any package name in this list if skipPackageNameChecks is false.
+// Values in the list should be lowercased.
+var defaultBadPackageNames = map[string]struct{}{
+	"common":     {},
+	"interfaces": {},
+	"misc":       {},
+	"types":      {},
+	"util":       {},
+	"utils":      {},
+}
+
 // VarNamingRule lints the name of a variable.
 type VarNamingRule struct {
 	allowList             []string
 	blockList             []string
-	allowUpperCaseConst   bool // if true - allows to use UPPER_SOME_NAMES for constants
+	allowUpperCaseConst   bool                // if true - allows to use UPPER_SOME_NAMES for constants
-	skipPackageNameChecks bool
+	skipPackageNameChecks bool                // check for meaningless and user-defined bad package names
+	extraBadPackageNames  map[string]struct{} // inactive if skipPackageNameChecks is false
 }
 
 // Configure validates the rule configuration, and configures the rule accordingly.
@@ -70,27 +84,62 @@ func (r *VarNamingRule) Configure(arguments lint.Arguments) error {
 				r.allowUpperCaseConst = fmt.Sprint(v) == "true"
 			case isRuleOption(k, "skipPackageNameChecks"):
 				r.skipPackageNameChecks = fmt.Sprint(v) == "true"
+			case isRuleOption(k, "extraBadPackageNames"):
+				extraBadPackageNames, ok := v.([]string)
+				if !ok {
+					return fmt.Errorf("invalid third argument to the var-naming rule. Expecting extraBadPackageNames of type slice of strings, but %T", v)
+				}
+				for _, name := range extraBadPackageNames {
+					if r.extraBadPackageNames == nil {
+						r.extraBadPackageNames = map[string]struct{}{}
+					}
+					r.extraBadPackageNames[strings.ToLower(name)] = struct{}{}
+				}
 			}
 		}
 	}
 	return nil
 }
 
-func (*VarNamingRule) applyPackageCheckRules(walker *lintNames) {
+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(walker.fileAst.Name.Name, "_") && !strings.HasSuffix(walker.fileAst.Name.Name, "_test") {
+	if strings.Contains(packageName, "_") && !strings.HasSuffix(packageName, "_test") {
 		walker.onFailure(lint.Failure{
 			Failure:    "don't use an underscore in package name",
 			Confidence: 1,
-			Node:       walker.fileAst.Name,
+			Node:       node,
 			Category:   lint.FailureCategoryNaming,
 		})
 	}
-	if anyCapsRE.MatchString(walker.fileAst.Name.Name) {
+	if anyCapsRE.MatchString(packageName) {
 		walker.onFailure(lint.Failure{
-			Failure:    fmt.Sprintf("don't use MixedCaps in package name; %s should be %s", walker.fileAst.Name.Name, strings.ToLower(walker.fileAst.Name.Name)),
+			Failure:    fmt.Sprintf("don't use MixedCaps in package name; %s should be %s", packageName, lowerPackageName),
 			Confidence: 1,
-			Node:       walker.fileAst.Name,
+			Node:       node,
 			Category:   lint.FailureCategoryNaming,
 		})
 	}

rule/var_naming_test.go

@@ -2,6 +2,7 @@ package rule
 
 import (
 	"errors"
+	"reflect"
 	"testing"
 
 	"github.com/mgechev/revive/lint"
@@ -16,6 +17,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 		wantBlockList             []string
 		wantAllowUpperCaseConst   bool
 		wantSkipPackageNameChecks bool
+		wantBadPackageNames       map[string]struct{}
 	}{
 		{
 			name:                      "no arguments",
@@ -34,6 +36,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 				[]any{map[string]any{
 					"upperCaseConst":        true,
 					"skipPackageNameChecks": true,
+					"extraBadPackageNames":  []string{"helpers", "models"},
 				}},
 			},
 			wantErr:                   nil,
@@ -41,6 +44,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 			wantBlockList:             []string{"VM"},
 			wantAllowUpperCaseConst:   true,
 			wantSkipPackageNameChecks: true,
+			wantBadPackageNames:       map[string]struct{}{"helpers": {}, "models": {}},
 		},
 		{
 			name: "valid lowercased arguments",
@@ -50,6 +54,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 				[]any{map[string]any{
 					"uppercaseconst":        true,
 					"skippackagenamechecks": true,
+					"extrabadpackagenames":  []string{"helpers", "models"},
 				}},
 			},
 			wantErr:                   nil,
@@ -57,6 +62,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 			wantBlockList:             []string{"VM"},
 			wantAllowUpperCaseConst:   true,
 			wantSkipPackageNameChecks: true,
+			wantBadPackageNames:       map[string]struct{}{"helpers": {}, "models": {}},
 		},
 		{
 			name: "valid kebab-cased arguments",
@@ -66,6 +72,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 				[]any{map[string]any{
 					"upper-case-const":         true,
 					"skip-package-name-checks": true,
+					"extra-bad-package-names":  []string{"helpers", "models"},
 				}},
 			},
 			wantErr:                   nil,
@@ -73,6 +80,7 @@ func TestVarNamingRule_Configure(t *testing.T) {
 			wantBlockList:             []string{"VM"},
 			wantAllowUpperCaseConst:   true,
 			wantSkipPackageNameChecks: true,
+			wantBadPackageNames:       map[string]struct{}{"helpers": {}, "models": {}},
 		},
 		{
 			name:      "invalid allowlist type",
@@ -104,6 +112,11 @@ func TestVarNamingRule_Configure(t *testing.T) {
 			arguments: lint.Arguments{[]any{"ID"}, []any{"VM"}, []any{123}},
 			wantErr:   errors.New("invalid third argument to the var-naming rule. Expecting a options of type slice, of len==1, with map, but int"),
 		},
+		{
+			name:      "invalid third argument extraBadPackageNames",
+			arguments: lint.Arguments{[]any{""}, []any{""}, []any{map[string]any{"extraBadPackageNames": []int{1}}}},
+			wantErr:   errors.New("invalid third argument to the var-naming rule. Expecting extraBadPackageNames of type slice of strings, but []int"),
+		},
 	}
 
 	for _, tt := range tests {
@@ -131,6 +144,9 @@ func TestVarNamingRule_Configure(t *testing.T) {
 			if rule.skipPackageNameChecks != tt.wantSkipPackageNameChecks {
 				t.Errorf("unexpected skipPackageNameChecks: got = %v, want %v", rule.skipPackageNameChecks, tt.wantSkipPackageNameChecks)
 			}
+			if !reflect.DeepEqual(rule.extraBadPackageNames, tt.wantBadPackageNames) {
+				t.Errorf("unexpected extraBadPackageNames: got = %v, want %v", rule.extraBadPackageNames, tt.wantBadPackageNames)
+			}
 		})
 	}
 }

test/var_naming_test.go

@@ -29,4 +29,15 @@ func TestVarNaming(t *testing.T) {
 	testRule(t, "var_naming_skip_package_name_checks_true", &rule.VarNamingRule{}, &lint.RuleConfig{
 		Arguments: []any{[]any{}, []any{}, []any{map[string]any{"skip-package-name-checks": true}}},
 	})
+	testRule(t, "var_naming_meaningless_package_name", &rule.VarNamingRule{}, &lint.RuleConfig{})
+	testRule(t, "var_naming_meaningless_package_name", &rule.VarNamingRule{}, &lint.RuleConfig{
+		Arguments: []any{[]any{}, []any{},
+			[]any{map[string]any{"skip-package-name-checks": false}},
+		},
+	})
+	testRule(t, "var_naming_bad_package_name", &rule.VarNamingRule{}, &lint.RuleConfig{
+		Arguments: []any{[]any{}, []any{},
+			[]any{map[string]any{"skip-package-name-checks": false, "extra-bad-package-names": []string{"helpers"}}},
+		},
+	})
 }

testdata/var_naming_bad_package_name.go

@@ -0,0 +1 @@
+package helpers // MATCH /avoid bad package names/

testdata/var_naming_meaningless_package_name.go

@@ -0,0 +1 @@
+package util // MATCH /avoid meaningless package names/