refactor: replace panic with error in rules #1126

mfederowicz · 2024-11-15T00:05:18Z

related with PR #1107

changes in areas:

refactor rules to return error in r.configure func
make changes in almost all rules
add return error for Apply func to handle error in upper level (bacause there were panic outisde configure)
and other places where panic was used
add comment for package rule :)

I am open to suggestion what to change in next steps @chavacava @denisvmedia

lint/file.go

chavacava · 2024-11-16T11:05:22Z

As is, the implementation of rules in this branch does not handle errors but put them "under the carpet"
Almost the single situation where rules might err is when reading arguments, therefore rule.Apply implementations shall handle errors returned by calls to r.configure

ccoVeille

It wasn't fun to review 😅 27 minutes

lint/package.go

rule/argument_limit.go

rule/enforce_map_style.go

rule/add_constant.go

rule/unchecked_type_assertion.go

rule/unhandled_error.go

rule/unused_param.go

rule/unused_receiver.go

lint/package.go

rule/add_constant.go

# Conflicts: # rule/add_constant.go # rule/argument_limit.go # rule/banned_characters.go # rule/cognitive_complexity.go # rule/comment_spacings.go # rule/comments_density.go # rule/context_as_argument.go # rule/cyclomatic.go # rule/defer.go # rule/dot_imports.go # rule/enforce_map_style.go # rule/enforce_repeated_arg_type_style.go # rule/enforce_slice_style.go # rule/error_strings.go # rule/exported.go # rule/file_header.go # rule/file_length_limit.go # rule/filename_format.go # rule/function_length.go # rule/function_result_limit.go # rule/import_alias_naming.go # rule/imports_blocklist.go # rule/line_length_limit.go # rule/max_control_nesting.go # rule/max_public_structs.go # rule/receiver_naming.go # rule/struct_tag.go # rule/unchecked_type_assertion.go # rule/unhandled_error.go # rule/unused_param.go # rule/unused_receiver.go # rule/var_naming.go

rule/add_constant.go

rule/early_return.go

alexandear · 2024-11-18T10:58:57Z

lint/rule.go

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


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.

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 :(

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?

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?

rule/add_constant.go

…-panic # Conflicts: # rule/add_constant.go

lint/linter.go

lint/package.go

ccoVeille

Please take a look at

#1126 (comment)

For me, it's blocking

chavacava · 2024-12-08T20:01:26Z

@mfederowicz thanks for the updates. Please run

revive --config revive.toml ./...

On the branch, you will see that the problem of error messages in uppercase is present in almost all configuration error messages

nit: we could add an utility function (in rules/utils.go ?) to avoid repeating []lint.Failure{lint.NewInternalFailure(configureErr.Error())} at each rule.

lint/file.go

rule/dot_imports.go

mfederowicz · 2024-12-08T23:01:59Z

ok @chavacava i replaced almost all errors msg in rules, but there are some cases in testdata what about them?

rule/utils.go

chavacava

Some details to adjust but overall OK.
Thanks @mfederowicz !

lint/linter.go

.gitignore

ccoVeille · 2024-12-09T08:24:02Z

You did great, you managed to fix the panic issue.

I will try to open a PR with ideas I have.

The errgroup in lint/linter.go (unsure it would work as I expect anyway) to avoid a deep-exit
the memorization on configureErr (I'm unsure we need it)

I cannot only suggest things, I have to code sometimes 🤭

Then I will be the one who will take feedbacks from you guys 😅

denisvmedia

LGTM

rule/add_constant.go

alexandear · 2024-12-09T10:21:43Z

rule/banned_characters.go

+	var configureErr error
+	r.configureOnce.Do(func() { configureErr = r.configure(arguments) })
+
+	if configureErr != nil {
+		return newInternalFailureError(configureErr)
+	}


I propose adding a test to check a situation where configureErr is not nil. Not for all rules but at least for a one.

rule/enforce_slice_style.go

rule/file_header.go

rule/file_length_limit.go

rule/import_alias_naming.go

rule/string_format.go

rule/max_public_structs.go

rule/struct_tag.go

alexandear · 2024-12-09T10:46:47Z

rule/string_format.go

@@ -43,10 +47,9 @@ func (StringFormatRule) ParseArgumentsTest(arguments lint.Arguments) *string {
 	// 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 := recover()
+			err := w.parseArguments(arguments)


Since we changed configure to return error instead of panicking, the ParseArgumentsTest became unnecessary. We can remove it.

chavacava · 2024-12-09T20:35:20Z

@mfederowicz I'll wait the OK from @alexandear to merge the PR. Thanks for the huge amount of work! And, of course, thanks for the reviews and discussions @ccoVeille @alexandear @denisvmedia

# Conflicts: # rule/string_format.go

rule/file_header.go

Co-authored-by: Oleksandr Redko <[email protected]>

refactor: replace panic with error in rules

89322bc

chavacava reviewed Nov 15, 2024

View reviewed changes

lint/file.go Outdated Show resolved Hide resolved

handle outside apply

5665063

ccoVeille suggested changes Nov 16, 2024

View reviewed changes

mfederowicz added 3 commits November 16, 2024 16:03

handle error from r.configure

cf72c3d

checkNumberOfArguments: handle error

689e3c9

rules after review cleanup

3109bd6

chavacava reviewed Nov 16, 2024

View reviewed changes

lint/package.go Outdated Show resolved Hide resolved

chavacava reviewed Nov 16, 2024

View reviewed changes

rule/add_constant.go Outdated Show resolved Hide resolved

mfederowicz added 4 commits November 16, 2024 18:28

package.go: revert to master version

c68d908

fix lack of variables after merge with master

f5e3a0e

handle configureErr in Apply func

5c98306

ccoVeille suggested changes Nov 17, 2024

View reviewed changes

rule/add_constant.go Outdated Show resolved Hide resolved

replace configureErr with local variable

63a0088

ccoVeille approved these changes Nov 17, 2024

View reviewed changes

alexandear suggested changes Nov 18, 2024

View reviewed changes

mfederowicz added 4 commits November 18, 2024 21:42

Merge branch 'master' into rules-avoid-using-panic

c7f09e0

Merge remote-tracking branch 'upstream/master' into rules-avoid-using…

a53dfc7

…-panic # Conflicts: # rule/add_constant.go

remove package comment, except doc.go

d47112b

revert new line above:configureOnce

fd97ee3

denisvmedia reviewed Nov 19, 2024

View reviewed changes

lint/linter.go Outdated Show resolved Hide resolved

denisvmedia reviewed Nov 19, 2024

View reviewed changes

lint/package.go Outdated Show resolved Hide resolved

mfederowicz added 3 commits November 19, 2024 19:07

cleanup code

a451c02

cleanup code

1c6e8e7

replace configureOnce with sync.OnceValue

f17c9b1

ccoVeille reviewed Nov 20, 2024

View reviewed changes

lint/package.go Outdated Show resolved Hide resolved

lint: remove error return, exit is last step

ae29706

ccoVeille suggested changes Nov 20, 2024

View reviewed changes

alexandear reviewed Dec 8, 2024

View reviewed changes

lint/file.go Outdated Show resolved Hide resolved

rule/dot_imports.go Outdated Show resolved Hide resolved

rule/dot_imports.go Outdated Show resolved Hide resolved

rule/dot_imports.go Outdated Show resolved Hide resolved

mfederowicz added 6 commits December 8, 2024 23:04

lowercace error msg

d78563b

.gitignore: add .idea dir

5685fad

merge with upstream

68e556a

merge with upstream

3191797

lowercase in error msg

f6c7ee0

fix rule name

0e4a8c4

helper: newInternalFailureError

ef674f6

chavacava reviewed Dec 9, 2024

View reviewed changes

rule/utils.go Outdated Show resolved Hide resolved

chavacava approved these changes Dec 9, 2024

View reviewed changes

lint/linter.go Outdated Show resolved Hide resolved

ccoVeille reviewed Dec 9, 2024

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

ccoVeille approved these changes Dec 9, 2024

View reviewed changes

mfederowicz added 2 commits December 9, 2024 10:10

add space in msg

5ebe459

newInternalFailureError: update helper comment

953b2a5

denisvmedia approved these changes Dec 9, 2024

View reviewed changes

alexandear suggested changes Dec 9, 2024

View reviewed changes

alexandear reviewed Dec 9, 2024

View reviewed changes

.gitignore: remove idea entry

ed5e149

mfederowicz added 2 commits December 10, 2024 10:19

cleanup after review

1628b5c

Merge branch 'master' into rules-avoid-using-panic

96de931

# Conflicts: # rule/string_format.go

alexandear reviewed Dec 10, 2024

View reviewed changes

rule/file_header.go Outdated Show resolved Hide resolved

mfederowicz and others added 3 commits December 10, 2024 11:54

cleanup after review

bad6ed8

Apply suggestions from code review

634fa48

Co-authored-by: Oleksandr Redko <[email protected]>

fix printf parameters

e56fa62

chavacava merged commit 9b15f3f into mgechev:master Dec 11, 2024
5 checks passed

alexandear mentioned this pull request Dec 12, 2024

refactor: error management #1185

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: replace panic with error in rules #1126

refactor: replace panic with error in rules #1126

mfederowicz commented Nov 15, 2024 •

edited

Loading

chavacava commented Nov 16, 2024 •

edited

Loading

ccoVeille left a comment •

edited

Loading

alexandear Nov 18, 2024

mfederowicz Nov 19, 2024

denisvmedia Nov 19, 2024 •

edited

Loading

mfederowicz Nov 19, 2024

ccoVeille left a comment

chavacava commented Dec 8, 2024

mfederowicz commented Dec 8, 2024

chavacava left a comment

ccoVeille commented Dec 9, 2024 •

edited

Loading

denisvmedia left a comment

alexandear Dec 9, 2024

alexandear Dec 9, 2024 •

edited

Loading

chavacava commented Dec 9, 2024

refactor: replace panic with error in rules #1126

refactor: replace panic with error in rules #1126

Conversation

mfederowicz commented Nov 15, 2024 • edited Loading

chavacava commented Nov 16, 2024 • edited Loading

ccoVeille left a comment • edited Loading

Choose a reason for hiding this comment

alexandear Nov 18, 2024

Choose a reason for hiding this comment

mfederowicz Nov 19, 2024

Choose a reason for hiding this comment

denisvmedia Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

mfederowicz Nov 19, 2024

Choose a reason for hiding this comment

ccoVeille left a comment

Choose a reason for hiding this comment

chavacava commented Dec 8, 2024

mfederowicz commented Dec 8, 2024

chavacava left a comment

Choose a reason for hiding this comment

ccoVeille commented Dec 9, 2024 • edited Loading

denisvmedia left a comment

Choose a reason for hiding this comment

alexandear Dec 9, 2024

Choose a reason for hiding this comment

alexandear Dec 9, 2024 • edited Loading

Choose a reason for hiding this comment

chavacava commented Dec 9, 2024

mfederowicz commented Nov 15, 2024 •

edited

Loading

chavacava commented Nov 16, 2024 •

edited

Loading

ccoVeille left a comment •

edited

Loading

denisvmedia Nov 19, 2024 •

edited

Loading

ccoVeille commented Dec 9, 2024 •

edited

Loading

alexandear Dec 9, 2024 •

edited

Loading