Hm, that's kinda interesting. @quasilyte do we share context between checks?

@zakcutner can you share the full stacktrace for this data race please?

@cristaloleg Unfortunately, I don't think I can share the stack trace dump of all goroutines, since this is from a private project, but here is the trace from the goroutine where the panic occurred:

github.com/go-critic/go-critic/checkers.(*importShadowChecker).VisitLocalDef(0xc005a150f0, {0xc000f7e380?, 0xc0041a1d60?, 0x415005?}, {0x10?, 0xc00817ddb0?})
	external/com_github_go_critic_go_critic/checkers/importShadow_checker.go:34 +0x6d
github.com/go-critic/go-critic/checkers/internal/astwalk.(*localDefWalker).walkSignature(0xc005a127b0, 0xc000f75920)
	external/com_github_go_critic_go_critic/checkers/internal/astwalk/local_def_walker.go:103 +0x187
github.com/go-critic/go-critic/checkers/internal/astwalk.(*localDefWalker).walkFunc(0xc005a127b0, 0xc000f75920)
	external/com_github_go_critic_go_critic/checkers/internal/astwalk/local_def_walker.go:25 +0x1d
github.com/go-critic/go-critic/checkers/internal/astwalk.(*localDefWalker).WalkFile(0xc005a127b0, 0x0?)
	external/com_github_go_critic_go_critic/checkers/internal/astwalk/local_def_walker.go:20 +0x98
github.com/go-critic/go-critic/linter.(*Checker).Check(...)
	external/com_github_go_critic_go_critic/linter/linter.go:160
github.com/go-critic/go-critic/checkers/analyzer.runAnalyzer.func2()
	external/com_github_go_critic_go_critic/checkers/analyzer/run.go:68 +0x115
created by github.com/go-critic/go-critic/checkers/analyzer.runAnalyzer in goroutine 175
	external/com_github_go_critic_go_critic/checkers/analyzer/run.go:61 +0x357

I've also just found another concurrent map access panic that looks quite similar in the ifElseChain checker:

github.com/go-critic/go-critic/checkers.(*ifElseChainChecker).countIfelseLen(...)
	external/com_github_go_critic_go_critic/checkers/ifElseChain_checker.go:97
github.com/go-critic/go-critic/checkers.(*ifElseChainChecker).checkIfStmt(0xc002454990, 0xc0064f29c0?)
	external/com_github_go_critic_go_critic/checkers/ifElseChain_checker.go:80 +0x39
github.com/go-critic/go-critic/checkers.(*ifElseChainChecker).VisitStmt(0xc002454990, {0xf9d700?, 0xc000fcbc40?})
	external/com_github_go_critic_go_critic/checkers/ifElseChain_checker.go:75 +0x8c
github.com/go-critic/go-critic/checkers/internal/astwalk.(*stmtWalker).WalkFile.func1({0xf9b0f8?, 0xc000fcbc40?})
	external/com_github_go_critic_go_critic/checkers/internal/astwalk/stmt_walker.go:23 +0x5b
go/ast.inspector.Visit(0xc0067842a0, {0xf9b0f8?, 0xc000fcbc40?})
	GOROOT/src/go/ast/walk.go:386 +0x2b
go/ast.Walk({0xf977e0?, 0xc0067842a0?}, {0xf9b0f8, 0xc000fcbc40})
	GOROOT/src/go/ast/walk.go:51 +0x4c
go/ast.walkStmtList(...)
	GOROOT/src/go/ast/walk.go:32
go/ast.Walk({0xf977e0?, 0xc0067842a0?}, {0xf9ac48, 0xc000f8db00})
	GOROOT/src/go/ast/walk.go:234 +0x31b7
go/ast.Inspect(...)
	GOROOT/src/go/ast/walk.go:397
github.com/go-critic/go-critic/checkers/internal/astwalk.(*stmtWalker).WalkFile(0xc000f04800, 0xc00081f900)
	external/com_github_go_critic_go_critic/checkers/internal/astwalk/stmt_walker.go:21 +0x10e
github.com/go-critic/go-critic/linter.(*Checker).Check(...)
	external/com_github_go_critic_go_critic/linter/linter.go:160
github.com/go-critic/go-critic/checkers/analyzer.runAnalyzer.func2()
	external/com_github_go_critic_go_critic/checkers/analyzer/run.go:68 +0x115
created by github.com/go-critic/go-critic/checkers/analyzer.runAnalyzer in goroutine 187
	external/com_github_go_critic_go_critic/checkers/analyzer/run.go:61 +0x357

Just in case it's relevant, I'm using nogo to run go-critic.

This looks like a gocritic integration issue to me.
None of the checkers update the context object, they only read from it.
If writes happen at these times, it means that the app itself doesn't synchronize the read/write operations.

@quasilyte Thanks for taking a look at this!

If writes happen at these times, it means that the app itself doesn't synchronize the read/write operations.

If I'm understanding correctly, the maps in the panics above are internal to go-critic? How should apps be synchronising reads/writes to these maps?

If you might have some pointers or example code for what integrations should be doing, I'd be happy to take a look at the nogo integration to see if this is the issue.

The linters need to be executed after the information is committed to the context.
In other words, not metadata should be added to the context while linters that use that information are executed.

Tbh, go-critic was created before go/analysis, therefore it expects more flow control from the application.
You can take a look how gocritic cmd works.

In my opinion, it would be bad to add synchronization to every read operation inside linters.
It makes sense to collect metadata (write) and then run linters over it (read) in my opinion.
No extra synchronizations should be required.
If both of these things are mixed together (indexing and linting), things can get messy.

@cristaloleg there are 2 contexts: shared and per-checker contexts.
The shared context contains the package info (types, etc.).
It's not possible to check a package that is not "loaded" into the context.
The problem can occur if linters are being executed on package A while package B is being loaded and added to the context.
The context should be updated only when linters are not being executed (otherwise you a get read+write race).

The basic execution loop looks like this (pseudo code!):

for _, pkg := range packagesToCheck {
  info := loadPackageInfo(pkg)
  ctx := createContext(info)
  done := runLintersInParallel(ctx)
  <-done
}

The parallelism can happen at both load and run phases, but they should not cross each other.
Load, then run.

Thanks for the helpful explanations, I think I understand this better now.

I'm using the go/analysis integration (through nogo), which I think might not adhere to this since the context is modified while checkers are being run: checkers/analyzer/run.go#L55-L74? Should this be changed to first finish calling ctx.SetFileInfo(filename, f) on all files before calling c.Check(f) on each file in parallel?

If I'm understanding correctly, I think the ifElseChain checker may still have a problem though, since its visited map seems to be mutated in its visitor: checkers/ifElseChain_checker.go#L97. Could the panic above be caused by this checker being run in parallel on multiple files, causing the visited map to be written to concurrently? If so, should this particular map be protected with a mutex (or replaced with a sync.Map) to prevent this happening?

If I'm understanding correctly, I think the ifElseChain checker may still have a problem though, since its visited map seems to be mutated in its visitor

Yes, that's because every instantiated checker is expected to be run on a dedicated goroutine. It's a way for us to have a state that doesn't need sync and doesn't need to be shared. I really don't like the idea of protecting every state possible with a sync mechanism. We have multiple layers to avoid redundant sync. It's a pity that go/analysis makes it very awkward (with that being said, go/analysis is not efficient, it's very slow if you compare it to the "old ways").

Originally, the parallelism model for gocritic looked like this: run every checker in a dedicated goroutine. So N checkers would result in N goroutines.

Yes, that's because every instantiated checker is expected to be run on a dedicated goroutine.

Ah, I see! So the go/analysis integration will also need to be updated to respect this (i.e. use a goroutine per checker, rather than a goroutine per file)?

In my opinion, it would be easier for you to check out the way golangci-lint does it.
It also uses go/analysis for that.

Although I personally don't like the fact that it creates checkers per every package but I guess that's the only way to make it work with go/analysis.

Makes sense! Would you be opposed to a PR to the go/analysis implementation in this project under checkers/analyzer, to make it behave more like the one in golangci-lint? It's of course possible to write my own implementation in my project using go-critic's regular API, but I think it would be most useful to improve go-critic's own go/analysis implementation, so it can be used directly.

It depends on whether it can be objectively seen as an improvement.

Okay, I think that might be easiest to discuss with some code—I'll open a draft PR to suggest some potential improvements.

Thanks for all the advice! I've opened pull request #1416 to discuss this further, let me know your thoughts.