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

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 14 commits
Commits
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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: I'd remove this line (it will be more consistent stylistically).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@denisvmedia maybe something like this: return pkg.lint(ruleSet, config, failures)?

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)
}
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
}

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
}
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