From ee1204dc2b5b712d65c61c6d0e98df4b2df42cd8 Mon Sep 17 00:00:00 2001 From: YangKeao Date: Wed, 10 Apr 2024 19:45:22 +0800 Subject: [PATCH] linter, *: enable linters appends and unusedwrite (#52452) ref pingcap/tidb#52449, close pingcap/tidb#52450 --- br/pkg/stream/rewrite_meta_rawkv_test.go | 8 ++------ build/BUILD.bazel | 2 ++ build/linter/misspell/analyzer.go | 4 +--- build/nogo_config.json | 14 ++++++++++++++ pkg/ddl/stat_test.go | 4 +--- pkg/executor/aggfuncs/func_group_concat.go | 14 ++++++++++++-- pkg/executor/test/aggregate/aggregate_test.go | 2 -- pkg/planner/core/logical_plans_test.go | 4 ++-- .../core/memtable_predicate_extractor.go | 2 +- pkg/planner/funcdep/fd_graph_test.go | 18 ++++++------------ pkg/server/conn_stmt_test.go | 10 +++++----- pkg/sessionctx/variable/session.go | 2 +- 12 files changed, 47 insertions(+), 37 deletions(-) diff --git a/br/pkg/stream/rewrite_meta_rawkv_test.go b/br/pkg/stream/rewrite_meta_rawkv_test.go index b7581e0a3eab1..040304cafdf18 100644 --- a/br/pkg/stream/rewrite_meta_rawkv_test.go +++ b/br/pkg/stream/rewrite_meta_rawkv_test.go @@ -468,18 +468,14 @@ func TestRewriteTableInfoForExchangePartition(t *testing.T) { Name: model.NewCIStr(tableName1), Partition: &pi, } - db1 := model.DBInfo{ - ID: dbID1, - } + db1 := model.DBInfo{} // construct table t2 without partition. t2 := model.TableInfo{ ID: tableID2, Name: model.NewCIStr(tableName2), } - db2 := model.DBInfo{ - ID: dbID2, - } + db2 := model.DBInfo{} // construct the SchemaReplace dbMap := make(map[UpstreamID]*DBReplace) diff --git a/build/BUILD.bazel b/build/BUILD.bazel index a1a180de16dd1..1d3f33dd8448a 100644 --- a/build/BUILD.bazel +++ b/build/BUILD.bazel @@ -105,6 +105,7 @@ nogo( config = ":nogo_config.json", visibility = ["//visibility:public"], # must have public visibility deps = [ + "@org_golang_x_tools//go/analysis/passes/appends:go_default_library", "@org_golang_x_tools//go/analysis/passes/asmdecl:go_default_library", "@org_golang_x_tools//go/analysis/passes/assign:go_default_library", "@org_golang_x_tools//go/analysis/passes/atomic:go_default_library", @@ -142,6 +143,7 @@ nogo( "@org_golang_x_tools//go/analysis/passes/unreachable:go_default_library", "@org_golang_x_tools//go/analysis/passes/unsafeptr:go_default_library", "@org_golang_x_tools//go/analysis/passes/unusedresult:go_default_library", + "@org_golang_x_tools//go/analysis/passes/unusedwrite:go_default_library", "//build/linter/asciicheck", "//build/linter/bodyclose", "//build/linter/bootstrap", diff --git a/build/linter/misspell/analyzer.go b/build/linter/misspell/analyzer.go index f4482aa8c4422..5246d82d527ff 100644 --- a/build/linter/misspell/analyzer.go +++ b/build/linter/misspell/analyzer.go @@ -50,9 +50,7 @@ func run(pass *analysis.Pass) (any, error) { } // Figure out regional variations - settings := &Misspell{ - Locale: "", - } + settings := &Misspell{} if len(settings.IgnoreWords) != 0 { r.RemoveRule(settings.IgnoreWords) diff --git a/build/nogo_config.json b/build/nogo_config.json index c6a9b25e27f49..39ae55d92fcf5 100644 --- a/build/nogo_config.json +++ b/build/nogo_config.json @@ -1348,5 +1348,19 @@ "only_files": { "pkg/session/": "bootstrap code" } + }, + "appends": { + "exclude_files": { + "pkg/parser/parser.go": "parser/parser.go code", + "external/": "no need to vet third party code", + ".*_generated\\.go$": "ignore generated code" + } + }, + "unusedwrite": { + "exclude_files": { + "pkg/parser/parser.go": "parser/parser.go code", + "external/": "no need to vet third party code", + ".*_generated\\.go$": "ignore generated code" + } } } diff --git a/pkg/ddl/stat_test.go b/pkg/ddl/stat_test.go index 7d9d0c3b9e607..562e51e155545 100644 --- a/pkg/ddl/stat_test.go +++ b/pkg/ddl/stat_test.go @@ -46,9 +46,7 @@ func TestGetDDLInfo(t *testing.T) { require.NoError(t, err) dbInfo2 := &model.DBInfo{ - ID: 2, - Name: model.NewCIStr("b"), - State: model.StateNone, + ID: 2, } job := &model.Job{ ID: 1, diff --git a/pkg/executor/aggfuncs/func_group_concat.go b/pkg/executor/aggfuncs/func_group_concat.go index f15b9936759be..b041078651d23 100644 --- a/pkg/executor/aggfuncs/func_group_concat.go +++ b/pkg/executor/aggfuncs/func_group_concat.go @@ -303,7 +303,8 @@ type topNRows struct { rows []sortRow desc []bool sctx AggFuncUpdateContext - err error + // TODO: this err is never assigned now. Please choose to make use of it or just remove it. + err error currSize uint64 limitSize uint64 @@ -325,7 +326,16 @@ func (h topNRows) Less(i, j int) bool { for k := 0; k < n; k++ { ret, err := h.rows[i].byItems[k].Compare(h.sctx.TypeCtx(), h.rows[j].byItems[k], h.collators[k]) if err != nil { - h.err = err + // TODO: check whether it's appropriate to just ignore the error here. + // + // Previously, the error is assigned to `h.err` and hope it can be accessed from outside. However, + // the `h` is copied when calling this method, and the assignment to `h.err` is meaningless. + // + // The linter `unusedwrite` found this issue. Therefore, the unused write to `h.err` is removed and + // it doesn't change the behavior. But we need to confirm whether it's correct to just ignore the error + // here. + // + // Ref https://github.com/pingcap/tidb/issues/52449 return false } if h.desc[k] { diff --git a/pkg/executor/test/aggregate/aggregate_test.go b/pkg/executor/test/aggregate/aggregate_test.go index 3aba7bf18ec30..3d30adde4a2b3 100644 --- a/pkg/executor/test/aggregate/aggregate_test.go +++ b/pkg/executor/test/aggregate/aggregate_test.go @@ -38,11 +38,9 @@ import ( func TestHashAggRuntimeStat(t *testing.T) { partialInfo := &aggregate.AggWorkerInfo{ Concurrency: 5, - WallTime: int64(time.Second * 20), } finalInfo := &aggregate.AggWorkerInfo{ Concurrency: 8, - WallTime: int64(time.Second * 10), } stats := &aggregate.HashAggRuntimeStats{ PartialConcurrency: 5, diff --git a/pkg/planner/core/logical_plans_test.go b/pkg/planner/core/logical_plans_test.go index 2511b5d8c30b7..bcc6f77ba2a2d 100644 --- a/pkg/planner/core/logical_plans_test.go +++ b/pkg/planner/core/logical_plans_test.go @@ -91,8 +91,8 @@ func createPlannerSuite() (s *plannerSuite) { if pi == nil { continue } - for _, def := range pi.Definitions { - def.ID = id + for i := range pi.Definitions { + pi.Definitions[i].ID = id id += 1 } } diff --git a/pkg/planner/core/memtable_predicate_extractor.go b/pkg/planner/core/memtable_predicate_extractor.go index c443037775d8e..762c1782548af 100644 --- a/pkg/planner/core/memtable_predicate_extractor.go +++ b/pkg/planner/core/memtable_predicate_extractor.go @@ -1354,7 +1354,7 @@ func (e *SlowQueryExtractor) decodeBytesToTime(bs []byte) (int64, error) { func (*SlowQueryExtractor) decodeToTime(handle kv.Handle) (int64, error) { tp := types.NewFieldType(mysql.TypeDatetime) - col := rowcodec.ColInfo{ID: 0, Ft: tp} + col := rowcodec.ColInfo{Ft: tp} chk := chunk.NewChunkWithCapacity([]*types.FieldType{tp}, 1) coder := codec.NewDecoder(chk, nil) _, err := coder.DecodeOne(handle.EncodedCol(0), 0, col.Ft) diff --git a/pkg/planner/funcdep/fd_graph_test.go b/pkg/planner/funcdep/fd_graph_test.go index f4724358d3991..50e5569c12742 100644 --- a/pkg/planner/funcdep/fd_graph_test.go +++ b/pkg/planner/funcdep/fd_graph_test.go @@ -26,22 +26,16 @@ func TestAddStrictFunctionalDependency(t *testing.T) { fdEdges: []*fdEdge{}, } fe1 := &fdEdge{ - from: intset.NewFastIntSet(1, 2), // AB -> CDEFG - to: intset.NewFastIntSet(3, 4, 5, 6, 7), - strict: true, - equiv: false, + from: intset.NewFastIntSet(1, 2), // AB -> CDEFG + to: intset.NewFastIntSet(3, 4, 5, 6, 7), } fe2 := &fdEdge{ - from: intset.NewFastIntSet(1, 2), // AB -> CD - to: intset.NewFastIntSet(3, 4), - strict: true, - equiv: false, + from: intset.NewFastIntSet(1, 2), // AB -> CD + to: intset.NewFastIntSet(3, 4), } fe3 := &fdEdge{ - from: intset.NewFastIntSet(1, 2), // AB -> EF - to: intset.NewFastIntSet(5, 6), - strict: true, - equiv: false, + from: intset.NewFastIntSet(1, 2), // AB -> EF + to: intset.NewFastIntSet(5, 6), } // fd: AB -> CDEFG implies all of others. assertF := func() { diff --git a/pkg/server/conn_stmt_test.go b/pkg/server/conn_stmt_test.go index 0bca8fee246da..8709202e7bb5f 100644 --- a/pkg/server/conn_stmt_test.go +++ b/pkg/server/conn_stmt_test.go @@ -357,8 +357,8 @@ func TestCursorFetchReset(t *testing.T) { require.NoError(t, c.flush(context.Background())) require.Equal(t, expected, out.Bytes()) // reset the statement - require.NoError(t, c.Dispatch(ctx, append( - appendUint32([]byte{mysql.ComStmtReset}, uint32(stmt.ID())), + require.NoError(t, c.Dispatch(ctx, appendUint32( + []byte{mysql.ComStmtReset}, uint32(stmt.ID()), ))) // the following fetch will fail require.Error(t, c.Dispatch(ctx, appendUint32(appendUint32([]byte{mysql.ComStmtFetch}, uint32(stmt.ID())), 1))) @@ -446,9 +446,9 @@ func TestCursorFetchSendLongDataReset(t *testing.T) { // send a parameter to the server require.NoError(t, dispatchSendLongData(c, stmt.ID(), 0, appendUint64([]byte{}, 1))) // reset the statement - require.NoError(t, c.Dispatch(ctx, append( - appendUint32([]byte{mysql.ComStmtReset}, uint32(stmt.ID())), - ))) + require.NoError(t, c.Dispatch(ctx, appendUint32( + []byte{mysql.ComStmtReset}, uint32(stmt.ID())), + )) // execute directly will fail require.Error(t, c.Dispatch(ctx, append( appendUint32([]byte{mysql.ComStmtExecute}, uint32(stmt.ID())), diff --git a/pkg/sessionctx/variable/session.go b/pkg/sessionctx/variable/session.go index c4bb81ee8d6ff..fa35cc8a7caf2 100644 --- a/pkg/sessionctx/variable/session.go +++ b/pkg/sessionctx/variable/session.go @@ -450,7 +450,7 @@ func (tc *TransactionContext) ReleaseSavepoint(name string) bool { name = strings.ToLower(name) for i, sp := range tc.Savepoints { if sp.Name == name { - tc.Savepoints = append(tc.Savepoints[:i]) + tc.Savepoints = tc.Savepoints[:i] return true } }