-
Notifications
You must be signed in to change notification settings - Fork 113
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
checkers/analyzer: remove concurrency from go/analysis
analyzer
#1416
Conversation
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 1. 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.
@quasilyte @cristaloleg Sorry for the ping, I just wanted to ask if either of you might get a chance to take a look at this please? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@quasilyte are you ok with this change? |
go/analysis
analyzergo/analysis
analyzer
@quasilyte @cristaloleg Just bumping this, would it be possible to get this merged? |
I'm totally fine with it, I will merge it. Thank you for the PR and sorry for the delay 🙌 |
No problem, thanks for the help with this! |
There are currently several concurrency issues with the analyzer:
ctx.SetFileInfo
andc.Check
can be called concurrently. However, go-critic does not support multiple operations on the same context at once.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.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 1. 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 theconcurrency
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. This would allow several go-critic analyzers to be run in parallel using thego/analysis
framework. However, this would be a much greater breaking change to go-critic'sgo/analysis
API, so I have not attempted this for now.Closes #1414