Add multiple scopes support to string-format rule (#1009)

* Add multiple scopes support to string-format rule

* Add new parsing rule

* Fix example

Author: KirillSluchaev

RULES_DESCRIPTIONS.md

@@ -759,7 +759,7 @@ This is geared towards user facing applications where string literals are often
 
 _Configuration_: Each argument is a slice containing 2-3 strings: a scope, a regex, and an optional error message.
 
-1. The first string defines a **scope**. This controls which string literals the regex will apply to, and is defined as a function argument. It must contain at least a function name (`core.WriteError`). Scopes may optionally contain a number specifying which argument in the function to check (`core.WriteError[1]`), as well as a struct field (`core.WriteError[1].Message`, only works for top level fields). Function arguments are counted starting at 0, so `[0]` would refer to the first argument, `[1]` would refer to the second, etc. If no argument number is provided, the first argument will be used (same as `[0]`).
+1. The first string defines a **scope**. This controls which string literals the regex will apply to, and is defined as a function argument. It must contain at least a function name (`core.WriteError`). Scopes may optionally contain a number specifying which argument in the function to check (`core.WriteError[1]`), as well as a struct field (`core.WriteError[1].Message`, only works for top level fields). Function arguments are counted starting at 0, so `[0]` would refer to the first argument, `[1]` would refer to the second, etc. If no argument number is provided, the first argument will be used (same as `[0]`). You can use multiple scopes to one regex. Split them by `,` (`core.WriteError,fmt.Errorf`). 
 
 2. The second string is a **regular expression** (beginning and ending with a `/` character), which will be used to check the string literals in the scope. The default semantics is "_strings matching the regular expression are OK_". If you need to inverse the semantics you can add a `!` just before the first `/`. Examples:
 
@@ -776,6 +776,7 @@ Example:
     ["core.WriteError[1].Message", "/^([^A-Z]|$)/", "must not start with a capital letter"],
     ["fmt.Errorf[0]", "/(^|[^\\.!?])$/", "must not end in punctuation"],
     ["panic", "/^[^\\n]*$/", "must not contain line breaks"],
+    ["fmt.Errorf[0],core.WriteError[1].Message", "!/^.*%w.*$/", "must not contain '%w'"],
   ]
 ```
 

rule/string-format.go

@@ -6,6 +6,7 @@ import (
 	"go/token"
 	"regexp"
 	"strconv"
+	"strings"
 
 	"github.com/mgechev/revive/lint"
 )
@@ -66,12 +67,14 @@ type lintStringFormatRule struct {
 
 type stringFormatSubrule struct {
 	parent       *lintStringFormatRule
-	scope        stringFormatSubruleScope
+	scopes       stringFormatSubruleScopes
 	regexp       *regexp.Regexp
 	negated      bool
 	errorMessage string
 }
 
+type stringFormatSubruleScopes []*stringFormatSubruleScope
+
 type stringFormatSubruleScope struct {
 	funcName string // Function name the rule is scoped to
 	argument int    // (optional) Which argument in calls to the function is checked against the rule (the first argument is checked by default)
@@ -90,18 +93,18 @@ var parseStringFormatScope = regexp.MustCompile(
 
 func (w *lintStringFormatRule) parseArguments(arguments lint.Arguments) {
 	for i, argument := range arguments {
-		scope, regex, negated, errorMessage := w.parseArgument(argument, i)
+		scopes, regex, negated, errorMessage := w.parseArgument(argument, i)
 		w.rules = append(w.rules, stringFormatSubrule{
 			parent:       w,
-			scope:        scope,
+			scopes:       scopes,
 			regexp:       regex,
 			negated:      negated,
 			errorMessage: errorMessage,
 		})
 	}
 }
 
-func (w lintStringFormatRule) parseArgument(argument any, ruleNum int) (scope stringFormatSubruleScope, regex *regexp.Regexp, negated bool, errorMessage string) {
+func (w lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes stringFormatSubruleScopes, regex *regexp.Regexp, negated bool, errorMessage string) {
 	g, ok := argument.([]any) // Cast to generic slice first
 	if !ok {
 		w.configError("argument is not a slice", ruleNum, 0)
@@ -125,26 +128,39 @@ func (w lintStringFormatRule) parseArgument(argument any, ruleNum int) (scope st
 		w.configError("regex is too small (regexes should begin and end with '/')", ruleNum, 1)
 	}
 
-	// Parse rule scope
-	scope = stringFormatSubruleScope{}
-	matches := parseStringFormatScope.FindStringSubmatch(rule[0])
-	if matches == nil {
-		// The rule's scope didn't match the parsing regex at all, probably a configuration error
-		w.parseError("unable to parse rule scope", ruleNum, 0)
-	} else if len(matches) != 4 {
-		// The rule's scope matched the parsing regex, but an unexpected number of submatches was returned, probably a bug
-		w.parseError(fmt.Sprintf("unexpected number of submatches when parsing scope: %d, expected 4", len(matches)), ruleNum, 0)
-	}
-	scope.funcName = matches[1]
-	if len(matches[2]) > 0 {
-		var err error
-		scope.argument, err = strconv.Atoi(matches[2])
-		if err != nil {
-			w.parseError("unable to parse argument number in rule scope", ruleNum, 0)
+	// Parse rule scopes
+	rawScopes := strings.Split(rule[0], ",")
+
+	scopes = make([]*stringFormatSubruleScope, 0, len(rawScopes))
+	for scopeNum, rawScope := range rawScopes {
+		rawScope = strings.TrimSpace(rawScope)
+
+		if len(rawScope) == 0 {
+			w.parseScopeError("empty scope in rule scopes:", ruleNum, 0, scopeNum)
 		}
-	}
-	if len(matches[3]) > 0 {
-		scope.field = matches[3]
+
+		scope := stringFormatSubruleScope{}
+		matches := parseStringFormatScope.FindStringSubmatch(rawScope)
+		if matches == nil {
+			// The rule's scope didn't match the parsing regex at all, probably a configuration error
+			w.parseScopeError("unable to parse rule scope", ruleNum, 0, scopeNum)
+		} else if len(matches) != 4 {
+			// The rule's scope matched the parsing regex, but an unexpected number of submatches was returned, probably a bug
+			w.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 {
+			var err error
+			scope.argument, err = strconv.Atoi(matches[2])
+			if err != nil {
+				w.parseScopeError("unable to parse argument number in rule scope", ruleNum, 0, scopeNum)
+			}
+		}
+		if len(matches[3]) > 0 {
+			scope.field = matches[3]
+		}
+
+		scopes = append(scopes, &scope)
 	}
 
 	// Strip / characters from the beginning and end of rule[1] before compiling
@@ -162,7 +178,7 @@ func (w lintStringFormatRule) parseArgument(argument any, ruleNum int) (scope st
 	if len(rule) == 3 {
 		errorMessage = rule[2]
 	}
-	return scope, regex, negated, errorMessage
+	return scopes, regex, negated, errorMessage
 }
 
 // Report an invalid config, this is specifically the user's fault
@@ -175,6 +191,11 @@ func (lintStringFormatRule) parseError(msg string, ruleNum, option int) {
 	panic(fmt.Sprintf("failed to parse configuration for string-format: %s [argument %d, option %d]", msg, ruleNum, option))
 }
 
+// Report a general scope config parsing failure, this may be the user's fault, but it isn't known for certain
+func (lintStringFormatRule) parseScopeError(msg string, ruleNum, option, scopeNum int) {
+	panic(fmt.Sprintf("failed to parse configuration for string-format: %s [argument %d, option %d, scope index %d]", msg, ruleNum, option, scopeNum))
+}
+
 // #endregion
 
 // #region Node traversal
@@ -193,8 +214,10 @@ func (w lintStringFormatRule) Visit(node ast.Node) ast.Visitor {
 	}
 
 	for _, rule := range w.rules {
-		if rule.scope.funcName == callName {
-			rule.Apply(call)
+		for _, scope := range rule.scopes {
+			if scope.funcName == callName {
+				rule.Apply(call)
+			}
 		}
 	}
 
@@ -230,45 +253,47 @@ func (lintStringFormatRule) getCallName(call *ast.CallExpr) (callName string, ok
 
 // Apply a single format rule to a call expression (should be done after verifying the that the call expression matches the rule's scope)
 func (r *stringFormatSubrule) Apply(call *ast.CallExpr) {
-	if len(call.Args) <= r.scope.argument {
-		return
-	}
-
-	arg := call.Args[r.scope.argument]
-	var lit *ast.BasicLit
-	if len(r.scope.field) > 0 {
-		// Try finding the scope's Field, treating arg as a composite literal
-		composite, ok := arg.(*ast.CompositeLit)
-		if !ok {
+	for _, scope := range r.scopes {
+		if len(call.Args) <= scope.argument {
 			return
 		}
-		for _, el := range composite.Elts {
-			kv, ok := el.(*ast.KeyValueExpr)
+
+		arg := call.Args[scope.argument]
+		var lit *ast.BasicLit
+		if len(scope.field) > 0 {
+			// Try finding the scope's Field, treating arg as a composite literal
+			composite, ok := arg.(*ast.CompositeLit)
 			if !ok {
-				continue
+				return
 			}
-			key, ok := kv.Key.(*ast.Ident)
-			if !ok || key.Name != r.scope.field {
-				continue
+			for _, el := range composite.Elts {
+				kv, ok := el.(*ast.KeyValueExpr)
+				if !ok {
+					continue
+				}
+				key, ok := kv.Key.(*ast.Ident)
+				if !ok || key.Name != scope.field {
+					continue
+				}
+
+				// We're now dealing with the exact field in the rule's scope, so if anything fails, we can safely return instead of continuing the loop
+				lit, ok = kv.Value.(*ast.BasicLit)
+				if !ok || lit.Kind != token.STRING {
+					return
+				}
 			}
-
-			// We're now dealing with the exact field in the rule's scope, so if anything fails, we can safely return instead of continuing the loop
-			lit, ok = kv.Value.(*ast.BasicLit)
+		} else {
+			var ok bool
+			// Treat arg as a string literal
+			lit, ok = arg.(*ast.BasicLit)
 			if !ok || lit.Kind != token.STRING {
 				return
 			}
 		}
-	} else {
-		var ok bool
-		// Treat arg as a string literal
-		lit, ok = arg.(*ast.BasicLit)
-		if !ok || lit.Kind != token.STRING {
-			return
-		}
+		// Unquote the string literal before linting
+		unquoted := lit.Value[1 : len(lit.Value)-1]
+		r.lintMessage(unquoted, lit)
 	}
-	// Unquote the string literal before linting
-	unquoted := lit.Value[1 : len(lit.Value)-1]
-	r.lintMessage(unquoted, lit)
 }
 
 func (r *stringFormatSubrule) lintMessage(s string, node ast.Node) {

test/string-format_test.go

@@ -76,7 +76,7 @@ func TestStringFormatArgumentParsing(t *testing.T) {
 				[]any{
 					"1.a",
 					"//"}},
-			expectedError: stringPtr("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0]")},
+			expectedError: stringPtr("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 0]")},
 		{
 			name: "Bad Regex",
 			config: lint.Arguments{
@@ -98,7 +98,65 @@ func TestStringFormatArgumentParsing(t *testing.T) {
 			config: lint.Arguments{
 				[]any{
 					"some_pkg._some_function_name[5].some_member",
-					"//"}}}}
+					"//"}}},
+		{
+			name: "Underscores in Multiple Scopes",
+			config: lint.Arguments{
+				[]any{
+					"fmt.Errorf[0],core.WriteError[1].Message",
+					"//"}}},
+		{
+			name: "', ' Delimiter",
+			config: lint.Arguments{
+				[]any{
+					"abc, mt.Errorf",
+					"//"}}},
+		{
+			name: "' ,' Delimiter",
+			config: lint.Arguments{
+				[]any{
+					"abc ,mt.Errorf",
+					"//"}}},
+		{
+			name: "',   ' Delimiter",
+			config: lint.Arguments{
+				[]any{
+					"abc,   mt.Errorf",
+					"//"}}},
+		{
+			name: "',   ' Delimiter",
+			config: lint.Arguments{
+				[]any{
+					"abc,   mt.Errorf",
+					"//"}}},
+		{
+			name: "Empty Middle Scope",
+			config: lint.Arguments{
+				[]any{
+					"abc, ,mt.Errorf",
+					"//"}},
+			expectedError: stringPtr("failed to parse configuration for string-format: empty scope in rule scopes: [argument 0, option 0, scope index 1]")},
+		{
+			name: "Empty First Scope",
+			config: lint.Arguments{
+				[]any{
+					",mt.Errorf",
+					"//"}},
+			expectedError: stringPtr("failed to parse configuration for string-format: empty scope in rule scopes: [argument 0, option 0, scope index 0]")},
+		{
+			name: "Bad First Scope",
+			config: lint.Arguments{
+				[]any{
+					"1.a,fmt.Errorf[0]",
+					"//"}},
+			expectedError: stringPtr("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 0]")},
+		{
+			name: "Bad Second Scope",
+			config: lint.Arguments{
+				[]any{
+					"fmt.Errorf[0],1.a",
+					"//"}},
+			expectedError: stringPtr("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 1]")}}
 
 	for _, a := range tests {
 		t.Run(a.name, func(t *testing.T) {