test: make Go code lint stricter

[gyuho]

Possible addition to our format tests

• public method on unexported struct clientv3: change Close() to close() for keepAlive and watchGrpcStream #8066
• leaky embedded mutexes *: Fix embedded mutexes leaking methods #7833 (pending PR Flag exported types and methods from the sync package golang/lint#289)

Either use external tools or propose to cmd/vet team

[heyitsanthony]

Unused function arguments would be a good one, CI should have caught this bug from #8074:

func (dec *messageDecoder) decodeLimit(numBytes uint64) (raftpb.Message, error) {
        var m raftpb.Message
        var l uint64
        if err := binary.Read(dec.r, binary.BigEndian, &l); err != nil {
                return m, err
        }
        if l > readBytesLimit {
                return m, ErrExceedSizeLimit
        }
        buf := make([]byte, int(l))
        if _, err := io.ReadFull(dec.r, buf); err != nil {
                return m, err
        }
        return m, m.Unmarshal(buf)
}

[gyuho]

TODO: try https://github.com/mvdan/unparam.

[heyitsanthony]

Gave it a spin, totally broken. Can't recognize import renaming. The ignore policy seems too weak anyway; if an argument is unused and not given an underscore name, there's still a bunch of ways it'll let that pass, even though it probably shouldn't...

[mvdan]

Hey, original author here. If you find any issues in particular, please file issues with a small example to reproduce them.

I realise the tool likely has some false negatives and positives, but it's impossible (or near impossible?) to have a perfect solution given how arguments might be used in lots of subtle ways.

But of course, I'm sure the tool can do better :) Happy to help if you give me concrete issues.

[mvdan]

Friendly ping - still happy to look into false positives or negatives if you'd give me pointers.

[mvdan]

I have been improving the tool the past couple of weeks, and I've been using etcd as one of the sources for ideas on how to improve.

Last week, the number of warnings was upwards of 100. Now, it's down to 31:

client/cancelreq.go:11:22: requestCanceler - tr is unused
clientv3/concurrency/election.go:49:33: ResumeElection - pfx is unused
contrib/recipes/key.go:47:56: newUniqueKV - result 0 (*github.com/coreos/etcd/contrib/recipes.RemoteKV) is never used
etcdctl/ctlv3/command/del_command.go:56:15: getDelOp - cmd is unused
etcdctl/ctlv3/command/get_command.go:77:15: getGetOp - cmd is unused
etcdctl/ctlv3/command/migrate_command.go:240:46: writeStore - result 0 (uint64) is never used
etcdctl/ctlv3/command/put_command.go:79:15: getPutOp - cmd is unused
etcdserver/api/v2http/capability.go:25:24: capabilityHandler - c always receives "auth"
etcdserver/api/v2http/client.go:624:48: trimErrorPrefix - result 0 (error) is just parameter err
etcdserver/api/v2http/metrics.go:67:59: reportRequestCompleted - response is unused
etcdserver/api/v2v3/store.go:94:56: (*v2v3Store).getDir - sorted is unused
etcdserver/auth/auth_requests.go:108:45: (*store).requestResource - dir always receives false
etcdserver/auth/auth_requests.go:151:45: (*store).deleteResource - result 0 (github.com/coreos/etcd/etcdserver.Response) is never used
etcdserver/raft.go:302:57: (*raftNode).processMessages - result 0 ([]github.com/coreos/etcd/raft/raftpb.Message) is just parameter ms
integration/bridge.go:203:25: (*bridge).ioCopy - bc is unused
integration/cluster.go:422:32: (*cluster).waitNoLeader - t is unused
integration/cluster.go:646:24: (*member).Clone - t is unused
integration/cluster.go:843:23: (*member).Stop - t is unused
integration/cluster.go:851:28: checkLeaderTransition - t is unused
integration/cluster.go:928:34: (*member).InjectPartition - t is unused
integration/cluster.go:936:35: (*member).RecoverPartition - t is unused
mvcc/kvstore_txn.go:222:45: (*storeTxnWrite).delete - rev is unused
pkg/transport/listener.go:52:14: wrapTLS - addr is unused
raft/log.go:94:44: (*raftLog).append - result 0 (uint64) is never used
tools/functional-tester/etcd-agent/rpc.go:49:40: (*Agent).RPCStop - reply is unused
tools/functional-tester/etcd-agent/rpc.go:70:43: (*Agent).RPCCleanup - reply is unused
tools/functional-tester/etcd-agent/rpc.go:80:45: (*Agent).RPCTerminate - reply is unused
tools/functional-tester/etcd-agent/rpc.go:89:39: (*Agent).RPCDropPort - reply is unused
tools/functional-tester/etcd-agent/rpc.go:98:42: (*Agent).RPCRecoverPort - reply is unused
tools/functional-tester/etcd-agent/rpc.go:107:43: (*Agent).RPCSetLatency - reply is unused
tools/functional-tester/etcd-agent/rpc.go:119:49: (*Agent).RPCRemoveLatency - reply is unused

Except the *Agent ones which seem a bit weird, the rest all look like real warnings to me.

Whether or not the signature of exported funcs can be changed is a different aspect - the tool does nothing about that, at the moment. Thinking of adding a flag, defaulting to assuming that backwards compatibility cannot be broken.

[gyuho]

@mvdan We will integrate https://github.com/mvdan/unparam in CI. Seems useful!