Skip to content

dev: rewrite linters Manager #4419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 26 commits into from
Mar 2, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
6762019
fix: load linters only once
ldez Feb 23, 2024
13936c8
chore: simplify the linters loading
ldez Feb 23, 2024
9789895
chore: move configuration modification to the Loader
ldez Feb 23, 2024
c9648c1
chore: remove Validator from EnabledSet
ldez Feb 26, 2024
9fe1be4
chore: move all the validations at the same place
ldez Feb 26, 2024
cf874b4
chore: plugin builder
ldez Feb 26, 2024
b29e407
chore: linter builder
ldez Feb 26, 2024
88342f5
chore: fusion of Manager and EnableSet
ldez Feb 26, 2024
c4f9898
chore: use the new Manager
ldez Feb 26, 2024
20df06d
chore: improve error message
ldez Feb 27, 2024
81badef
chore: remove useless struct tags
ldez Feb 27, 2024
d30c3f2
review: dummy child return dummy
ldez Feb 27, 2024
2328668
review
ldez Feb 27, 2024
bfd4182
chore: sepration of concerns
ldez Feb 27, 2024
cf975a4
chore: separation of concerns: move validation to configuration struct
ldez Feb 27, 2024
df61470
chore: merge validateEnabledDisabledLintersConfig with Validate
ldez Feb 27, 2024
1d822ca
chore: fix a typo
ldez Feb 28, 2024
1fe1894
Apply suggestions from code review
ldez Feb 28, 2024
4647f2f
chore: linting
ldez Feb 28, 2024
3dafb43
docs: add godoc on Manager and the builders
ldez Feb 28, 2024
9d196f4
docs: add init section inside the architecture documentation
ldez Feb 28, 2024
45fef5d
review
ldez Feb 29, 2024
aaef7fc
chore: minor changes
ldez Feb 29, 2024
a701970
chore: remove the dummyLogger
ldez Mar 2, 2024
83c68d6
doc: update some pages
ldez Mar 2, 2024
ad6b6e3
review: improve error messages
ldez Mar 2, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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