From 3e14868bd627cfbcfaebf694ed052acd3fbd4144 Mon Sep 17 00:00:00 2001 From: Dev Ojha Date: Thu, 28 Jun 2018 10:12:02 -0700 Subject: [PATCH] Merge PR #1429: tools: Add ineffassign linter * tools: Add ineffassign linter This errors on assignments that don't actually do anything. i.e. x, err := myFunc(1) y, err = myFunc(2) This will call out that the first function's call error was never used. * Fix makefile, add misspell to makefile --- .circleci/config.yml | 1 - CHANGELOG.md | 1 + Makefile | 2 +- client/lcd/lcd_test.go | 46 +++++++++---------- examples/democoin/app/app_test.go | 1 + examples/democoin/cmd/democoind/main.go | 3 ++ .../democoin/x/simplestake/keeper_test.go | 2 + store/iavlstore_test.go | 4 +- tools/Makefile | 30 ++++++++++++ x/stake/client/cli/tx.go | 12 +++++ x/stake/keeper/delegation.go | 3 ++ x/stake/keeper/inflation_test.go | 2 +- x/stake/keeper/validator_test.go | 2 + 13 files changed, 81 insertions(+), 28 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 149b5546de..d58cbad5fc 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -62,7 +62,6 @@ jobs: command: | export PATH="$GOBIN:$PATH" make get_tools - go get -u github.com/client9/misspell/cmd/misspell - run: name: Lint source command: | diff --git a/CHANGELOG.md b/CHANGELOG.md index 488ce17edc..0aca8de106 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -39,6 +39,7 @@ FEATURES * gofmt * go vet -composites=false * unconvert + * ineffassign * [server] Default config now creates a profiler at port 6060, and increase p2p send/recv rates * [tests] Add WaitForNextNBlocksTM helper method * [types] Switches internal representation of Int/Uint/Rat to use pointers diff --git a/Makefile b/Makefile index 8316955f33..9cf1b14bc8 100644 --- a/Makefile +++ b/Makefile @@ -108,7 +108,7 @@ test_cover: @bash tests/test_cover.sh test_lint: - gometalinter.v2 --disable-all --enable='golint' --enable='misspell' --enable='unconvert' --linter='vet:go vet -composites=false:PATH:LINE:MESSAGE' --enable='vet' --vendor ./... + gometalinter.v2 --disable-all --enable='golint' --enable='misspell' --enable='unconvert' --enable='ineffassign' --linter='vet:go vet -composites=false:PATH:LINE:MESSAGE' --enable='vet' --deadline=500s --vendor ./... find . -name '*.go' -type f -not -path "./vendor*" -not -path "*.git*" | xargs gofmt -d -s benchmark: diff --git a/client/lcd/lcd_test.go b/client/lcd/lcd_test.go index 07bb4b252e..9776e9c453 100644 --- a/client/lcd/lcd_test.go +++ b/client/lcd/lcd_test.go @@ -51,7 +51,7 @@ func TestKeys(t *testing.T) { var jsonStr = []byte(fmt.Sprintf(`{"name":"test_fail", "password":"%s"}`, password)) res, body = Request(t, port, "POST", "/keys", jsonStr) - assert.Equal(t, http.StatusBadRequest, res.StatusCode, "Account creation should require a seed") + assert.Equal(t, http.StatusBadRequest, res.StatusCode, "Account creation should require a seed "+body) jsonStr = []byte(fmt.Sprintf(`{"name":"%s", "password":"%s", "seed": "%s"}`, newName, newPassword, newSeed)) res, body = Request(t, port, "POST", "/keys", jsonStr) @@ -212,7 +212,7 @@ func TestValidators(t *testing.T) { // -- res, body = Request(t, port, "GET", "/validatorsets/1000000000", nil) - require.Equal(t, http.StatusNotFound, res.StatusCode) + require.Equal(t, http.StatusNotFound, res.StatusCode, body) } func TestCoinSend(t *testing.T) { @@ -520,43 +520,43 @@ func TestVote(t *testing.T) { } func TestProposalsQuery(t *testing.T) { - name, password := "test", "1234567890" - name2, password := "test2", "1234567890" - addr, seed := CreateAddr(t, "test", password, GetKB(t)) - addr2, seed2 := CreateAddr(t, "test2", password, GetKB(t)) + name, password1 := "test", "1234567890" + name2, password2 := "test2", "1234567890" + addr, seed := CreateAddr(t, "test", password1, GetKB(t)) + addr2, seed2 := CreateAddr(t, "test2", password2, GetKB(t)) cleanup, _, port := InitializeTestLCD(t, 1, []sdk.Address{addr, addr2}) defer cleanup() // Addr1 proposes (and deposits) proposals #1 and #2 - resultTx := doSubmitProposal(t, port, seed, name, password, addr) + resultTx := doSubmitProposal(t, port, seed, name, password1, addr) var proposalID1 int64 cdc.UnmarshalBinaryBare(resultTx.DeliverTx.GetData(), &proposalID1) tests.WaitForHeight(resultTx.Height+1, port) - resultTx = doSubmitProposal(t, port, seed, name, password, addr) + resultTx = doSubmitProposal(t, port, seed, name, password1, addr) var proposalID2 int64 cdc.UnmarshalBinaryBare(resultTx.DeliverTx.GetData(), &proposalID2) tests.WaitForHeight(resultTx.Height+1, port) // Addr2 proposes (and deposits) proposals #3 - resultTx = doSubmitProposal(t, port, seed2, name2, password, addr2) + resultTx = doSubmitProposal(t, port, seed2, name2, password2, addr2) var proposalID3 int64 cdc.UnmarshalBinaryBare(resultTx.DeliverTx.GetData(), &proposalID3) tests.WaitForHeight(resultTx.Height+1, port) // Addr2 deposits on proposals #2 & #3 - resultTx = doDeposit(t, port, seed2, name2, password, addr2, proposalID2) + resultTx = doDeposit(t, port, seed2, name2, password2, addr2, proposalID2) tests.WaitForHeight(resultTx.Height+1, port) - resultTx = doDeposit(t, port, seed2, name2, password, addr2, proposalID3) + resultTx = doDeposit(t, port, seed2, name2, password2, addr2, proposalID3) tests.WaitForHeight(resultTx.Height+1, port) // Addr1 votes on proposals #2 & #3 - resultTx = doVote(t, port, seed, name, password, addr, proposalID2) + resultTx = doVote(t, port, seed, name, password1, addr, proposalID2) tests.WaitForHeight(resultTx.Height+1, port) - resultTx = doVote(t, port, seed, name, password, addr, proposalID3) + resultTx = doVote(t, port, seed, name, password1, addr, proposalID3) tests.WaitForHeight(resultTx.Height+1, port) // Addr2 votes on proposal #3 - resultTx = doVote(t, port, seed2, name2, password, addr2, proposalID3) + resultTx = doVote(t, port, seed2, name2, password2, addr2, proposalID3) tests.WaitForHeight(resultTx.Height+1, port) // Test query all proposals @@ -717,9 +717,9 @@ func doDelegate(t *testing.T, port, seed, name, password string, delegatorAddr, "bond": { "denom": "%s", "amount": 60 } } ], - "begin_unbondings": [], - "complete_unbondings": [], - "begin_redelegates": [], + "begin_unbondings": [], + "complete_unbondings": [], + "begin_redelegates": [], "complete_redelegates": [] }`, name, password, accnum, sequence, chainID, delegatorAddrBech, validatorAddrBech, "steak")) res, body := Request(t, port, "POST", "/stake/delegations", jsonStr) @@ -760,9 +760,9 @@ func doBeginUnbonding(t *testing.T, port, seed, name, password string, "validator_addr": "%s", "shares": "30" } - ], - "complete_unbondings": [], - "begin_redelegates": [], + ], + "complete_unbondings": [], + "begin_redelegates": [], "complete_redelegates": [] }`, name, password, accnum, sequence, chainID, delegatorAddrBech, validatorAddrBech)) res, body := Request(t, port, "POST", "/stake/delegations", jsonStr) @@ -798,8 +798,8 @@ func doBeginRedelegation(t *testing.T, port, seed, name, password string, "gas": 10000, "chain_id": "%s", "delegations": [], - "begin_unbondings": [], - "complete_unbondings": [], + "begin_unbondings": [], + "complete_unbondings": [], "begin_redelegates": [ { "delegator_addr": "%s", @@ -807,7 +807,7 @@ func doBeginRedelegation(t *testing.T, port, seed, name, password string, "validator_dst_addr": "%s", "shares": "30" } - ], + ], "complete_redelegates": [] }`, name, password, accnum, sequence, chainID, delegatorAddrBech, validatorSrcAddrBech, validatorDstAddrBech)) res, body := Request(t, port, "POST", "/stake/delegations", jsonStr) diff --git a/examples/democoin/app/app_test.go b/examples/democoin/app/app_test.go index a477ef79bb..a75eee0f38 100644 --- a/examples/democoin/app/app_test.go +++ b/examples/democoin/app/app_test.go @@ -43,6 +43,7 @@ func TestGenesis(t *testing.T) { }, } stateBytes, err := json.MarshalIndent(genesisState, "", "\t") + require.Nil(t, err) vals := []abci.Validator{} bapp.InitChain(abci.RequestInitChain{Validators: vals, AppStateBytes: stateBytes}) diff --git a/examples/democoin/cmd/democoind/main.go b/examples/democoin/cmd/democoind/main.go index 76d29075e9..9e84c02571 100644 --- a/examples/democoin/cmd/democoind/main.go +++ b/examples/democoin/cmd/democoind/main.go @@ -34,6 +34,9 @@ func CoolAppGenState(cdc *wire.Codec, appGenTxs []json.RawMessage) (appState jso "trend": "ice-cold" }`) appState, err = server.AppendJSON(cdc, appState, key, value) + if err != nil { + return + } key = "pow" value = json.RawMessage(`{ "difficulty": 1, diff --git a/examples/democoin/x/simplestake/keeper_test.go b/examples/democoin/x/simplestake/keeper_test.go index 2b73800324..9315a7ccf6 100644 --- a/examples/democoin/x/simplestake/keeper_test.go +++ b/examples/democoin/x/simplestake/keeper_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" abci "github.com/tendermint/abci/types" crypto "github.com/tendermint/go-crypto" @@ -79,6 +80,7 @@ func TestBonding(t *testing.T) { assert.Nil(t, err) power, err := stakeKeeper.bondWithoutCoins(ctx, addr, pubKey, sdk.NewCoin("steak", 10)) + require.Nil(t, err) assert.Equal(t, int64(20), power) pk, _, err := stakeKeeper.unbondWithoutCoins(ctx, addr) diff --git a/store/iavlstore_test.go b/store/iavlstore_test.go index e2f2f9c174..25e8724c9a 100644 --- a/store/iavlstore_test.go +++ b/store/iavlstore_test.go @@ -155,7 +155,7 @@ func TestIAVLSubspaceIterator(t *testing.T) { iavlStore.Set([]byte{byte(255), byte(255), byte(1)}, []byte("test4")) iavlStore.Set([]byte{byte(255), byte(255), byte(255)}, []byte("test4")) - i := 0 + var i int iter := sdk.KVStorePrefixIterator(iavlStore, []byte("test")) expected := []string{"test1", "test2", "test3"} @@ -214,7 +214,7 @@ func TestIAVLReverseSubspaceIterator(t *testing.T) { iavlStore.Set([]byte{byte(255), byte(255), byte(1)}, []byte("test4")) iavlStore.Set([]byte{byte(255), byte(255), byte(255)}, []byte("test4")) - i := 0 + var i int iter := sdk.KVStoreReversePrefixIterator(iavlStore, []byte("test")) expected := []string{"test3", "test2", "test1"} diff --git a/tools/Makefile b/tools/Makefile index bfa6a790db..7fb1e886c5 100644 --- a/tools/Makefile +++ b/tools/Makefile @@ -8,11 +8,15 @@ DEP = github.com/golang/dep/cmd/dep GOLINT = github.com/tendermint/lint/golint GOMETALINTER = gopkg.in/alecthomas/gometalinter.v2 UNCONVERT = github.com/mdempsky/unconvert +INEFFASSIGN = github.com/gordonklaus/ineffassign +MISSPELL = github.com/client9/misspell/cmd/misspell DEP_CHECK := $(shell command -v dep 2> /dev/null) GOLINT_CHECK := $(shell command -v golint 2> /dev/null) GOMETALINTER_CHECK := $(shell command -v gometalinter.v2 2> /dev/null) UNCONVERT_CHECK := $(shell command -v unconvert 2> /dev/null) +INEFFASSIGN_CHECK := $(shell command -v ineffassign 2> /dev/null) +MISSPELL_CHECK := $(shell command -v misspell 2> /dev/null) check_tools: ifndef DEP_CHECK @@ -35,6 +39,16 @@ ifndef UNCONVERT_CHECK else @echo "Found unconvert in path." endif +ifndef INEFFASSIGN_CHECK + @echo "No ineffassign in path. Install with 'make get_tools'." +else + @echo "Found ineffassign in path." +endif +ifndef MISSPELL_CHECK + @echo "No misspell in path. Install with 'make get_tools'." +else + @echo "Found misspell in path." +endif get_tools: ifdef DEP_CHECK @@ -61,6 +75,18 @@ else @echo "Installing unconvert" go get -v $(UNCONVERT) endif +ifdef INEFFASSIGN_CHECK + @echo "Ineffassign is already installed. Run 'make update_tools' to update." +else + @echo "Installing ineffassign" + go get -v $(INEFFASSIGN) +endif +ifdef MISSPELL_CHECK + @echo "misspell is already installed. Run 'make update_tools' to update." +else + @echo "Installing misspell" + go get -v $(MISSPELL) +endif update_tools: @echo "Updating dep" @@ -71,6 +97,10 @@ update_tools: go get -u -v $(GOMETALINTER) @echo "Updating unconvert" go get -u -v $(UNCONVERT) + @echo "Updating ineffassign" + go get -u -v $(INEFFASSIGN) + @echo "Updating misspell" + go get -u -v $(MISSPELL) # To avoid unintended conflicts with file names, always add to .PHONY # unless there is a reason not to. diff --git a/x/stake/client/cli/tx.go b/x/stake/client/cli/tx.go index 7543d2707b..b7941e2bb8 100644 --- a/x/stake/client/cli/tx.go +++ b/x/stake/client/cli/tx.go @@ -117,6 +117,9 @@ func GetCmdDelegate(cdc *wire.Codec) *cobra.Command { } delegatorAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressDelegator)) + if err != nil { + return err + } validatorAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressValidator)) if err != nil { return err @@ -259,7 +262,13 @@ func GetCmdCompleteRedelegate(cdc *wire.Codec) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { delegatorAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressDelegator)) + if err != nil { + return err + } validatorSrcAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressValidatorSrc)) + if err != nil { + return err + } validatorDstAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressValidatorDst)) if err != nil { return err @@ -351,6 +360,9 @@ func GetCmdCompleteUnbonding(cdc *wire.Codec) *cobra.Command { RunE: func(cmd *cobra.Command, args []string) error { delegatorAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressDelegator)) + if err != nil { + return err + } validatorAddr, err := sdk.GetAccAddressBech32(viper.GetString(FlagAddressValidator)) if err != nil { return err diff --git a/x/stake/keeper/delegation.go b/x/stake/keeper/delegation.go index e9155d7f98..844ffb9b48 100644 --- a/x/stake/keeper/delegation.go +++ b/x/stake/keeper/delegation.go @@ -321,6 +321,9 @@ func (k Keeper) BeginRedelegation(ctx sdk.Context, delegatorAddr, validatorSrcAd return types.ErrBadRedelegationDst(k.Codespace()) } sharesCreated, err := k.Delegate(ctx, delegatorAddr, returnCoin, dstValidator) + if err != nil { + return err + } // create the unbonding delegation minTime := ctx.BlockHeader().Time + params.UnbondingTime diff --git a/x/stake/keeper/inflation_test.go b/x/stake/keeper/inflation_test.go index cb2f6007a4..d748ce0cb2 100644 --- a/x/stake/keeper/inflation_test.go +++ b/x/stake/keeper/inflation_test.go @@ -212,7 +212,7 @@ func TestLargeBond(t *testing.T) { keeper.SetParams(ctx, params) // validator[9] will be bonded, bringing us from 25% to ~50% (bonding 400,000,000 tokens) - pool, validator, _, _ = types.OpBondOrUnbond(r, pool, validator) + pool, _, _, _ = types.OpBondOrUnbond(r, pool, validator) keeper.SetPool(ctx, pool) // process provisions after the bonding, to compare the difference in expProvisions and expInflation diff --git a/x/stake/keeper/validator_test.go b/x/stake/keeper/validator_test.go index 245954ad13..3bbcee94c0 100644 --- a/x/stake/keeper/validator_test.go +++ b/x/stake/keeper/validator_test.go @@ -34,6 +34,7 @@ func TestSetValidator(t *testing.T) { // Check each store for being saved resVal, found := keeper.GetValidator(ctx, addrVals[0]) assert.True(ValEq(t, validator, resVal)) + assert.True(t, found) resVals := keeper.GetValidatorsBonded(ctx) require.Equal(t, 1, len(resVals)) @@ -87,6 +88,7 @@ func TestUpdateValidatorByPowerIndex(t *testing.T) { pool = keeper.GetPool(ctx) validator, found = keeper.GetValidator(ctx, addrVals[0]) + assert.True(t, found) power = GetValidatorsByPowerIndexKey(validator, pool) assert.True(t, keeper.validatorByPowerIndexExists(ctx, power)) }