dev: new commands system

[ldez]

Part 2 of the rewrite and the death of the Executor.

First, this PR will be more complex to review than #4404 because of the complexity of the Executor.

I did my best to split changes into commits:

• a commit by one new command
• a commit by removing one old command

But as the previous code was very tangled, it's really complex to provide something easy to read.

What were the problems?

It will be complex to explain all the problems with the previous design of the Executor, so I will try to make a summary.

I was not implying inside golangci-lint dev when the Executor has been created, but based on git history I can give a short history.
The Executor was created when golangci-lint was just a single run command and this has a major importance.
At the moment, in this context, this was a good design.

The NewExecutor did a lot of things, but I will focus on 3 majors:

• the configuration loading
• the creation of the commands
• a (partial) building of the linters

The NewExecutor was called before everything.
This means that every command, even the version command, before running, will load the configuration and build linters.
In the past, this has led to breaking all the commands because of a linter panic or a configuration error.

The configuration structures were a mix of real configuration (Run, Linters, etc.) and specific needs of each command (Version, IsVerbose, etc.).

The configuration was filled with 2 different techniques (Cobra and Viper), after 3 parsing phases.
In detail, the first parsing (Cobra) was mainly for the logger (early access), the data obtained were used to initialize the configuration.
And then Viper was reading the configuration files by overriding the previous data.
The resulting extracted data was used as default values (because it was done before the creation of the commands) for the next and last parsing.
The flags were parsed (Cobra), through the commands, and partially overridden the previous data (I will explain later why I say "partially").

The linters were built before the full configuration was loaded.

And I didn't talk about the complexity of Executor itself: everything was linked together, and it was tough to know when something was used or loaded.

I think based on my previous explanation, you can now see the problems:

• performance issues
• very complex and unreliable configuration parsing
• timing issues that lead to load linters without the proper configuration
• impossible to fix something quickly, so very hard to maintain

PR Content

Now I will summarize the PR: it kills the Executor 😄

• the configuration structures contain only user configuration (except some test elements)
• the configuration is loaded only for commands that need the configuration
• the configuration is loaded only by Viper (via a simple binding)
  • replacement of config.Reader with config.Loader
• the linters are loaded only for commands that need linters
• the specific options of each command are handled by each command itself
• the flag sets are organized with simple functions (and without a link to the configuration)
• each command is independent of the other commands and manages its dependencies (lintersdb.Manager, lintersdb.EnabledSet, etc.)

Side Effect

I implemented a hack, no side effects now.

previous explanation

Now, all flags override the configuration from the file, not partially:
before, for some slice of string options (those usable with a flag), the values were appended instead of overridden.

So it's breaking.

Based on the code, this hack has been created to being able to merge linters from the files (linters.enable, linters.disable) with the flags one (-E, --enable, -D, --disable).
It was possible to add/append a linter to the current configuration (obtained by the file) with the command line.
I think it was interesting when only a few linters existed but the number of linter has increased since the beginning of golangci-lint.

Example:

file	flag	Old behavior	New behavior
govet	gofmt	govet,gofmt	gofmt

So I have several proposals:

• consider that as a breaking change -> major version
• find a hack to keep the same behavior:
  • the previous hack (fixSlicesFlags) used inside golangci-lint doesn't work anymore with the new way to handle the configuration because it was specific to Cobra.
  • based on issues I found, Viper doesn't support that.
    I tried several "pure" Viper hacks (ex: UnmarshalText) but it doesn't work.
  • I found a dirty hack: the duplication of the fields and the merge of the 2 fields during the load of the configuration:

  type Linters struct {
    FlagsEnable []string `mapstructure:"flags-enable"` // should never be used inside a configuration file
    Enable      []string
    // ...
  }

  • I found a less dirty hack but still not perfect: Viper loads the configuration and flags that are not slices. For the slices, we don't bind them with Viper, so we can have the value via Cobra.

  fs.StringSliceP("enable", "E", nil, wh("Enable specific linter"))
  
  // ...
  
  if cmd.Flags().Changed("enable") {
    val, err := cmd.Flags().GetStringSlice("enable")
    // ...
  }

• a mix of both previous proposals -> major version
• something else?

Next step

The rewrite of lintersdb.Manager to avoid the build of the linters 5 times by run.

--update-refs is still the best git rebase option ever!

[ldez]

Diff of the help commands to help the review:

go run ./cmd/golangci-lint --help

diff

17,24c17,20
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<   -h, --help                      help for golangci-lint
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -h, --help           help for golangci-lint
>   -v, --verbose        Verbose output
>       --version        Print version

---

go run ./cmd/golangci-lint cache --help

diff

15,21c15,16
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint cache clean --help

diff

10,16c10,11
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint cache status --help

diff

10,16c10,11
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint completion --help

diff

17,23c17,18
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint config --help

diff

14,20c14,15
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint config path --help

diff

10,16c10,11
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint help --help

diff

14,20c14,15
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint help linters --help

diff

10,16c10,11
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint linters --help

diff

18,24c18,19
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint run --help

diff

7,13d6
<       --out-format string              Format of output: colored-line-number|line-number|json|tab|checkstyle|code-climate|html|junit-xml|github-actions|teamcity (default "colored-line-number")
<       --print-issued-lines             Print lines of code with issue (default true)
<       --print-linter-name              Print linter name in issue line (default true)
<       --uniq-by-line                   Make issues output unique by line (default true)
<       --sort-results                   Sort linter results
<       --print-welcome                  Print welcome message
<       --path-prefix string             Path prefix to add to output
15a9,15
>   -D, --disable strings                Disable specific linter
>       --disable-all                    Disable all linters
>   -E, --enable strings                 Enable specific linter
>       --enable-all                     Enable all linters
>       --fast                           Enable only fast linters from enabled linters set (first run won't be fast)
>   -p, --presets strings                Enable presets (bugs|comment|complexity|error|format|import|metalinter|module|performance|sql|style|test|unused) of linters. Run 'golangci-lint help linters' to see them. This option implies option --disable-all
>   -j, --concurrency int                Number of CPUs to use (Default: number of logical CPUs) (default 16)
22d21
<       --print-resources-usage          Print avg and max memory usage of golangci-lint and total time
36,41c35,40
<   -D, --disable strings                Disable specific linter
<       --disable-all                    Disable all linters
<   -E, --enable strings                 Enable specific linter
<       --enable-all                     Enable all linters
<       --fast                           Enable only fast linters from enabled linters set (first run won't be fast)
<   -p, --presets strings                Enable presets (bugs|comment|complexity|error|format|import|metalinter|module|performance|sql|style|test|unused) of linters. Run 'golangci-lint help linters' to see them. This option implies option --disable-all
---
>       --out-format string              Format of output: colored-line-number|line-number|json|tab|checkstyle|code-climate|html|junit-xml|github-actions|teamcity (default "colored-line-number")
>       --print-issued-lines             Print lines of code with issue (default true)
>       --print-linter-name              Print linter name in issue line (default true)
>       --uniq-by-line                   Make issues output unique by line (default true)
>       --sort-results                   Sort linter results
>       --path-prefix string             Path prefix to add to output
89c88
<       --exclude-case-sensitive         If set to true exclude and exclude rules regular expressions are case sensitive
---
>       --exclude-case-sensitive         If set to true exclude and exclude rules regular expressions are case-sensitive
99a99,102
>       --cpu-profile-path string        Path to CPU profile output file
>       --mem-profile-path string        Path to memory profile output file
>       --print-resources-usage          Print avg and max memory usage of golangci-lint and total time
>       --trace-path string              Path to trace output file
103,109c106,107
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

---

go run ./cmd/golangci-lint version --help

diff

12,18c12,13
<       --color string              Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
<   -j, --concurrency int           Number of CPUs to use (Default: number of logical CPUs) (default 16)
<       --cpu-profile-path string   Path to CPU profile output file
<       --mem-profile-path string   Path to memory profile output file
<       --trace-path string         Path to trace output file
<   -v, --verbose                   Verbose output
<       --version                   Print version
---
>       --color string   Use color when printing; can be 'always', 'auto', or 'never' (default "auto")
>   -v, --verbose        Verbose output

[bombsimon]

Sorry for slow reaction, took a first quick pass! Mostly added some questions and some minor suggestions. Seems like a good improvement and split of tangled code. 👍

[ldez]

It's emotional for me to merge this PR, there is a lot of work behind it.
And the next step will also be great! The next PR will happen quickly!

[Antonboom]