tools: linters we could add

[ValarDragon]

This PR adds several linters we can potentially use. Please comment with your thoughts on using the following linters! I've left their errors in the linting step, so we can evaluate how useful they are.

Also take a look here if theres any more linters you think we should run on circle ci: https://github.com/alecthomas/gometalinter

• deadcode - checks for deadcode
• unparam - (Check for unused function inputs #1084) checks for unused function parameters on unexported functions
• go vet - meta: suggestion: require govet for linting #1370 catches bugs that can otherwise compile. (Disabled the compound struct thing)
• errcheck - makes sure that we check all errors
• maligned - closes Reorder variables structs to optimize memory #1257, I've switched this to my own fork, which prints out how to change the structs.
• ineffassign - Detect when assignments to existing variables are not used.
• goconst - Finds repeated strings that could be replaced by a constant.
• varcheck - look for unused variables
• structcheck - look for unused struct fields tools: Add a linter to check for struct fields that aren't used anywhere in the project #1408
• unconvert - Look for unneccessary type conversions

Closes #1084
I've left the errors in linting to make it easier to judge whether its something we want to use. Once we decide which ones to keep, I'll update the make file and make test_lint accordingly.

Changelog also needs to be updated, will do once we decide on which linters to use.
Also remember, if we agree with a linter in most places, but not in a couple, we should still use it, and just add nolint in the places we disagree. (nolint can be extended to cover an entire file iirc too)

• Updated all relevant documentation in docs
• Updated all code comments where relevant
• Wrote tests
• Updated CHANGELOG.md
• Updated Gaia/Examples
• Squashed all commits, uses message "Merge pull request #XYZ: [title]" (coding standards)

[codecov]

Codecov Report

Merging #1372 into develop will decrease coverage by 0.92%.
The diff coverage is 35.93%.

@@             Coverage Diff             @@
##           develop    #1372      +/-   ##
===========================================
- Coverage    62.81%   61.88%   -0.93%     
===========================================
  Files          109      103       -6     
  Lines         6016     5864     -152     
===========================================
- Hits          3779     3629     -150     
+ Misses        2020     2018       -2     
  Partials       217      217

[ValarDragon]

For some reason, all the output doesn't show on circle CI. So check out the following ghost bin for the sample error: https://ghostbin.com/paste/cpdaz

[cwgoes]

Can we ensure that local output and CI output are the same and add a make lint target so that developers can check linter output without waiting for CI?

[ValarDragon]

Will do. I'll add a make test_lint command to this PR, but I'd rather we decide which linters to use first before making the command.

[cwgoes]

Will do. I'll add a make test_lint command to this PR, but I'd rather we decide which linters to use first before making the command.

That's fine - but ideally we avoid special-casing the CircleCI config; CircleCI should just run make get_tools lint.

[UnitylChaos]

I'm all for maximal metalinting 👍

[rigelrozanski]

unparam is really cool - we don't pass unparam right now aka there are a few limp-dead variables

[ValarDragon]

I'm conflicted on whether or not deadcode is useful. Here is its output:

baseapp/baseapp.go:26:1:warning: dbHeaderKey is unused (deadcode)
cmd/gaia/cmd/gaiadebug/hack.go:94:1:warning: base64ToPub is unused (deadcode)
cmd/gaia/cmd/gaiadebug/hack.go:117:1:warning: DefaultCLIHome is unused (deadcode)
cmd/gaia/cmd/gaiadebug/hack.go:117:1:warning: DefaultNodeHome is unused (deadcode)
server/test_helpers.go:34:1:warning: setupViper is unused (deadcode)
types/int.go:34:1:warning: mod is unused (deadcode)
types/lib/linear.go:245:1:warning: subspace is unused (deadcode)
x/gov/test_common.go:21:1:warning: getMockApp is unused (deadcode)
x/slashing/wire.go:12:1:warning: cdcEmpty is unused (deadcode)
x/slashing/test_common.go:25:1:warning: pks is unused (deadcode)
x/slashing/test_common.go:49:1:warning: createTestInput is unused (deadcode)
x/slashing/test_common.go:92:1:warning: newTestMsgCreateValidator is unused (deadcode)
x/stake/keeper/test_common.go:25:1:warning: addrDels is unused (deadcode)
x/stake/keeper/test_common.go:25:1:warning: emptyPubkey is unused (deadcode)
x/stake/keeper/test_common.go:25:1:warning: addrVals is unused (deadcode)
x/stake/keeper/test_common.go:25:1:warning: emptyAddr is unused (deadcode)
x/stake/types/test_common.go:13:1:warning: emptyAddr is unused (deadcode)
x/stake/types/test_common.go:13:1:warning: addr2 is unused (deadcode)
x/stake/types/test_common.go:13:1:warning: emptyPubkey is unused (deadcode)
x/stake/types/test_common.go:13:1:warning: addr3 is unused (deadcode)

It gave a couple of useful points, e.g. baseapp/baseapp.go:26:1:warning: dbHeaderKey is unused (deadcode), means we should either remove that or write a test case to cover that. But the majority of the output is for unused vars in test helper files, which I don't think is code that we need to change. We'd have to add a bunch of nolint statements to the test files though. I'm leaning towards removing deadcode.

[ValarDragon]

Looks like structcheck doesn't do what we want. It only checks at the module level, not at the whole project level. This may be something that can be fixed though. I'm going to remove it from here for now, until a better tool / structcheck has been forked. I'll remove deadcode too, we can always add it back if we like its output.

[ValarDragon]

Note, I've disabled errcheck output on the client directories. This was because the errors there don't really contribute to failing fast / aren't that helpful. (i.e. not checking errors on writing the http responses)

[rigelrozanski]

@ValarDragon As mentioned on our call two days ago - we should close this PR and open an issue which contains all the linters (and discussions) - then we should open up individual PRs for each linter being added which simultaneously adds the linter as well as fixes all bugs which the linter pointed out

• want to take this on? - we should probably copy all the comments from this PR into the issue as well