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.

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

refactor: replace panic with error in rules #1126

Merged
merged 44 commits into from
Dec 11, 2024
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
44 commits
Select commit Hold shift + click to select a range
89322bc
refactor: replace panic with error in rules
mfederowicz Nov 14, 2024
5665063
handle outside apply
mfederowicz Nov 16, 2024
cf72c3d
handle error from r.configure
mfederowicz Nov 16, 2024
689e3c9
checkNumberOfArguments: handle error
mfederowicz Nov 16, 2024
3109bd6
rules after review cleanup
mfederowicz Nov 16, 2024
c68d908
package.go: revert to master version
mfederowicz Nov 16, 2024
b607ec6
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Nov 16, 2024
f5e3a0e
fix lack of variables after merge with master
mfederowicz Nov 16, 2024
5c98306
handle configureErr in Apply func
mfederowicz Nov 17, 2024
63a0088
replace configureErr with local variable
mfederowicz Nov 17, 2024
c7f09e0
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Nov 18, 2024
a53dfc7
Merge remote-tracking branch 'upstream/master' into rules-avoid-using…
mfederowicz Nov 18, 2024
d47112b
remove package comment, except doc.go
mfederowicz Nov 18, 2024
fd97ee3
revert new line above:configureOnce
mfederowicz Nov 18, 2024
a451c02
cleanup code
mfederowicz Nov 19, 2024
1c6e8e7
cleanup code
mfederowicz Nov 19, 2024
f17c9b1
replace configureOnce with sync.OnceValue
mfederowicz Nov 20, 2024
ae29706
lint: remove error return, exit is last step
mfederowicz Nov 20, 2024
fdcaa68
back to r.configureOnce.Do() variant with local error variable
mfederowicz Dec 7, 2024
af86a92
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 7, 2024
0022083
cleanup after merge with master
mfederowicz Dec 7, 2024
9c9e05e
package: handle errgroup.Group
mfederowicz Dec 7, 2024
92bcf9e
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 7, 2024
aee44cb
Merge remote-tracking branch 'upstream/master' into rules-avoid-using…
mfederowicz Dec 8, 2024
2d6cafa
after review changes
mfederowicz Dec 8, 2024
ea832f4
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 8, 2024
d6f17ff
cleanup rules, remove error from return signature
mfederowicz Dec 8, 2024
3086a32
cleanup lint/rule.go
mfederowicz Dec 8, 2024
093971c
error msg lowercase
mfederowicz Dec 8, 2024
d78563b
lowercace error msg
mfederowicz Dec 8, 2024
5685fad
.gitignore: add .idea dir
mfederowicz Dec 8, 2024
68e556a
merge with upstream
mfederowicz Dec 8, 2024
3191797
merge with upstream
mfederowicz Dec 8, 2024
f6c7ee0
lowercase in error msg
mfederowicz Dec 8, 2024
0e4a8c4
fix rule name
mfederowicz Dec 8, 2024
ef674f6
helper: newInternalFailureError
mfederowicz Dec 9, 2024
5ebe459
add space in msg
mfederowicz Dec 9, 2024
953b2a5
newInternalFailureError: update helper comment
mfederowicz Dec 9, 2024
ed5e149
.gitignore: remove idea entry
mfederowicz Dec 9, 2024
1628b5c
cleanup after review
mfederowicz Dec 10, 2024
96de931
Merge branch 'master' into rules-avoid-using-panic
mfederowicz Dec 10, 2024
bad6ed8
cleanup after review
mfederowicz Dec 10, 2024
634fa48
Apply suggestions from code review
chavacava Dec 11, 2024
e56fa62
fix printf parameters
Dec 11, 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
8 changes: 6 additions & 2 deletions lint/file.go
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ func (f *File) isMain() bool {

const directiveSpecifyDisableReason = "specify-disable-reason"

func (f *File) lint(rules []Rule, config Config, failures chan Failure) {
func (f *File) lint(rules []Rule, config Config, failures chan Failure) error {
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
rulesConfig := config.Rules
_, mustSpecifyDisableReason := config.Directives[directiveSpecifyDisableReason]
disabledIntervals := f.disabledIntervals(rules, mustSpecifyDisableReason, failures)
Expand All @@ -105,7 +105,10 @@ func (f *File) lint(rules []Rule, config Config, failures chan Failure) {
if ruleConfig.MustExclude(f.Name) {
continue
}
currentFailures := currentRule.Apply(f, ruleConfig.Arguments)
currentFailures, err := currentRule.Apply(f, ruleConfig.Arguments)
if err != nil {
return err
}
for idx, failure := range currentFailures {
if failure.RuleName == "" {
failure.RuleName = currentRule.Name()
Expand All @@ -122,6 +125,7 @@ func (f *File) lint(rules []Rule, config Config, failures chan Failure) {
}
}
}
return nil
}

type enableDisableConfig struct {
Expand Down
6 changes: 5 additions & 1 deletion lint/linter.go
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,11 @@ func (l *Linter) lintPackage(filenames []string, gover *goversion.Version, ruleS
return nil
}

pkg.lint(ruleSet, config, failures)
err := pkg.lint(ruleSet, config, failures)

denisvmedia marked this conversation as resolved.
Show resolved Hide resolved
if err != nil {
return err
}

return nil
}
Expand Down
11 changes: 9 additions & 2 deletions lint/package.go
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
package lint

import (
"fmt"
"go/ast"
"go/importer"
"go/token"
"go/types"
"os"
"sync"

goversion "github.com/hashicorp/go-version"
Expand Down Expand Up @@ -182,17 +184,22 @@ func (p *Package) scanSortable() {
}
}

func (p *Package) lint(rules []Rule, config Config, failures chan Failure) {
func (p *Package) lint(rules []Rule, config Config, failures chan Failure) error {
p.scanSortable()
var wg sync.WaitGroup
for _, file := range p.files {
wg.Add(1)
go (func(file *File) {
file.lint(rules, config, failures)
err := file.lint(rules, config, failures)
if err != nil {
fmt.Fprintln(os.Stderr, err)
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
os.Exit(1)
}
wg.Done()
})(file)
}
wg.Wait()
return nil
}

// IsAtLeastGo121 returns true if the Go version for this package is 1.21 or higher, false otherwise
Expand Down
2 changes: 1 addition & 1 deletion lint/rule.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ type DisabledInterval struct {
// Rule defines an abstract rule interface
type Rule interface {
Name() string
Apply(*File, Arguments) []Failure
Apply(*File, Arguments) ([]Failure, error)
Copy link
Contributor

Choose a reason for hiding this comment

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

We can't simply add a new return argument to the Apply function due to the backward compatibility. Someone who uses revivelib may depend on this interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok @alexandear any idea how to handle errors without of course excuting panic(), we can use in each Apply:

if err != nil {                                                                                                                                                
   fmt.Fprintln(os.Stderr, err)                                                                         
   os.Exit(1)                                                                                          
   }

but it is not elegant way :(

Copy link
Collaborator

@denisvmedia denisvmedia Nov 19, 2024

Choose a reason for hiding this comment

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

To maintain backwards compatibility we would have to add backwards compatible wrappers around the newly included changes. And one thing that came to my mind, how do these changes fit the use of Revive in golangci-lint?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok got it @denisvmedia you want something like that: https://github.com/golangci/golangci-lint/blob/master/pkg/golinters/revive/revive.go#L53 ? but is it possible to do that in separate PR, or it should be done in that PR?

}

// AbstractRule defines an abstract rule.
Expand Down
4 changes: 2 additions & 2 deletions revivelib/core_internal_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,8 +43,8 @@ func (*mockRule) Name() string {
return "mock-rule"
}

func (*mockRule) Apply(_ *lint.File, _ lint.Arguments) []lint.Failure {
return nil
func (*mockRule) Apply(_ *lint.File, _ lint.Arguments) ([]lint.Failure, error) {
return nil, nil
}

func getMockRevive(t *testing.T) *Revive {
Expand Down
4 changes: 2 additions & 2 deletions revivelib/core_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,8 @@ func (r *mockRule) Name() string {
return "mock-rule"
}

func (r *mockRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
return nil
func (r *mockRule) Apply(file *lint.File, arguments lint.Arguments) ([]lint.Failure, error) {
return nil, nil
}

func getMockRevive(t *testing.T) *revivelib.Revive {
Expand Down
31 changes: 19 additions & 12 deletions rule/add_constant.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package rule

import (
"errors"
"fmt"
"go/ast"
"regexp"
Expand Down Expand Up @@ -41,8 +42,12 @@ type AddConstantRule struct {
}

// Apply applies the rule to given file.
func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) ([]lint.Failure, error) {
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })
if configureErr != nil {
return nil, configureErr
}
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved

var failures []lint.Failure

Expand All @@ -61,7 +66,7 @@ func (r *AddConstantRule) Apply(file *lint.File, arguments lint.Arguments) []lin

ast.Walk(w, file.AST)

return failures
return failures, nil
}

// Name returns the rule name.
Expand Down Expand Up @@ -201,15 +206,15 @@ func (w *lintAddConstantRule) isStructTag(n *ast.BasicLit) bool {
return ok
}

func (r *AddConstantRule) configure(arguments lint.Arguments) {
func (r *AddConstantRule) configure(arguments lint.Arguments) error {
r.strLitLimit = defaultStrLitLimit
r.allowList = newAllowList()
if len(arguments) == 0 {
return
return nil
}
args, ok := arguments[0].(map[string]any)
if !ok {
panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting a k,v map. Got %T", arguments[0]))
return fmt.Errorf("Invalid argument to the add-constant rule, expecting a k,v map. Got %T", arguments[0])
}
for k, v := range args {
kind := ""
Expand All @@ -228,39 +233,41 @@ func (r *AddConstantRule) configure(arguments lint.Arguments) {
}
list, ok := v.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v))
return fmt.Errorf("Invalid argument to the add-constant rule, string expected. Got '%v' (%T)", v, v)
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
}
r.allowList.add(kind, list)
case "maxLitCount":
sl, ok := v.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v))
return fmt.Errorf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v' (%T)", v, v)
}

limit, err := strconv.Atoi(sl)
if err != nil {
panic(fmt.Sprintf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v))
return fmt.Errorf("Invalid argument to the add-constant rule, expecting string representation of an integer. Got '%v'", v)
}
r.strLitLimit = limit
case "ignoreFuncs":
excludes, ok := v.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument to the ignoreFuncs parameter of add-constant rule, string expected. Got '%v' (%T)", v, v))
return fmt.Errorf("Invalid argument to the ignoreFuncs parameter of add-constant rule, string expected. Got '%v' (%T)", v, v)
}

for _, exclude := range strings.Split(excludes, ",") {
exclude = strings.Trim(exclude, " ")
if exclude == "" {
panic("Invalid argument to the ignoreFuncs parameter of add-constant rule, expected regular expression must not be empty.")
return errors.New("Invalid argument to the ignoreFuncs parameter of add-constant rule, expected regular expression must not be empty")
}

exp, err := regexp.Compile(exclude)
if err != nil {
panic(fmt.Sprintf("Invalid argument to the ignoreFuncs parameter of add-constant rule: regexp %q does not compile: %v", exclude, err))
return fmt.Errorf("Invalid argument to the ignoreFuncs parameter of add-constant rule: regexp %q does not compile: %w", exclude, err)
}

r.ignoreFunctions = append(r.ignoreFunctions, exp)
}
}
}

return nil
}
18 changes: 12 additions & 6 deletions rule/argument_limit.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package rule

import (
"errors"
"fmt"
"go/ast"
"sync"
Expand All @@ -17,22 +18,27 @@ type ArgumentsLimitRule struct {

const defaultArgumentsLimit = 8

func (r *ArgumentsLimitRule) configure(arguments lint.Arguments) {
func (r *ArgumentsLimitRule) configure(arguments lint.Arguments) error {
if len(arguments) < 1 {
r.max = defaultArgumentsLimit
return
return nil
}

maxArguments, ok := arguments[0].(int64) // Alt. non panicking version
if !ok {
panic(`invalid value passed as argument number to the "argument-limit" rule`)
return errors.New(`invalid value passed as argument number to the "argument-limit" rule`)
}
r.max = int(maxArguments)
return nil
}

// Apply applies the rule to given file.
func (r *ArgumentsLimitRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
func (r *ArgumentsLimitRule) Apply(file *lint.File, arguments lint.Arguments) ([]lint.Failure, error) {
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })
if configureErr != nil {
return nil, configureErr
}

var failures []lint.Failure
onFailure := func(failure lint.Failure) {
Expand All @@ -46,7 +52,7 @@ func (r *ArgumentsLimitRule) Apply(file *lint.File, arguments lint.Arguments) []

ast.Walk(walker, file.AST)

return failures
return failures, nil
}

// Name returns the rule name.
Expand Down
4 changes: 2 additions & 2 deletions rule/atomic.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ import (
type AtomicRule struct{}

// Apply applies the rule to given file.
func (*AtomicRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
func (*AtomicRule) Apply(file *lint.File, _ lint.Arguments) ([]lint.Failure, error) {
var failures []lint.Failure
walker := atomic{
pkgTypesInfo: file.Pkg.TypesInfo(),
Expand All @@ -23,7 +23,7 @@ func (*AtomicRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {

ast.Walk(walker, file.AST)

return failures
return failures, nil
}

// Name returns the rule name.
Expand Down
34 changes: 23 additions & 11 deletions rule/banned_characters.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,34 @@ import (
// BannedCharsRule checks if a file contains banned characters.
type BannedCharsRule struct {
bannedCharList []string

configureOnce sync.Once
configureOnce sync.Once
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
}

const bannedCharsRuleName = "banned-characters"

func (r *BannedCharsRule) configure(arguments lint.Arguments) {
func (r *BannedCharsRule) configure(arguments lint.Arguments) error {
if len(arguments) > 0 {
checkNumberOfArguments(1, arguments, bannedCharsRuleName)
r.bannedCharList = r.getBannedCharsList(arguments)
check := checkNumberOfArguments(1, arguments, bannedCharsRuleName)
if check != nil {
return check
mfederowicz marked this conversation as resolved.
Show resolved Hide resolved
}
list, err := r.getBannedCharsList(arguments)
if err != nil {
return err
}

r.bannedCharList = list
}
return nil
}

// Apply applied the rule to the given file.
func (r *BannedCharsRule) Apply(file *lint.File, arguments lint.Arguments) []lint.Failure {
r.configureOnce.Do(func() { r.configure(arguments) })
func (r *BannedCharsRule) Apply(file *lint.File, arguments lint.Arguments) ([]lint.Failure, error) {
var configureErr error
r.configureOnce.Do(func() { configureErr = r.configure(arguments) })
if configureErr != nil {
return nil, configureErr
}

var failures []lint.Failure
onFailure := func(failure lint.Failure) {
Expand All @@ -40,7 +52,7 @@ func (r *BannedCharsRule) Apply(file *lint.File, arguments lint.Arguments) []lin
}

ast.Walk(w, file.AST)
return failures
return failures, nil
}

// Name returns the rule name
Expand All @@ -49,17 +61,17 @@ func (*BannedCharsRule) Name() string {
}

// getBannedCharsList converts arguments into the banned characters list
func (r *BannedCharsRule) getBannedCharsList(args lint.Arguments) []string {
func (r *BannedCharsRule) getBannedCharsList(args lint.Arguments) ([]string, error) {
var bannedChars []string
for _, char := range args {
charStr, ok := char.(string)
if !ok {
panic(fmt.Sprintf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), char))
return nil, fmt.Errorf("Invalid argument for the %s rule: expecting a string, got %T", r.Name(), char)
}
bannedChars = append(bannedChars, charStr)
}

return bannedChars
return bannedChars, nil
}

type lintBannedCharsRule struct {
Expand Down
4 changes: 2 additions & 2 deletions rule/bare_return.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (
type BareReturnRule struct{}

// Apply applies the rule to given file.
func (*BareReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
func (*BareReturnRule) Apply(file *lint.File, _ lint.Arguments) ([]lint.Failure, error) {
var failures []lint.Failure

onFailure := func(failure lint.Failure) {
Expand All @@ -19,7 +19,7 @@ func (*BareReturnRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {

w := lintBareReturnRule{onFailure: onFailure}
ast.Walk(w, file.AST)
return failures
return failures, nil
}

// Name returns the rule name.
Expand Down
6 changes: 3 additions & 3 deletions rule/blank_imports.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ func (*BlankImportsRule) Name() string {
}

// Apply applies the rule to given file.
func (r *BlankImportsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failure {
func (r *BlankImportsRule) Apply(file *lint.File, _ lint.Arguments) ([]lint.Failure, error) {
if file.Pkg.IsMain() || file.IsTest() {
return nil
return nil, nil
}

const (
Expand Down Expand Up @@ -59,7 +59,7 @@ func (r *BlankImportsRule) Apply(file *lint.File, _ lint.Arguments) []lint.Failu
}
}

return failures
return failures, nil
}

func (*BlankImportsRule) fileHasValidEmbedComment(fileAst *ast.File) bool {
Expand Down
Loading