Skip to content

Commit

Permalink
dev: rewrite linters Manager (#4419)
Browse files Browse the repository at this point in the history
  • Loading branch information
ldez authored Mar 2, 2024
1 parent 26f8088 commit b14d05c
Show file tree
Hide file tree
Showing 27 changed files with 1,749 additions and 1,825 deletions.
42 changes: 25 additions & 17 deletions docs/src/docs/contributing/architecture.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,18 @@ graph LR

</ResponsiveContainer>

## Init

The configuration is loaded from file and flags by `config.Loader` inside `PersistentPreRun` (or `PreRun`) of the commands that require configuration.

The linter database (`linterdb.Manager`) is fill based on the configuration:
- The linters ("internals" and plugins) are built by `linterdb.LinterBuilder` and `linterdb.PluginBuilder` builders.
- The configuration is validated by `linterdb.Validator`.

## Load Packages

Loading packages is listing all packages and their recursive dependencies for analysis.
Also, depending on the enabled linters set some parsing of the source code can be performed
at this step.
Also, depending on the enabled linters set some parsing of the source code can be performed at this step.

Packages loading starts here:

Expand Down Expand Up @@ -79,10 +86,10 @@ and outputs list of packages and requested information about them: filenames, ty

First, we need to find all enabled linters. All linters are registered here:

```go title=pkg/lint/lintersdb/manager.go
func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
```go title=pkg/lint/lintersdb/builder_linter.go
func (b LinterBuilder) Build(cfg *config.Config) []*linter.Config {
// ...
linters = append(linters,
return []*linter.Config{
// ...
linter.NewConfig(golinters.NewBodyclose()).
WithSince("v1.18.0").
Expand All @@ -97,26 +104,25 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
WithPresets(linter.PresetBugs, linter.PresetMetaLinter).
WithAlternativeNames("vet", "vetshadow").
WithURL("https://pkg.go.dev/cmd/vet"),
// ...
}
// ...
}
```

We filter requested in config and command-line linters in `EnabledSet`:

```go title=pkg/lint/lintersdb/enabled_set.go
func (es EnabledSet) GetEnabledLintersMap() (map[string]*linter.Config, error)
```go title=pkg/lint/lintersdb/manager.go
func (m *Manager) GetEnabledLintersMap() (map[string]*linter.Config, error)
```

We merge enabled linters into one `MetaLinter` to improve execution time if we can:

```go title=pkg/lint/lintersdb/enabled_set.go
// GetOptimizedLinters returns enabled linters after optimization (merging) of multiple linters
// into a fewer number of linters. E.g. some go/analysis linters can be optimized into
// one metalinter for data reuse and speed up.
func (es EnabledSet) GetOptimizedLinters() ([]*linter.Config, error) {
```go titlepkg/lint/lintersdb/manager.go
// GetOptimizedLinters returns enabled linters after optimization (merging) of multiple linters into a fewer number of linters.
// E.g. some go/analysis linters can be optimized into one metalinter for data reuse and speed up.
func (m *Manager) GetOptimizedLinters() ([]*linter.Config, error) {
// ...
es.combineGoAnalysisLinters(resultLintersSet)
m.combineGoAnalysisLinters(resultLintersSet)
// ...
}
```
Expand All @@ -133,9 +139,11 @@ type MetaLinter struct {
Currently, all linters except `unused` can be merged into this meta linter.
The `unused` isn't merged because it has high memory usage.

Linters execution starts in `runAnalyzers`. It's the most complex part of the `golangci-lint`.
We use custom [go/analysis](https://pkg.go.dev/golang.org/x/tools/go/analysis) runner there. It runs as much as it can in parallel. It lazy-loads as much as it can
to reduce memory usage. Also, it sets all heavyweight data to `nil` as becomes unneeded to save memory.
Linters execution starts in `runAnalyzers`.
It's the most complex part of the `golangci-lint`.
We use custom [go/analysis](https://pkg.go.dev/golang.org/x/tools/go/analysis) runner there.
It runs as much as it can in parallel. It lazy-loads as much as it can to reduce memory usage.
Also, it sets all heavyweight data to `nil` as becomes unneeded to save memory.

We don't use existing [multichecker](https://pkg.go.dev/golang.org/x/tools/go/analysis/multichecker) because
it doesn't use caching and doesn't have some important performance optimizations.
Expand Down
4 changes: 2 additions & 2 deletions docs/src/docs/contributing/new-linters.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ After that:
Look at other linters in this directory.
Implement linter integration and check that test passes.
3. Add the new struct for the linter (which you've implemented in `pkg/golinters/{yourlintername}.go`) to the
list of all supported linters in [`pkg/lint/lintersdb/manager.go`](https://github.com/golangci/golangci-lint/blob/master/pkg/lint/lintersdb/manager.go)
to the function `GetAllSupportedLinterConfigs`.
list of all supported linters in [`pkg/lint/lintersdb/builder_linter.go`](https://github.com/golangci/golangci-lint/blob/master/pkg/lint/lintersdb/builder_linter.go)
to the method `LinterBuilder.Build`.
- Add `WithSince("next_version")`, where `next_version` must be replaced by the next minor version. (ex: v1.2.0 if the current version is v1.1.0)
4. Find out what options do you need to configure for the linter.
For example, `nakedret` has only 1 option: [`max-func-lines`](https://github.com/golangci/golangci-lint/blob/master/.golangci.reference.yml).
Expand Down
14 changes: 11 additions & 3 deletions pkg/commands/help.go
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ func newHelpCommand(logger logutils.Log) *helpCommand {
Args: cobra.NoArgs,
ValidArgsFunction: cobra.NoFileCompletions,
Run: c.execute,
PreRun: c.preRun,
PreRunE: c.preRunE,
},
)

Expand All @@ -50,10 +50,18 @@ func newHelpCommand(logger logutils.Log) *helpCommand {
return c
}

func (c *helpCommand) preRun(_ *cobra.Command, _ []string) {
func (c *helpCommand) preRunE(_ *cobra.Command, _ []string) error {
// The command doesn't depend on the real configuration.
// It just needs the list of all plugins and all presets.
c.dbManager = lintersdb.NewManager(config.NewDefault(), c.log)
dbManager, err := lintersdb.NewManager(c.log.Child(logutils.DebugKeyLintersDB), config.NewDefault(),
lintersdb.NewPluginBuilder(c.log), lintersdb.NewLinterBuilder())
if err != nil {
return err
}

c.dbManager = dbManager

return nil
}

func (c *helpCommand) execute(_ *cobra.Command, _ []string) {
Expand Down
15 changes: 9 additions & 6 deletions pkg/commands/linters.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,8 +27,7 @@ type lintersCommand struct {

log logutils.Log

dbManager *lintersdb.Manager
enabledLintersSet *lintersdb.EnabledSet
dbManager *lintersdb.Manager
}

func newLintersCommand(logger logutils.Log, cfg *config.Config) *lintersCommand {
Expand Down Expand Up @@ -65,15 +64,19 @@ func (c *lintersCommand) preRunE(cmd *cobra.Command, _ []string) error {
return fmt.Errorf("can't load config: %w", err)
}

c.dbManager = lintersdb.NewManager(c.cfg, c.log)
c.enabledLintersSet = lintersdb.NewEnabledSet(c.dbManager,
lintersdb.NewValidator(c.dbManager), c.log.Child(logutils.DebugKeyLintersDB), c.cfg)
dbManager, err := lintersdb.NewManager(c.log.Child(logutils.DebugKeyLintersDB), c.cfg,
lintersdb.NewPluginBuilder(c.log), lintersdb.NewLinterBuilder())
if err != nil {
return err
}

c.dbManager = dbManager

return nil
}

func (c *lintersCommand) execute(_ *cobra.Command, _ []string) error {
enabledLintersMap, err := c.enabledLintersSet.GetEnabledLintersMap()
enabledLintersMap, err := c.dbManager.GetEnabledLintersMap()
if err != nil {
return fmt.Errorf("can't get enabled linters: %w", err)
}
Expand Down
21 changes: 12 additions & 9 deletions pkg/commands/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -80,8 +80,7 @@ type runCommand struct {

buildInfo BuildInfo

dbManager *lintersdb.Manager
enabledLintersSet *lintersdb.EnabledSet
dbManager *lintersdb.Manager

log logutils.Log
debugf logutils.DebugFunc
Expand Down Expand Up @@ -171,9 +170,13 @@ func (c *runCommand) persistentPostRunE(_ *cobra.Command, _ []string) error {
}

func (c *runCommand) preRunE(_ *cobra.Command, _ []string) error {
c.dbManager = lintersdb.NewManager(c.cfg, c.log)
c.enabledLintersSet = lintersdb.NewEnabledSet(c.dbManager,
lintersdb.NewValidator(c.dbManager), c.log.Child(logutils.DebugKeyLintersDB), c.cfg)
dbManager, err := lintersdb.NewManager(c.log.Child(logutils.DebugKeyLintersDB), c.cfg,
lintersdb.NewPluginBuilder(c.log), lintersdb.NewLinterBuilder())
if err != nil {
return err
}

c.dbManager = dbManager

c.goenv = goutil.NewEnv(c.log.Child(logutils.DebugKeyGoEnv))

Expand Down Expand Up @@ -340,12 +343,12 @@ func (c *runCommand) runAndPrint(ctx context.Context, args []string) error {
func (c *runCommand) runAnalysis(ctx context.Context, args []string) ([]result.Issue, error) {
c.cfg.Run.Args = args

lintersToRun, err := c.enabledLintersSet.GetOptimizedLinters()
lintersToRun, err := c.dbManager.GetOptimizedLinters()
if err != nil {
return nil, err
}

enabledLintersMap, err := c.enabledLintersSet.GetEnabledLintersMap()
enabledLintersMap, err := c.dbManager.GetEnabledLintersMap()
if err != nil {
return nil, err
}
Expand All @@ -361,8 +364,8 @@ func (c *runCommand) runAnalysis(ctx context.Context, args []string) ([]result.I
}
lintCtx.Log = c.log.Child(logutils.DebugKeyLintersContext)

runner, err := lint.NewRunner(c.cfg, c.log.Child(logutils.DebugKeyRunner),
c.goenv, c.enabledLintersSet, c.lineCache, c.fileCache, c.dbManager, lintCtx.Packages)
runner, err := lint.NewRunner(c.log.Child(logutils.DebugKeyRunner),
c.cfg, c.goenv, c.lineCache, c.fileCache, c.dbManager, lintCtx.Packages)
if err != nil {
return nil, err
}
Expand Down
4 changes: 2 additions & 2 deletions pkg/commands/version.go
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,8 @@ type versionInfo struct {
}

type versionOptions struct {
Format string `mapstructure:"format"`
Debug bool `mapstructure:"debug"`
Format string
Debug bool
}

type versionCommand struct {
Expand Down
40 changes: 28 additions & 12 deletions pkg/config/config.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
package config

import (
"errors"
"fmt"
"os"
"regexp"
"strings"

hcversion "github.com/hashicorp/go-version"
Expand Down Expand Up @@ -33,18 +32,16 @@ func (c *Config) GetConfigDir() string {
}

func (c *Config) Validate() error {
for i, rule := range c.Issues.ExcludeRules {
if err := rule.Validate(); err != nil {
return fmt.Errorf("error in exclude rule #%d: %w", i, err)
}
validators := []func() error{
c.Issues.Validate,
c.Severity.Validate,
c.LintersSettings.Validate,
c.Linters.Validate,
}

if len(c.Severity.Rules) > 0 && c.Severity.Default == "" {
return errors.New("can't set severity rule option: no default severity defined")
}
for i, rule := range c.Severity.Rules {
if err := rule.Validate(); err != nil {
return fmt.Errorf("error in severity rule #%d: %w", i, err)
for _, v := range validators {
if err := v(); err != nil {
return err
}
}

Expand Down Expand Up @@ -90,3 +87,22 @@ func detectGoVersion() string {

return "1.17"
}

// Trims the Go version to keep only M.m.
// Since Go 1.21 the version inside the go.mod can be a patched version (ex: 1.21.0).
// The version can also include information which we want to remove (ex: 1.21alpha1)
// https://go.dev/doc/toolchain#versions
// This a problem with staticcheck and gocritic.
func trimGoVersion(v string) string {
if v == "" {
return ""
}

exp := regexp.MustCompile(`(\d\.\d+)(?:\.\d+|[a-z]+\d)`)

if exp.MatchString(v) {
return exp.FindStringSubmatch(v)[1]
}

return v
}
49 changes: 49 additions & 0 deletions pkg/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -84,3 +84,52 @@ func TestIsGoGreaterThanOrEqual(t *testing.T) {
})
}
}

func Test_trimGoVersion(t *testing.T) {
testCases := []struct {
desc string
version string
expected string
}{
{
desc: "patched version",
version: "1.22.0",
expected: "1.22",
},
{
desc: "minor version",
version: "1.22",
expected: "1.22",
},
{
desc: "RC version",
version: "1.22rc1",
expected: "1.22",
},
{
desc: "alpha version",
version: "1.22alpha1",
expected: "1.22",
},
{
desc: "beta version",
version: "1.22beta1",
expected: "1.22",
},
{
desc: "semver RC version",
version: "1.22.0-rc1",
expected: "1.22",
},
}

for _, test := range testCases {
test := test
t.Run(test.desc, func(t *testing.T) {
t.Parallel()

version := trimGoVersion(test.version)
assert.Equal(t, test.expected, version)
})
}
}
10 changes: 10 additions & 0 deletions pkg/config/issues.go
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,16 @@ type Issues struct {
NeedFix bool `mapstructure:"fix"`
}

func (i *Issues) Validate() error {
for i, rule := range i.ExcludeRules {
if err := rule.Validate(); err != nil {
return fmt.Errorf("error in exclude rule #%d: %w", i, err)
}
}

return nil
}

type ExcludeRule struct {
BaseRule `mapstructure:",squash"`
}
Expand Down
Loading

0 comments on commit b14d05c

Please sign in to comment.