refactor: error management

[ccoVeille]

• refactor: avoid running rule once configuration failed
• refactor: remove deep-exit in lint/linter.go

Closes #1182

[mfederowicz]

ok someone can explain to me why and when implementing dedicated field in struct:

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

is better then implementing dedicated var configureErr error :

var configureErr error
	r.configureOnce.Do(func() { configureErr = r.configure(arguments) })

	if configureErr != nil {
		return newInternalFailureError(configureErr)
	}

iam just curious thats all :P

[ccoVeille]

sure, the problem is the fact that Apply is called on multiple files

On first run, it's the same thing.
But on next run, when another package is tried to be linted, the Apply is launched again.

This time the r.configureOnce won't be launched so the var configureErr (that is not in the struct) will be nil.
Then the if err != nil won't detect it, so the linter will be launched, more likely with setting that were not the one intended.

Now with the r.configureErr (the one in the struct), on initiializayion, it equals nil
On first run, when configureOnce is launched, we will store the error in r.configureErr
on next runs, the configureOnce is not launched, but r.configureErr is still equal to the error detected in the first run

[alexandear]

ok someone can explain to me why and when implementing dedicated field in struct:

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

is better then implementing dedicated var configureErr error :

From this article https://victoriametrics.com/blog/go-sync-once/index.html#what-is-synconce:

---

Also, if you need to handle errors that might come out of f, it can get a little awkward to write:

var once sync.Once
var config Config

func GetConfig() (Config, error) {
    var err error
    once.Do(func() {
        config, err = fetchConfig()
    })
    return config, err
}

This works well, but only the first time.

The thing is, if fetchConfig() fails, only the first call will receive the error. Any subsequent calls—second, third, and so on—won’t return that error. To make the behavior consistent across calls, we need to make err a package-scoped variable, like this:

var once sync.Once
var config Config
var err error

func GetConfig() (Config, error) {
    once.Do(func() {
        config, err = fetchConfig()
    })
    return config, err
}

[alexandear]

Could we add a test to demonstrate that the fix actually works?

I previously requested tests in this comment: #1126 (comment), but it seems to have been overlooked. It's likely that this test would catch the situation addressed by this PR.

[ccoVeille]

Of course.

I plan to add them, but I was waiting a nonnegative reaction, telling me I was breaking everything.

[ccoVeille]

I implemented the Configurable interface.

I had to export a few things, please let me know what you guys think about it.

Let me know what you think about it.

I didn't add the tests yet.

[ccoVeille]

Please note that except one rule (that could be refactored) the arguments of Apply no longer need the Arguments

Apply(file *lint.File, _ lint.Arguments)

[mfederowicz]

@ccoVeille what about with configureOnce sync.Once I saw that you removed it from all struct of rules using it in selected apply methods?

[ccoVeille]

@ccoVeille what about with configureOnce sync.Once I saw that you removed it from all struct of rules using it in selected apply methods?

I'm sorry, but I don't understand your question, especially if you are replying to my previous comment, or the whole changes I made

[mfederowicz]

@ccoVeille I understand concept of Configurable interface and when it is executed (at config level when iteration over list of rules, and if rule have r.(lint.ConfigurableRule) then r.Configure(ruleConfig.Arguments) is executed), but how many times it is executed? only once or many? sorry for my questions maybe my level of golang knowledge is not sufficient yet :(

[ccoVeille]

@ccoVeille I understand concept of Configurable interface and when it is executed (at config level when iteration over list of rules, and if rule have r.(lint.ConfigurableRule) then r.Configure(ruleConfig.Arguments) is executed), but how many times it is executed? only once or many? sorry for my questions maybe my level of golang knowledge is not sufficient yet :(

No problem, I just needed to know what you were asking for.

So this refactoring is about moving the calls to Configure in the code that computes all the available rules.

revive/config/config.go

Lines 152 to 156 in dfa9f2b

	if r, ok := r.(lint.ConfigurableRule); ok {
	if err := r.Configure(ruleConfig.Arguments); err != nil {
	return nil, fmt.Errorf("cannot configure rule: %s", name)
	}
	}

So it's done only once when the app starts, GetLintingRules is called only once

revive/revivelib/core.go

Line 54 in 45731a3

lintingRules, err := config.GetLintingRules(conf, extraRuleInstances)

[mfederowicz]

ok my question is because not all rules have configure methods only basic code in apply, for example rules: use_any, duplicated_imports, call_to_gc, and many others :P

[alexandear]

ok my question is because not all rules have configure methods only basic code in apply, for example rules: use_any, duplicated_imports, call_to_gc, and many others :P

We can add empty configure methods to all rules, regardless of whether they have parameters or not. These methods will simply do nothing.

[ccoVeille]

It would change the signature, and so we will need to update all rules.

This option was not considered as it would bring a lot of changes.

But, as I spotted the arguments in Apply is no longer needed (once we will do a small refactoring for one rule)

So now, I would say it's a good move.

What do you guys think?

[chavacava]

@ccoVeille thanks for the work on this PR.
I've ran your branch on huge code bases and it works fine. Do you plan to introduce other modifications or we can go ahead with the merge?

[ccoVeille]

It's up to you @chavacava

I was expecting to get a feedback from you about this

It would change the signature, and so we will need to update all rules.

This option was not considered as it would bring a lot of changes.

But, as I spotted the arguments in Apply is no longer needed (once we will do a small refactoring for one rule)

So now, I would say it's a good move.

What do you guys think?

So it would be to move from

This (currently in this PR)

// Rule defines an abstract rule interface
type Rule interface {
        Name() string
        Apply(*File, Arguments) []Failure
}

// ConfigurableRule defines an abstract configurable rule interface.
type ConfigurableRule interface {
        Configure(Arguments) error
}

To this

// Rule defines an abstract rule interface
type Rule interface {
        Name() string
        Apply(*File) []Failure
        Configure(Arguments) error
}

So this change

// Rule defines an abstract rule interface
type Rule interface {
        Name() string
-        Apply(*File, Arguments) []Failure
-}

-// ConfigurableRule defines an abstract configurable rule interface.
-type ConfigurableRule interface {
+       Apply(*File) []Failure
        Configure(Arguments) error
}

[ccoVeille]

We can of course merge as is, an iterate in another PR

[chavacava]

I suggest to merge as is, without changing the Rule interface.

[ccoVeille]

@ccoVeille thanks for the work on this PR.
I've ran your branch on huge code bases and it works fine. Do you plan to introduce other modifications or we can go ahead with the merge?

Also, I'm curious. Do you see any improvement in term of speed?

We are removing a lot of mutex.

[ccoVeille]

I suggest to merge as is, without changing the Rule interface.

Go for it, then.

[chavacava]

Also, I'm curious. Do you see any improvement in term of speed?

Indeed, I'm setting up some benchmarks, I'll share the results when done

[ccoVeille]

Oh I forgot to add tests ... 😭

[chavacava]

Comparing performance of new and previous version:
Spoil: new version is (statistically irrelevant) less performant (I suspect due to the sequential rule config that compensates the perfs gains due to removed mutex)

Benchmark on revive code base

hyperfine --ignore-failure --warmup 2 --runs 5 './revive -config revive.toml ./...' './revive-prev -config revive.toml ./...'
Benchmark 1: ./revive -config revive.toml ./...
  Time (mean ± σ):      1.337 s ±  0.026 s    [User: 3.416 s, System: 1.170 s]
  Range (min … max):    1.306 s …  1.371 s    5 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: ./revive-prev -config revive.toml ./...
  Time (mean ± σ):      1.334 s ±  0.014 s    [User: 3.479 s, System: 1.112 s]
  Range (min … max):    1.320 s …  1.350 s    5 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  ./revive-prev -config revive.toml ./... ran
    1.00 ± 0.02 times faster than ./revive -config revive.toml ./...

Benchmark on gitea

hyperfine --ignore-failure --warmup 2 --runs 5 './revive -config revive.toml ../deleteme/gitea/...' './revive-prev -config revive.toml ../deleteme/gitea/...'
Benchmark 1: ./revive -config revive.toml ../deleteme/gitea/...
  Time (mean ± σ):      8.912 s ±  0.032 s    [User: 26.932 s, System: 5.337 s]
  Range (min … max):    8.888 s …  8.966 s    5 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: ./revive-prev -config revive.toml ../deleteme/gitea/...
  Time (mean ± σ):      8.979 s ±  0.221 s    [User: 27.024 s, System: 5.453 s]
  Range (min … max):    8.729 s …  9.292 s    5 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  ./revive -config revive.toml ../deleteme/gitea/... ran
    1.01 ± 0.03 times faster than ./revive-prev -config revive.toml ../deleteme/gitea/...

Benchmark on forgejo

hyperfine --ignore-failure --warmup 2 --runs 5 './revive -config revive.toml ../deleteme/forgejo/...' './revive-prev -config revive.toml ../deleteme/forgejo/...'
Benchmark 1: ./revive -config revive.toml ../deleteme/forgejo/...
  Time (mean ± σ):      9.805 s ±  0.145 s    [User: 29.002 s, System: 5.650 s]
  Range (min … max):    9.625 s … 10.006 s    5 runs
 
  Warning: Ignoring non-zero exit code.
 
Benchmark 2: ./revive-prev -config revive.toml ../deleteme/forgejo/...
  Time (mean ± σ):      9.631 s ±  0.159 s    [User: 28.860 s, System: 5.611 s]
  Range (min … max):    9.447 s …  9.816 s    5 runs
 
  Warning: Ignoring non-zero exit code.
 
Summary
  ./revive-prev -config revive.toml ../deleteme/forgejo/... ran
    1.02 ± 0.02 times faster than ./revive -config revive.toml ../deleteme/forgejo/...