Cosmos-sdk: Enable more Linters

[marbar3778]

Summary

I would propose we enable more linters. Currently, we only have:

  disable-all: true
  enable:
    - errcheck
    - golint
    - ineffassign
    - unconvert
    - misspell
    - govet

Proposed Linters to add

• [x] unused - Checks Go code for unused constants, variables, functions and types
• [x] deadcode - Finds unused code
• [x] gosec - Inspects source code for security problems
• [ ] interfacer - Linter that suggests narrower interface types
• [ ] dupl - Tool for code clone detection
• [x] goconst - Finds repeated strings that could be replaced by a constant
• [x] maligned - Tool to detect Go structs that would take less memory if their fields were sorted
• [x] gocritic - The most opinionated Go source code linter
• [ ] scopelint - Scopelint checks for unpinned variables in go programs
• [x] gochecknoglobals - Checks that no globals are present in Go code
• [x] unused
• [x] statikcheck

Also remove disable-all: true to enable default linters

Problem Definition

More Linting

Proposal

We can break each one into separate PRS, but I think it would be good to add more linting to the project.

---

For Admin Use

• [x] Not duplicate issue
• [x] Appropriate labels applied
• [ ] Appropriate contributors tagged
• [ ] Contributor assigned/self-assigned

[alexanderbez]

💯

I would definitely love to see in order of preference:

• maligned
• deadcode
• gosec
• unused (this may be a problem though because we have alias.go)

Feel free to add these @marbar3778

[alessio]

This is what happens when I enable maligned:

alessio@bangalter:~/work/cosmos-sdk$ make lint 
golangci-lint run
types/config.go:9:13: struct of size 80 bytes could be of size 72 bytes (maligned)
type Config struct {
            ^
store/iavl/store.go:299:19: struct of size 152 bytes could be of size 144 bytes (maligned)
type iavlIterator struct {
                  ^
baseapp/baseapp.go:45:14: struct of size 272 bytes could be of size 264 bytes (maligned)
type BaseApp struct {
             ^
client/context/context.go:34:17: struct of size 232 bytes could be of size 208 bytes (maligned)
type CLIContext struct {
                ^
WARN [runner/nolint] Found unknown linters in //nolint directives: structtag 
Makefile:150: recipe for target 'ci-lint' failed
make: *** [ci-lint] Error 1

[alessio]

gosec:

$ golangci-lint run
server/test_helpers.go:19: G102: Binds to all network interfaces (gosec)
    l, err := net.Listen("tcp", "0.0.0.0:0")
tests/util.go:100: G107: Potential HTTP request made with variable url (gosec)
        res, err = http.Get(url)
tests/util.go:153: G107: Potential HTTP request made with variable url (gosec)
        res, err = http.Get(url)
WARN [runner/nolint] Found unknown linters in //nolint directives: structtag 

[alessio]

deadcode (this is interesting yet a number of false positives are detected):

$ golangci-lint run
store/types/gas.go:18:2: `cachedKVGasConfig` is unused (deadcode)
    cachedKVGasConfig        = KVGasConfig()
    ^
store/types/gas.go:19:2: `cachedTransientGasConfig` is unused (deadcode)
    cachedTransientGasConfig = TransientGasConfig()
    ^
store/list/list.go:107:6: `subspace` is unused (deadcode)
func subspace(prefix []byte) (start, end []byte) {
     ^
store/cachekv/memiterator.go:93:6: `keyCompare` is unused (deadcode)
func keyCompare(k1, k2 []byte) int {
     ^
store/prefix/store.go:193:6: `cpDecr` is unused (deadcode)
func cpDecr(bz []byte) (ret []byte) {
     ^
store/prefix/store.go:212:6: `skipOne` is unused (deadcode)
func skipOne(iter types.Iterator, skipKey []byte) {
     ^
types/uint.go:110:6: `randomUint` is unused (deadcode)
func randomUint(u Uint) Uint { return NewUintFromBigInt(random(u.i)) }
     ^
types/coin.go:572:6: `copyCoins` is unused (deadcode)
func copyCoins(coins Coins) Coins {
     ^
types/errors.go:304:6: `mustGetMsgIndex` is unused (deadcode)
func mustGetMsgIndex(abciLog string) int {
     ^
store/wire.go:7:5: `cdc` is unused (deadcode)
var cdc = codec.New()
    ^
client/keys/errors.go:5:6: `errKeyNameConflict` is unused (deadcode)
func errKeyNameConflict(name string) error {
     ^
client/keys/errors.go:9:6: `errMissingName` is unused (deadcode)
func errMissingName() error {
     ^
client/keys/errors.go:13:6: `errMissingPassword` is unused (deadcode)
func errMissingPassword() error {
     ^
client/keys/errors.go:17:6: `errMissingMnemonic` is unused (deadcode)
func errMissingMnemonic() error {
     ^
client/keys/errors.go:21:6: `errInvalidMnemonic` is unused (deadcode)
func errInvalidMnemonic() error {
     ^
client/keys/errors.go:25:6: `errInvalidAccountNumber` is unused (deadcode)
func errInvalidAccountNumber() error {
     ^
client/keys/errors.go:29:6: `errInvalidIndexNumber` is unused (deadcode)
func errInvalidIndexNumber() error {
     ^
x/params/test_common.go:40:6: `testComponents` is unused (deadcode)
func testComponents() (*codec.Codec, sdk.Context, sdk.StoreKey, sdk.StoreKey, Keeper) {
     ^
x/staking/types/test_utils.go:17:2: `valAddr1` is unused (deadcode)
    valAddr1 = sdk.ValAddress(addr1)
    ^
x/staking/types/test_utils.go:18:2: `valAddr2` is unused (deadcode)
    valAddr2 = sdk.ValAddress(addr2)
    ^
x/staking/types/test_utils.go:19:2: `valAddr3` is unused (deadcode)
    valAddr3 = sdk.ValAddress(addr3)
    ^
x/staking/types/test_utils.go:21:2: `emptyAddr` is unused (deadcode)
    emptyAddr   sdk.ValAddress
    ^
x/staking/types/test_utils.go:22:2: `emptyPubkey` is unused (deadcode)
    emptyPubkey crypto.PubKey
    ^
x/staking/client/cli/flags.go:46:2: `fsDelegator` is unused (deadcode)
    fsDelegator         = flag.NewFlagSet("", flag.ContinueOnError)
    ^
x/staking/keeper/test_common.go:32:2: `emptyAddr` is unused (deadcode)
    emptyAddr   sdk.AccAddress
    ^
x/staking/keeper/test_common.go:33:2: `emptyPubkey` is unused (deadcode)
    emptyPubkey crypto.PubKey
    ^
x/staking/keeper/test_common.go:35:2: `addrDels` is unused (deadcode)
    addrDels = []sdk.AccAddress{
    ^
x/staking/keeper/test_common.go:39:2: `addrVals` is unused (deadcode)
    addrVals = []sdk.ValAddress{
    ^
x/staking/keeper/test_common.go:261:6: `validatorByPowerIndexExists` is unused (deadcode)
func validatorByPowerIndexExists(k Keeper, ctx sdk.Context, power []byte) bool {
     ^
x/staking/test_common.go:14:2: `addr1` is unused (deadcode)
    addr1 = sdk.AccAddress(priv1.PubKey().Address())
    ^
x/staking/test_common.go:16:2: `addr2` is unused (deadcode)
    addr2 = sdk.AccAddress(priv2.PubKey().Address())
    ^
x/staking/test_common.go:17:2: `addr3` is unused (deadcode)
    addr3 = sdk.AccAddress(secp256k1.GenPrivKey().PubKey().Address())
    ^
x/staking/test_common.go:19:2: `addr4` is unused (deadcode)
    addr4 = sdk.AccAddress(priv4.PubKey().Address())
    ^
x/staking/test_common.go:20:2: `coins` is unused (deadcode)
    coins = sdk.Coins{sdk.NewCoin("foocoin", sdk.NewInt(10))}
    ^
x/staking/test_common.go:21:2: `fee` is unused (deadcode)
    fee   = auth.NewStdFee(
    ^
x/distribution/types/test_common.go:14:2: `delAddr1` is unused (deadcode)
    delAddr1     = sdk.AccAddress(delPk1.Address())
    ^
x/distribution/types/test_common.go:15:2: `delAddr2` is unused (deadcode)
    delAddr2     = sdk.AccAddress(delPk2.Address())
    ^
x/distribution/types/test_common.go:16:2: `delAddr3` is unused (deadcode)
    delAddr3     = sdk.AccAddress(delPk3.Address())
    ^
x/distribution/types/test_common.go:17:2: `emptyDelAddr` is unused (deadcode)
    emptyDelAddr sdk.AccAddress
    ^
x/distribution/types/test_common.go:22:2: `valAddr1` is unused (deadcode)
    valAddr1     = sdk.ValAddress(valPk1.Address())
    ^
x/distribution/types/test_common.go:23:2: `valAddr2` is unused (deadcode)
    valAddr2     = sdk.ValAddress(valPk2.Address())
    ^
x/distribution/types/test_common.go:24:2: `valAddr3` is unused (deadcode)
    valAddr3     = sdk.ValAddress(valPk3.Address())
    ^
x/distribution/types/test_common.go:25:2: `emptyValAddr` is unused (deadcode)
    emptyValAddr sdk.ValAddress
    ^
x/distribution/types/test_common.go:27:2: `emptyPubkey` is unused (deadcode)
    emptyPubkey crypto.PubKey
    ^
x/gov/test_common.go:34:6: `getMockApp` is unused (deadcode)
func getMockApp(t *testing.T, numGenAccs int, genState GenesisState, genAccs []auth.Account) testInput {
     ^
x/gov/test_common.go:155:6: `testProposal` is unused (deadcode)
func testProposal() Content {
     ^
x/gov/test_common.go:164:6: `badProposalHandler` is unused (deadcode)
func badProposalHandler(ctx sdk.Context, c Content) sdk.Error {
     ^
x/gov/test_common.go:198:6: `createValidators` is unused (deadcode)
func createValidators(t *testing.T, stakingHandler sdk.Handler, ctx sdk.Context, addrs []sdk.ValAddress, powerAmt []int64) {
     ^
x/genutil/gentx.go:19:2: `defaultAmount` is unused (deadcode)
    defaultAmount                  = defaultTokens.String() + sdk.DefaultBondDenom
    ^
x/genutil/gentx.go:20:2: `defaultCommissionRate` is unused (deadcode)
    defaultCommissionRate          = "0.1"
    ^
WARN [runner/nolint] Found unknown linters in //nolint directives: structtag 

[alessio]

@marbar3778 I've added goconst to the enabled linters. Please feel free to take over

[marbar3778]

yea added deadcode and unused, there are places where i will have to disable just those two, but in places it is helping.