golangci-lint: enable more linters

[melekes]

It would be great to eventually support all the https://golangci-lint.run/usage/linters/ linters (except deprecated). Can be done linter by linter. Feel free to comment on the issue if you've picked a specific linter to work on. Currently, we ignore the following linters in .golangci.yml (see disabled section):

• [ ] containedctx
• [ ] contextcheck
• [ ] cyclop
• [ ] dupword
• [ ] errname
• [ ] errorlint
• [ ] exhaustive
• [ ] exhaustruct
• [ ] forbidigo
• [ ] forcetypeassert
• [ ] funlen
• [ ] gocognit
• [ ] gocyclo
• [ ] godox
• [ ] goerr113
• [ ] gomnd
• [x] gomoddirectives
• [ ] interfacebloat
• [ ] ireturn
• [ ] lll
• [ ] maintidx
• [ ] nestif
• [ ] nilnil
• [ ] nlreturn
• [ ] nonamedreturns
• [ ] perfsprint
• [ ] predeclared
• [x] revive
• [ ] tagliatelle
• [ ] testifylint
• [ ] typecheck
• [x] unparam
• [ ] varnamelen
• [ ] wrapcheck
• [ ] wsl

[AbhinavMir]

Seeking some clarify here: What does "supporting all linters" mean? I was looking through the yaml file and these seem to be in there, and enable-all being set to true, just a little unclear on what the goal is here. A little new to the Golang ecosystem, appreciate any support, thanks!

[faddat]

using all linters will create a house of pain because they conflict with one another.

I know this because I've tried to do it before. The key linter in golangci-lint is revive which replaced golint. It has tons of features and is quite configurable.

• https://github.com/cometbft/cometbft/pull/2232

Here's a list of linters we don't want, and why:

• deadcode - deprecated and replaced by unused, with overlap in revive

• maligned - deprecated

• goerr113 - prohibits dynamic errors, and results in 800+ changes, I don't think we want it

• dupword - high rate of false positives

• nonamedreturns - named returns are nice in my opinion

• ireturn - prohibits returning interfaces

• interfacebloat - do we care?

• godox - we aren't ready for it. The codebase is littered with TODO's and many of them are nearly a decade old.

• golint - deprecated and replaced by revive

• gofmt - overlaps gofumpt, which is a wrapper over gofmt

• gomnd - finds numbers in the codebase and tries to prohibit them, more or less. Results in 426 changes. Don't think we want it.

• goimports - overlaps gofumpt and gci

• lll - should be done later, if at all, because it gets quite arbitrary

• wrapcheck - ensures all errors are wrapped (I don't think we want to use it)

• gochecknoinits - we don't want to use this unless we want to do a major refactor

• gochecknoglobals - I don't think we want this either

• containedctx - don't get good feelings here

• contextcheck - it causes us to pass the context much more often, even when it is not used, upsetting unparam

• tagliatelle - I think that this would break some URLs that people are accustomed to

• varnamelen - produces over 1,000 changes and doesn't have autofix

• wrapcheck - produces over 1,000 changes in a quest to ensure that all errors are wrapped

• wsl - this produces over 7,000 changes and does not have autofix

Here are some linters we do want

• testifylint - this might actually help with the flaky tests. It found some issues with tests in goroutines immediately. I like it.
  • https://github.com/cometbft/cometbft/pull/2288
• nestif - finds and aids in fixing deeply nested if statements
  • https://github.com/cometbft/cometbft/pull/2286 (nestif is complete)
    • pr configures sensitivity to tolerate a bit higher than the default
• funlen - fits nicely with:
  • https://github.com/cometbft/cometbft/pull/2286
  • should be configured for use in comet, same as cyclomatic and cognitive complexity, or the change set is enormous
  • this PR already centers on making overly large functions smaller
• gomoddirectives, typecheck, and unparam
  • https://github.com/cometbft/cometbft/pull/2290
  • gomoddirectives and typecheck don't produce changes, so why not?
  • the changes produced by unparam are in #2232
• perfsprint
  • https://github.com/cometbft/cometbft/pull/2291
• cognitive complexity and cyclomatic complexity linters entails refactoring, rather a lot of it, but we should do it, the results are good
  • https://github.com/cometbft/cometbft/pull/2152
  • https://github.com/cometbft/cometbft/pull/2130
  • https://github.com/cometbft/cometbft/pull/2286
• maintidx - maintainability index
  • fits with
    • https://github.com/cometbft/cometbft/pull/2152
    • https://github.com/cometbft/cometbft/pull/2130
    • https://github.com/cometbft/cometbft/pull/2286
  • adds some new tricks to cognitive/cyclomatic complexity but seems to focus on the same functions
  • adding to #2286 since maintainability index will get solved naturally along the way
• nilnil
  • Awesome :)
  • https://github.com/cometbft/cometbft/pull/2293

linters we might want

• forcetypeassert
  • will make our type assertions of the comma, ok variety:

types/event_bus_test.go:195:5: type assertion must be checked (forcetypeassert)
                                data := msg.Data().(EventDataTx)
                                ^
types/event_bus_test.go:246:3: type assertion must be checked (forcetypeassert)
                edt := msg.Data().(EventDataNewBlockHeader)
                ^
types/event_bus_test.go:281:3: type assertion must be checked (forcetypeassert)
                edt := msg.Data().(EventDataNewBlockEvents)
                ^
types/event_bus_test.go:325:3: type assertion must be checked (forcetypeassert)
                edt := msg.Data().(EventDataNewEvidence)

I think that there are a few other deprecated linters that I haven't marked as deprecated yet.

A lot of this is solved in #2232

There are numerous linters that have been folded into revive over the years. If we would like to enable all of the linters, beginning with revive is pretty critical because so much has joined it in the past few years.

Basically, I don't actually think that it's a good idea to enable all of the linters. Instead, we should enable as many as it is sensible and practical to do.

completed

• https://github.com/cometbft/cometbft/pull/2232
  • revive
    • since so much functionality has been bundled into revive, this has covered many of the other linters mentioned in this issue.
    • complete since January per #1907, which originally targeted main but was put on hold for pbts, so I linted pbts
• cyclomatic complexity and cognitive complexity set specifically per the PR's description
  • https://github.com/cometbft/cometbft/pull/2130
    • by setting higher values for these, we've avoided too much refactoring, these values can be brought down over time.
  • enabled the nestif linter
  • going to reopen as a new PR, because if it is successfully broken out into other PRs, then the original PR will pass CI.

the point of this comment

Most of the linters are implemented, if and only if #2232 is merged. It covers the majority of the ones listed above, and doesn't cover the ones we don't want, and is kinda the ur-linter of golang. Many of the other PR's I've made depend on #2232 and that's because it is the uber-linter / ur-linter.

cool thing

If we do end up doing this here, I'll add what we've done here to the "linter of record", the one in ibc-go:

https://github.com/cosmos/ibc-go/blob/main/.golangci.yml

One of the goals with all the linting I've been doing for so long is to create a reproducible standard that can be used across Cosmos with confidence.

[faddat]

Here's perfsprint:

• https://github.com/cometbft/cometbft/pull/2287

[faddat]

Here's what goerr113 would do:

internal/state/indexer/sink/psql/psql_test.go:291:10: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"dropping tables: %v\", err)" (goerr113)
                return fmt.Errorf("dropping tables: %v", err)
                       ^
internal/state/indexer/sink/psql/psql_test.go:295:10: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"dropping views: %v\", err)" (goerr113)
                return fmt.Errorf("dropping views: %v", err)
                       ^
internal/state/indexer/sink/psql/psql_test.go:322:15: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"lookup transaction for hash %q failed: %v\", hashString, err)" (goerr113)
                return nil, fmt.Errorf("lookup transaction for hash %q failed: %v", hashString, err)
                            ^
internal/state/indexer/sink/psql/psql_test.go:327:15: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"unmarshaling txr: %v\", err)" (goerr113)
                return nil, fmt.Errorf("unmarshaling txr: %v", err)
                            ^
internal/state/indexer/sink/psql/psql_test.go:346:19: err113: do not compare errors directly "err == sql.ErrNoRows", use "errors.Is(err, sql.ErrNoRows)" instead (goerr113)
`, height).Err(); err == sql.ErrNoRows {
                  ^
internal/state/indexer/sink/psql/psql_test.go:356:52: err113: do not compare errors directly "err == sql.ErrNoRows", use "errors.Is(err, sql.ErrNoRows)" instead (goerr113)
`, height, eventTypeFinalizeBlock, chainID).Err(); err == sql.ErrNoRows {
                                                   ^
rpc/grpc/server/services/blockservice/service.go:144:14: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"unexpected event type: %v\", eventType)" (goerr113)
                return -1, fmt.Errorf("unexpected event type: %v", eventType)
                           ^
abci/example/kvstore/kvstore.go:214:11: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"wanted to punish val %q but can't find it\", addr)" (goerr113)
                                panic(fmt.Errorf("wanted to punish val %q but can't find it", addr))
                                      ^
abci/example/kvstore/kvstore.go:424:22: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"expected 'pubkeytype!pubkey!power'. Got %v\", typePubKeyAndPower)" (goerr113)
                return "", nil, 0, fmt.Errorf("expected 'pubkeytype!pubkey!power'. Got %v", typePubKeyAndPower)
                                   ^
abci/example/kvstore/kvstore.go:431:22: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"pubkey (%s) is invalid base64\", pubkeyS)" (goerr113)
                return "", nil, 0, fmt.Errorf("pubkey (%s) is invalid base64", pubkeyS)
                                   ^
abci/example/kvstore/kvstore.go:437:22: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"power (%s) is not an int\", powerS)" (goerr113)
                return "", nil, 0, fmt.Errorf("power (%s) is not an int", powerS)
                                   ^
abci/example/kvstore/kvstore.go:441:22: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"power can not be less than 0, got %d\", power)" (goerr113)
                return "", nil, 0, fmt.Errorf("power can not be less than 0, got %d", power)
                                   ^
internal/state/txindex/kv/kv.go:105:33: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"error setting last retain height '%v' while handling result deletion error '%v' for tx indexer\", errSetLastRetainHeight, errDeleteResult)" (goerr113)
                                return 0, lastRetainHeight, fmt.Errorf("error setting last retain height '%v' while handling result deletion error '%v' for tx indexer", errSetLastRetainHeight, errDeleteResult)
                                                            ^
internal/state/txindex/kv/kv.go:109:33: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"error writing tx indexer batch '%v' while handling result deletion error '%v'\", errWriteBatch, errDeleteResult)" (goerr113)
                                return 0, lastRetainHeight, fmt.Errorf("error writing tx indexer batch '%v' while handling result deletion error '%v'", errWriteBatch, errDeleteResult)
                                                            ^
internal/state/txindex/kv/kv.go:205:15: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"error reading TxResult: %v\", err)" (goerr113)
                return nil, fmt.Errorf("error reading TxResult: %v", err)
                            ^
internal/state/txindex/kv/kv.go:367:12: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"event type and attribute key \\\"%s\\\" is reserved; please use a different key\", compositeTag)" (goerr113)
                                return fmt.Errorf("event type and attribute key \"%s\" is reserved; please use a different key", compositeTag)
                                       ^
internal/state/txindex/kv/utils.go:48:13: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"unexpected remainder in key: %s\", remaining)" (goerr113)
                return 0, fmt.Errorf("unexpected remainder in key: %s", remaining)
                          ^
proxy/multi_app_conn_test.go:70:32: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"EOF\")" (goerr113)
        clientMock.On("Error").Return(errors.New("EOF")).Once()
                                      ^
cmd/cometbft/commands/debug/dump.go:48:10: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"invalid output directory\")" (goerr113)
                return errors.New("invalid output directory")
                       ^
cmd/cometbft/commands/debug/dump.go:52:10: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"frequency must be positive\")" (goerr113)
                return errors.New("frequency must be positive")
                       ^
cmd/cometbft/commands/debug/kill.go:43:10: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"invalid output file\")" (goerr113)
                return errors.New("invalid output file")
                       ^
internal/store/store.go:312:17: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"height must be greater than 0\")" (goerr113)
                return 0, -1, errors.New("height must be greater than 0")
                              ^
internal/store/store.go:317:17: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"cannot prune beyond the latest height %v\", bs.height)" (goerr113)
                return 0, -1, fmt.Errorf("cannot prune beyond the latest height %v", bs.height)
                              ^
internal/store/store.go:322:17: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"cannot prune to height %v, it is lower than base height %v\",\n\theight, base)" (goerr113)
                return 0, -1, fmt.Errorf("cannot prune to height %v, it is lower than base height %v",
                              ^
internal/store/store.go:484:10: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"BlockStore can only save contiguous blocks. Wanted %v, got %v\", w, g)" (goerr113)
                return fmt.Errorf("BlockStore can only save contiguous blocks. Wanted %v, got %v", w, g)
                       ^
internal/store/store.go:487:10: err113: do not define dynamic errors, use wrapped static errors instead: "errors.New(\"BlockStore can only save complete block part sets\")" (goerr113)
                return errors.New("BlockStore can only save complete block part sets")
                       ^
internal/store/store.go:490:10: err113: do not define dynamic errors, use wrapped static errors instead: "fmt.Errorf(\"BlockStore cannot save seen commit of a different height (block: %d, commit: %d)\", height, seenCommit.Height)" (goerr113)
                return fmt.Errorf("BlockStore cannot save seen commit of a different height (block: %d, commit: %d)", height, seenCommit.Height)
                       ^

I don't really think we want that, many hundreds of times?

[faddat]

@AbhinavMir - hey, so, basically, right now we have all of the linters enabled in .golangci.yml. The ones that are not enabled are listed like this:

run:
  tests: true
  timeout: 10m

linters:
  enable-all: true
  disable:
    - containedctx
    - contextcheck
    - cyclop
    - deadcode
    - dupword
    - errorlint
    - errname
    - exhaustive
    - exhaustivestruct
    - exhaustruct
    - forbidigo
    - forcetypeassert
    - funlen
    - gomoddirectives
    - gochecknoglobals
    - gochecknoinits
    - gocognit
    - gocyclo
    - godox
    - goerr113
    - golint
    - gomnd
    - ifshort
    - interfacebloat
    - interfacer
    - ireturn
    - lll
    - maintidx
    - maligned
    - nestif
    - nilnil
    - nlreturn
    - nonamedreturns
    - nosnakecase
    - predeclared
    - scopelint
    - structcheck
    - tagliatelle
    - typecheck
    - varcheck
    - varnamelen
    - wrapcheck
    - wsl

to try out a new linter, remove it from the disabled list. Some of the ones that are disabled are disabled for a good reason, like being deprecated, but there are others that aren't. If you come across one that you think produces productive changes when you golangci-lint run ./... --fix you can make a PR enabling it.

Note: Most of the linters in the list have been enabled via revive so I recommend starting any work from:

• https://github.com/cometbft/cometbft/pull/2232

...otherwise you'll likely be recreating the wheel.

The only time that you would not want to start from that PR would be when the linter produces a small changeset, in that case, feel free to write your PR against main directly.

Please also note that some of these linters actually kick off large refactors, the best example of this so far is the cyclomatic complexity / cognitive complexity linters that you can see here:

• https://github.com/cometbft/cometbft/pull/2286

.... but I bet there are others, for example, the testifylint linter, which could be coming towards root cause on some of the flaky tests:

• https://github.com/cometbft/cometbft/pull/2288

and here's an example of a linter being enabled that produces no changes:

• https://github.com/cometbft/cometbft/pull/2289

If you find one of those, I do recommend enabling it and submitting a PR, just to enhance future rigor.

[faddat]

I'd like to request it this issue be closed if we're not serious about pursuing it.

The revive inter PR has been open for months now, and it resolves most of these items.

There's other derivative work from various linters that I have submitted as pull requests.

Many of the linters requested here get into the realm of refactoring, and enabling testifylint made it easy to write #2005 , which seems to resolve long-standing test reliability issues.

But at this point I'm basically showing up here to keep the pull request from being marked as stale.