Add errgroupctx linter

[leonklingele]

Repository URL

https://github.com/m-ocean-it/errgroup-ctx-lint

Analyzer Requirements

• The analyzer must be more than one month old.
• The usage of AI must be explicitly expressed inside the readme.
• It must use go/analysis.
• It must not be a duplicate or too close to an existing analyzer.
• It must have a Go version inside the go.mod <= 1.25.0.
• The dependencies must be minimal.
• It must be available as a plugin.

Analyzer Goal

Finds bugs

Analyzer Description

Quoting the README:

This linter catches cases when, within an error-group goroutine, a non-errgroup context is passed to a function or method, while there is a context, specifically attached to the said errgroup. In most cases, you want that specific context to be passed to functions invoked within the errgroup's Go methods.

eg, egCtx := errgroup.WithContext(ctx)

eg.Go(func() error {
	return doSmth(ctx) // want `errgroup callback should probably not reference outer context "ctx", use the errgroup-derived context "egCtx"`
})

eg.Go(func() error {
	return doSmth(egCtx) // Correctly uses the context returned by "errgroup.WithContext"
})

eg.TryTo(func() error {
	return doSmth(ctx) // want `errgroup callback should probably not reference outer context "ctx", use the errgroup-derived context "egCtx"`
})

Analyzer License

GPL-3.0-only

Analyzer License Checks

• The file must start with LICENSE (capitalized).
• The file(s) must contain the required information by the license, e.g.: author, year, etc.

Analyzer Details

• It must have tests.
• It must not contain init().
• It must not contain panic().
• It must not contain log.Fatal().
• It must not contain os.Exit().
• It must not modify the AST.
• The configuration must not rely on external files.

Repository Details

• It must have a semver tag, e.g.: v1.0.0, v0.1.0 (0.0.x are not valid).
• I understand that tags must never be deleted or recreated once published.
• It must have a readme.
• The repository must have a CI.
• The CI must run on PR.
• The CI must run on the default branch.

Repository Recommendations

• It should have linting.
• It should have a .gitignore (IDE files, binaries, OS files, etc. should not be committed).
• Use main as the default branch name.

Pull Request

• Yes, I will wait for the approval of a maintainer before creating a PR.

[leonklingele]

I have created a branch for the integration already: https://github.com/leonklingele/golangci-lint/commits/feat/add-errgroupctx/

[ldez]

The implementation is not right:

• The tests are not in the right place.
• The tests are not OS neutral.
• The CI runs only on Linux.
• The CI uses go get . to download dependencies
• The version of the actions are outdated
• The analyzer embedded a nolint directive parser (an analyzer must not do that)
• The package naming (with _) is not right
• Having a func_visitor package is not useful (the code doesn't need to be "protected")
• Exported constructor returns non-exported structure
• Usage of global variables
• No linting
• The idea of fake the errgroup is interesting but this is only a workaround, only for tests, that can be handled differently.
• Useless package cmd/err-group-ctx-lint, I guess this is because the name of the module (and repo) contains - and a not really conventional pattern.
• The dependencies are not up-to-date
• The plugin implementation is not isolated to there is an init() function
• There are wrong type assertion patterns (ex: leftVar, _ := leftObj.(*types.Var))

The idea is interesting, but the implementation is not Go idiomatic, elements must be removed, the name of the module and the repo should be cleaned.

[m-ocean-it]

Hi! Thank you for review!

The idea of fake the errgroup is interesting but this is only a workaround, only for tests, that can be handled differently.

I wonder if you're talking about the parameter that allows specifying a list of alternative errgroup-packages? I actually planned it as a feature from the get-go. In my organization there are several forks/wrappers of the original errgroup-package. I suspect it is like that in many other places. So, a parameter like that would be handy for the users

[ldez]

I think this is a niche use-case, but OK you can keep the option and the current test structure.

[m-ocean-it]

Hi there! Thank you for the PR. I merged it and, also, fixed almost everything else from the list.

Useless package cmd/err-group-ctx-lint, I guess this is because the name of the module (and repo) contains - and a not really conventional pattern.

I renamed it to errgroupctxlint, but other than that, I'm not sure, what concerns you here. Having a separate package for the executable is a well-known pattern. Here's the cyclop's cmd/cyclop package as an example.

The plugin implementation is not isolated to there is an init() function

I've hidden the implementation in a separate package called plugin. The init won't be triggered unless you import said package, which one only needs to do when building a custom binary of golangci-lint. Would that be fine? Or do you want the init to not be in the project? If so, I should probably move the plugin implementation to a separate repo.

There are wrong type assertion patterns (ex: leftVar, _ := leftObj.(*types.Var))

Such a pattern (anti-pattern?) followed by if leftVar != nil is a way to compactly combine type check with a nilness check. Is it really a problem? I suspect, nil should really not be present there, though.

[ldez]

I've hidden the implementation in a separate package called plugin.
Would that be fine?

yes this is fine, no need to extract the plugin to another module.

---

Such a pattern (anti-pattern?)

The following pattern is better:

leftVar, ok := leftObj.(*types.Var)
if !ok || leftVar == nil {
	continue
}

With Go a nil can be typed: https://go.dev/play/p/MbC3cCHmevJ

[m-ocean-it]

The following pattern is better

done m-ocean-it/errgroupctxlint#13

[ldez]

Note for @leonklingele: let's the author of an analyzer opening a discussion for its analyzer.

If a linter seems interesting to you, I recommend opening an issue on the analyzer repository to ask the author for doing the process.

Edit: an issue already exists and the author was on the road to follow the process.
You should not bypass the author: he didn't ask for help.

[m-ocean-it]

@ldez Hi there! Should I open a new discussion?

[ldez]

No, unless you want to do it.

There are still some design problems related to the configuration.
I will open a PR to fix them: m-ocean-it/errgroupctxlint#14

Note that, for now, I didn't evaluate the analyzer by itself, only the code.
This mean I didn't know if the analyzer fits behavior requirements.