Repository: mgechev/revive
Pull request #852 (2023-07-17 07:12:27)

imporve `var-naming`  - add upperCaseConst  option to allow UPPER_CASED constants #851

it's improvement for  #851 task


chavacava: Hi @comdiv, thanks for the PR! (and sorry for the late response)
I've made some little modifications to the code. 
Could you please provide a fix for the failing test case and update the description of the rule in RULES_DESCRIPTIONS.md ?

Patch:
diff --git a/README.md b/README.md
index e164826..43800ae 100644
--- a/README.md
+++ b/README.md
@@ -501,6 +501,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a
 | [`use-any`](./RULES_DESCRIPTIONS.md#use-any)          |  n/a   |  Proposes to replace `interface{}` with its alias `any` |    no    |  no   |
 | [`datarace`](./RULES_DESCRIPTIONS.md#datarace)          |  n/a   |  Spots potential dataraces |    no    |  no   |
 | [`comment-spacings`](./RULES_DESCRIPTIONS.md#comment-spacings)          |  []string   |  Warns on malformed comments |    no    |  no   |
+| [`redundant-import-alias`](./RULES_DESCRIPTIONS.md#redundant-import-alias)          |  n/a   |  Warns on import aliases matching the imported package name |    no    |  no   |
 
 
 ## Configurable rules
diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md
index e6fa3d0..1709188 100644
--- a/RULES_DESCRIPTIONS.md
+++ b/RULES_DESCRIPTIONS.md
@@ -75,6 +75,7 @@ List of all available rules.
   - [use-any](#use-any)
   - [useless-break](#useless-break)
   - [waitgroup-by-value](#waitgroup-by-value)
+  - [redundant-import-alias](#redundant-import-alias)
 
 ## add-constant
 
@@ -669,13 +670,17 @@ _Configuration_: N/A
 
 _Description_: This rule warns when [initialism](https://github.com/golang/go/wiki/CodeReviewComments#initialisms), [variable](https://github.com/golang/go/wiki/CodeReviewComments#variable-names) or [package](https://github.com/golang/go/wiki/CodeReviewComments#package-names) naming conventions are not followed.
 
-_Configuration_: This rule accepts two slices of strings, a whitelist and a blacklist of initialisms. 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))
+_Configuration_: This rule accepts two slices of strings and one optional slice with single map with named parameters.
+(it's due to TOML hasn't "slice of any" and we keep backward compatibility with previous config version)
+First slice is a whitelist and second one is a blacklist of initialisms. 
+In map, you can add "upperCaseConst=true" parameter to allow `UPPER_CASE` for `const` 
+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))
 
 Example:
 
 ```toml
 [rule.var-naming]
-  arguments = [["ID"], ["VM"]]
+  arguments = [["ID"], ["VM"], [{upperCaseConst=true}]]
 ```
 
 ## var-declaration
@@ -760,3 +765,9 @@ _Description_: Function parameters that are passed by value, are in fact a copy
 This rule warns when a `sync.WaitGroup` expected as a by-value parameter in a function or method.
 
 _Configuration_: N/A
+
+## redundant-import-alias
+
+_Description_: This rule warns on redundant import aliases. This happens when the alias used on the import statement matches the imported package name.
+
+_Configuration_: N/A
\ No newline at end of file
diff --git a/config/config.go b/config/config.go
index f6ee2a4..225a570 100644
--- a/config/config.go
+++ b/config/config.go
@@ -87,6 +87,7 @@ var allRules = append([]lint.Rule{
 	&rule.DataRaceRule{},
 	&rule.CommentSpacingsRule{},
 	&rule.IfReturnRule{},
+	&rule.RedundantImportAlias{},
 }, defaultRules...)
 
 var allFormatters = []lint.Formatter{
diff --git a/formatter/sarif.go b/formatter/sarif.go
index c6288db..c42da73 100644
--- a/formatter/sarif.go
+++ b/formatter/sarif.go
@@ -88,7 +88,7 @@ func (l *reviveRunLog) AddResult(failure lint.Failure) {
 	location := garif.NewLocation().WithURI(filename).WithLineColumn(line, column)
 	result.Locations = append(result.Locations, location)
 	result.RuleId = failure.RuleName
-	result.Level = l.rules[failure.RuleName].Severity
+	result.Level = garif.ResultLevel(l.rules[failure.RuleName].Severity)
 
 	l.run.Results = append(l.run.Results, result)
 }
diff --git a/go.mod b/go.mod
index 1bb14e9..87fce01 100644
--- a/go.mod
+++ b/go.mod
@@ -4,7 +4,7 @@ go 1.19
 
 require (
 	github.com/BurntSushi/toml v1.3.2
-	github.com/chavacava/garif v0.0.0-20230531072157-8144c224b063
+	github.com/chavacava/garif v0.0.0-20230608123814-4bd63c2919ab
 	github.com/fatih/color v1.15.0
 	github.com/fatih/structtag v1.2.0
 	github.com/mgechev/dots v0.0.0-20210922191527-e955255bf517
diff --git a/go.sum b/go.sum
index 5a26346..e05c9ea 100644
--- a/go.sum
+++ b/go.sum
@@ -14,6 +14,8 @@ github.com/chavacava/garif v0.0.0-20230519080132-4752330f72df h1:1uGdlpQT0irrGcF
 github.com/chavacava/garif v0.0.0-20230519080132-4752330f72df/go.mod h1:cFP7fAFavJ2DrYBmZYBETNKwSTFJiOIirm5N4/PqY/I=
 github.com/chavacava/garif v0.0.0-20230531072157-8144c224b063 h1:wiJ8AtgOw7gDs2I9uhrxW59fMPWXONSdpXU3/dRv0to=
 github.com/chavacava/garif v0.0.0-20230531072157-8144c224b063/go.mod h1:XMyYCkEL58DF0oyW4qDjjnPWONs2HBqYKI+UIPD+Gww=
+github.com/chavacava/garif v0.0.0-20230608123814-4bd63c2919ab h1:5JxePczlyGAtj6R1MUEFZ/UFud6FfsOejq7xLC2ZIb0=
+github.com/chavacava/garif v0.0.0-20230608123814-4bd63c2919ab/go.mod h1:XMyYCkEL58DF0oyW4qDjjnPWONs2HBqYKI+UIPD+Gww=
 github.com/davecgh/go-spew v1.1.0/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
 github.com/davecgh/go-spew v1.1.1 h1:vj9j/u1bqnvCEfJOwUhtlOARqs3+rkHYY13jYWTU97c=
 github.com/davecgh/go-spew v1.1.1/go.mod h1:J7Y8YcW2NihsgmVo/mv3lAwl/skON4iLHjSsI+c5H38=
diff --git a/rule/redundant-import-alias.go b/rule/redundant-import-alias.go
new file mode 100644
index 0000000..fa5281f
--- /dev/null
+++ b/rule/redundant-import-alias.go
@@ -0,0 +1,52 @@
+package rule
+
+import (
+	"fmt"
+	"go/ast"
+	"strings"
+
+	"github.com/mgechev/revive/lint"
+)
+
+// RedundantImportAlias lints given else constructs.
+type RedundantImportAlias struct{}
+
+// Apply applies the rule to given file.
+func (*RedundantImportAlias) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
+	var failures []lint.Failure
+
+	for _, imp := range file.AST.Imports {
+		if imp.Name == nil {
+			continue
+		}
+
+		if getImportPackageName(imp) == imp.Name.Name {
+			failures = append(failures, lint.Failure{
+				Confidence: 1,
+				Failure:    fmt.Sprintf("Import alias \"%s\" is redundant", imp.Name.Name),
+				Node:       imp,
+				Category:   "imports",
+			})
+		}
+	}
+
+	return failures
+}
+
+// Name returns the rule name.
+func (*RedundantImportAlias) Name() string {
+	return "redundant-import-alias"
+}
+
+func getImportPackageName(imp *ast.ImportSpec) string {
+	const pathSep = "/"
+	const strDelim = `"`
+
+	path := imp.Path.Value
+	i := strings.LastIndex(path, pathSep)
+	if i == -1 {
+		return strings.Trim(path, strDelim)
+	}
+
+	return strings.Trim(path[i+1:], strDelim)
+}
diff --git a/rule/string-format.go b/rule/string-format.go
index 0e30ebf..3edd624 100644
--- a/rule/string-format.go
+++ b/rule/string-format.go
@@ -211,10 +211,14 @@ func (lintStringFormatRule) getCallName(call *ast.CallExpr) (callName string, ok
 	if selector, ok := call.Fun.(*ast.SelectorExpr); ok {
 		// Scoped function call
 		scope, ok := selector.X.(*ast.Ident)
-		if !ok {
-			return "", false
+		if ok {
+			return scope.Name + "." + selector.Sel.Name, true
+		}
+		// Scoped function call inside structure
+		recv, ok := selector.X.(*ast.SelectorExpr)
+		if ok {
+			return recv.Sel.Name + "." + selector.Sel.Name, true
 		}
-		return scope.Name + "." + selector.Sel.Name, true
 	}
 
 	return "", false
diff --git a/rule/var-naming.go b/rule/var-naming.go
index fa4a188..2aa5ae2 100644
--- a/rule/var-naming.go
+++ b/rule/var-naming.go
@@ -13,11 +13,15 @@ import (
 
 var anyCapsRE = regexp.MustCompile(`[A-Z]`)
 
+// regexp for constant names like `SOME_CONST`, `SOME_CONST_2`, `X123_3` (#851)
+var upperCaseConstRE = regexp.MustCompile(`^[A-Z][A-Z\d]*(_[A-Z\d]+)*$`)
+
 // VarNamingRule lints given else constructs.
 type VarNamingRule struct {
-	configured bool
-	whitelist  []string
-	blacklist  []string
+	configured     bool
+	whitelist      []string
+	blacklist      []string
+	upperCaseConst bool // if true - allows to use UPPER_SOME_NAMES for constants
 	sync.Mutex
 }
 
@@ -31,6 +35,23 @@ func (r *VarNamingRule) configure(arguments lint.Arguments) {
 		if len(arguments) >= 2 {
 			r.blacklist = getList(arguments[1], "blacklist")
 		}
+
+		if len(arguments) >= 3 {
+			// not pretty code because should keep compatibility with TOML (no mixed array types) and new map parameters
+			thirdArgument := arguments[2]
+			asSlice, ok := thirdArgument.([]interface{})
+			if !ok {
+				panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, got %T", "options", arguments[2]))
+			}
+			if len(asSlice) != 1 {
+				panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, of len==1, but %d", "options", len(asSlice)))
+			}
+			args, ok := asSlice[0].(map[string]interface{})
+			if !ok {
+				panic(fmt.Sprintf("Invalid third argument to the var-naming rule. Expecting a %s of type slice, of len==1, with map, but %T", "options", asSlice[0]))
+			}
+			r.upperCaseConst = fmt.Sprint(args["upperCaseConst"]) == "true"
+		}
 		r.configured = true
 	}
 	r.Unlock()
@@ -52,6 +73,7 @@ func (r *VarNamingRule) Apply(file *lint.File, arguments lint.Arguments) []lint.
 		onFailure: func(failure lint.Failure) {
 			failures = append(failures, failure)
 		},
+		upperCaseConst: r.upperCaseConst,
 	}
 
 	// Package names need slightly different handling than other names.
@@ -82,18 +104,18 @@ func (*VarNamingRule) Name() string {
 	return "var-naming"
 }
 
-func checkList(fl *ast.FieldList, thing string, w *lintNames) {
+func (w *lintNames) checkList(fl *ast.FieldList, thing string) {
 	if fl == nil {
 		return
 	}
 	for _, f := range fl.List {
 		for _, id := range f.Names {
-			check(id, thing, w)
+			w.check(id, thing)
 		}
 	}
 }
 
-func check(id *ast.Ident, thing string, w *lintNames) {
+func (w *lintNames) check(id *ast.Ident, thing string) {
 	if id.Name == "_" {
 		return
 	}
@@ -101,6 +123,12 @@ func check(id *ast.Ident, thing string, w *lintNames) {
 		return
 	}
 
+	// #851 upperCaseConst support
+	// if it's const
+	if thing == token.CONST.String() && w.upperCaseConst && upperCaseConstRE.Match([]byte(id.Name)) {
+		return
+	}
+
 	// Handle two common styles from other languages that don't belong in Go.
 	if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") {
 		w.onFailure(lint.Failure{
@@ -144,11 +172,12 @@ func check(id *ast.Ident, thing string, w *lintNames) {
 }
 
 type lintNames struct {
-	file      *lint.File
-	fileAst   *ast.File
-	onFailure func(lint.Failure)
-	whitelist []string
-	blacklist []string
+	file           *lint.File
+	fileAst        *ast.File
+	onFailure      func(lint.Failure)
+	whitelist      []string
+	blacklist      []string
+	upperCaseConst bool
 }
 
 func (w *lintNames) Visit(n ast.Node) ast.Visitor {
@@ -159,7 +188,7 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor {
 		}
 		for _, exp := range v.Lhs {
 			if id, ok := exp.(*ast.Ident); ok {
-				check(id, "var", w)
+				w.check(id, "var")
 			}
 		}
 	case *ast.FuncDecl:
@@ -181,31 +210,24 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor {
 		// not exported in the Go API.
 		// See https://github.com/golang/lint/issues/144.
 		if ast.IsExported(v.Name.Name) || !isCgoExported(v) {
-			check(v.Name, thing, w)
+			w.check(v.Name, thing)
 		}
 
-		checkList(v.Type.Params, thing+" parameter", w)
-		checkList(v.Type.Results, thing+" result", w)
+		w.checkList(v.Type.Params, thing+" parameter")
+		w.checkList(v.Type.Results, thing+" result")
 	case *ast.GenDecl:
 		if v.Tok == token.IMPORT {
 			return w
 		}
-		var thing string
-		switch v.Tok {
-		case token.CONST:
-			thing = "const"
-		case token.TYPE:
-			thing = "type"
-		case token.VAR:
-			thing = "var"
-		}
+
+		thing := v.Tok.String()
 		for _, spec := range v.Specs {
 			switch s := spec.(type) {
 			case *ast.TypeSpec:
-				check(s.Name, thing, w)
+				w.check(s.Name, thing)
 			case *ast.ValueSpec:
 				for _, id := range s.Names {
-					check(id, thing, w)
+					w.check(id, thing)
 				}
 			}
 		}
@@ -217,23 +239,23 @@ func (w *lintNames) Visit(n ast.Node) ast.Visitor {
 			if !ok { // might be an embedded interface name
 				continue
 			}
-			checkList(ft.Params, "interface method parameter", w)
-			checkList(ft.Results, "interface method result", w)
+			w.checkList(ft.Params, "interface method parameter")
+			w.checkList(ft.Results, "interface method result")
 		}
 	case *ast.RangeStmt:
 		if v.Tok == token.ASSIGN {
 			return w
 		}
 		if id, ok := v.Key.(*ast.Ident); ok {
-			check(id, "range var", w)
+			w.check(id, "range var")
 		}
 		if id, ok := v.Value.(*ast.Ident); ok {
-			check(id, "range var", w)
+			w.check(id, "range var")
 		}
 	case *ast.StructType:
 		for _, f := range v.Fields.List {
 			for _, id := range f.Names {
-				check(id, "struct field", w)
+				w.check(id, "struct field")
 			}
 		}
 	}


Test patch:
diff --git a/test/redundant-import-alias_test.go b/test/redundant-import-alias_test.go
new file mode 100644
index 0000000..e7bff5f
--- /dev/null
+++ b/test/redundant-import-alias_test.go
@@ -0,0 +1,11 @@
+package test
+
+import (
+	"github.com/mgechev/revive/rule"
+	"testing"
+)
+
+// TestRedundantImportAlias rule.
+func TestRedundantImportAlias(t *testing.T) {
+	testRule(t, "redundant-import-alias", &rule.RedundantImportAlias{})
+}
diff --git a/test/string-format_test.go b/test/string-format_test.go
index f58c10c..1d425c6 100644
--- a/test/string-format_test.go
+++ b/test/string-format_test.go
@@ -21,7 +21,11 @@ func TestStringFormat(t *testing.T) {
 			[]interface{}{
 				"s.Method3[2]",
 				"!/^[Tt][Hh]/",
-				"must not start with 'th'"}}})
+				"must not start with 'th'"},
+			[]interface{}{
+				"s.Method4", // same as before, but called from a struct
+				"!/^[Ot][Tt]/",
+				"must not start with 'ot'"}}})
 }
 
 func TestStringFormatArgumentParsing(t *testing.T) {
diff --git a/test/var-naming_test.go b/test/var-naming_test.go
index b6ccd41..56108b0 100644
--- a/test/var-naming_test.go
+++ b/test/var-naming_test.go
@@ -13,4 +13,9 @@ func TestVarNaming(t *testing.T) {
 	})
 
 	testRule(t, "var-naming_test", &rule.VarNamingRule{}, &lint.RuleConfig{})
+
+	testRule(t, "var-naming_upperCaseConst-false", &rule.VarNamingRule{}, &lint.RuleConfig{})
+	testRule(t, "var-naming_upperCaseConst-true", &rule.VarNamingRule{}, &lint.RuleConfig{
+		Arguments: []interface{}{[]interface{}{}, []interface{}{}, []interface{}{map[string]interface{}{"upperCaseConst": true}}},
+	})
 }
diff --git a/testdata/redundant-import-alias.go b/testdata/redundant-import-alias.go
new file mode 100644
index 0000000..555bca2
--- /dev/null
+++ b/testdata/redundant-import-alias.go
@@ -0,0 +1,12 @@
+package fixtures
+
+import(
+	"crypto/md5"
+		"strings"
+		_ "crypto/md5"
+		str "strings"
+		strings "strings"  // MATCH /Import alias "strings" is redundant/
+		crypto "crypto/md5"
+		md5 "crypto/md5"  // MATCH /Import alias "md5" is redundant/
+)
+
diff --git a/testdata/string-format.go b/testdata/string-format.go
index 69c3204..08e0fa2 100644
--- a/testdata/string-format.go
+++ b/testdata/string-format.go
@@ -18,6 +18,16 @@ func (s stringFormatMethods) Method3(a, b, c string) {
 
 }
 
+type stringFormatMethodsInjected struct{}
+
+func (s stringFormatMethodsInjected) Method4(a, b, c string) {
+
+}
+
+type container struct {
+	s stringFormatMethodsInjected
+}
+
 func stringFormat() {
 	stringFormatMethod1("This string is fine", "")
 	stringFormatMethod1("this string is not capitalized", "") // MATCH /must start with a capital letter/
@@ -27,4 +37,9 @@ func stringFormat() {
 		d: "This string is capitalized, but ends with a period."}) // MATCH /string literal doesn't match user defined regex /[^\.]$//
 	s := stringFormatMethods{}
 	s.Method3("", "", "This string starts with th") // MATCH /must not start with 'th'/
+
+	c := container{
+		s: stringFormatMethods{},
+	}
+	c.s.Method4("Other string starts with ot") // MATCH /must not start with 'ot'/
 }
diff --git a/testdata/var-naming_upperCaseConst-false.go b/testdata/var-naming_upperCaseConst-false.go
new file mode 100644
index 0000000..3bb628e
--- /dev/null
+++ b/testdata/var-naming_upperCaseConst-false.go
@@ -0,0 +1,9 @@
+// should fail if upperCaseConst = false (by default) #851
+
+package fixtures
+
+const SOME_CONST_2 = 1 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/
+
+const (
+	SOME_CONST_3 = 3 // MATCH /don't use ALL_CAPS in Go names; use CamelCase/
+)
diff --git a/testdata/var-naming_upperCaseConst-true.go b/testdata/var-naming_upperCaseConst-true.go
new file mode 100644
index 0000000..272b708
--- /dev/null
+++ b/testdata/var-naming_upperCaseConst-true.go
@@ -0,0 +1,10 @@
+// should pass if upperCaseConst = true
+
+package fixtures
+
+const SOME_CONST_2 = 2
+
+const (
+	SOME_CONST_3 = 3
+	VER          = 0
+)