refactor: modifies linting machinery to use Failure as a mean to signal erros in rules application #1178
Conversation
lint/package.go
Outdated
| errChan := make(chan error) | ||
| doneChan := make(chan struct{}) | ||
|
|
||
| wg.Add(len(p.files)) | ||
| go func() { // This goroutine will signal when all files where linted | ||
| wg.Wait() | ||
| doneChan <- struct{}{} | ||
| }() | ||
|
|
||
| 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 { | ||
| errChan <- err // signal the error | ||
| } | ||
| wg.Done() | ||
| })(file) | ||
| } | ||
| wg.Wait() | ||
|
|
||
| select { // We block until... | ||
| case <-doneChan: //...all files were linted | ||
| return nil | ||
| case err := <-errChan: //...or there is an error | ||
| return err | ||
| } |
There was a problem hiding this comment.
What about using
https://pkg.go.dev/golang.org/x/sync/errgroup
The problems I see with your code are:
-
- the channels make it complicated
-
- errgroup exists
-
- all linters will be configured, no matter if one fail
-
- but you will return on first error
-
- so the go routines keeps being called for nothing
I would have said why not, if each errors was reported or returned
Of course, this feedback is based on my understanding of the code. I can be wrong 😅
There was a problem hiding this comment.
Thanks for the feedback
(I've edited your comment to add numbers to the list of problems)
My meta-feedback:
- Fair (even if I personally prefer explicit code over "hidden magic")
- True (more on errorgroup below)
- Not necessary all but I get the point
- True
- On error, control exits the whole program thus gorutines will die.
On errgroup:
- as used in the PR refactor: replace panic with error in rules #1126, errgroup does not necessarily prevent for configuring "all linters, no matter if one fail"
revive -config defaults.toml ./...
>>>>> configure addconstant
>>>>> configure arguments limits
Error during linting:Invalid argument to the add-constant rule, expecting a k,v map. Got string
- as used in the PR refactor: replace panic with error in rules #1126, errgroup does not necessarily prevent for "goroutines keep being called"
(each>>>> linting ...is a goroutine)
revive -config defaults.toml ./...
>>>>>> linting file internal/typeparams/typeparams.go
>>>>>> linting file internal/typeparams/typeparams_go118.go
>>>>> configure addconstant
>>>>>> linting file config/config.go
>>>>>> linting file config/config_test.go
>>>>>> linting file internal/ifelse/branch.go
>>>>>> linting file internal/ifelse/branch_kind.go
>>>>>> linting file internal/ifelse/chain.go
>>>>>> linting file internal/ifelse/doc.go
>>>>>> linting file internal/ifelse/func.go
>>>>>> linting file internal/ifelse/rule.go
>>>>>> linting file internal/ifelse/target.go
>>>>>> linting file internal/ifelse/args.go
>>>>>> linting file main.go
>>>>> configure arguments limits
>>>>> configure banned chars
>>>>>> linting file formatter/ndjson.go
>>>>>> linting file formatter/plain.go
>>>>>> linting file formatter/sarif.go
>>>>>> linting file formatter/stylish.go
>>>>>> linting file formatter/default.go
>>>>>> linting file formatter/doc.go
>>>>>> linting file formatter/json.go
>>>>>> linting file formatter/severity.go
>>>>>> linting file formatter/unix.go
>>>>>> linting file formatter/checkstyle.go
>>>>>> linting file formatter/friendly.go
>>>>>> linting file internal/astutils/ast_utils.go
>>>>>> linting file cli/main.go
>>>>>> linting file cli/main_test.go
>>>>>> linting file lint/doc.go
>>>>>> linting file lint/file.go
>>>>>> linting file lint/formatter.go
>>>>>> linting file lint/package.go
>>>>>> linting file lint/linter_test.go
>>>>>> linting file lint/name_test.go
>>>>>> linting file lint/config.go
>>>>>> linting file lint/failure.go
>>>>>> linting file lint/filefilter.go
>>>>>> linting file lint/linter.go
>>>>>> linting file lint/name.go
>>>>>> linting file lint/rule.go
Error during linting:Invalid argument to the add-constant rule, expecting a k,v map. Got string
To resume, errgroup is (subjectively) simpler but still shares the same drawbacks of explicitly using channels. (and to be fair, these drawbacks are also present in the current -with panics- implementation of the linting machinery)
Anyway, the single significant point of my proposal is: use the already available failure mechanism instead of adding an error return value to rules.
There was a problem hiding this comment.
Thanks for your detailed reply and feedback.
I agree with your conclusion
👍
| r.configureOnce.Do(func() { configureErr = r.configure(arguments) }) | ||
| if configureErr != nil { | ||
| return []lint.Failure{lint.NewInternalFailure(configureErr.Error())} | ||
| } |
There was a problem hiding this comment.
This idea is great, but it doesn't address this
But it could be addressed separately
There was a problem hiding this comment.
Yes, I know. It is not my intention to address this problem right now.
IMO configureOnce is cumbersome (vs just lock/unlock) for dealing with errors
There was a problem hiding this comment.
Yes, I agree
Maybe linter could call configure if there is a Configure interface on the Rule
But, some tests will need to be rewritten
There was a problem hiding this comment.
I've studied before the alternative of adding a Configure function to rules: it simplifies configuration of rules but it has the drawback of changing the interface.
There was a problem hiding this comment.
My point was to add an interface for Configure only.
type Configure interface {
Configure() error
}
If detected, you call it after a type assertion.
But it's maybe part of what you considered
There was a problem hiding this comment.
ok @chavacava nice concept, can you deploy that changes for lint/* files without add_constant.go rule, and in #1126 I do the rest of job (add []lint.Failure{lint.NewInternalFailure(configureErr.Error())} where it is needed) :P
chavacava
changed the title
chavacava
changed the title
This is PR adds the necessary modifications in the linting machinery to allow removing panics from rules.
It leverages the failure mechanism to communicate errors thus no need to change the rules interface.
It has the nice property of not requiring all rules to be modified at once.