proposal: enable staticcheck

[p0mvn]

Background

I noticed that our linters do not catch unused variables. Upon running staticcheck with:

staticcheck $(go list ./...)

I discovered around 30 lint issues related to unused code. We should consider enabling staticcheck as this is a useful static analysis tool.

Suggested Design

We can enable it via golanglint-ci without much effort:

osmosis/.golangci.yml

Line 71 in a785de1

# - staticcheck <- later

Acceptance Criteria

• staticcheck is enabled
• all dead code / unused variables are removed

[p0mvn]

I can go ahead and enable it but I just wanted to ask first if there is a reason for disabling it in the first place? Is everyone okay with me enabling this linter?

[ValarDragon]

IIRC, staticcheck is super slow and warns for deprecations

would prefer not having a linter that takes many seconds. Linting should feel almost instant.

[p0mvn]

You're right. It's slow. Took 5 seconds on the first pass. Reused cache a second time, taking approximately 1 second.

To mitigate, what do you think about the following:

• adding a makefile step that is separate from our main golangci-lint linter i.e make staticcheck
  • This should only be used if someone sees CI failing and wants to reproduce locally
  • Our regular linting is done separately by golangci-lint where it has staticcheck disabled
• separate CI job for staticcheck that runs in parallel with other jobs
  • Our bottlenecks are tests, both e2e and unit so 5 seconds should not be a problem
• Disabling noise like deprecation linter and anything else we might see later

I just shot myself in the foot in #1857 by leaving a lot of unused code. Since we tend to be working on many PRs concurrently while waiting on feedback, it's easy to miss things like this when context switching.

staticcheck in CI should be able to automate away this cognitive overhead for both the PR author and the reviewer.

[ValarDragon]

Why not just add the deadcode linter? Just does this one thing and is pretty fast

[p0mvn]

I've been framing this in terms of the dead code detection benefit so far because of recency bias.

However, staticcheck is also useful for the variety of other checks it supports, helping spot erroneous errors and maintain code standards. It would also help reduce manual linting in PRs.

With only a quick skim, these are the linters that I've seen to be manually pointed out in our reviews that can be covered by staticcheck instead:

• Simplify error construction with fmt.Errorf - https://staticcheck.io/docs/checks/#S1028
• Merge variable declaration and assignment - https://staticcheck.io/docs/checks/#S1021
• Convert if/else-if chain to tagged switch - https://staticcheck.io/docs/checks/#QF1003
• The documentation of an exported function should start with the function’s name - https://staticcheck.io/docs/checks/#ST1020
• Simplify returning boolean expression - https://staticcheck.io/docs/checks/#S1008

Since our goal is to minimize manual linting in the code reviews and offset this work to linter, staticcheck can help, reducing the workload on the reviewer.

It would also help standardize the coding standards/styles across the repository. Currently, the styles are inconsistent, reducing the overall readability. We can always remove linters that we don't like as we go along.

From my experience, staticcheck has proved to be a useful tool for catching erroneous errors. It has also been helpful in pointing out the details and optimizations of Go, helping me learn.

[ValarDragon]

rip, I'm torn.

I remember ~4 years ago with SDK development, static check frequently making my computer unusable at lint on save, hence not wanting it. I also remember it being far slower than 5 seconds (it was in the 30 second to minutes regime, and would halt youtube in the background lol), so its possible theres been massive speedups / resource usage improvements since.

I find it even more annoying to have multiple lint commands though, I want my editor to handle most linting annoyances for me. I would vastly prefer just one linting process that my editor does on save. Not sure 5 seconds is an ok overhead for that though.

[ValarDragon]

Just checked:

• On my laptop first run takes 16 seconds
• Second run immediately after took 1.5 ish seconds

It had a 5x speedup in 2019: https://staticcheck.io/changes/2019.2/ (Look at those old cockroach db stats! SDK was similar), and another big one in 2020: https://staticcheck.io/changes/2020.2/ , so thats why the recollection of minutes / my system getting unusable is outdated. (and im on faster hardware now lol)

Of the lints you were listing, I'm not totally sold on a lot of them being important or worth the slowdown. (The godoc one is an anti-feature imo! People consistently fought against it in golint and it got removed from many repos IIRC)

I feel mediumly strongly that there should be:

• Only one linting process for main
• Should be blazingly quick (It should be doable on save, and I don't think about it)
  • I'm open to playing around with staticcheck for a week or two, and see if its current speed with caching satisfies being "blazingly quick"

Maybe its the case that static check on a small number of items is fast! Or that we can have offload some of these goals to other linters.

I'm down for a second pre-release or pre-tag linting process thats slower as well

cc @alexanderbez for further thoughts

[ValarDragon]

Looking into this more, the golangci-lint gosimple linter is a subset of the staticcheck linters, namely these: https://github.com/dominikh/go-tools/blob/master/simple/doc.go (Every lint that is S####)

Which of the ones you listed is most of them, and has ~no execution time tradeoff. If we want stylecheck, thats also separated out into its own linter with golangci-lint. Similar for QF as "QuickFix". (I'm unconvinced of these atm, but should look at the diff to see!)

Perhaps we should go with the approach of enabling the components of staticcheck we want via golangci-lint?
(Though if its in the couple second regime, and no longer the 1+ minute, may just be fine to add everything, down for us to try that and see if it causes problems)

[p0mvn]

Good catch, seems that there are 4 components:

• staticcheck: https://github.com/dominikh/go-tools/blob/331bcca40621580531c9b8a9cb7d5c2c06c709d2/staticcheck/analysis.go#L23
• stylecheck: https://github.com/dominikh/go-tools/blob/331bcca40621580531c9b8a9cb7d5c2c06c709d2/stylecheck/analysis.go#L14
• simple: https://github.com/dominikh/go-tools/blob/331bcca40621580531c9b8a9cb7d5c2c06c709d2/simple/analysis.go#L11
• unused: https://github.com/dominikh/go-tools/blob/331bcca40621580531c9b8a9cb7d5c2c06c709d2/unused/unused.go#L418

From what I understand, there is a staticcheck binary that has these all and a staticcheck set of rules. Seems that golangci-lint is referring to the set of rules in its configuration.

I will try to start enabling these ones by one in upcoming PRs so we can see what makes sense

[p0mvn]

The candidate to be removed if golangci-lint run time becomes an issue, in my opinion, is:

• gosimple: lint: enable gosimple #1957 (comment)

[ValarDragon]

I think we added all sub-components! If make lint times are getting annoying, we'll come back and start turning some off