From aaf1b815e1ee8dffa1f11b3564ac7fb513132f34 Mon Sep 17 00:00:00 2001 From: Yang Keao Date: Tue, 8 Oct 2024 23:43:32 +0800 Subject: [PATCH] fix the constructor linter Signed-off-by: Yang Keao --- build/linter/constructor/analyzer.go | 40 ++++++++++++++++--- build/linter/constructor/analyzer_test.go | 2 +- .../util/linter/constructor/BUILD.bazel | 2 +- .../linter/constructor/constructorflag.go | 0 .../constructor/testdata/src/t/construct.go | 14 ++++++- pkg/executor/builder.go | 14 +++---- pkg/executor/detach.go | 5 +++ pkg/executor/detach_test.go | 1 + pkg/executor/internal/exec/BUILD.bazel | 1 + pkg/executor/internal/exec/executor.go | 5 +++ pkg/executor/sortexec/topn_spill_test.go | 1 + pkg/executor/test/seqtest/prepared_test.go | 1 + pkg/kv/key_test.go | 4 +- .../handle/updatetest/update_test.go | 2 +- 14 files changed, 71 insertions(+), 21 deletions(-) rename build/linter/constructor/testdata/src/github.com/pingcap/tidb/{ => pkg}/util/linter/constructor/BUILD.bazel (77%) rename build/linter/constructor/testdata/src/github.com/pingcap/tidb/{ => pkg}/util/linter/constructor/constructorflag.go (100%) diff --git a/build/linter/constructor/analyzer.go b/build/linter/constructor/analyzer.go index d0a552d2c356f..e7dadd9395cd3 100644 --- a/build/linter/constructor/analyzer.go +++ b/build/linter/constructor/analyzer.go @@ -46,7 +46,7 @@ var Analyzer = &analysis.Analyzer{ Run: run, } -func getConstructorList(t types.Type) []string { +func getConstructorList(t types.Type, ignoreFields map[string]struct{}) []string { structTyp, ok := t.(*types.Struct) if !ok { var ptr *types.Pointer @@ -67,7 +67,12 @@ func getConstructorList(t types.Type) []string { if !ok { continue } - if named.Obj().Name() == "Constructor" && named.Obj().Pkg().Path() == "github.com/pingcap/tidb/util/linter/constructor" { + if ignoreFields != nil { + if _, ok := ignoreFields[field.Name()]; ok { + continue + } + } + if named.Obj().Name() == "Constructor" && named.Obj().Pkg().Path() == "github.com/pingcap/tidb/pkg/util/linter/constructor" { tags, err := structtag.Parse(structTyp.Tag(i)) // skip invalid tags if err != nil { @@ -82,7 +87,7 @@ func getConstructorList(t types.Type) []string { } if fieldStruct, ok := named.Underlying().(*types.Struct); ok { - ctors = append(ctors, getConstructorList(fieldStruct)...) + ctors = append(ctors, getConstructorList(fieldStruct, nil)...) } } return ctors @@ -116,7 +121,25 @@ func handleCompositeLit(pass *analysis.Pass, n *ast.CompositeLit, push bool, sta return true } - ctors := getConstructorList(t) + // Just ignore the specified fields. They'll be checked recursively later. In this round, we only need to avoid + // the case that the struct is implicitly initiated. + ignoreFields := make(map[string]struct{}) + for i, elt := range n.Elts { + switch elt := elt.(type) { + case *ast.KeyValueExpr: + if ident, ok := elt.Key.(*ast.Ident); ok { + ignoreFields[ident.Name] = struct{}{} + } + default: + strctTyp, ok := t.(*types.Struct) + if !ok { + continue + } + ignoreFields[strctTyp.Field(i).Name()] = struct{}{} + } + } + + ctors := getConstructorList(t, ignoreFields) if len(ctors) == 0 { return true } @@ -134,7 +157,7 @@ func handleCallExpr(pass *analysis.Pass, n *ast.CallExpr, push bool, stack []ast } t := pass.TypesInfo.TypeOf(n).Underlying() - ctors := getConstructorList(t) + ctors := getConstructorList(t, nil) if len(ctors) == 0 { return true } @@ -148,7 +171,12 @@ func handleValueSpec(pass *analysis.Pass, n *ast.ValueSpec, _ bool, stack []ast. return true } - ctors := getConstructorList(t.Underlying()) + // allow declaring a pointer, as it's actually not constructed. + if _, ok := t.(*types.Pointer); ok { + return true + } + + ctors := getConstructorList(t.Underlying(), nil) if len(ctors) == 0 { return true } diff --git a/build/linter/constructor/analyzer_test.go b/build/linter/constructor/analyzer_test.go index 863d45d9ec0d3..4664ad49c4f5e 100644 --- a/build/linter/constructor/analyzer_test.go +++ b/build/linter/constructor/analyzer_test.go @@ -28,6 +28,6 @@ import ( func Test(t *testing.T) { testdata := analysistest.TestData() - pkgs := []string{"t", "github.com/pingcap/tidb/util/linter/constructor"} + pkgs := []string{"t", "github.com/pingcap/tidb/pkg/util/linter/constructor"} analysistest.Run(t, testdata, constructor.Analyzer, pkgs...) } diff --git a/build/linter/constructor/testdata/src/github.com/pingcap/tidb/util/linter/constructor/BUILD.bazel b/build/linter/constructor/testdata/src/github.com/pingcap/tidb/pkg/util/linter/constructor/BUILD.bazel similarity index 77% rename from build/linter/constructor/testdata/src/github.com/pingcap/tidb/util/linter/constructor/BUILD.bazel rename to build/linter/constructor/testdata/src/github.com/pingcap/tidb/pkg/util/linter/constructor/BUILD.bazel index 0b3cca7b6c2ea..c7d6cea617b4b 100644 --- a/build/linter/constructor/testdata/src/github.com/pingcap/tidb/util/linter/constructor/BUILD.bazel +++ b/build/linter/constructor/testdata/src/github.com/pingcap/tidb/pkg/util/linter/constructor/BUILD.bazel @@ -3,6 +3,6 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library") go_library( name = "constructor", srcs = ["constructorflag.go"], - importpath = "github.com/pingcap/tidb/build/linter/constructor/testdata/src/github.com/pingcap/tidb/util/linter/constructor", + importpath = "github.com/pingcap/tidb/build/linter/constructor/testdata/src/github.com/pingcap/tidb/pkg/util/linter/constructor", visibility = ["//visibility:public"], ) diff --git a/build/linter/constructor/testdata/src/github.com/pingcap/tidb/util/linter/constructor/constructorflag.go b/build/linter/constructor/testdata/src/github.com/pingcap/tidb/pkg/util/linter/constructor/constructorflag.go similarity index 100% rename from build/linter/constructor/testdata/src/github.com/pingcap/tidb/util/linter/constructor/constructorflag.go rename to build/linter/constructor/testdata/src/github.com/pingcap/tidb/pkg/util/linter/constructor/constructorflag.go diff --git a/build/linter/constructor/testdata/src/t/construct.go b/build/linter/constructor/testdata/src/t/construct.go index 863d08fbd3bcf..396f68015a679 100644 --- a/build/linter/constructor/testdata/src/t/construct.go +++ b/build/linter/constructor/testdata/src/t/construct.go @@ -59,8 +59,8 @@ func otherFunction() { _ = new(StructWithSpecificConstructor) // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` // var - var _ StructWithSpecificConstructor // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` - var _ *StructWithSpecificConstructor // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` + var _ StructWithSpecificConstructor // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` + var _ *StructWithSpecificConstructor type compositeImplicitInitiate1 struct { StructWithSpecificConstructor @@ -68,6 +68,16 @@ func otherFunction() { var _ compositeImplicitInitiate1 // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` _ = new(compositeImplicitInitiate1) // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` + // specified field is also allowed + _ = compositeImplicitInitiate1{ + StructWithSpecificConstructor: *NewStructWithSpecificConstructor(), + } + + // specified field with manually constructed struct is not allowed + _ = compositeImplicitInitiate1{ + StructWithSpecificConstructor: StructWithSpecificConstructor{}, // want `struct can only be constructed in constructors NewStructWithSpecificConstructor, AnotherConstructor` + } + // pointer field is allowed type compositeImplicitInitiate2 struct { *StructWithSpecificConstructor diff --git a/pkg/executor/builder.go b/pkg/executor/builder.go index fc413b85b0d26..1fe9759dcee10 100644 --- a/pkg/executor/builder.go +++ b/pkg/executor/builder.go @@ -957,12 +957,11 @@ func (b *executorBuilder) buildInsert(v *plannercore.Insert) exec.Executor { if b.err != nil { return nil } - var baseExec exec.BaseExecutor + var children []exec.Executor if selectExec != nil { - baseExec = exec.NewBaseExecutor(b.ctx, nil, v.ID(), selectExec) - } else { - baseExec = exec.NewBaseExecutor(b.ctx, nil, v.ID()) + children = append(children, selectExec) } + baseExec := exec.NewBaseExecutor(b.ctx, nil, v.ID(), children...) baseExec.SetInitCap(chunk.ZeroCapacity) ivs := &InsertValues{ @@ -1016,17 +1015,16 @@ func (b *executorBuilder) buildImportInto(v *plannercore.ImportInto) exec.Execut var ( selectExec exec.Executor - base exec.BaseExecutor + children []exec.Executor ) if v.SelectPlan != nil { selectExec = b.build(v.SelectPlan) if b.err != nil { return nil } - base = exec.NewBaseExecutor(b.ctx, v.Schema(), v.ID(), selectExec) - } else { - base = exec.NewBaseExecutor(b.ctx, v.Schema(), v.ID()) + children = append(children, selectExec) } + base := exec.NewBaseExecutor(b.ctx, v.Schema(), v.ID(), children...) executor, err := newImportIntoExec(base, selectExec, b.ctx, v, tbl) if err != nil { b.err = err diff --git a/pkg/executor/detach.go b/pkg/executor/detach.go index 2f64ab934dba6..1dfc65852ac1d 100644 --- a/pkg/executor/detach.go +++ b/pkg/executor/detach.go @@ -107,6 +107,7 @@ func (sCtx selectionExecutorContext) Detach() selectionExecutorContext { // Detach detaches the current executor from the session context. func (e *TableReaderExecutor) Detach() (exec.Executor, bool) { + //nolint:constructor newExec := new(TableReaderExecutor) *newExec = *e @@ -117,6 +118,7 @@ func (e *TableReaderExecutor) Detach() (exec.Executor, bool) { // Detach detaches the current executor from the session context. func (e *IndexReaderExecutor) Detach() (exec.Executor, bool) { + //nolint:constructor newExec := new(IndexReaderExecutor) *newExec = *e @@ -127,6 +129,7 @@ func (e *IndexReaderExecutor) Detach() (exec.Executor, bool) { // Detach detaches the current executor from the session context. func (e *IndexLookUpExecutor) Detach() (exec.Executor, bool) { + //nolint:constructor newExec := new(IndexLookUpExecutor) *newExec = *e @@ -144,6 +147,7 @@ func (e *ProjectionExec) Detach() (exec.Executor, bool) { return nil, false } + //nolint:constructor newExec := new(ProjectionExec) *newExec = *e @@ -163,6 +167,7 @@ func (e *SelectionExec) Detach() (exec.Executor, bool) { } } + //nolint:constructor newExec := new(SelectionExec) *newExec = *e diff --git a/pkg/executor/detach_test.go b/pkg/executor/detach_test.go index 5a9e87aa81cae..5d736c72b8f47 100644 --- a/pkg/executor/detach_test.go +++ b/pkg/executor/detach_test.go @@ -27,6 +27,7 @@ type mockSimpleExecutor struct { exec.BaseExecutorV2 } +//nolint:constructor func TestDetachExecutor(t *testing.T) { // call `Detach` on a mock executor will fail _, ok := Detach(&mockSimpleExecutor{}) diff --git a/pkg/executor/internal/exec/BUILD.bazel b/pkg/executor/internal/exec/BUILD.bazel index 4f998b4731132..9b970add795e9 100644 --- a/pkg/executor/internal/exec/BUILD.bazel +++ b/pkg/executor/internal/exec/BUILD.bazel @@ -23,6 +23,7 @@ go_library( "//pkg/util", "//pkg/util/chunk", "//pkg/util/execdetails", + "//pkg/util/linter/constructor", "//pkg/util/topsql", "//pkg/util/topsql/state", "//pkg/util/tracing", diff --git a/pkg/executor/internal/exec/executor.go b/pkg/executor/internal/exec/executor.go index 6feb131b67306..8dce75e47909b 100644 --- a/pkg/executor/internal/exec/executor.go +++ b/pkg/executor/internal/exec/executor.go @@ -31,6 +31,7 @@ import ( "github.com/pingcap/tidb/pkg/util" "github.com/pingcap/tidb/pkg/util/chunk" "github.com/pingcap/tidb/pkg/util/execdetails" + "github.com/pingcap/tidb/pkg/util/linter/constructor" "github.com/pingcap/tidb/pkg/util/topsql" topsqlstate "github.com/pingcap/tidb/pkg/util/topsql/state" "github.com/pingcap/tidb/pkg/util/tracing" @@ -276,6 +277,8 @@ func newExecutorKillerHandler(handler signalHandler) executorKillerHandler { // BaseExecutorV2 is a simplified version of `BaseExecutor`, which doesn't contain a full session context type BaseExecutorV2 struct { + _ constructor.Constructor `ctor:"NewBaseExecutorV2,BuildNewBaseExecutorV2"` + executorMeta executorKillerHandler executorStats @@ -352,6 +355,8 @@ func (e *BaseExecutorV2) BuildNewBaseExecutorV2(stmtRuntimeStatsColl *execdetail // BaseExecutor holds common information for executors. type BaseExecutor struct { + _ constructor.Constructor `ctor:"NewBaseExecutor"` + ctx sessionctx.Context BaseExecutorV2 diff --git a/pkg/executor/sortexec/topn_spill_test.go b/pkg/executor/sortexec/topn_spill_test.go index 159e33e7c0876..7f8887c597d59 100644 --- a/pkg/executor/sortexec/topn_spill_test.go +++ b/pkg/executor/sortexec/topn_spill_test.go @@ -376,6 +376,7 @@ func severalChunksInDiskCase(t *testing.T, topnExec *sortexec.TopNExec) { } func TestGenerateTopNResultsWhenSpillOnlyOnce(t *testing.T) { + //nolint:constructor topnExec := &sortexec.TopNExec{} topnExec.Limit = &plannercore.PhysicalLimit{} diff --git a/pkg/executor/test/seqtest/prepared_test.go b/pkg/executor/test/seqtest/prepared_test.go index 0905c820d3ace..41d6b92804b17 100644 --- a/pkg/executor/test/seqtest/prepared_test.go +++ b/pkg/executor/test/seqtest/prepared_test.go @@ -246,6 +246,7 @@ func TestPrepared(t *testing.T) { tk.MustQuery("select a from prepare1;").Check(testkit.Rows("7")) // Coverage. + //nolint:constructor exec := &executor.ExecuteExec{} err = exec.Next(ctx, nil) require.NoError(t, err) diff --git a/pkg/kv/key_test.go b/pkg/kv/key_test.go index afd2b0cb65a7a..0afacd882c829 100644 --- a/pkg/kv/key_test.go +++ b/pkg/kv/key_test.go @@ -396,7 +396,7 @@ func nativeIntMap(size int, handles []Handle) int { } func BenchmarkMemAwareHandleMap(b *testing.B) { - var sc stmtctx.StatementContext + sc := stmtctx.NewStmtCtx() for _, s := range inputs { handles := make([]Handle, s.input) for i := 0; i < s.input; i++ { @@ -418,7 +418,7 @@ func BenchmarkMemAwareHandleMap(b *testing.B) { } func BenchmarkNativeHandleMap(b *testing.B) { - var sc stmtctx.StatementContext + sc := stmtctx.NewStmtCtx() for _, s := range inputs { handles := make([]Handle, s.input) for i := 0; i < s.input; i++ { diff --git a/pkg/statistics/handle/updatetest/update_test.go b/pkg/statistics/handle/updatetest/update_test.go index 88616fbddbe20..dddb6191cb602 100644 --- a/pkg/statistics/handle/updatetest/update_test.go +++ b/pkg/statistics/handle/updatetest/update_test.go @@ -564,7 +564,7 @@ func TestSplitRange(t *testing.T) { result: "[8,9)", }, } - sc := new(stmtctx.StatementContext) + sc := stmtctx.NewStmtCtx() sc.SetTimeZone(time.UTC) for _, test := range tests { ranges := make([]*ranger.Range, 0, len(test.points)/2)