forbiddenmethod: add (disabled) lint against (*errgroup.Group).Go and go
#62243
Conversation
tbg
requested review from
a team and
erikgrinaker
and removed request for
a team
|
This change is |
There was a problem hiding this comment.
roachprod and workload where this isn't really a concern.
Reviewable status:
complete! 1 of 0 LGTMs obtained
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status:
tbg
changed the title
(*errgroup.Group).Go and go(*errgroup.Group).Go and go
|
Thanks for the quick reviews! I have some WIP to actually fix a bunch of the lint failures but figured it was better to merge this as-is first, with the lints there but not part of I also finally figured out how to make the |
There was a problem hiding this comment.
Reviewed 2 of 5 files at r2.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 2006 at r2 (raw file):
nakedGoroutineExceptions := `(` + strings.Join([]string{ `pkg/.*_test.go`,
Escape the ..
pkg/testutils/lint/passes/forbiddenmethod/naked_go.go, line 46 at r2 (raw file):
// The approach here is inspired by `analysistest.check` - the // nolint comment has to be on the same line as the *end* of the // `*GoStmt`.
Hm, this is a bit unfortunate -- for inline large-body goroutines it'd be most natural to put the nolint above it. Although I suppose we won't be using a whole lot of go keywords anyway, so maybe it's ok if the ergonomics are a bit off. Happy to have an escape hatch at all!
pkg/testutils/lint/passes/forbiddenmethod/naked_go.go, line 67 at r2 (raw file):
} } if cc != nil && strings.Contains(cc.Text, "nolint:"+nakedGoPassName) {
This won't handle comma-separated components, but maybe that's ok.
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 2006 at r2 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
Escape the
..
Done.
pkg/testutils/lint/passes/forbiddenmethod/naked_go.go, line 46 at r2 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
// The approach here is inspired by `analysistest.check` - the // nolint comment has to be on the same line as the *end* of the // `*GoStmt`.Hm, this is a bit unfortunate -- for inline large-body goroutines it'd be most natural to put the nolint above it. Although I suppose we won't be using a whole lot of go keywords anyway, so maybe it's ok if the ergonomics are a bit off. Happy to have an escape hatch at all!
I'll leave as is - I agree, but also it's not easy to make these things happen and as you noted possibly not worth it.
pkg/testutils/lint/passes/forbiddenmethod/naked_go.go, line 67 at r2 (raw file):
Previously, erikgrinaker (Erik Grinaker) wrote…
This won't handle comma-separated components, but maybe that's ok.
It will, you just have to say nolint:foo,nolint:bar but also, hopefully these never need to stack and if so, I'd rather spend time fixing it when the time comes.
There was a problem hiding this comment.
Reviewed 3 of 7 files at r1, 2 of 5 files at r2, 1 of 1 files at r3.
Reviewable status:
There was a problem hiding this comment.
Reviewed 3 of 5 files at r2, 1 of 1 files at r3.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 2033 at r3 (raw file):
`pkg/workload/`, `pkg/cli/systembench/`, `pkg/cmd/roachprod/`,
Feels to me like anything under /pkg/cmd can be ignored
There was a problem hiding this comment.
Thanks for the reviews! Going to shake out any remaining lint failures and merge.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 2033 at r3 (raw file):
Previously, knz (kena) wrote…
Feels to me like anything under
/pkg/cmdcan be ignored
Maybe, but once we hang correctness guarantees of this lint I don't know if we want to take any chances. And definitely roachtest will want this hygiene too (since it needs to recover from all panics in the subtests) and that too sits fully in cmd right now. I'll leave as is and we can revisit.
There was a problem hiding this comment.
Reviewable status:
pkg/testutils/lint/lint_test.go, line 2033 at r3 (raw file):
Previously, tbg (Tobias Grieger) wrote…
Maybe, but once we hang correctness guarantees of this lint I don't know if we want to take any chances. And definitely roachtest will want this hygiene too (since it needs to recover from all panics in the subtests) and that too sits fully in
cmdright now. I'll leave as is and we can revisit.
no objection from me - my approval stands.
…nd `go` This helps move the needle on cockroachdb#58164 by introducing linters that force both the use of a `ctxgroup` over an `errgroup` and prevent direct use of the `go` keyword. They are disabled by default because we need to fix the issues they find first. This can be done (for development) in `forbiddenmethod.Analyzers`. They can then also be invoked in a targeted fashion: ``` go vet -vettool ./bin/roachvet -errgroupgo ./pkg/somewhere go vet -vettool ./bin/roachvet -nakedgo ./pkg/somewhere ``` Release note: None
|
bors r=knz,erikgrinaker,jordanlewis |
|
Build succeeded: |
This helps move the needle on #58164 by introducing linters that
force both the use of a
ctxgroupover anerrgroupand preventdirect use of the
gokeyword.They are disabled by default because we need to fix the issues they
find first. This can be done (for development) in
forbiddenmethod.Analyzers. They can then also be invoked in a targetedfashion:
Release note: None