fix lint issues #1000

rootulp · 2023-04-17T19:12:15Z

Context

golangci-lint identifies a bunch of issues but they aren't caught by CI. It's possible the timeout on CI is too low.

Proposal

Fix all golangci-lint issues
Ensure it is running in CI

Additional Details

golangci-lint has unused which should be enabled by default. It is explicitly enabled here. This unused function is flagged along with a bunch of other lint warnings if we run lint with an increased timeout:

$ golangci-lint run --timeout 10m
state/execution_test.go:624:21: Error return value of `eventBus.Stop` is not checked (errcheck)
	defer eventBus.Stop()
	                   ^
mempool/cat/cache.go:125: File is not `gofmt`-ed with `-s` (gofmt)
			peers: map[uint16]struct{}{peer: struct{}{}},
pkg/trace/flags.go:7:2: G101: Potential hardcoded credentials (gosec)
	FlagInfluxDBTokenDescription = "Token to use when writing to the InfluxDB instance. Must be specified if 'influxdb-url' is specified"
	^
mempool/cat/reactor.go:323:27: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec)
	time.Sleep(time.Duration(rand.Intn(10)*10) * time.Millisecond)
	                         ^
types/block_test.go:895:3: `descripton` is a misspelling of `description` (misspell)
		descripton string
		^
node/node.go:839:48: `arbitary` is a misspelling of `arbitrary` (misspell)
	// create an optional influxdb client to send arbitary data to a remote
	                                              ^
libs/pubsub/query/empty.go:8:22: unused-parameter: parameter 'tags' seems to be unused, consider removing or renaming it as _ (revive)
func (Empty) Matches(tags map[string][]string) (bool, error) {
                     ^
libs/autofile/cmd/logjack.go:81:11: superfluous-else: if block ends with call to os.Exit function, so drop this else and outdent its block (revive)
			} else {
				fmt.Println("logjack errored")
				os.Exit(1)
			}
p2p/upnp/upnp.go:384:68: unused-parameter: parameter 'internalPort' seems to be unused, consider removing or renaming it as _ (revive)
func (n *upnpNAT) DeletePortMapping(protocol string, externalPort, internalPort int) (err error) {
                                                                   ^
rpc/jsonrpc/test/main.go:18:17: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func HelloWorld(ctx *rpctypes.Context, name string, num int) (Result, error) {
                ^
libs/clist/clist.go:255:2: redefines-builtin-id: redefinition of the built-in function len (revive)
	len := l.len
	^
libs/strings/string.go:41:26: empty-block: this block is empty, you can remove it (revive)
		if 32 <= b && b <= 126 {
			// good
		} else {
libs/rand/random_test.go:77:31: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestRngConcurrencySafety(t *testing.T) {
                              ^
libs/rand/random.go:165:11: superfluous-else: if block ends with a continue statement, so drop this else and outdent its block (revive)
			} else {
				chars = append(chars, strChars[v])
				if len(chars) == length {
					break MAIN_LOOP
				}
				val >>= 6
			}
libs/cli/setup.go:141:45: empty-block: this block is empty, you can remove it (revive)
	if err := viper.ReadInConfig(); err == nil {
		// stderr, so if we redirect output to json file, this doesn't appear
		// fmt.Fprintln(os.Stderr, "Using config file:", viper.ConfigFileUsed())
	} else if _, ok := err.(viper.ConfigFileNotFoundError); !ok {
libs/cli/setup.go:128:45: unused-parameter: parameter 'args' seems to be unused, consider removing or renaming it as _ (revive)
func bindFlagsLoadViper(cmd *cobra.Command, args []string) error {
                                            ^
libs/cli/setup.go:151:21: unused-parameter: parameter 'cmd' seems to be unused, consider removing or renaming it as _ (revive)
func validateOutput(cmd *cobra.Command, args []string) error {
                    ^
p2p/conn/secret_connection_test.go:312:30: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func createGoldenTestVectors(t *testing.T) string {
                             ^
p2p/conn/connection.go:660:2: empty-block: this block is empty, you can remove it (revive)
	for range c.pong {
		// Drain
	}
rpc/jsonrpc/server/http_json_handler_test.go:198:11: superfluous-else: if block ends with a continue statement, so drop this else and outdent its block (revive)
			} else {
				// we were expecting an error here, so let's unmarshal a single response
				var response types.RPCResponse
				err = json.Unmarshal(blob, &response)
				if err != nil {
					t.Errorf("#%d: expected successful parsing of an RPCResponse\nblob: %s", i, blob)
					continue
				}
				// have a single-element result
				responses = []types.RPCResponse{response}
			}
libs/json/helpers_test.go:48:35: unused-parameter: parameter 'bz' seems to be unused, consider removing or renaming it as _ (revive)
func (c *CustomPtr) UnmarshalJSON(bz []byte) error {
                                  ^
libs/json/helpers_test.go:63:36: unused-parameter: parameter 'bz' seems to be unused, consider removing or renaming it as _ (revive)
func (c CustomValue) UnmarshalJSON(bz []byte) error {
                                   ^
libs/bits/bit_array_test.go:146:16: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestBytes(t *testing.T) {
               ^
libs/bits/bit_array_test.go:191:28: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestUpdateNeverPanics(t *testing.T) {
                           ^
libs/bits/bit_array_test.go:213:45: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func TestNewBitArrayNeverCrashesOnNegatives(t *testing.T) {
                                            ^
rpc/jsonrpc/jsonrpc_test.go:74:17: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func EchoResult(ctx *types.Context, v string) (*ResultEcho, error) {
                ^
rpc/jsonrpc/jsonrpc_test.go:78:19: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func EchoWSResult(ctx *types.Context, v string) (*ResultEcho, error) {
                  ^
rpc/jsonrpc/jsonrpc_test.go:82:20: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func EchoIntResult(ctx *types.Context, v int) (*ResultEchoInt, error) {
                   ^
rpc/jsonrpc/jsonrpc_test.go:86:22: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func EchoBytesResult(ctx *types.Context, v []byte) (*ResultEchoBytes, error) {
                     ^
rpc/jsonrpc/jsonrpc_test.go:90:26: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func EchoDataBytesResult(ctx *types.Context, v cmtbytes.HexBytes) (*ResultEchoDataBytes, error) {
                         ^
rpc/jsonrpc/jsonrpc_test.go:94:22: unused-parameter: parameter 'ctx' seems to be unused, consider removing or renaming it as _ (revive)
func EchoWithDefault(ctx *types.Context, v *int) (*ResultEchoWithDefault, error) {
                     ^
p2p/mock/reactor.go:23:27: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (r *Reactor) AddPeer(peer p2p.Peer)                             {}
                          ^
p2p/mock/reactor.go:24:30: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (r *Reactor) RemovePeer(peer p2p.Peer, reason interface{})      {}
                             ^
p2p/mock/peer.go:46:33: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (mp *Peer) TrySendEnvelope(e p2p.Envelope) bool { return true }
                                ^
p2p/mock/peer.go:47:30: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (mp *Peer) SendEnvelope(e p2p.Envelope) bool    { return true }
                             ^
p2p/mock/reactor.go:25:35: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (r *Reactor) ReceiveEnvelope(e p2p.Envelope)                    {}
                                  ^
p2p/mock/reactor.go:26:27: unused-parameter: parameter 'chID' seems to be unused, consider removing or renaming it as _ (revive)
func (r *Reactor) Receive(chID byte, peer p2p.Peer, msgBytes []byte) {}
                          ^
p2p/peer_set_test.go:22:37: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (mp *mockPeer) TrySendEnvelope(e Envelope) bool { return true }
                                    ^
p2p/switch_test.go:68:32: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (tr *TestReactor) AddPeer(peer Peer) {}
                               ^
p2p/peer_set_test.go:23:34: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (mp *mockPeer) SendEnvelope(e Envelope) bool    { return true }
                                 ^
p2p/switch_test.go:70:35: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (tr *TestReactor) RemovePeer(peer Peer, reason interface{}) {}
                                  ^
p2p/switch_test.go:110:21: unused-parameter: parameter 't' seems to be unused, consider removing or renaming it as _ (revive)
func MakeSwitchPair(t testing.TB, initSwitch func(int, *Switch) *Switch) (*Switch, *Switch) {
                    ^
p2p/switch_test.go:116:21: unused-parameter: parameter 'i' seems to be unused, consider removing or renaming it as _ (revive)
func initSwitchFunc(i int, sw *Switch) *Switch {
                    ^
p2p/switch_test.go:722:33: unused-parameter: parameter 'c' seems to be unused, consider removing or renaming it as _ (revive)
func (et errorTransport) Accept(c peerConfig) (Peer, error) {
                                ^
p2p/switch_test.go:771:34: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (r *mockReactor) RemovePeer(peer Peer, reason interface{}) {
                                 ^
p2p/base_reactor.go:84:29: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (*BaseReactor) AddPeer(peer Peer)                             {}
                            ^
p2p/base_reactor.go:85:32: unused-parameter: parameter 'peer' seems to be unused, consider removing or renaming it as _ (revive)
func (*BaseReactor) RemovePeer(peer Peer, reason interface{})      {}
                               ^
p2p/base_reactor.go:86:37: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (*BaseReactor) ReceiveEnvelope(e Envelope)                    {}
                                    ^
p2p/base_reactor.go:87:29: unused-parameter: parameter 'chID' seems to be unused, consider removing or renaming it as _ (revive)
func (*BaseReactor) Receive(chID byte, peer Peer, msgBytes []byte) {}
                            ^
p2p/test_util.go:34:39: unused-parameter: parameter 'other' seems to be unused, consider removing or renaming it as _ (revive)
func (ni mockNodeInfo) CompatibleWith(other NodeInfo) error { return nil }
                                      ^
p2p/test_util.go:186:2: unused-parameter: parameter 'network' seems to be unused, consider removing or renaming it as _ (revive)
	network, version string,
	^
p2p/test_util.go:299:56: unused-parameter: parameter 'src' seems to be unused, consider removing or renaming it as _ (revive)
func (book *AddrBookMock) AddAddress(addr *NetAddress, src *NetAddress) error {
                                                       ^
test/fuzz/p2p/pex/reactor_receive.go:83:34: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (fp *fuzzPeer) SendEnvelope(e p2p.Envelope) bool    { return true }
                                 ^
test/fuzz/p2p/pex/reactor_receive.go:84:37: unused-parameter: parameter 'e' seems to be unused, consider removing or renaming it as _ (revive)
func (fp *fuzzPeer) TrySendEnvelope(e p2p.Envelope) bool { return true }
                                    ^
p2p/pex/pex_reactor.go:221:38: unused-parameter: parameter 'reason' seems to be unused, consider removing or renaming it as _ (revive)
func (r *Reactor) RemovePeer(p Peer, reason interface{}) {
                                     ^
abci/types/application.go:48:29: unused-parameter: parameter 'req' seems to be unused, consider removing or renaming it as _ (revive)
func (BaseApplication) Info(req RequestInfo) ResponseInfo {
                            ^
types/block_test.go:200:5: var `emptyBytes` is unused (unused)
var emptyBytes = []byte{0xe3, 0xb0, 0xc4, 0x42, 0x98, 0xfc, 0x1c, 0x14, 0x9a, 0xfb, 0xf4, 0xc8,
    ^
rpc/core/blocks.go:474:6: func `heightsByQuery` is unused (unused)
func heightsByQuery(ctx *rpctypes.Context, query string) ([]int64, error) {
     ^
light/proxy/routes.go:112:6: type `rpcSignedBlockFunc` is unused (unused)
type rpcSignedBlockFunc func(ctx *rpctypes.Context, height *int64) (*ctypes.ResultSignedBlock, error)
     ^
light/proxy/routes.go:114:6: func `makeSignedBlockFunc` is unused (unused)
func makeSignedBlockFunc(c *lrpc.Client) rpcSignedBlockFunc {
     ^
light/proxy/routes.go:144:6: type `rpcDataCommitmentFunc` is unused (unused)
type rpcDataCommitmentFunc func(
     ^
light/proxy/routes.go:150:6: type `rpcDataRootInclusionProofFunc` is unused (unused)
type rpcDataRootInclusionProofFunc func(
     ^
light/proxy/routes.go:157:6: func `makeDataCommitmentFunc` is unused (unused)
func makeDataCommitmentFunc(c *lrpc.Client) rpcDataCommitmentFunc {
     ^
light/proxy/routes.go:167:6: func `makeDataRootInclusionProofFunc` is unused (unused)
func makeDataRootInclusionProofFunc(c *lrpc.Client) rpcDataRootInclusionProofFunc {
     ^
mempool/cat/store_test.go:94:4: S1000: should use for range instead of for { select {} } (gosimple)
			for {
			^
mempool/cat/pool_test.go:730:5: testinggoroutine: call to (*T).Fatalf from a non-test goroutine (govet)
				t.Fatalf("failed to receive all txs (got %d/%d)", i+1, txs)
				^
rpc/core/events.go:49:5: SA4023: this comparison is never true (staticcheck)
	if sub == nil {
	   ^
rpc/core/events.go:45:2: SA4023(related information): the lhs of the comparison is the 1st return value of this function call (staticcheck)
	sub, err := env.EventBus.Subscribe(subCtx, addr, q, env.Config.SubscriptionBufferSize)
	^
types/event_bus.go:75:20: SA4023(related information): (*github.com/tendermint/tendermint/types.EventBus).Subscribe never returns a nil interface value (staticcheck)
func (b *EventBus) Subscribe(
                   ^
mempool/cat/pool_test.go:725:2: SA2002: the goroutine calls T.Fatalf, which must be called in the same goroutine as the test (staticcheck)
	go func() {
	^
mempool/cat/reactor.go:285:7: SA1019: p2p.SendEnvelopeShim is deprecated: Will be removed in v0.37. (staticcheck)
			if p2p.SendEnvelopeShim(e.Src, p2p.Envelope{
			   ^
mempool/cat/reactor.go:341:3: SA1019: peer.Send is deprecated: entities looking to act as peers should implement SendEnvelope instead. Send will be removed in v0.37. (staticcheck)
		peer.Send(MempoolStateChannel, bz)
		^
mempool/cat/reactor.go:374:6: SA1019: peer.Send is deprecated: entities looking to act as peers should implement SendEnvelope instead. Send will be removed in v0.37. (staticcheck)
		if peer.Send(mempool.MempoolChannel, bz) {
		   ^
mempool/cat/reactor.go:398:13: SA1019: peer.Send is deprecated: entities looking to act as peers should implement SendEnvelope instead. Send will be removed in v0.37. (staticcheck)
	success := peer.Send(MempoolStateChannel, bz)
	           ^
state/execution_test.go:6:2: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package io or package os, and those implementations should be preferred in new code. See the specific function documentation for details. (staticcheck)
	"io/ioutil"
	^

Originally posted by @rootulp in #998 (comment)

Chirag018 · 2023-05-01T16:08:00Z

Hey, @rootulp, Would like to work on this

Chirag018 · 2023-05-08T16:14:57Z

Hey @rootulp, some functions are not performing anything ie., left empty, so can I remove them from the file or comment them out?

One such example,

In file /blockchain/v2/processor_context.go
func (mpc *mockPContext) saveBlock(block *types.Block, blockParts *types.PartSet, seenCommit *types.Commit) { }

rootulp · 2023-05-08T19:10:20Z

Hi @Chirag018, thanks for working on this! Ideally we minimize the diff with https://github.com/cometbft/cometbft so in order of preference I would say:

If the issue is resolved on a more recent cometBFT v0.34.x release, we should update this repo to that
If the issue remains on cometBFT, we can use nolint directive to disable the false positives

Even if the functions have empty function bodies, they may be necessary to satisfy an interface. Ideally we avoid commented out code.

* Add `CMT_HOME` (or remove it?) (#983) Closes #982 Added `CMT_HOME` everywhere `CMTHOME` is used. ### Notes to reviewers This could be fixed the opposite way, by removing the only reference to `CMT_HOME` in the code, and also the reference in `UPGRADING.md` (two lines of code). However, the reference in `UPGRADING.md`, which is part of our documentation, is already present in `v0.34.x` (not in `v0.37.x` though!). That's why this PR introduces `CMT_HOME` to work in equal conditions as `CMTHOME`. If reviewers lean toward removing `CMT_HOME` from the doc in `v0.34.x` (and unreleased `v0.38.x` and `main`), I can do it easily. --- #### PR checklist - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments (cherry picked from commit b7be568f4a59edf41942ec7e0e25edd72df604d8) # Conflicts: # cmd/cometbft/commands/root_test.go * Revert "Add `CMT_HOME` (or remove it?) (#983)" * Add `CMT_HOME` (or remove it?) (#983) Closes #982 Added `CMT_HOME` everywhere `CMTHOME` is used. This could be fixed the opposite way, by removing the only reference to `CMT_HOME` in the code, and also the reference in `UPGRADING.md` (two lines of code). However, the reference in `UPGRADING.md`, which is part of our documentation, is already present in `v0.34.x` (not in `v0.37.x` though!). That's why this PR introduces `CMT_HOME` to work in equal conditions as `CMTHOME`. If reviewers lean toward removing `CMT_HOME` from the doc in `v0.34.x` (and unreleased `v0.38.x` and `main`), I can do it easily. --- - [x] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Sergio Mena <sergio@informal.systems>

rootulp · 2024-02-28T15:37:30Z

I think these have been fixed b/c I can no longer repro on main or v0.34.x-celestia:

# main
$ golangci-lint run --timeout 10m

# v0.34.x-celestia
$ golangci-lint run --timeout 10m

cc: @ramin were you able to repro the lint issues identified in the OP of this issue before #1242 ?

… up to date (#1242) ## Description Decided to take a look at #1000 and see what was up, seems linting DOES run and is not catching issues that are passed locally, but, while here i noticed some very out of date deprecation warnings on github action runtimes. So: - updated `actions/checkout` to `v3` -> `v4` - updated `actions/setup-go` to `v4` -> `v5` - noticed the `git diff` action was stuck on node16 runtime and deprecated, so replicated behavior with paths in workflow trigger, even though it does require 2 entries (1 for push, 1 for pull_request) seemed cleaner than an unmaintained github action workflow - finally: set `setup-go` to use `go.mod` for sourcing the go version. If welcome, i intend to update warnings across all workflows next. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

… up to date (#1242) Decided to take a look at #1000 and see what was up, seems linting DOES run and is not catching issues that are passed locally, but, while here i noticed some very out of date deprecation warnings on github action runtimes. So: - updated `actions/checkout` to `v3` -> `v4` - updated `actions/setup-go` to `v4` -> `v5` - noticed the `git diff` action was stuck on node16 runtime and deprecated, so replicated behavior with paths in workflow trigger, even though it does require 2 entries (1 for push, 1 for pull_request) seemed cleaner than an unmaintained github action workflow - finally: set `setup-go` to use `go.mod` for sourcing the go version. If welcome, i intend to update warnings across all workflows next. --- - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

rootulp added good first issue Good for newcomers C:CI Continuous integration and automation related issues labels Apr 17, 2023

rootulp mentioned this issue Apr 20, 2023

Update linter version in v0.34 branch and fix typos #678

Closed

rootulp added this to the Mainnet milestone Apr 20, 2023

rootulp assigned Chirag018 May 1, 2023

rootulp unassigned Chirag018 Jul 12, 2023

rootulp removed this from the Mainnet milestone Jul 12, 2023

ramin mentioned this issue Feb 27, 2024

ci(lint): remove GitHub workflow runtime deprecations, ensure lint is up to date #1242

Merged

3 tasks

rootulp closed this as completed Feb 28, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix lint issues #1000

fix lint issues #1000

rootulp commented Apr 17, 2023

Chirag018 commented May 1, 2023

Chirag018 commented May 8, 2023

rootulp commented May 8, 2023

rootulp commented Feb 28, 2024 •

edited

Loading

fix lint issues #1000

fix lint issues #1000

Comments

rootulp commented Apr 17, 2023

Context

Proposal

Additional Details

Chirag018 commented May 1, 2023

Chirag018 commented May 8, 2023

rootulp commented May 8, 2023

rootulp commented Feb 28, 2024 • edited Loading

rootulp commented Feb 28, 2024 •

edited

Loading