Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add multiple scopes support to string-format rule #1009

Merged
merged 3 commits into from
Aug 9, 2024
Merged

Add multiple scopes support to string-format rule #1009

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
8392f32
Add multiple scopes support to string-format rule
KirillSluchaev Jul 10, 2024
96a1700
Add new parsing rule
KirillSluchaev Jul 14, 2024
291e78a
Fix example
KirillSluchaev Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion RULES_DESCRIPTIONS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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:

Expand All @@ -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 contain '%w'"],
]
```

Expand Down
135 changes: 80 additions & 55 deletions rule/string-format.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import (
"go/token"
"regexp"
"strconv"
"strings"

"github.com/mgechev/revive/lint"
)
Expand Down Expand Up @@ -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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

out of curiosity: why is that a slice of pointers? (not a slice of values)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw one video on youtube where this two approaches were compared and slice of pointers use less memory little bit)


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)
Expand All @@ -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)
Expand All @@ -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], ",")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm a bit worried about the fact people may use , , or ,, or , as delimiter.

So maybe, you could use strings.TrimSpace on the rawScope in the for loop

Also, xhecking that the rawScope is not empty could avoid surprises and bugs

I'm pretty sure someone would face a

foo,,bar, or foo , , bar

No matter the implementation choice to either clean them, or forbid them. It should be covered by a test


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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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)
}
}
}

Expand Down Expand Up @@ -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) {
Expand Down
62 changes: 60 additions & 2 deletions test/string-format_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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) {
Expand Down
Loading