From 8392f3261286d21166672580ffd7996ff7fefa6b Mon Sep 17 00:00:00 2001 From: Sluchaev Kirill Date: Wed, 10 Jul 2024 18:11:50 +0300 Subject: [PATCH 1/3] Add multiple scopes support to string-format rule --- RULES_DESCRIPTIONS.md | 2 +- rule/string-format.go | 129 +++++++++++++++++++++---------------- test/string-format_test.go | 24 ++++++- 3 files changed, 97 insertions(+), 58 deletions(-) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index b0fa9fd28..3cae6d2e8 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -758,7 +758,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: diff --git a/rule/string-format.go b/rule/string-format.go index 70edf7387..831a8ea6b 100644 --- a/rule/string-format.go +++ b/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,10 +93,10 @@ 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, @@ -101,7 +104,7 @@ func (w *lintStringFormatRule) parseArguments(arguments lint.Arguments) { } } -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,33 @@ 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 { + 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) } - } - if len(matches[3]) > 0 { - scope.field = matches[3] + 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 +172,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 +185,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 +208,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 +247,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) { diff --git a/test/string-format_test.go b/test/string-format_test.go index d10329d41..f7f131aaa 100644 --- a/test/string-format_test.go +++ b/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,27 @@ 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: "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) { From 96a17003c204849cebeb99a30e39cb1e51bca278 Mon Sep 17 00:00:00 2001 From: Sluchaev Kirill Date: Sun, 14 Jul 2024 21:44:34 +0300 Subject: [PATCH 2/3] Add new parsing rule --- RULES_DESCRIPTIONS.md | 1 + rule/string-format.go | 6 ++++++ test/string-format_test.go | 38 ++++++++++++++++++++++++++++++++++++++ 3 files changed, 45 insertions(+) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 3cae6d2e8..4ddb38d04 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -775,6 +775,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 contains '%w'"], ] ``` diff --git a/rule/string-format.go b/rule/string-format.go index 831a8ea6b..62283cbb2 100644 --- a/rule/string-format.go +++ b/rule/string-format.go @@ -133,6 +133,12 @@ func (w lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes s 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) + } + scope := stringFormatSubruleScope{} matches := parseStringFormatScope.FindStringSubmatch(rawScope) if matches == nil { diff --git a/test/string-format_test.go b/test/string-format_test.go index f7f131aaa..5135ff00b 100644 --- a/test/string-format_test.go +++ b/test/string-format_test.go @@ -105,6 +105,44 @@ func TestStringFormatArgumentParsing(t *testing.T) { []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{ From 291e78a13c2cfc72272ea5f038be8d9f6b6ada57 Mon Sep 17 00:00:00 2001 From: Sluchaev Kirill Date: Mon, 5 Aug 2024 20:10:04 +0300 Subject: [PATCH 3/3] Fix example --- RULES_DESCRIPTIONS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 4ddb38d04..86a107ec8 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -775,7 +775,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 contains '%w'"], + ["fmt.Errorf[0],core.WriteError[1].Message", "!/^.*%w.*$/", "must not contain '%w'"], ] ```