expression, planner: make the Value field of the Constant struct private | tidb-test=pr/2355

build/BUILD.bazel

@@ -152,6 +152,7 @@ nogo(
                "//build/linter/durationcheck",
                "//build/linter/etcdconfig",
                "//build/linter/exportloopref",
+               "//build/linter/extraprivate",
                "//build/linter/forcetypeassert",
                "//build/linter/gofmt",
                "//build/linter/gci",

build/linter/extraprivate/BUILD.bazel

@@ -0,0 +1,26 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test")
+
+go_library(
+    name = "extraprivate",
+    srcs = ["analyzer.go"],
+    importpath = "github.com/pingcap/tidb/build/linter/extraprivate",
+    visibility = ["//visibility:public"],
+    deps = [
+        "//build/linter/util",
+        "@com_github_fatih_structtag//:structtag",
+        "@org_golang_x_tools//go/analysis",
+        "@org_golang_x_tools//go/analysis/passes/inspect",
+        "@org_golang_x_tools//go/ast/inspector",
+    ],
+)
+
+go_test(
+    name = "extraprivate_test",
+    timeout = "short",
+    srcs = ["analyzer_test.go"],
+    flaky = True,
+    deps = [
+        ":extraprivate",
+        "@org_golang_x_tools//go/analysis/analysistest",
+    ],
+)

build/linter/extraprivate/analyzer.go

@@ -0,0 +1,138 @@
+// Copyright 2024 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package extraprivate
+
+import (
+	"go/ast"
+	"go/types"
+
+	"github.com/fatih/structtag"
+	"github.com/pingcap/tidb/build/linter/util"
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+// Analyzer is the analyzer of extraprivate.
+// Access to fields with `extraprivate` tag outside its struct's methods is not allowed. However, this
+// analyzer allows to construct the struct manually with the field with `extraprivate` tag.
+var Analyzer = &analysis.Analyzer{
+	Name:     "extraprivate",
+	Doc:      "Check developers don't read or write fields with extraprivate tag outside the method",
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (any, error) {
+	inspect := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	nodeFilter := []ast.Node{
+		(*ast.FuncDecl)(nil),
+		(*ast.SelectorExpr)(nil),
+	}
+
+	inspect.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) (proceed bool) {
+		if push {
+			return true
+		}
+
+		se, ok := n.(*ast.SelectorExpr)
+		if !ok {
+			return
+		}
+
+		xType := pass.TypesInfo.Types[se.X].Type
+		if !isExtraPrivateField(xType, se.Sel) {
+			return
+		}
+
+		if isAccessWithinStructMethod(pass, stack, xType) {
+			return
+		}
+
+		pass.Reportf(se.Pos(), "access to extraprivate field outside its struct's methods")
+		return
+	})
+
+	return nil, nil
+}
+
+func isExtraPrivateField(xAnyType types.Type, fieldName *ast.Ident) bool {
+	switch xType := xAnyType.(type) {
+	case *types.Named:
+		underlyingType := xType.Underlying()
+		structType, ok := underlyingType.(*types.Struct)
+		if !ok {
+			return false
+		}
+		for i := 0; i < structType.NumFields(); i++ {
+			field := structType.Field(i)
+			if field.Name() == fieldName.Name {
+				tags, err := structtag.Parse(structType.Tag(i))
+				if err != nil {
+					continue
+				}
+				_, err = tags.Get("extraprivate")
+				if err != nil {
+					continue
+				}
+				return true
+			}
+		}
+
+		return false
+	case *types.Pointer:
+		return isExtraPrivateField(xType.Elem(), fieldName)
+	default:
+		return false
+	}
+}
+
+func isAccessWithinStructMethod(pass *analysis.Pass, stack []ast.Node, se types.Type) bool {
+	for i := len(stack) - 1; i >= 0; i-- {
+		funcDecl, ok := stack[i].(*ast.FuncDecl)
+		if !ok {
+			continue
+		}
+
+		if funcDecl.Recv == nil {
+			continue
+		}
+
+		recvType := pass.TypesInfo.TypeOf(funcDecl.Recv.List[0].Type)
+		if recvType == nil {
+			continue
+		}
+
+		if resolvePointer(recvType) == resolvePointer(se) {
+			return true
+		}
+	}
+
+	return false
+}
+
+func resolvePointer(xType types.Type) types.Type {
+	switch xType := xType.(type) {
+	case *types.Pointer:
+		return xType.Elem()
+	default:
+		return xType
+	}
+}
+
+func init() {
+	util.SkipAnalyzerByConfig(Analyzer)
+}

build/linter/extraprivate/analyzer_test.go

@@ -0,0 +1,33 @@
+// Copyright 2024 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+//go:build !intest
+
+package extraprivate_test
+
+import (
+	"testing"
+
+	"github.com/pingcap/tidb/build/linter/extraprivate"
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+// TODO: investigate the CI environment and check how to run this test in CI.
+// The CI environment doesn't have `go` executable in $PATH.
+
+func Test(t *testing.T) {
+	testdata := analysistest.TestData()
+	pkgs := []string{"t"}
+	analysistest.Run(t, testdata, extraprivate.Analyzer, pkgs...)
+}

build/linter/extraprivate/cmd/BUILD.bazel

@@ -0,0 +1,18 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_binary", "go_library")
+
+go_library(
+    name = "cmd_lib",
+    srcs = ["main.go"],
+    importpath = "github.com/pingcap/tidb/build/linter/extraprivate/cmd",
+    visibility = ["//visibility:private"],
+    deps = [
+        "//build/linter/extraprivate",
+        "@org_golang_x_tools//go/analysis/singlechecker",
+    ],
+)
+
+go_binary(
+    name = "cmd",
+    embed = [":cmd_lib"],
+    visibility = ["//visibility:public"],
+)

build/linter/extraprivate/cmd/main.go

@@ -0,0 +1,24 @@
+// Copyright 2024 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package main
+
+import (
+	"github.com/pingcap/tidb/build/linter/extraprivate"
+	"golang.org/x/tools/go/analysis/singlechecker"
+)
+
+func main() {
+	singlechecker.Main(extraprivate.Analyzer)
+}

build/linter/extraprivate/testdata/src/t/BUILD.bazel

@@ -0,0 +1,8 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_library")
+
+go_library(
+    name = "t",
+    srcs = ["extraprivate.go"],
+    importpath = "github.com/pingcap/tidb/build/linter/extraprivate/testdata/src/t",
+    visibility = ["//visibility:public"],
+)

build/linter/extraprivate/testdata/src/t/extraprivate.go

@@ -0,0 +1,51 @@
+// Copyright 2024 PingCAP, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//      http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package t
+
+type structWithExtraPrivateField struct {
+	field1 int `extraprivate:""`
+	field2 int
+}
+
+func readField1(s structWithExtraPrivateField) int {
+	return s.field1 // want `access to extraprivate field outside its struct's methods`
+}
+
+func readField1Ptr(s *structWithExtraPrivateField) int {
+	return s.field1 // want `access to extraprivate field outside its struct's methods`
+}
+
+func readField2(s structWithExtraPrivateField) int {
+	return s.field2
+}
+func readField2Ptr(s *structWithExtraPrivateField) int {
+	return s.field2
+}
+
+func (s structWithExtraPrivateField) Field1() int {
+	return s.field1
+}
+
+func (s *structWithExtraPrivateField) Field1Ptr() int {
+	return s.field1
+}
+
+func (s structWithExtraPrivateField) Field2() int {
+	return s.field2
+}
+
+func (s *structWithExtraPrivateField) Field2Ptr() int {
+	return s.field2
+}

build/nogo_config.json

@@ -1362,5 +1362,14 @@
       "external/": "no need to vet third party code",
       ".*_generated\\.go$": "ignore generated code"
     }
+  },
+  "extraprivate": {
+    "exclude_files": {
+      "pkg/parser/parser.go": "parser/parser.go code",
+      "external/": "no need to vet third party code",
+      ".*_generated\\.go$": "ignore generated code",
+      "build/linter/extraprivate/testdata/": "no need to vet the test inside the linter",
+      "_test.go": "ignore all test code"
+    }
   }
 }

pkg/executor/aggfuncs/builder.go

@@ -720,10 +720,13 @@ func buildLeadLag(ctx AggFuncBuildContext, aggFuncDesc *aggregation.AggFuncDesc,
 	if len(aggFuncDesc.Args) == 3 {
 		defaultExpr = aggFuncDesc.Args[2]
 		if et, ok := defaultExpr.(*expression.Constant); ok {
-			evalCtx := ctx.GetEvalCtx()
-			res, err1 := et.Value.ConvertTo(evalCtx.TypeCtx(), aggFuncDesc.RetTp)
-			if err1 == nil {
-				defaultExpr = &expression.Constant{Value: res, RetType: aggFuncDesc.RetTp}
+			etVal, ok := et.GetValueWithoutOverOptimization(ctx)
+			if ok {
+				evalCtx := ctx.GetEvalCtx()
+				res, err1 := etVal.ConvertTo(evalCtx.TypeCtx(), aggFuncDesc.RetTp)
+				if err1 == nil {
+					defaultExpr = &expression.Constant{Value: res, RetType: aggFuncDesc.RetTp}
+				}
 			}
 		}
 	}

pkg/executor/set.go

@@ -78,7 +78,8 @@ func (e *SetExecutor) Next(ctx context.Context, req *chunk.Chunk) error {
 			cs := dt.GetString()
 			var co string
 			if v.ExtendValue != nil {
-				co = v.ExtendValue.Value.GetString()
+				val, _ := v.ExtendValue.GetValue()
+				co = val.GetString()
 			}
 			err = e.setCharset(cs, co, v.Name == ast.SetNames)
 			if err != nil {