Disable all golangci linters and then enable the ones we want

[mstoykov]

Status quo

Currently, we have golangci-lint config where we enable all linters, and then we go and disable a bunch of them. This has a few problems ranging from annoyance to in practice terrible UX unless the developer uses the make file to run the linters, which has other problems.

Basic update of golangci-lint

For starters each time we want to bump the version we currently have to discuss if any of the newly added (and now by our config enabled) linters should be disabled. Based on my memory - most of the times we disable all but a few. As most of them are just somebodies opinion as a linter with which we disagree. Also, most of the newly added linters have bugs which on our fairly big codebase we hit pretty heads on.

The (theoretical IMO) benefit here is that we might enable a really nice new linter.

The problem is that because we decided to do this at the same time we update the version, in practice we update the version very rarely. IMO this is mostly because someone now needs to go through the new linters, ask the team if they want them enabled or disabled ,wait for a week or two to get results back. And then make the changes and update - and ain't nobody got time for this.

We could probably just split the process in two : bump the version with all new linters disabled, and open an issue to discuss which one to enable back. This will help on this front but not on the others.

Due to the way we currently list the linters we do not want instead of the ones that we want to run, we will get breaking changes when one of those gets removed from golangci-lint. As far as I know this hasn't happened yet, but they have now deprecated a bunch and I would expect they will be removed at some point.

It also means that anyone running older version locally will get error if we disabled a linter that is from a newer one. This might be a good thing - making them update. But it also will break even if the newer version is just ... newer and hasn't actually added anything new that we use - which arguably is just bad UX.

The config does not tell you what we care about it tells you what we don't care about

The configuration will not tell anyone just by looking at it what linters we want to run, unless they know all the linters and can remove the one we don't care about.

This also has the problem of when we see a new linter - we don't know if we already have something similar enabled - you need to run golanci-lint linters and look at the output.

Proposal:

1. Make a PR that "reverses" the config - enables all the currently enabled linters and disables all else. Merge it
2. Make PR to update to the latest golangci-lint, and an issue with the new linters (maybe one per linter ?!?). Merge the PR
3. Over time discuss the above issues and enable any linter we find useful
4. Over time maybe discuss the currently enabled ones and see if we can disable some (we have both gofmt and gofumpt for example)
5. Use 2 + 3 as the way to update the golangci-lint and do that every once in a while - for example k6 release.

This way developers:

1. can update golangci-lint locally without suddenly getting lint issues we don't care about
2. if we want a particular linter and the version ran does not support it - we will get an error.
3. Users won't get errors if they run old versions that support everything we want but don't have a linter that we do not want.

p.s. the xk6-browser project already has done exactly this.

[codebien]

The list of the current disabled and not deprecated linters:

Disabled by your configuration linters:
containedctx: containedctx is a linter that detects struct contained context.Context field [fast: true, auto-fix: false]
decorder: check declaration order and count of types, constants, variables and functions [fast: true, auto-fix: false]
dupword: checks for duplicate words in the source code [fast: true, auto-fix: true]
execinquery: execinquery is a linter about query string checker in Query function which reads your Go src files and warning it finds [fast: false, auto-fix: false]
exhaustruct: Checks if all structure fields are initialized [fast: false, auto-fix: false]
gci: Gci controls golang package import order and makes it always deterministic. [fast: true, auto-fix: false]
ginkgolinter: enforces standards of using ginkgo and gomega [fast: false, auto-fix: false]
gochecknoinits: Checks that no init functions are present in Go code [fast: true, auto-fix: false]
gocyclo: Computes and checks the cyclomatic complexity of functions [fast: true, auto-fix: false]
godot: Check if comments end in a period [fast: true, auto-fix: true]
godox: Tool for detection of FIXME, TODO and other comment keywords [fast: true, auto-fix: false]
goerr113: Golang linter to check the errors handling expressions [fast: false, auto-fix: false]
goheader: Checks is file header matches to pattern [fast: true, auto-fix: false]
gomnd: An analyzer to detect magic numbers. [fast: true, auto-fix: false]
gomodguard: Allow and block list linter for direct Go module dependencies. This is different from depguard where there are different block types for example version constraints and module recommendations. [fast: true, auto-fix: false]
grouper: An analyzer to analyze expression groups. [fast: true, auto-fix: false]
interfacebloat: A linter that checks the number of methods inside an interface. [fast: true, auto-fix: false]
ireturn: Accept Interfaces, Return Concrete Types [fast: false, auto-fix: false]
loggercheck (logrlint): Checks key valur pairs for common logger libraries (kitlog,klog,logr,zap). [fast: false, auto-fix: false]
maintidx: maintidx measures the maintainability index of each function. [fast: true, auto-fix: false]
musttag: enforce field tags in (un)marshaled structs [fast: false, auto-fix: false]
nlreturn: nlreturn checks for a new line before return and branch statements to increase code clarity [fast: true, auto-fix: false]
nonamedreturns: Reports all named returns [fast: false, auto-fix: false]
tagliatelle: Checks the struct tags. [fast: true, auto-fix: false]
testableexamples: linter checks if examples are testable (have an expected output) [fast: true, auto-fix: false]
testpackage: linter that makes you use a separate _test package [fast: true, auto-fix: false]
thelper: thelper detects golang test helpers without t.Helper() call and checks the consistency of test helpers [fast: false, auto-fix: false]
varnamelen: checks that the length of a variable's name matches its scope [fast: false, auto-fix: false]
wrapcheck: Checks that errors returned from external packages are wrapped [fast: false, auto-fix: false]
wsl: Whitespace Linter - Forces you to use empty lines! [fast: true, auto-fix: false]

I quickly reviewed them and below are my suggestions.

Good to have:

• gomnd: I see it requested multiple times during our reviews

Something I think does make sense but not sure if we should enable it:

• decorder: it makes file redable and predictable, probably I would like to exclude this check You have to use parenthesis to declare e.g multiple global types inside a file.
• gochecknoinits: Potentially it would force us to have a good reason for doing it and to comment it.
• godot: It makes sense to be good writers but I don't think it is realistic probably so many false positives.
• interfacebloat: Still, it would force us to have a good reason for doing it and to comment on it.
• wrapcheck: Sounds interesting but I don't know if it is realistic for us.

Do you see a case for it?

• gomodguard

[mstoykov]

Thanks for the list @codebien 🙇

gomnd: I see it requested multiple times during our reviews

I would agree, but running it on the codebase returns 50 104(see last linter) current cases.

Going through them - huge amount is config defaults. And I don't think that will make those better. It will just now be a constant somewhere which will be "defaultBatchPerHost" and then if you need to look it up - you will need to jump to it. Repeat that 100x times.

Some of the others are file masks, which ... might be okay .. but I doubt we will have a package with file masks constants to imports so 🤷

And the majority of the rest magic numbers seem sensible.

The bigger problem for some of them is that they are not documented on what they mean. And adding a name will help, but not always.

So I am slightly 👎 at this points as this in practice requires more or less naming every constant number in the codebase.

p.s.: with the increased count even more of them are arguably false positives. I don't know if this is literally us catching this all the time and fixing it in PRs. Or just the amount of actually "good" magical numbers to be much higher in this codebase, but it seems like we have quite a lot of things that I would not change.

decorder

I don't really care about this. It gave me 0 errors currently ... which I find sus. Looking at the things it does:

• I don't really find having const before vars to be all the important. Arguably grouping them makes more sense.
• same for var, const blocks - it looks better with 5+ declarations but for 2-4 I think it makes things worse by adding 2 lines
• init on top is probably the most interesting. But to be honest ... I am the kind of developer that just don't read files top to bottom. Which is the same reason I find import block order or something like that to be 🤷

gochecknoinits

🤷 I find that init are hated way too much IMO. WE have 3 in the whole codebase , 1 in a test code that likely shouldn't be needed. One to facilitate extensions and one that I will remove as it does a thing that is bad ...even if it was not in an init 🤦

I am okay with this being added I guess. Not certain how useful it is.

godot

3 occurrences currently. Which also seems sus to be honest.

interfacebloat

I am okay with that. The two current occurrences are Runner and ExecutorConfig.

If anything I find the default 10 methods to be way too big for an interface.

wrapcheck

50 (sus) cases

A lot of those seem like false positives. And most of them are in the cloudapi and output and lib/executor/*. The only other places is the har converter which is deprecated.

I understand the idea, but I feel like in a lot of the cases this will just add 1-3 more lines everywhere for error handling.

And to be honest a bunch of the error cases are likely wrapped later on by a function in the calling chain. As most of those are in utility functions.

so I guess a 👍

326 cases I figured out there is a default limit 🤦 With this many cases I am slightly less sure. There are quire a lot of errors. And again most of them likely get wrapped later on.

And I don't really want our users to get an error that has been wrapped 5 times just to appease the linter.

So ... uh kind of 👎

gomodguard

I don't intend on linting our dependencies.