Should we fix more static check errors?

[xiekeyi98]

1. In the PR: Makefile: Replace Gometalinter to Golangci-lint #13405 will changed CI from gometalinter to golangci-lint. And it will easy to run more check in the future. ( This PR is not merged already , PTAL plz. And later I will try to check new code and ignore the issues already exist )
2. In PRs say we should refine some code with staticcheck help:

• Refine some code in TiDB #13736
• staticcheck report some errors #13456

However, Shouldwe be clear about what checks are necessary?
There are too many checks for us, which is neceeary and which is not?

I think we should clear about that first.

By the way:

golangci-lint run ./... --deadline=20m                                   
bindinfo/bind_test.go:42:20: Error return value of `logutil.InitLogger` is not checked (errcheck)
        logutil.InitLogger(logutil.NewLogConfig(logLevel, logutil.DefaultLogFormat, "", logutil.EmptyFileLogConfig, false))
                          ^
bindinfo/bind_test.go:164:83: Error return value of `(github.com/prometheus/client_golang/prometheus.Metric).Write` is not checked (errcheck)
        metrics.BindTotalGauge.WithLabelValues(metrics.ScopeGlobal, bindinfo.Using).Write(pb)
                                                                                         ^
bindinfo/bind_test.go:166:84: Error return value of `(github.com/prometheus/client_golang/prometheus.Metric).Write` is not checked (errcheck)
        metrics.BindMemoryUsage.WithLabelValues(metrics.ScopeGlobal, bindinfo.Using).Write(pb)
                                                                                          ^
bindinfo/bind_test.go:221:83: Error return value of `(github.com/prometheus/client_golang/prometheus.Metric).Write` is not checked (errcheck)
        metrics.BindTotalGauge.WithLabelValues(metrics.ScopeGlobal, bindinfo.Using).Write(pb)
                                                                                         ^
cmd/ddltest/column_test.go:192:11: Error return value of `s.exec` is not checked (errcheck)
                                s.exec(fmt.Sprintf("insert into test_column values (%d, %d, %d)", key, key, key))
                                      ^
cmd/ddltest/column_test.go:195:11: Error return value of `s.exec` is not checked (errcheck)
                                s.exec(fmt.Sprintf("update test_column set c2 = %d, c3 = %d where c1 = %d",
                                      ^
cmd/ddltest/column_test.go:215:12: Error return value of `s1.Execute` is not checked (errcheck)
        s1.Execute(ctx, "begin")
                  ^
cmd/ddltest/column_test.go:216:12: Error return value of `s1.Execute` is not checked (errcheck)
        s1.Execute(ctx, "insert into test_commit values (3, 3)")
                  ^
cmd/ddltest/column_test.go:221:12: Error return value of `s1.Execute` is not checked (errcheck)
        s1.Execute(ctx, "insert into test_commit values (4, 4)")
                  ^
cmd/ddltest/ddl_test.go:95:20: Error return value of `logutil.InitLogger` is not checked (errcheck)
        logutil.InitLogger(&logutil.LogConfig{Config: zaplog.Config{Level: *logLevel}})
                          ^
cmd/ddltest/ddl_test.go:623:11: Error return value of `s.exec` is not checked (errcheck)
                                s.exec(fmt.Sprintf("insert into test_conflict_insert values (%d, %d)", k, k))
                                      ^
cmd/ddltest/ddl_test.go:1020:16: Error return value of `store.Register` is not checked (errcheck)
        store.Register("tikv", tikv.Driver{})
                      ^
cmd/ddltest/index_test.go:67:15: Error return value of `txn.Rollback` is not checked (errcheck)
                txn.Rollback()
                            ^
cmd/ddltest/index_test.go:105:20: Error return value of `txn.Rollback` is not checked (errcheck)
        defer txn.Rollback()
                          ^
config/config_test.go:110:13: Error return value of `f.Truncate` is not checked (errcheck)
                f.Truncate(0)
                          ^
config/config_test.go:111:9: Error return value of `f.Seek` is not checked (errcheck)
                f.Seek(0, 0)
                      ^
config/config_test.go:173:12: Error return value of `f.Truncate` is not checked (errcheck)
        f.Truncate(0)
                  ^
config/config_test.go:174:8: Error return value of `f.Seek` is not checked (errcheck)
        f.Seek(0, 0)
              ^
ddl/column_change_test.go:67:14: Error return value of `d.Stop` is not checked (errcheck)
        defer d.Stop()
                    ^
ddl/column_change_test.go:154:8: Error return value of `d.Stop` is not checked (errcheck)
        d.Stop()
              ^
ddl/column_change_test.go:204:8: Error return value of `d.Stop` is not checked (errcheck)
        d.Stop()
              ^
ddl/column_test.go:60:10: Error return value of `s.d.Stop` is not checked (errcheck)
        s.d.Stop()
                ^
ddl/column_test.go:279:14: Error return value of `txn.Commit` is not checked (errcheck)
                        txn.Commit(context.Background())
                                  ^
ddl/column_test.go:678:15: Error return value of `t.IterRecords` is not checked (errcheck)
        t.IterRecords(ctx, t.FirstKey(), t.Cols(), func(h int64, data []types.Datum, cols []*table.Column) (bool, error) {
                     ^
ddl/column_test.go:822:10: Error return value of `s.d.Stop` is not checked (errcheck)
        s.d.Stop()
                ^
ddl/column_test.go:904:10: Error return value of `s.d.Stop` is not checked (errcheck)
        s.d.Stop()
                ^
ddl/db_change_test.go:86:14: Error return value of `s.se.Execute` is not checked (errcheck)
        s.se.Execute(context.Background(), "drop database if exists test_db_state")
                    ^
ddl/db_change_test.go:190:29: Error return value of `originalCallback.OnChanged` is not checked (errcheck)
                originalCallback.OnChanged(nil)
                                          ^
ddl/db_change_test.go:261:20: Error return value of `s.se.Execute` is not checked (errcheck)
        defer s.se.Execute(context.Background(), "drop table t")
                          ^
ddl/db_change_test.go:466:20: Error return value of `s.se.Execute` is not checked (errcheck)
        defer s.se.Execute(context.Background(), "drop table t")
                          ^
ddl/db_change_test.go:498:13: Error return value of `checkResult` is not checked (errcheck)
        checkResult(result, testkit.Rows(expected...))
                   ^
ddl/db_change_test.go:505:13: Error return value of `checkResult` is not checked (errcheck)
        checkResult(result, testkit.Rows(expected...))
                   ^
ddl/db_change_test.go:870:18: Error return value of `kv.RunInNewTxn` is not checked (errcheck)
                        kv.RunInNewTxn(s.store, false, func(txn kv.Transaction) error {
                                      ^
ddl/db_change_test.go:907:18: Error return value of `kv.RunInNewTxn` is not checked (errcheck)
                        kv.RunInNewTxn(s.store, false, func(txn kv.Transaction) error {
                                      ^
ddl/db_integration_test.go:538:16: Error return value of `kv.RunInNewTxn` is not checked (errcheck)
        kv.RunInNewTxn(s.store, false, func(txn kv.Transaction) error {
                      ^
ddl/db_test.go:115:13: Error return value of `s.s.Execute` is not checked (errcheck)
        s.s.Execute(context.Background(), "drop database if exists test_db")
                   ^
ddl/db_test.go:1068:14: Error return value of `txn.Rollback` is not checked (errcheck)
        txn.Rollback()
                    ^
ddl/db_test.go:1621:12: Error return value of `ctx.NewTxn` is not checked (errcheck)
        ctx.NewTxn(context.Background())
                  ^
ddl/ddl_test.go:97:20: Error return value of `logutil.InitLogger` is not checked (errcheck)
        logutil.InitLogger(logutil.NewLogConfig(logLevel, "", "", logutil.EmptyFileLogConfig, false))
                          ^
ddl/ddl_worker_test.go:61:15: Error return value of `d1.Stop` is not checked (errcheck)
        defer d1.Stop()
                     ^
ddl/ddl_worker_test.go:93:15: Error return value of `d1.Stop` is not checked (errcheck)
        defer d1.Stop()
                     ^
ddl/ddl_worker_test.go:1071:13: Error return value of `d.addDDLJob` is not checked (errcheck)
        d.addDDLJob(ctx, job1)
                   ^
ddl/ddl_worker_test.go:1073:13: Error return value of `d.addDDLJob` is not checked (errcheck)
        d.addDDLJob(ctx, job2)
                   ^
ddl/ddl_worker_test.go:1075:13: Error return value of `d.addDDLJob` is not checked (errcheck)
        d.addDDLJob(ctx, job3)
                   ^
ddl/index_change_test.go:318:23: Error return value of `publicTbl.IterRecords` is not checked (errcheck)
        publicTbl.IterRecords(ctx, publicTbl.FirstKey(), publicTbl.Cols(),
                             ^
ddl/reorg_test.go:171:15: Error return value of `d1.Stop` is not checked (errcheck)
        defer d1.Stop()
                     ^
ddl/reorg_test.go:182:15: Error return value of `d2.Stop` is not checked (errcheck)
        defer d2.Stop()
                     ^
ddl/schema_test.go:207:15: Error return value of `d2.Stop` is not checked (errcheck)
        defer d2.Stop()
                     ^
ddl/util/syncer_test.go:222:40: Error return value of `(github.com/pingcap/tidb/ddl/util.SchemaSyncer).RemoveSelfVersionPath` is not checked (errcheck)
        d.SchemaSyncer().RemoveSelfVersionPath()
                                              ^
domain/db_test.go:53:12: Error return value of `se.Execute` is not checked (errcheck)
        se.Execute(context.Background(), "use test")
                  ^
executor/aggfuncs/func_group_concat.go:69:2: `buffer` is unused (structcheck)
        buffer  *bytes.Buffer
        ^
executor/aggfuncs/func_group_concat.go:68:2: `valsBuf` is unused (structcheck)
        valsBuf *bytes.Buffer
        ^
executor/aggfuncs/func_first_row.go:26:2: `isNull` is unused (structcheck)
        isNull bool
        ^
executor/aggfuncs/func_first_row.go:29:2: `gotFirstRow` is unused (structcheck)
        gotFirstRow bool
        ^
executor/insert_common.go:40:2: `maxRowsInBatch` is unused (structcheck)
        maxRowsInBatch uint64
        ^
executor/insert_common.go:39:2: `curBatchCnt` is unused (structcheck)
        curBatchCnt    uint64
        ^
executor/split.go:61:2: `finishScatterNum` is unused (structcheck)
        finishScatterNum int
        ^
executor/split.go:60:2: `splitRegions` is unused (structcheck)
        splitRegions     int
        ^
executor/trace.go:51:2: `rootTrace` is unused (structcheck)
        rootTrace opentracing.Span
        ^
executor/analyze.go:248:2: `priority` is unused (structcheck)
        priority        int
        ^
executor/analyze.go:400:2: `priority` is unused (structcheck)
        priority        int
        ^
executor/sort.go:41:2: `keyExprs` is unused (structcheck)
        keyExprs []expression.Expression
        ^
executor/sort.go:42:2: `keyTypes` is unused (structcheck)
        keyTypes []*types.FieldType
        ^
executor/distsql.go:367:2: `idxTblCols` is unused (structcheck)
        idxTblCols []*table.Column
        ^
executor/update_test.go:37:2: `ctx` is unused (structcheck)
        ctx *mock.Context
        ^
executor/executor_test.go:131:2: `ctx` is unused (structcheck)
        ctx *mock.Context
        ^
executor/seqtest/seq_executor_test.go:75:2: `ctx` is unused (structcheck)
        ctx *mock.Context
        ^
expression/builtin_encryption.go:81:2: `modeName` is unused (structcheck)
        modeName   string
        ^
expression/builtin_encryption.go:82:2: `keySize` is unused (structcheck)
        keySize    int
        ^
expression/constant_propagation.go:34:2: `unionSet` is unused (structcheck)
        unionSet  *disjointset.IntSet // unionSet stores the relations like col_i = col_j
        ^
kv/memdb_buffer.go:31:2: `bufferLenLimit` is unused (structcheck)
        bufferLenLimit  uint64
        ^
kv/memdb/memdb.go:322:2: `height` is unused (structcheck)
        height uint16
        ^
kv/memdb/memdb.go:323:2: `keyLen` is unused (structcheck)
        keyLen uint16
        ^
kv/memdb/memdb.go:324:2: `valLen` is unused (structcheck)
        valLen uint32
        ^
planner/core/point_get_plan.go:51:2: `expr` is unused (structcheck)
        expr             expression.Expression
        ^
planner/core/logical_plans.go:236:2: `calculateGenCols` is unused (structcheck)
        calculateGenCols bool
        ^
planner/core/physical_plans.go:345:2: `outerSchema` is unused (structcheck)
        outerSchema *expression.Schema
        ^
planner/core/rule_join_reorder.go:108:2: `curJoinGroup` is unused (structcheck)
        curJoinGroup []*jrNode
        ^
planner/core/exhaust_physical_plans.go:617:2: `chosenIndexInfo` is unused (structcheck)
        chosenIndexInfo *model.IndexInfo
        ^
planner/memo/group_test.go:134:2: `cost` is unused (structcheck)
        cost float64
        ^
server/http_handler.go:567:2: `first` is unused (structcheck)
        first  *FrameItem        // start frame of the region
        ^
server/http_handler.go:568:2: `last` is unused (structcheck)
        last   *FrameItem        // end frame of the region
        ^
server/http_handler.go:569:2: `region` is unused (structcheck)
        region *tikv.KeyLocation // the region
        ^
store/mockstore/mocktikv/cluster.go:645:2: `tokenCount` is unused (structcheck)
        tokenCount atomic.Int64
        ^
store/mockstore/mocktikv/mvcc.go:245:2: `value` is unused (structcheck)
        value []byte
        ^
store/mockstore/mocktikv/cluster_test.go:37:2: `store` is unused (structcheck)
        store kv.Storage
        ^
store/tikv/txn.go:75:2: `confirmed` is unused (structcheck)
        confirmed  int
        ^
store/tikv/txn.go:74:2: `assertions` is unused (structcheck)
        assertions []assertionPair
        ^
types/mydecimal_test.go:659:3: `err` is unused (structcheck)
                err    error
                ^
tablecodec/tablecodec.go:721:2: `codeInvalidColumnCount` is unused (deadcode)
        codeInvalidColumnCount = 5
        ^
ddl/ddl.go:103:2: `errUnknownTypeLength` is unused (deadcode)
        errUnknownTypeLength        = terror.ClassDDL.New(mysql.ErrUnknownTypeLength, mysql.MySQLErrName[mysql.ErrUnknownTypeLength])
        ^
ddl/ddl.go:104:2: `errUnknownFractionLength` is unused (deadcode)
        errUnknownFractionLength    = terror.ClassDDL.New(mysql.ErrUnknownFractionLength, mysql.MySQLErrName[mysql.ErrUnknownFractionLength])
        ^
plugin/conn_ip_example/conn_ip_example.go:25:6: `Validate` is unused (deadcode)
func Validate(ctx context.Context, m *plugin.Manifest) error {
     ^
plugin/conn_ip_example/conn_ip_example.go:31:6: `OnInit` is unused (deadcode)
func OnInit(ctx context.Context, manifest *plugin.Manifest) error {
     ^
plugin/conn_ip_example/conn_ip_example.go:38:6: `OnShutdown` is unused (deadcode)
func OnShutdown(ctx context.Context, manifest *plugin.Manifest) error {
     ^
plugin/conn_ip_example/conn_ip_example.go:44:6: `OnGeneralEvent` is unused (deadcode)
func OnGeneralEvent(ctx context.Context, sctx *variable.SessionVars, event plugin.GeneralEvent, cmd string) {
     ^
session/bootstrap.go:346:2: `version29` is unused (deadcode)
        version29 = 29
        ^
WARN [runner] Can't process result by autogenerated_exclude processor: can't filter issue result.Issue{FromLinter:"govet", Text:"nilness: impossible condition: non-nil == nil", Pos:token.Position{Filename:"", Offset:0, Line:0, Column:0}, LineRange:(*result.Range)(nil), HunkPos:0, SourceLines:[]string(nil), Replacement:(*result.Replacement)(nil)}: no file path for issue 
WARN [runner] Can't process result by nolint processor: can't filter issue result.Issue{FromLinter:"govet", Text:"nilness: impossible condition: non-nil == nil", Pos:token.Position{Filename:"", Offset:0, Line:0, Column:0}, LineRange:(*result.Range)(nil), HunkPos:0, SourceLines:[]string(nil), Replacement:(*result.Replacement)(nil)}: no file path for issue 
WARN [runner/source_code] Failed to get line 0 for file : failed to get file  lines cache: can't get file  bytes from cache: can't read file : open : no such file or directory 
ddl/partition.go:596:11: nilness: impossible condition: nil != nil (govet)
                        if err != nil {
                               ^
domain/domain_test.go:132:24: nilness: tautological condition: non-nil != nil (govet)
        if err == nil || (err != nil && err.Error() != "[info-syncer] get /tidb/server/info/not_exist_id failed") {
                              ^
:0: nilness: impossible condition: non-nil == nil (govet)
expression/builtin_time.go:6126:9: nilness: impossible condition: nil != nil (govet)
        if err != nil {
               ^
statistics/table.go:594:10: nilness: impossible condition: nil != nil (govet)
                if err != nil {
                       ^
statistics/table.go:577:18: nilness: tautological condition: nil == nil (govet)
                        } else if err == nil {
                                      ^
[1]    6478 killed     golangci-lint run ./... --deadline=20m

[ngaut]

Yes, we should.

[dveeden]

How does this relate to #22979 ?

[disksing]

The issue mentioned here is tracked by #22979. I'm going to close it.