diff --git a/rule/string_format.go b/rule/string_format.go index a3beac4..d539577 100644 --- a/rule/string_format.go +++ b/rule/string_format.go @@ -12,20 +12,24 @@ import ( ) // StringFormatRule lints strings and/or comments according to a set of regular expressions given as Arguments -type StringFormatRule struct{} +type StringFormatRule struct { + rules []stringFormatSubrule +} // Apply applies the rule to the given file. -func (*StringFormatRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { +func (r *StringFormatRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { var failures []lint.Failure onFailure := func(failure lint.Failure) { failures = append(failures, failure) } - w := &lintStringFormatRule{onFailure: onFailure} - err := w.parseArguments(arguments) - if err != nil { - return newInternalFailureError(err) + for i := range r.rules { + r.rules[i].onFailure = onFailure + } + + w := &lintStringFormatRule{ + rules: r.rules, } ast.Walk(w, file.AST) @@ -38,32 +42,31 @@ func (*StringFormatRule) Name() string { return "string-format" } -// ParseArgumentsTest is a public wrapper around w.parseArguments used for testing. Returns the error message provided to panic, or nil if no error was encountered -func (*StringFormatRule) ParseArgumentsTest(arguments lint.Arguments) *string { - w := lintStringFormatRule{} - c := make(chan any) - // Parse the arguments in a goroutine, defer a recover() call, return the error encountered (or nil if there was no error) - go func() { - defer func() { - err := w.parseArguments(arguments) - c <- err - }() - }() - err := <-c - if err != nil { - e := fmt.Sprintf("%s", err) - return &e +// Configure validates the rule configuration, and configures the rule accordingly. +// +// Configuration implements the [lint.ConfigurableRule] interface. +func (r *StringFormatRule) Configure(arguments lint.Arguments) error { + for i, argument := range arguments { + scopes, regex, negated, errorMessage, err := r.parseArgument(argument, i) + if err != nil { + return err + } + r.rules = append(r.rules, stringFormatSubrule{ + scopes: scopes, + regexp: regex, + negated: negated, + errorMessage: errorMessage, + }) } return nil } type lintStringFormatRule struct { - onFailure func(lint.Failure) - rules []stringFormatSubrule + rules []stringFormatSubrule } type stringFormatSubrule struct { - parent *lintStringFormatRule + onFailure func(lint.Failure) scopes stringFormatSubruleScopes regexp *regexp.Regexp negated bool @@ -84,45 +87,28 @@ const identRegex = "[_A-Za-z][_A-Za-z0-9]*" var parseStringFormatScope = regexp.MustCompile( fmt.Sprintf("^(%s(?:\\.%s)?)(?:\\[([0-9]+)\\](?:\\.(%s))?)?$", identRegex, identRegex, identRegex)) -func (w *lintStringFormatRule) parseArguments(arguments lint.Arguments) error { - for i, argument := range arguments { - scopes, regex, negated, errorMessage, err := w.parseArgument(argument, i) - if err != nil { - return err - } - w.rules = append(w.rules, stringFormatSubrule{ - parent: w, - scopes: scopes, - regexp: regex, - negated: negated, - errorMessage: errorMessage, - }) - } - return nil -} - -func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes stringFormatSubruleScopes, regex *regexp.Regexp, negated bool, errorMessage string, err error) { +func (r *StringFormatRule) parseArgument(argument any, ruleNum int) (scopes stringFormatSubruleScopes, regex *regexp.Regexp, negated bool, errorMessage string, err error) { g, ok := argument.([]any) // Cast to generic slice first if !ok { - return stringFormatSubruleScopes{}, regex, false, "", w.configError("argument is not a slice", ruleNum, 0) + return stringFormatSubruleScopes{}, regex, false, "", r.configError("argument is not a slice", ruleNum, 0) } if len(g) < 2 { - return stringFormatSubruleScopes{}, regex, false, "", w.configError("less than two slices found in argument, scope and regex are required", ruleNum, len(g)-1) + return stringFormatSubruleScopes{}, regex, false, "", r.configError("less than two slices found in argument, scope and regex are required", ruleNum, len(g)-1) } rule := make([]string, len(g)) for i, obj := range g { val, ok := obj.(string) if !ok { - return stringFormatSubruleScopes{}, regex, false, "", w.configError("unexpected value, string was expected", ruleNum, i) + return stringFormatSubruleScopes{}, regex, false, "", r.configError("unexpected value, string was expected", ruleNum, i) } rule[i] = val } // Validate scope and regex length if rule[0] == "" { - return stringFormatSubruleScopes{}, regex, false, "", w.configError("empty scope provided", ruleNum, 0) + return stringFormatSubruleScopes{}, regex, false, "", r.configError("empty scope provided", ruleNum, 0) } else if len(rule[1]) < 2 { - return stringFormatSubruleScopes{}, regex, false, "", w.configError("regex is too small (regexes should begin and end with '/')", ruleNum, 1) + return stringFormatSubruleScopes{}, regex, false, "", r.configError("regex is too small (regexes should begin and end with '/')", ruleNum, 1) } // Parse rule scopes @@ -133,25 +119,25 @@ func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes rawScope = strings.TrimSpace(rawScope) if len(rawScope) == 0 { - return stringFormatSubruleScopes{}, regex, false, "", w.parseScopeError("empty scope in rule scopes:", ruleNum, 0, scopeNum) + return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("empty scope in rule scopes:", ruleNum, 0, scopeNum) } 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 - return stringFormatSubruleScopes{}, regex, false, "", w.parseScopeError("unable to parse rule scope", ruleNum, 0, scopeNum) + return stringFormatSubruleScopes{}, regex, false, "", r.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 return stringFormatSubruleScopes{}, regex, false, "", - w.parseScopeError(fmt.Sprintf("unexpected number of submatches when parsing scope: %d, expected 4", len(matches)), ruleNum, 0, scopeNum) + r.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 { - return stringFormatSubruleScopes{}, regex, false, "", w.parseScopeError("unable to parse argument number in rule scope", ruleNum, 0, scopeNum) + return stringFormatSubruleScopes{}, regex, false, "", r.parseScopeError("unable to parse argument number in rule scope", ruleNum, 0, scopeNum) } } if len(matches[3]) > 0 { @@ -169,7 +155,7 @@ func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes } regex, errr := regexp.Compile(rule[1][offset : len(rule[1])-1]) if errr != nil { - return stringFormatSubruleScopes{}, regex, false, "", w.parseError(fmt.Sprintf("unable to compile %s as regexp", rule[1]), ruleNum, 1) + return stringFormatSubruleScopes{}, regex, false, "", r.parseError(fmt.Sprintf("unable to compile %s as regexp", rule[1]), ruleNum, 1) } // Use custom error message if provided @@ -180,17 +166,17 @@ func (w *lintStringFormatRule) parseArgument(argument any, ruleNum int) (scopes } // Report an invalid config, this is specifically the user's fault -func (*lintStringFormatRule) configError(msg string, ruleNum, option int) error { +func (*StringFormatRule) configError(msg string, ruleNum, option int) error { return fmt.Errorf("invalid configuration for string-format: %s [argument %d, option %d]", msg, ruleNum, option) } // Report a general config parsing failure, this may be the user's fault, but it isn't known for certain -func (*lintStringFormatRule) parseError(msg string, ruleNum, option int) error { +func (*StringFormatRule) parseError(msg string, ruleNum, option int) error { return fmt.Errorf("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) error { +func (*StringFormatRule) parseScopeError(msg string, ruleNum, option, scopeNum int) error { return fmt.Errorf("failed to parse configuration for string-format: %s [argument %d, option %d, scope index %d]", msg, ruleNum, option, scopeNum) } @@ -314,7 +300,7 @@ func (r *stringFormatSubrule) generateFailure(node ast.Node) { failure = fmt.Sprintf("string literal doesn't match user defined regex /%s/", r.regexp.String()) } - r.parent.onFailure(lint.Failure{ + r.onFailure(lint.Failure{ Confidence: 1, Failure: failure, Node: node, diff --git a/rule/string_format_test.go b/rule/string_format_test.go new file mode 100644 index 0000000..ae904d4 --- /dev/null +++ b/rule/string_format_test.go @@ -0,0 +1,154 @@ +package rule_test + +import ( + "errors" + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestStringFormatConfigure(t *testing.T) { + type argumentsTest struct { + name string + arguments lint.Arguments + wantErr error + } + tests := []argumentsTest{ + { + name: "Not a Slice", + arguments: lint.Arguments{ + "this is not a slice"}, + wantErr: errors.New("invalid configuration for string-format: argument is not a slice [argument 0, option 0]")}, + { + name: "Missing Regex", + arguments: lint.Arguments{ + []any{ + "method[0]"}}, + wantErr: errors.New("invalid configuration for string-format: less than two slices found in argument, scope and regex are required [argument 0, option 0]")}, + { + name: "Bad Argument Type", + arguments: lint.Arguments{ + []any{ + 1}}, + wantErr: errors.New("invalid configuration for string-format: less than two slices found in argument, scope and regex are required [argument 0, option 0]")}, + { + name: "Empty Scope", + arguments: lint.Arguments{ + []any{ + "", + "//"}}, + wantErr: errors.New("invalid configuration for string-format: empty scope provided [argument 0, option 0]")}, + { + name: "Small or Empty Regex", + arguments: lint.Arguments{ + []any{ + "method[1].a", + "-"}}, + wantErr: errors.New("invalid configuration for string-format: regex is too small (regexes should begin and end with '/') [argument 0, option 1]")}, + { + name: "Bad Scope", + arguments: lint.Arguments{ + []any{ + "1.a", + "//"}}, + wantErr: errors.New("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 0]")}, + { + name: "Bad Regex", + arguments: lint.Arguments{ + []any{ + "method[1].a", + "/(/"}}, + wantErr: errors.New("failed to parse configuration for string-format: unable to compile /(/ as regexp [argument 0, option 1]")}, + { + name: "Sample Config", + arguments: lint.Arguments{ + []any{ + "core.WriteError[1].Message", "/^([^A-Z]$)/", "must not start with a capital letter"}, + []any{ + "fmt.Errorf[0]", "/^|[^\\.!?]$/", "must not end in punctuation"}, + []any{ + "panic", "/^[^\\n]*$/", "must not contain line breaks"}}}, + { + name: "Underscores in Scope", + arguments: lint.Arguments{ + []any{ + "some_pkg._some_function_name[5].some_member", + "//"}}}, + { + name: "Underscores in Multiple Scopes", + arguments: lint.Arguments{ + []any{ + "fmt.Errorf[0],core.WriteError[1].Message", + "//"}}}, + { + name: "', ' Delimiter", + arguments: lint.Arguments{ + []any{ + "abc, mt.Errorf", + "//"}}}, + { + name: "' ,' Delimiter", + arguments: lint.Arguments{ + []any{ + "abc ,mt.Errorf", + "//"}}}, + { + name: "', ' Delimiter", + arguments: lint.Arguments{ + []any{ + "abc, mt.Errorf", + "//"}}}, + { + name: "', ' Delimiter", + arguments: lint.Arguments{ + []any{ + "abc, mt.Errorf", + "//"}}}, + { + name: "Empty Middle Scope", + arguments: lint.Arguments{ + []any{ + "abc, ,mt.Errorf", + "//"}}, + wantErr: errors.New("failed to parse configuration for string-format: empty scope in rule scopes: [argument 0, option 0, scope index 1]")}, + { + name: "Empty First Scope", + arguments: lint.Arguments{ + []any{ + ",mt.Errorf", + "//"}}, + wantErr: errors.New("failed to parse configuration for string-format: empty scope in rule scopes: [argument 0, option 0, scope index 0]")}, + { + name: "Bad First Scope", + arguments: lint.Arguments{ + []any{ + "1.a,fmt.Errorf[0]", + "//"}}, + wantErr: errors.New("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 0]")}, + { + name: "Bad Second Scope", + arguments: lint.Arguments{ + []any{ + "fmt.Errorf[0],1.a", + "//"}}, + wantErr: errors.New("failed to parse configuration for string-format: unable to parse rule scope [argument 0, option 0, scope index 1]")}} + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var r rule.StringFormatRule + + err := r.Configure(tt.arguments) + + if tt.wantErr == nil { + if err != nil { + t.Errorf("Configure() unexpected non-nil error %q", err) + } + return + } + if err == nil || err.Error() != tt.wantErr.Error() { + t.Errorf("Configure() unexpected error: got %q, want %q", err, tt.wantErr) + } + }) + } +} diff --git a/test/string_format_test.go b/test/string_format_test.go index f83a99b..2ef4e57 100644 --- a/test/string_format_test.go +++ b/test/string_format_test.go @@ -28,150 +28,6 @@ func TestStringFormat(t *testing.T) { "must not start with 'ot'"}}}) } -func TestStringFormatArgumentParsing(t *testing.T) { - r := &rule.StringFormatRule{} - type argumentsTest struct { - name string - config lint.Arguments - expectedError *string - } - stringPtr := func(s string) *string { - return &s - } - tests := []argumentsTest{ - { - name: "Not a Slice", - config: lint.Arguments{ - "this is not a slice"}, - expectedError: stringPtr("invalid configuration for string-format: argument is not a slice [argument 0, option 0]")}, - { - name: "Missing Regex", - config: lint.Arguments{ - []any{ - "method[0]"}}, - expectedError: stringPtr("invalid configuration for string-format: less than two slices found in argument, scope and regex are required [argument 0, option 0]")}, - { - name: "Bad Argument Type", - config: lint.Arguments{ - []any{ - 1}}, - expectedError: stringPtr("invalid configuration for string-format: less than two slices found in argument, scope and regex are required [argument 0, option 0]")}, - { - name: "Empty Scope", - config: lint.Arguments{ - []any{ - "", - "//"}}, - expectedError: stringPtr("invalid configuration for string-format: empty scope provided [argument 0, option 0]")}, - { - name: "Small or Empty Regex", - config: lint.Arguments{ - []any{ - "method[1].a", - "-"}}, - expectedError: stringPtr("invalid configuration for string-format: regex is too small (regexes should begin and end with '/') [argument 0, option 1]")}, - { - name: "Bad Scope", - config: lint.Arguments{ - []any{ - "1.a", - "//"}}, - 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{ - []any{ - "method[1].a", - "/(/"}}, - expectedError: stringPtr("failed to parse configuration for string-format: unable to compile /(/ as regexp [argument 0, option 1]")}, - { - name: "Sample Config", - config: lint.Arguments{ - []any{ - "core.WriteError[1].Message", "/^([^A-Z]$)/", "must not start with a capital letter"}, - []any{ - "fmt.Errorf[0]", "/^|[^\\.!?]$/", "must not end in punctuation"}, - []any{ - "panic", "/^[^\\n]*$/", "must not contain line breaks"}}}, - { - name: "Underscores in Scope", - 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) { - err := r.ParseArgumentsTest(a.config) - if err != nil { - if a.expectedError == nil || *err != *a.expectedError { - t.Errorf("unexpected panic message: %s", *err) - } - } else if a.expectedError != nil { - t.Error("error expected but not received") - } - }) - } -} - func TestStringFormatDuplicatedStrings(t *testing.T) { testRule(t, "string_format_issue_1063", &rule.StringFormatRule{}, &lint.RuleConfig{ Arguments: lint.Arguments{[]any{