Skip to content

Commit

Permalink
Remove concurrency from go/analysis analyzer
Browse files Browse the repository at this point in the history
There are currently several concurrency issues with the analyzer:

1. `ctx.SetFileInfo` and `c.Check` can be called concurrently. However,
   go-critic does not support multiple operations on the same context
   at once.

2. This issue also means that checkers may be called with a context
   containing the wrong file, because the checker being run in a
   goroutine may not be complete before `ctx.SetFileInfo` is called for
   the next file.

3. The same checker is called multiple times in parallel. This is not
   supported by go-critic, because checkers rely on non-synchronized
   local state.

It is also worth noting that these issues currently exist even if the
`concurrency` flag is set to 0. This is because registering files and
running checkers are still executed in parallel, even when concurrency
is disabled.

Ultimately, I do not see a good way to integrate concurrency, go-critic
and the `go/analysis` framework together, and the current implementation
is unsound. Therefore, I have removed the `concurrency` flag and
changed both the registration of files and running of checkers to run
sequentially.

One potential way to improve this in the future would be to provide a
separate `go/analysis` analyzer for each checker, similar to [how
staticcheck does](https://pkg.go.dev/honnef.co/go/tools@v0.4.7/staticcheck#pkg-variables).
This would allow several go-critic analyzers to be run in parallel
using the `go/analysis` framework. However, this would be a much greater
breaking change to go-critic's `go/analysis` API, so I have not
attempted this for now.
  • Loading branch information
zakcutner committed Apr 29, 2024
1 parent e429408 commit c0cb41f
Show file tree
Hide file tree
Showing 2 changed files with 10 additions and 40 deletions.
15 changes: 5 additions & 10 deletions checkers/analyzer/analyzer.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,6 @@
package analyzer

import (
"runtime"

"github.com/go-critic/go-critic/linter"

"golang.org/x/tools/go/analysis"
Expand All @@ -23,12 +21,11 @@ var Analyzer = &analysis.Analyzer{
var DisableCache = false

var (
flagGoVersion string
flagEnable string
flagDisable string
flagEnableAll bool
flagDebugInit bool
flagConcurrency int
flagGoVersion string
flagEnable string
flagDisable string
flagEnableAll bool
flagDebugInit bool
)

var (
Expand All @@ -50,8 +47,6 @@ func init() {
`comma-separated list of checkers to be disabled. Can include #tags`)
Analyzer.Flags.StringVar(&flagGoVersion, "go", "",
`select the Go version to target. Leave as string for the latest`)
Analyzer.Flags.IntVar(&flagConcurrency, "concurrency", runtime.GOMAXPROCS(0),
`how many checks to run concurrently (defaults to runtime.GOMAXPROCS(0))`)

for _, info := range registeredCheckers {
for pname, param := range info.Params {
Expand Down
35 changes: 5 additions & 30 deletions checkers/analyzer/run.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,44 +39,19 @@ func runAnalyzer(pass *analysis.Pass) (interface{}, error) {
return nil, err
}

doneCh := make(chan struct{})
diagCh := make(chan analysis.Diagnostic)
go func() {
for diag := range diagCh {
pass.Report(diag)
}
close(doneCh)
}()

sema := make(chan struct{}, flagConcurrency)
var wg sync.WaitGroup
wg.Add(len(pass.Files))

for _, f := range pass.Files {
f := f
filename := filepath.Base(pass.Fset.Position(f.Pos()).Filename)
ctx.SetFileInfo(filename, f)

sema <- struct{}{}
go func() {
defer func() {
wg.Done()
<-sema
}()

for _, c := range checkers {
warnings := c.Check(f)
for _, warning := range warnings {
diagCh <- asDiag(c, warning)
}
for _, c := range checkers {
warnings := c.Check(f)
for _, warning := range warnings {
pass.Report(asDiag(c, warning))
}
}()
}
}

wg.Wait()
close(diagCh)
<-doneCh

return nil, nil
}

Expand Down

0 comments on commit c0cb41f

Please sign in to comment.