From 77b1bf6e0427bb0019f47681210b4f490a3acf5e Mon Sep 17 00:00:00 2001 From: chavacava Date: Mon, 18 Nov 2024 21:02:41 +0100 Subject: [PATCH 1/8] initial working version --- config/config.go | 1 + rule/use_errors_new.go | 60 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 rule/use_errors_new.go diff --git a/config/config.go b/config/config.go index b8b236003..1afaa7444 100644 --- a/config/config.go +++ b/config/config.go @@ -98,6 +98,7 @@ var allRules = append([]lint.Rule{ &rule.CommentsDensityRule{}, &rule.FileLengthLimitRule{}, &rule.FilenameFormatRule{}, + &rule.UseErrorsNewRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/rule/use_errors_new.go b/rule/use_errors_new.go new file mode 100644 index 000000000..adf7fa766 --- /dev/null +++ b/rule/use_errors_new.go @@ -0,0 +1,60 @@ +package rule + +import ( + "go/ast" + + "github.com/mgechev/revive/lint" +) + +// UseErrorsNewRule lints given else constructs. +type UseErrorsNewRule struct{} + +// Apply applies the rule to given file. +func (*UseErrorsNewRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure { + var failures []lint.Failure + + walker := lintFmtErrorf{ + onFailure: func(failure lint.Failure) { + failures = append(failures, failure) + }, + } + + ast.Walk(walker, file.AST) + + return failures +} + +// Name returns the rule name. +func (*UseErrorsNewRule) Name() string { + return "use-errors-new" +} + +type lintFmtErrorf struct { + onFailure func(lint.Failure) +} + +func (w lintFmtErrorf) Visit(n ast.Node) ast.Visitor { + funcCall, ok := n.(*ast.CallExpr) + if !ok { + return w // not a function call + } + + isFmtErrorf := isPkgDot(funcCall.Fun, "fmt", "Errorf") + if !isFmtErrorf { + return w // not a call to fmt.Errorf + } + + if len(funcCall.Args) > 1 { + return w // the use of fmt.Errorf is legit + } + + // the call is of the form fmt.Errorf("...") + w.onFailure(lint.Failure{ + Category: "errors", + Node: n, + Confidence: 1, + Failure: "replace fmt.Errorf by errors.New ", + }) + + return w +} From 8d72c32389dfcf12e58265704aeaa4e207b626fd Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Tue, 19 Nov 2024 16:43:04 +0200 Subject: [PATCH 2/8] refactor: simplify with strings.Prefix, strings.CutPrefix (#1137) --- cli/main.go | 5 +---- rule/exported.go | 4 ++-- rule/struct_tag.go | 4 ++-- 3 files changed, 5 insertions(+), 8 deletions(-) diff --git a/cli/main.go b/cli/main.go index c00f4b012..992e52dc8 100644 --- a/cli/main.go +++ b/cli/main.go @@ -189,10 +189,7 @@ func getVersion(builtBy, date, commit, version string) string { if version == defaultVersion { bi, ok := debug.ReadBuildInfo() if ok { - version = bi.Main.Version - if strings.HasPrefix(version, "v") { - version = strings.TrimLeft(bi.Main.Version, "v") - } + version = strings.TrimPrefix(bi.Main.Version, "v") if len(buildInfo) == 0 { return fmt.Sprintf("version %s\n", version) } diff --git a/rule/exported.go b/rule/exported.go index b9f1e4687..a9eeb83f8 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -252,8 +252,8 @@ func (w *lintExported) lintTypeDoc(t *ast.TypeSpec, doc *ast.CommentGroup) { if t.Name.Name == a { continue } - if strings.HasPrefix(s, a+" ") { - s = s[len(a)+1:] + var found bool + if s, found = strings.CutPrefix(s, a+" "); found { break } } diff --git a/rule/struct_tag.go b/rule/struct_tag.go index 4dd927827..ac7ac210d 100644 --- a/rule/struct_tag.go +++ b/rule/struct_tag.go @@ -139,8 +139,8 @@ func (lintStructTagRule) getTagName(tag *structtag.Tag) string { switch tag.Key { case keyProtobuf: for _, option := range tag.Options { - if strings.HasPrefix(option, "name=") { - return strings.TrimPrefix(option, "name=") + if tagName, found := strings.CutPrefix(option, "name="); found { + return tagName } } return "" // protobuf tag lacks 'name' option From 6f6316c2231163676e5258378f72edf00ac5a034 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Tue, 19 Nov 2024 23:58:14 +0200 Subject: [PATCH 3/8] refactor: move functions out from utils.go (#1139) --- rule/blank_imports.go | 4 +++ rule/bool_literal_in_expr.go | 20 +++++++++++ rule/exported.go | 9 +++++ rule/utils.go | 69 ------------------------------------ rule/var_declarations.go | 22 ++++++++++++ rule/var_naming.go | 7 ++++ 6 files changed, 62 insertions(+), 69 deletions(-) diff --git a/rule/blank_imports.go b/rule/blank_imports.go index 0ddb4aad2..10d84a43a 100644 --- a/rule/blank_imports.go +++ b/rule/blank_imports.go @@ -73,3 +73,7 @@ func (*BlankImportsRule) fileHasValidEmbedComment(fileAst *ast.File) bool { return false } + +// isBlank returns whether id is the blank identifier "_". +// If id == nil, the answer is false. +func isBlank(id *ast.Ident) bool { return id != nil && id.Name == "_" } diff --git a/rule/bool_literal_in_expr.go b/rule/bool_literal_in_expr.go index 71551e55a..9e193f619 100644 --- a/rule/bool_literal_in_expr.go +++ b/rule/bool_literal_in_expr.go @@ -70,3 +70,23 @@ func (w lintBoolLiteral) addFailure(node ast.Node, msg, cat string) { Failure: msg, }) } + +// isBoolOp returns true if the given token corresponds to a bool operator. +func isBoolOp(t token.Token) bool { + switch t { + case token.LAND, token.LOR, token.EQL, token.NEQ: + return true + } + + return false +} + +func isExprABooleanLit(n ast.Node) (lexeme string, ok bool) { + oper, ok := n.(*ast.Ident) + + if !ok { + return "", false + } + + return oper.Name, (oper.Name == "true" || oper.Name == "false") +} diff --git a/rule/exported.go b/rule/exported.go index a9eeb83f8..1fdd4affd 100644 --- a/rule/exported.go +++ b/rule/exported.go @@ -53,6 +53,15 @@ func (dc *disabledChecks) isDisabled(checkName string) bool { } } +var commonMethods = map[string]bool{ + "Error": true, + "Read": true, + "ServeHTTP": true, + "String": true, + "Write": true, + "Unwrap": true, +} + // ExportedRule lints given else constructs. type ExportedRule struct { stuttersMsg string diff --git a/rule/utils.go b/rule/utils.go index 1267c2d39..a22219e3d 100644 --- a/rule/utils.go +++ b/rule/utils.go @@ -6,31 +6,11 @@ import ( "go/ast" "go/printer" "go/token" - "go/types" "regexp" - "strings" "github.com/mgechev/revive/lint" ) -// isBlank returns whether id is the blank identifier "_". -// If id == nil, the answer is false. -func isBlank(id *ast.Ident) bool { return id != nil && id.Name == "_" } - -var commonMethods = map[string]bool{ - "Error": true, - "Read": true, - "ServeHTTP": true, - "String": true, - "Write": true, - "Unwrap": true, -} - -var knownNameExceptions = map[string]bool{ - "LastInsertId": true, // must match database/sql - "kWh": true, -} - func isCgoExported(f *ast.FuncDecl) bool { if f.Recv != nil || f.Doc == nil { return false @@ -45,34 +25,11 @@ func isCgoExported(f *ast.FuncDecl) bool { return false } -var allCapsRE = regexp.MustCompile(`^[A-Z0-9_]+$`) - func isIdent(expr ast.Expr, ident string) bool { id, ok := expr.(*ast.Ident) return ok && id.Name == ident } -var zeroLiteral = map[string]bool{ - "false": true, // bool - // runes - `'\x00'`: true, - `'\000'`: true, - // strings - `""`: true, - "``": true, - // numerics - "0": true, - "0.": true, - "0.0": true, - "0i": true, -} - -func validType(t types.Type) bool { - return t != nil && - t != types.Typ[types.Invalid] && - !strings.Contains(t.String(), "invalid type") // good but not foolproof -} - // isPkgDot checks if the expression is . func isPkgDot(expr ast.Expr, pkg, name string) bool { sel, ok := expr.(*ast.SelectorExpr) @@ -125,32 +82,6 @@ func (p picker) Visit(node ast.Node) ast.Visitor { return p } -// isBoolOp returns true if the given token corresponds to -// a bool operator -func isBoolOp(t token.Token) bool { - switch t { - case token.LAND, token.LOR, token.EQL, token.NEQ: - return true - } - - return false -} - -const ( - trueName = "true" - falseName = "false" -) - -func isExprABooleanLit(n ast.Node) (lexeme string, ok bool) { - oper, ok := n.(*ast.Ident) - - if !ok { - return "", false - } - - return oper.Name, (oper.Name == trueName || oper.Name == falseName) -} - // gofmt returns a string representation of an AST subtree. func gofmt(x any) string { buf := bytes.Buffer{} diff --git a/rule/var_declarations.go b/rule/var_declarations.go index 3f9d7068a..0186c3d54 100644 --- a/rule/var_declarations.go +++ b/rule/var_declarations.go @@ -5,10 +5,26 @@ import ( "go/ast" "go/token" "go/types" + "strings" "github.com/mgechev/revive/lint" ) +var zeroLiteral = map[string]bool{ + "false": true, // bool + // runes + `'\x00'`: true, + `'\000'`: true, + // strings + `""`: true, + "``": true, + // numerics + "0": true, + "0.": true, + "0.0": true, + "0i": true, +} + // VarDeclarationsRule lints given else constructs. type VarDeclarationsRule struct{} @@ -120,3 +136,9 @@ func (w *lintVarDeclarations) Visit(node ast.Node) ast.Visitor { } return w } + +func validType(t types.Type) bool { + return t != nil && + t != types.Typ[types.Invalid] && + !strings.Contains(t.String(), "invalid type") // good but not foolproof +} diff --git a/rule/var_naming.go b/rule/var_naming.go index 2c2198dbd..61f621a6c 100644 --- a/rule/var_naming.go +++ b/rule/var_naming.go @@ -13,9 +13,16 @@ import ( var anyCapsRE = regexp.MustCompile(`[A-Z]`) +var allCapsRE = regexp.MustCompile(`^[A-Z0-9_]+$`) + // regexp for constant names like `SOME_CONST`, `SOME_CONST_2`, `X123_3`, `_SOME_PRIVATE_CONST` (#851, #865) var upperCaseConstRE = regexp.MustCompile(`^_?[A-Z][A-Z\d]*(_[A-Z\d]+)*$`) +var knownNameExceptions = map[string]bool{ + "LastInsertId": true, // must match database/sql + "kWh": true, +} + // VarNamingRule lints given else constructs. type VarNamingRule struct { allowList []string From 0c08023e6184ecf3076c38492c9db26a31f35a2f Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Wed, 20 Nov 2024 21:19:17 +0200 Subject: [PATCH 4/8] test: add default case when no arguments provided to a rule (#1140) --- test/argument_limit_test.go | 4 + test/banned_characters_test.go | 4 + test/cognitive_complexity_test.go | 4 + test/context_as_argument_test.go | 4 + test/cyclomatic_test.go | 5 + test/dot_imports_test.go | 14 +- test/enforce_repeated_arg_type_style_test.go | 4 + test/file_header_test.go | 4 + test/function_length_test.go | 4 + test/line_length_limit_test.go | 4 + test/max_control_nesting_test.go | 4 + testdata/argument_limit_default.go | 7 + testdata/banned_characters_default.go | 3 + testdata/cognitive_complexity_default.go | 13 + testdata/context_as_argument_default.go | 9 + testdata/cyclomatic_default.go | 19 ++ testdata/{import_dot.go => dot_imports.go} | 0 testdata/dot_imports_default.go | 5 + ...enforce_repeated_arg_type_style_default.go | 21 ++ testdata/function_length3.go | 285 ++++++++++++++++++ testdata/function_length_default.go | 134 ++++++++ testdata/line_length_limit_default.go | 9 + testdata/lint_file_header_default.go | 1 + testdata/max_control_nesting_default.go | 16 + 24 files changed, 571 insertions(+), 6 deletions(-) create mode 100644 testdata/argument_limit_default.go create mode 100644 testdata/banned_characters_default.go create mode 100644 testdata/cognitive_complexity_default.go create mode 100644 testdata/context_as_argument_default.go create mode 100644 testdata/cyclomatic_default.go rename testdata/{import_dot.go => dot_imports.go} (100%) create mode 100644 testdata/dot_imports_default.go create mode 100644 testdata/enforce_repeated_arg_type_style_default.go create mode 100644 testdata/function_length_default.go create mode 100644 testdata/line_length_limit_default.go create mode 100644 testdata/lint_file_header_default.go create mode 100644 testdata/max_control_nesting_default.go diff --git a/test/argument_limit_test.go b/test/argument_limit_test.go index c4f0d93ef..cbe6be2c4 100644 --- a/test/argument_limit_test.go +++ b/test/argument_limit_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestArgumentLimitDefault(t *testing.T) { + testRule(t, "argument_limit_default", &rule.ArgumentsLimitRule{}, &lint.RuleConfig{}) +} + func TestArgumentLimit(t *testing.T) { testRule(t, "argument_limit", &rule.ArgumentsLimitRule{}, &lint.RuleConfig{ Arguments: []any{int64(3)}, diff --git a/test/banned_characters_test.go b/test/banned_characters_test.go index a639842d7..bbd756b33 100644 --- a/test/banned_characters_test.go +++ b/test/banned_characters_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestBannedCharactersDefault(t *testing.T) { + testRule(t, "banned_characters_default", &rule.BannedCharsRule{}, &lint.RuleConfig{}) +} + // Test banned characters in a const, var and func names. // One banned character is in the comment and should not be checked. // One banned character from the list is not present in the fixture file. diff --git a/test/cognitive_complexity_test.go b/test/cognitive_complexity_test.go index f35a445dc..4524240fb 100644 --- a/test/cognitive_complexity_test.go +++ b/test/cognitive_complexity_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestCognitiveComplexityDefault(t *testing.T) { + testRule(t, "cognitive_complexity_default", &rule.CognitiveComplexityRule{}, &lint.RuleConfig{}) +} + func TestCognitiveComplexity(t *testing.T) { testRule(t, "cognitive_complexity", &rule.CognitiveComplexityRule{}, &lint.RuleConfig{ Arguments: []any{int64(0)}, diff --git a/test/context_as_argument_test.go b/test/context_as_argument_test.go index 2e8af0a39..67fa54da9 100644 --- a/test/context_as_argument_test.go +++ b/test/context_as_argument_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestContextAsArgumentDefault(t *testing.T) { + testRule(t, "context_as_argument_default", &rule.ContextAsArgumentRule{}, &lint.RuleConfig{}) +} + func TestContextAsArgument(t *testing.T) { testRule(t, "context_as_argument", &rule.ContextAsArgumentRule{}, &lint.RuleConfig{ Arguments: []any{ diff --git a/test/cyclomatic_test.go b/test/cyclomatic_test.go index cdfce30a5..b09f1c0a2 100644 --- a/test/cyclomatic_test.go +++ b/test/cyclomatic_test.go @@ -7,7 +7,12 @@ import ( "github.com/mgechev/revive/rule" ) +func TestCyclomaticDefault(t *testing.T) { + testRule(t, "cyclomatic_default", &rule.CyclomaticRule{}, &lint.RuleConfig{}) +} + func TestCyclomatic(t *testing.T) { + testRule(t, "cyclomatic_default", &rule.CyclomaticRule{}, &lint.RuleConfig{}) testRule(t, "cyclomatic", &rule.CyclomaticRule{}, &lint.RuleConfig{ Arguments: []any{int64(1)}, }) diff --git a/test/dot_imports_test.go b/test/dot_imports_test.go index e6067a641..368021b10 100644 --- a/test/dot_imports_test.go +++ b/test/dot_imports_test.go @@ -7,12 +7,14 @@ import ( "github.com/mgechev/revive/rule" ) -func TestDotImports(t *testing.T) { - args := []any{map[string]any{ - "allowedPackages": []any{"errors", "context", "github.com/BurntSushi/toml"}, - }} +func TestDotImportsDefault(t *testing.T) { + testRule(t, "dot_imports_default", &rule.DotImportsRule{}, &lint.RuleConfig{}) +} - testRule(t, "import_dot", &rule.DotImportsRule{}, &lint.RuleConfig{ - Arguments: args, +func TestDotImports(t *testing.T) { + testRule(t, "dot_imports", &rule.DotImportsRule{}, &lint.RuleConfig{ + Arguments: []any{map[string]any{ + "allowedPackages": []any{"errors", "context", "github.com/BurntSushi/toml"}, + }}, }) } diff --git a/test/enforce_repeated_arg_type_style_test.go b/test/enforce_repeated_arg_type_style_test.go index 51c18bb33..153e3aed6 100644 --- a/test/enforce_repeated_arg_type_style_test.go +++ b/test/enforce_repeated_arg_type_style_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestEnforceRepeatedArgTypeStyleDefault(t *testing.T) { + testRule(t, "enforce_repeated_arg_type_style_default", &rule.EnforceRepeatedArgTypeStyleRule{}, &lint.RuleConfig{}) +} + func TestEnforceRepeatedArgTypeStyleShort(t *testing.T) { testRule(t, "enforce_repeated_arg_type_style_short_args", &rule.EnforceRepeatedArgTypeStyleRule{}, &lint.RuleConfig{ Arguments: []any{"short"}, diff --git a/test/file_header_test.go b/test/file_header_test.go index b340a96b8..71d0840ba 100644 --- a/test/file_header_test.go +++ b/test/file_header_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestLintFileHeaderDefault(t *testing.T) { + testRule(t, "lint_file_header_default", &rule.FileHeaderRule{}, &lint.RuleConfig{}) +} + func TestLintFileHeader(t *testing.T) { testRule(t, "lint_file_header1", &rule.FileHeaderRule{}, &lint.RuleConfig{ Arguments: []any{"foobar"}, diff --git a/test/function_length_test.go b/test/function_length_test.go index fb531a501..be7bea148 100644 --- a/test/function_length_test.go +++ b/test/function_length_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestFuncLengthDefault(t *testing.T) { + testRule(t, "function_length_default", &rule.FunctionLength{}, &lint.RuleConfig{}) +} + func TestFuncLengthLimitsStatements(t *testing.T) { testRule(t, "function_length1", &rule.FunctionLength{}, &lint.RuleConfig{ Arguments: []any{int64(2), int64(100)}, diff --git a/test/line_length_limit_test.go b/test/line_length_limit_test.go index c29e19bcb..4aad357ce 100644 --- a/test/line_length_limit_test.go +++ b/test/line_length_limit_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestLineLengthLimitDefault(t *testing.T) { + testRule(t, "line_length_limit_default", &rule.LineLengthLimitRule{}, &lint.RuleConfig{}) +} + func TestLineLengthLimit(t *testing.T) { testRule(t, "line_length_limit", &rule.LineLengthLimitRule{}, &lint.RuleConfig{ Arguments: []any{int64(100)}, diff --git a/test/max_control_nesting_test.go b/test/max_control_nesting_test.go index d6906ba5a..38250e2dd 100644 --- a/test/max_control_nesting_test.go +++ b/test/max_control_nesting_test.go @@ -7,6 +7,10 @@ import ( "github.com/mgechev/revive/rule" ) +func TestMaxControlNestingDefault(t *testing.T) { + testRule(t, "max_control_nesting_default", &rule.MaxControlNestingRule{}, &lint.RuleConfig{}) +} + func TestMaxControlNesting(t *testing.T) { testRule(t, "max_control_nesting", &rule.MaxControlNestingRule{}, &lint.RuleConfig{ Arguments: []any{int64(2)}}, diff --git a/testdata/argument_limit_default.go b/testdata/argument_limit_default.go new file mode 100644 index 000000000..264f40a0d --- /dev/null +++ b/testdata/argument_limit_default.go @@ -0,0 +1,7 @@ +package fixtures + +func foo(a, b, c, d, e, f, g, h int) { +} + +func bar(a, b, c, d, e, f, g, h, i int64) { // MATCH /maximum number of arguments per function exceeded; max 8 but got 9/ +} diff --git a/testdata/banned_characters_default.go b/testdata/banned_characters_default.go new file mode 100644 index 000000000..2c3b6ea9c --- /dev/null +++ b/testdata/banned_characters_default.go @@ -0,0 +1,3 @@ +package fixtures + +const Ω = "Omega" diff --git a/testdata/cognitive_complexity_default.go b/testdata/cognitive_complexity_default.go new file mode 100644 index 000000000..0b643f783 --- /dev/null +++ b/testdata/cognitive_complexity_default.go @@ -0,0 +1,13 @@ +package pkg + +func l() { // MATCH /function l has cognitive complexity 8 (> max enabled 7)/ + for i := 1; i <= max; i++ { + for j := 2; j < i; j++ { + if (i%j == 0) || (i%j == 1) { + continue + } + total += i + } + } + return total && max +} diff --git a/testdata/context_as_argument_default.go b/testdata/context_as_argument_default.go new file mode 100644 index 000000000..6ca81e587 --- /dev/null +++ b/testdata/context_as_argument_default.go @@ -0,0 +1,9 @@ +package foo + +import ( + "context" + "testing" +) + +func x(_ AllowedBeforePtrStruct, ctx context.Context) { // MATCH /context.Context should be the first parameter of a function/ +} diff --git a/testdata/cyclomatic_default.go b/testdata/cyclomatic_default.go new file mode 100644 index 000000000..b60751e4b --- /dev/null +++ b/testdata/cyclomatic_default.go @@ -0,0 +1,19 @@ +package pkg + +import "log" + +func f(x int) bool { // MATCH /function f has cyclomatic complexity 11 (> max enabled 10)/ + if x > 0 && true || false { + return true + } else { + log.Printf("non-positive x: %d", x) + } + switch x { + case 1: + case 2: + case 3: + case 4: + default: + } + return true || true && true +} diff --git a/testdata/import_dot.go b/testdata/dot_imports.go similarity index 100% rename from testdata/import_dot.go rename to testdata/dot_imports.go diff --git a/testdata/dot_imports_default.go b/testdata/dot_imports_default.go new file mode 100644 index 000000000..4a33659fa --- /dev/null +++ b/testdata/dot_imports_default.go @@ -0,0 +1,5 @@ +package fixtures + +import ( + . "context" // MATCH /should not use dot imports/ +) diff --git a/testdata/enforce_repeated_arg_type_style_default.go b/testdata/enforce_repeated_arg_type_style_default.go new file mode 100644 index 000000000..b88ad72a0 --- /dev/null +++ b/testdata/enforce_repeated_arg_type_style_default.go @@ -0,0 +1,21 @@ +package fixtures + +func compliantFunc(a, b int, c string) {} + +func nonCompliantFunc1(a int, b int, c string) {} +func nonCompliantFunc2(a int, b, c int) {} + +type myStruct struct{} + +func (m myStruct) compliantMethod(a, b int, c string) {} + +func (m myStruct) nonCompliantMethod1(a int, b int, c string) {} +func (m myStruct) nonCompliantMethod2(a int, b, c int) {} + +func variadicFunction(a int, b ...int) {} + +func singleArgFunction(a int) {} + +func multiTypeArgs(a int, b string, c float64) {} + +func mixedCompliance(a, b int, c int, d string) {} diff --git a/testdata/function_length3.go b/testdata/function_length3.go index 876bdfa34..0ae23125a 100644 --- a/testdata/function_length3.go +++ b/testdata/function_length3.go @@ -21,4 +21,289 @@ func funLengthA() (a int) { println() println() println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() } diff --git a/testdata/function_length_default.go b/testdata/function_length_default.go new file mode 100644 index 000000000..2b5663eef --- /dev/null +++ b/testdata/function_length_default.go @@ -0,0 +1,134 @@ +package fixtures + +func funLengthA() (a int) { // MATCH /maximum number of statements per function exceeded; max 50 but got 51/ + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() + println() +} + +func funLengthB(file *ast.File, fset *token.FileSet, lineLimit, stmtLimit int) []Message { // MATCH /maximum number of lines per function exceeded; max 75 but got 76/ + if true { + a = b + if false { + c = d + for _, f := range list { + _, ok := f.(int64) + if !ok { + continue + } + } + } + } + if true { + a = b + if false { + c = d + for _, f := range list { + _, ok := f.(int64) + if !ok { + continue + } + } + switch a { + case 1: + println() + case 2: + println() + println() + default: + println() + + } + } + } + if true { + a = b + if false { + c = d + for _, f := range list { + _, ok := f.(int64) + if !ok { + continue + } + } + switch a { + case 1: + println() + case 2: + println() + println() + default: + println() + + } + } + } + if true { + a = b + if false { + c = d + for _, f := range list { + _, ok := f.(int64) + if !ok { + continue + } + } + switch a { + case 1: + println() + default: + println() + + } + } + } + return +} \ No newline at end of file diff --git a/testdata/line_length_limit_default.go b/testdata/line_length_limit_default.go new file mode 100644 index 000000000..e92a1f650 --- /dev/null +++ b/testdata/line_length_limit_default.go @@ -0,0 +1,9 @@ +package fixtures + +import "fmt" + +func foo(a, b int) { + fmt.Printf("loooooooooooooooooooooooooooooooooooooooooong line out of limit") +} + +// MATCH:6 /line is 81 characters, out of limit 80/ diff --git a/testdata/lint_file_header_default.go b/testdata/lint_file_header_default.go new file mode 100644 index 000000000..331393d4e --- /dev/null +++ b/testdata/lint_file_header_default.go @@ -0,0 +1 @@ +package fixtures diff --git a/testdata/max_control_nesting_default.go b/testdata/max_control_nesting_default.go new file mode 100644 index 000000000..357adf62a --- /dev/null +++ b/testdata/max_control_nesting_default.go @@ -0,0 +1,16 @@ +package fixtures + +func mcn() { + if true { + if true { + if true { + if true { + if true { + if true { // MATCH /control flow nesting exceeds 5/ + } + } + } + } + } + } +} From 76b64c0c851e90eac29de40fc082cdbf770bd6f1 Mon Sep 17 00:00:00 2001 From: Oleksandr Redko Date: Wed, 20 Nov 2024 21:26:28 +0200 Subject: [PATCH 5/8] feat: add `redundant-build-tag` rule (#1135) --- README.md | 1 + RULES_DESCRIPTIONS.md | 8 ++++ config/config.go | 1 + rule/redundant_build_tag.go | 43 ++++++++++++++++++++++ test/redundant_build_tag_test.go | 20 ++++++++++ testdata/redundant_build_tag.go | 6 +++ testdata/redundant_build_tag_go116.go | 6 +++ testdata/redundant_build_tag_no_failure.go | 3 ++ 8 files changed, 88 insertions(+) create mode 100644 rule/redundant_build_tag.go create mode 100644 test/redundant_build_tag_test.go create mode 100644 testdata/redundant_build_tag.go create mode 100644 testdata/redundant_build_tag_go116.go create mode 100644 testdata/redundant_build_tag_no_failure.go diff --git a/README.md b/README.md index e40bf9793..6a5aa611b 100644 --- a/README.md +++ b/README.md @@ -550,6 +550,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`comments-density`](./RULES_DESCRIPTIONS.md#comments-density) | int (defaults to 0) | Enforces a minimum comment / code relation | no | no | | [`file-length-limit`](./RULES_DESCRIPTIONS.md#file-length-limit) | map (optional)| Enforces a maximum number of lines per file | no | no | | [`filename-format`](./RULES_DESCRIPTIONS.md#filename-format) | regular expression (optional) | Enforces the formatting of filenames | no | no | +| [`redundant-build-tag`](./RULES_DESCRIPTIONS.md#redundant-build-tag) | n/a | Warns about redundant `// +build` comment lines | no | no | ## Configurable rules diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index d76f6b11e..31b0a7e95 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -65,6 +65,7 @@ List of all available rules. - [receiver-naming](#receiver-naming) - [redefines-builtin-id](#redefines-builtin-id) - [redundant-import-alias](#redundant-import-alias) + - [redundant-build-tag](#redundant-build-tag) - [string-format](#string-format) - [string-of-int](#string-of-int) - [struct-tag](#struct-tag) @@ -799,6 +800,13 @@ _Description_: This rule warns on redundant import aliases. This happens when th _Configuration_: N/A +## redundant-build-tag + +_Description_: This rule warns about redundant build tag comments `// +build` when `//go:build` is present. +`gofmt` in Go 1.17+ automatically adds the `//go:build` constraint, making the `// +build` comment unnecessary. + +_Configuration_: N/A + ## string-format _Description_: This rule allows you to configure a list of regular expressions that string literals in certain function calls are checked against. diff --git a/config/config.go b/config/config.go index 1afaa7444..2eb7fe833 100644 --- a/config/config.go +++ b/config/config.go @@ -99,6 +99,7 @@ var allRules = append([]lint.Rule{ &rule.FileLengthLimitRule{}, &rule.FilenameFormatRule{}, &rule.UseErrorsNewRule{}, + &rule.RedundantBuildTagRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ diff --git a/rule/redundant_build_tag.go b/rule/redundant_build_tag.go new file mode 100644 index 000000000..b7bc0d4b9 --- /dev/null +++ b/rule/redundant_build_tag.go @@ -0,0 +1,43 @@ +package rule + +import ( + "strings" + + "github.com/mgechev/revive/lint" +) + +// RedundantBuildTagRule lints the presence of redundant build tags. +type RedundantBuildTagRule struct{} + +// Apply triggers if an old build tag `// +build` is found after a new one `//go:build`. +// `//go:build` comments are automatically added by gofmt when Go 1.17+ is used. +// See https://pkg.go.dev/cmd/go#hdr-Build_constraints +func (*RedundantBuildTagRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { + var failures []lint.Failure + + for _, group := range file.AST.Comments { + hasGoBuild := false + for _, comment := range group.List { + if strings.HasPrefix(comment.Text, "//go:build ") { + hasGoBuild = true + continue + } + if hasGoBuild && strings.HasPrefix(comment.Text, "// +build ") { + failures = append(failures, lint.Failure{ + Category: "style", + Confidence: 1, + Node: comment, + Failure: `The build tag "// +build" is redundant since Go 1.17 and can be removed`, + }) + return failures + } + } + } + + return failures +} + +// Name returns the rule name. +func (*RedundantBuildTagRule) Name() string { + return "redundant-build-tag" +} diff --git a/test/redundant_build_tag_test.go b/test/redundant_build_tag_test.go new file mode 100644 index 000000000..736587021 --- /dev/null +++ b/test/redundant_build_tag_test.go @@ -0,0 +1,20 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/lint" + "github.com/mgechev/revive/rule" +) + +func TestRedundantBuildTagRule(t *testing.T) { + testRule(t, "redundant_build_tag", &rule.RedundantBuildTagRule{}, &lint.RuleConfig{}) +} + +func TestRedundantBuildTagRuleNoFailure(t *testing.T) { + testRule(t, "redundant_build_tag_no_failure", &rule.RedundantBuildTagRule{}, &lint.RuleConfig{}) +} + +func TestRedundantBuildTagRuleGo116(t *testing.T) { + testRule(t, "redundant_build_tag_go116", &rule.RedundantBuildTagRule{}, &lint.RuleConfig{}) +} diff --git a/testdata/redundant_build_tag.go b/testdata/redundant_build_tag.go new file mode 100644 index 000000000..8a38f96d9 --- /dev/null +++ b/testdata/redundant_build_tag.go @@ -0,0 +1,6 @@ +//go:build tag +// +build tag + +// MATCH:2 /The build tag "// +build" is redundant since Go 1.17 and can be removed/ + +package pkg diff --git a/testdata/redundant_build_tag_go116.go b/testdata/redundant_build_tag_go116.go new file mode 100644 index 000000000..81b72821d --- /dev/null +++ b/testdata/redundant_build_tag_go116.go @@ -0,0 +1,6 @@ +// +build tag + +// This means that the file is for Go less than 1.17 because +// gofmt automatically adds the new build tag `//go:build` when Go 1.17+ is used. + +package pkg diff --git a/testdata/redundant_build_tag_no_failure.go b/testdata/redundant_build_tag_no_failure.go new file mode 100644 index 000000000..4f58c8877 --- /dev/null +++ b/testdata/redundant_build_tag_no_failure.go @@ -0,0 +1,3 @@ +//go:build tag + +package pkg From 60af01589b3330e1c9a2d272066269916fe1addb Mon Sep 17 00:00:00 2001 From: chavacava Date: Mon, 18 Nov 2024 21:02:41 +0100 Subject: [PATCH 6/8] initial working version --- config/config.go | 1 + 1 file changed, 1 insertion(+) diff --git a/config/config.go b/config/config.go index 2eb7fe833..5daad794a 100644 --- a/config/config.go +++ b/config/config.go @@ -100,6 +100,7 @@ var allRules = append([]lint.Rule{ &rule.FilenameFormatRule{}, &rule.UseErrorsNewRule{}, &rule.RedundantBuildTagRule{}, + &rule.UseErrorsNewRule{}, }, defaultRules...) var allFormatters = []lint.Formatter{ From 182296fd6d27751acde1116920fbc45afad49852 Mon Sep 17 00:00:00 2001 From: chavacava Date: Wed, 20 Nov 2024 21:17:47 +0100 Subject: [PATCH 7/8] final version --- README.md | 1 + RULES_DESCRIPTIONS.md | 8 ++++++++ rule/use_errors_new.go | 4 ++-- test/use_errors_new_test.go | 11 +++++++++++ testdata/use_errors_new.go | 12 ++++++++++++ 5 files changed, 34 insertions(+), 2 deletions(-) create mode 100644 test/use_errors_new_test.go create mode 100644 testdata/use_errors_new.go diff --git a/README.md b/README.md index 6a5aa611b..8a4e6e10c 100644 --- a/README.md +++ b/README.md @@ -551,6 +551,7 @@ List of all available rules. The rules ported from `golint` are left unchanged a | [`file-length-limit`](./RULES_DESCRIPTIONS.md#file-length-limit) | map (optional)| Enforces a maximum number of lines per file | no | no | | [`filename-format`](./RULES_DESCRIPTIONS.md#filename-format) | regular expression (optional) | Enforces the formatting of filenames | no | no | | [`redundant-build-tag`](./RULES_DESCRIPTIONS.md#redundant-build-tag) | n/a | Warns about redundant `// +build` comment lines | no | no | +| [`use-errors-new`](./RULES_DESCRIPTIONS.md#use-errors-new) | n/a | Spots calls to `fmt.Errorf` that can be replaced by `errors.New` | no | no | ## Configurable rules diff --git a/RULES_DESCRIPTIONS.md b/RULES_DESCRIPTIONS.md index 31b0a7e95..e084756a6 100644 --- a/RULES_DESCRIPTIONS.md +++ b/RULES_DESCRIPTIONS.md @@ -82,6 +82,7 @@ List of all available rules. - [unused-parameter](#unused-parameter) - [unused-receiver](#unused-receiver) - [use-any](#use-any) + - [use-errors-new](#use-errors-new) - [useless-break](#useless-break) - [var-declaration](#var-declaration) - [var-naming](#var-naming) @@ -987,6 +988,13 @@ _Description_: Since Go 1.18, `interface{}` has an alias: `any`. This rule propo _Configuration_: N/A +## use-errors-new + +_Description_: This rules identifies calls to `fmt.Errorf` that can be safely replaced by, the more efficient, `errors.New`. + +_Configuration_: N/A + + ## useless-break _Description_: This rule warns on useless `break` statements in case clauses of switch and select statements. Go, unlike other programming languages like C, only executes statements of the selected case while ignoring the subsequent case clauses. diff --git a/rule/use_errors_new.go b/rule/use_errors_new.go index adf7fa766..d21bbf7d8 100644 --- a/rule/use_errors_new.go +++ b/rule/use_errors_new.go @@ -6,7 +6,7 @@ import ( "github.com/mgechev/revive/lint" ) -// UseErrorsNewRule lints given else constructs. +// UseErrorsNewRule spots calls to fmt.Errorf that can be replaced by errors.New. type UseErrorsNewRule struct{} // Apply applies the rule to given file. @@ -53,7 +53,7 @@ func (w lintFmtErrorf) Visit(n ast.Node) ast.Visitor { Category: "errors", Node: n, Confidence: 1, - Failure: "replace fmt.Errorf by errors.New ", + Failure: "replace fmt.Errorf by errors.New", }) return w diff --git a/test/use_errors_new_test.go b/test/use_errors_new_test.go new file mode 100644 index 000000000..135beaa2e --- /dev/null +++ b/test/use_errors_new_test.go @@ -0,0 +1,11 @@ +package test + +import ( + "testing" + + "github.com/mgechev/revive/rule" +) + +func TestUseErrorsNew(t *testing.T) { + testRule(t, "use_errors_new", &rule.UseErrorsNewRule{}) +} diff --git a/testdata/use_errors_new.go b/testdata/use_errors_new.go new file mode 100644 index 000000000..9fae22648 --- /dev/null +++ b/testdata/use_errors_new.go @@ -0,0 +1,12 @@ +package pkg + +import "fmt" + +func errorsNew() (int, error) { + fmt.Errorf("repo cannot be nil") // MATCH /replace fmt.Errorf by errors.New/ + errs := append(errs, fmt.Errorf("commit cannot be nil")) // MATCH /replace fmt.Errorf by errors.New/ + fmt.Errorf("unable to load base repo: %w", err) + fmt.Errorf("Failed to get full commit id for origin/%s: %w", pr.BaseBranch, err) + + return 0, fmt.Errorf(msg + "something") // MATCH /replace fmt.Errorf by errors.New/ +} From 44fca1748b7f3af1522e924063d19ef9ec598684 Mon Sep 17 00:00:00 2001 From: chavacava Date: Wed, 20 Nov 2024 21:27:04 +0100 Subject: [PATCH 8/8] Delete rule/redundant_build_tag.go --- rule/redundant_build_tag.go | 41 ------------------------------------- 1 file changed, 41 deletions(-) delete mode 100644 rule/redundant_build_tag.go diff --git a/rule/redundant_build_tag.go b/rule/redundant_build_tag.go deleted file mode 100644 index 4a880b021..000000000 --- a/rule/redundant_build_tag.go +++ /dev/null @@ -1,41 +0,0 @@ -package rule - -import ( - "strings" - - "github.com/mgechev/revive/lint" -) - -// RedundantBuildTagRule lints the presence of redundant build tags. -type RedundantBuildTagRule struct{} - -// Apply triggers if an old build tag `// +build` is found after a new one `//go:build`. -// `//go:build` comments are automatically added by gofmt when Go 1.17+ is used. -// See https://pkg.go.dev/cmd/go#hdr-Build_constraints -func (*RedundantBuildTagRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure { - for _, group := range file.AST.Comments { - hasGoBuild := false - for _, comment := range group.List { - if strings.HasPrefix(comment.Text, "//go:build ") { - hasGoBuild = true - continue - } - - if hasGoBuild && strings.HasPrefix(comment.Text, "// +build ") { - return []lint.Failure{{ - Category: "style", - Confidence: 1, - Node: comment, - Failure: `The build tag "// +build" is redundant since Go 1.17 and can be removed`, - }} - } - } - } - - return []lint.Failure{} -} - -// Name returns the rule name. -func (*RedundantBuildTagRule) Name() string { - return "redundant-build-tag" -}