Shadowed Predeclared Indentifier analyzer (#7215)

* wip

* working for some ast nodes

* works for all but assignment

* works for all but mixed assignments

* change test name

* plug in the analyzer

* remove `copy` from predeclared list

* rename few shadowing names

* rename `len` to `length`

* add one more test case

* rename `panic` to `panicResult`

* replace `panic` with `panicResult` in error message

* remove test case not covered by the tool

* add `byte` to predeclared list

* additional test cases

* rename `copy` to `copyHandler`

* exclude functions with receivers

* revert to good names

Co-authored-by: prylabs-bulldozer[bot] <58059840+prylabs-bulldozer[bot]@users.noreply.github.com>

BUILD.bazel

@@ -109,6 +109,7 @@ nogo(
         "//tools/analyzers/errcheck:go_tool_library",
         "//tools/analyzers/featureconfig:go_tool_library",
         "//tools/analyzers/comparesame:go_tool_library",
+        "//tools/analyzers/shadowpredecl:go_tool_library",
     ] + select({
         # nogo checks that fail with coverage enabled.
         ":coverage_enabled": [],

nogo_config.json

@@ -116,5 +116,12 @@
       "rules_go_work-.*": "Third party code",
       "tools/analyzers/comparesame/testdata/compare_len.go": "Analyzer testdata has to break rules"
     }
+  },
+  "shadowpredecl": {
+    "exclude_files": {
+      "external/.*": "Third party code",
+      "rules_go_work-.*": "Third party code",
+      "tools/analyzers/shadowpredecl/testdata/shadow.go": "Analyzer testdata has to break rules"
+    }
   }
 }

shared/cmd/password_reader.go

@@ -17,6 +17,6 @@ type StdInPasswordReader struct {
 
 // ReadPassword reads a password from stdin.
 func (pr StdInPasswordReader) ReadPassword() (string, error) {
-	pwd, error := terminal.ReadPassword(int(os.Stdin.Fd()))
-	return string(pwd), error
+	pwd, err := terminal.ReadPassword(int(os.Stdin.Fd()))
+	return string(pwd), err
 }

shared/event/feed_test.go

@@ -58,11 +58,11 @@ func TestFeedPanics(t *testing.T) {
 
 func checkPanic(want error, fn func()) (err error) {
 	defer func() {
-		panic := recover()
-		if panic == nil {
+		panicResult := recover()
+		if panicResult == nil {
 			err = fmt.Errorf("didn't panic")
-		} else if !reflect.DeepEqual(panic, want) {
-			err = fmt.Errorf("panicked with wrong error: got %q, want %q", panic, want)
+		} else if !reflect.DeepEqual(panicResult, want) {
+			err = fmt.Errorf("panicked with wrong error: got %q, want %q", panicResult, want)
 		}
 	}()
 	fn()

shared/params/loader.go

@@ -35,15 +35,15 @@ func LoadChainConfigFile(chainConfigFileName string) {
 
 func replaceHexStringWithYAMLFormat(line string) []string {
 	parts := strings.Split(line, "0x")
-	b, err := hex.DecodeString(parts[1])
+	decoded, err := hex.DecodeString(parts[1])
 	if err != nil {
 		log.WithError(err).Error("Failed to decode hex string.")
 	}
-	switch l := len(b); {
+	switch l := len(decoded); {
 	case l == 1:
-		var byte byte
-		byte = b[0]
-		fixedByte, err := yaml.Marshal(byte)
+		var b byte
+		b = decoded[0]
+		fixedByte, err := yaml.Marshal(b)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
 		}
@@ -51,7 +51,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts = parts[:1]
 	case l > 1 && l <= 4:
 		var arr [4]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -59,7 +59,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 4 && l <= 8:
 		var arr [8]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -67,7 +67,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 8 && l <= 16:
 		var arr [16]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -75,7 +75,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 16 && l <= 20:
 		var arr [20]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -83,7 +83,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 20 && l <= 32:
 		var arr [32]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -91,7 +91,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 32 && l <= 48:
 		var arr [48]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -99,7 +99,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 48 && l <= 64:
 		var arr [64]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")
@@ -107,7 +107,7 @@ func replaceHexStringWithYAMLFormat(line string) []string {
 		parts[1] = string(fixedByte)
 	case l > 64 && l <= 96:
 		var arr [96]byte
-		copy(arr[:], b)
+		copy(arr[:], decoded)
 		fixedByte, err := yaml.Marshal(arr)
 		if err != nil {
 			log.WithError(err).Error("Failed to marshal config file.")

shared/testutil/state.go

@@ -44,9 +44,9 @@ func NewBeaconState() *stateTrie.BeaconState {
 
 // SSZ will fill 2D byte slices with their respective values, so we must fill these in too for round
 // trip testing.
-func filledByteSlice2D(len, innerLen uint64) [][]byte {
-	b := make([][]byte, len)
-	for i := uint64(0); i < len; i++ {
+func filledByteSlice2D(length, innerLen uint64) [][]byte {
+	b := make([][]byte, length)
+	for i := uint64(0); i < length; i++ {
 		b[i] = make([]byte, innerLen)
 	}
 	return b

shared/trieutil/sparse_merkle_test.go

@@ -169,13 +169,13 @@ func TestCopy_OK(t *testing.T) {
 	}
 	source, err := GenerateTrieFromItems(items, int(params.BeaconConfig().DepositContractTreeDepth)+1)
 	require.NoError(t, err)
-	copy := source.Copy()
+	copiedTrie := source.Copy()
 
-	if copy == source {
+	if copiedTrie == source {
 		t.Errorf("Original trie returned.")
 	}
-	copyHash := copy.HashTreeRoot()
-	require.DeepEqual(t, copyHash, copy.HashTreeRoot())
+	copyHash := copiedTrie.HashTreeRoot()
+	require.DeepEqual(t, copyHash, copiedTrie.HashTreeRoot())
 }
 
 func BenchmarkGenerateTrieFromItems(b *testing.B) {

slasher/db/kv/spanner_new.go

@@ -99,7 +99,7 @@ func (db *Store) SaveEpochSpans(ctx context.Context, epoch uint64, es *types.Epo
 func (db *Store) CacheLength(ctx context.Context) int {
 	ctx, span := trace.StartSpan(ctx, "slasherDB.CacheLength")
 	defer span.End()
-	len := db.flatSpanCache.Length()
-	log.Debugf("Span cache length %d", len)
-	return len
+	length := db.flatSpanCache.Length()
+	log.Debugf("Span cache length %d", length)
+	return length
 }

tools/analyzers/shadowpredecl/BUILD.bazel

@@ -0,0 +1,28 @@
+load("@prysm//tools/go:def.bzl", "go_library")
+load("@io_bazel_rules_go//go:def.bzl", "go_tool_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["analyzer.go"],
+    importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/shadowpredecl",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@org_golang_x_tools//go/analysis:go_default_library",
+        "@org_golang_x_tools//go/analysis/passes/inspect:go_default_library",
+        "@org_golang_x_tools//go/ast/inspector:go_default_library",
+    ],
+)
+
+go_tool_library(
+    name = "go_tool_library",
+    srcs = ["analyzer.go"],
+    importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/shadowpredecl",
+    visibility = ["//visibility:public"],
+    deps = [
+        "@org_golang_x_tools//go/analysis:go_tool_library",
+        "@org_golang_x_tools//go/analysis/passes/inspect:go_tool_library",
+        "@org_golang_x_tools//go/ast/inspector:go_tool_library",
+    ],
+)
+
+# gazelle:exclude analyzer_test.go

tools/analyzers/shadowpredecl/analyzer.go

@@ -0,0 +1,107 @@
+package shadowpredecl
+
+import (
+	"errors"
+	"go/ast"
+	"go/token"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+// Doc explaining the tool.
+const Doc = "Tool to detect declarations that shadow predeclared identifiers by having the same name."
+
+const messageTemplate = "%s '%s' shadows a predeclared identifier with the same name. Choose another name."
+
+// Aligned with https://golang.org/ref/spec#Predeclared_identifiers
+var predeclared = []string{"bool", "byte", "complex64", "complex128", "error", "float32", "float64", "int", "int8",
+	"int16", "int32", "int64", "rune", "string", "uint", "uint8", "uint16", "uint32", "uint64", "uintptr", "true",
+	"false", "iota", "nil", "append", "cap", "close", "complex", "copy", "delete", "imag", "len", "make", "new",
+	"panic", "print", "println", "real", "recover"}
+
+// Analyzer runs static analysis.
+var Analyzer = &analysis.Analyzer{
+	Name:     "shadowpredecl",
+	Doc:      Doc,
+	Requires: []*analysis.Analyzer{inspect.Analyzer},
+	Run:      run,
+}
+
+func run(pass *analysis.Pass) (interface{}, error) {
+	inspect, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+	if !ok {
+		return nil, errors.New("analyzer is not type *inspector.Inspector")
+	}
+
+	nodeFilter := []ast.Node{
+		(*ast.FuncDecl)(nil),
+		(*ast.FuncLit)(nil),
+		(*ast.AssignStmt)(nil),
+		(*ast.TypeSpec)(nil),
+		(*ast.ValueSpec)(nil),
+	}
+
+	inspect.Preorder(nodeFilter, func(node ast.Node) {
+		switch declaration := node.(type) {
+		case *ast.FuncDecl:
+			if declaration.Recv != nil {
+				return
+			}
+			name := declaration.Name.Name
+			if shadows(name) {
+				pass.Reportf(declaration.Name.NamePos, messageTemplate, "Function", name)
+			}
+			inspectFunctionParams(pass, declaration.Type.Params.List)
+		case *ast.FuncLit:
+			inspectFunctionParams(pass, declaration.Type.Params.List)
+		case *ast.AssignStmt:
+			if declaration.Tok == token.ASSIGN {
+				return
+			}
+			for _, expr := range declaration.Lhs {
+				if identifier, ok := expr.(*ast.Ident); ok {
+					name := identifier.Name
+					if shadows(name) {
+						pass.Reportf(identifier.NamePos, messageTemplate, "Identifier", name)
+					}
+				}
+			}
+		case *ast.TypeSpec:
+			name := declaration.Name.Name
+			if shadows(name) {
+				pass.Reportf(declaration.Name.NamePos, messageTemplate, "Type", name)
+			}
+		case *ast.ValueSpec:
+			for _, identifier := range declaration.Names {
+				name := identifier.Name
+				if shadows(name) {
+					pass.Reportf(identifier.NamePos, messageTemplate, "Identifier", name)
+				}
+			}
+		}
+	})
+
+	return nil, nil
+}
+
+func inspectFunctionParams(pass *analysis.Pass, paramList []*ast.Field) {
+	for _, field := range paramList {
+		for _, identifier := range field.Names {
+			name := identifier.Name
+			if shadows(name) {
+				pass.Reportf(identifier.NamePos, messageTemplate, "Identifier", name)
+			}
+		}
+	}
+}
+
+func shadows(name string) bool {
+	for _, identifier := range predeclared {
+		if identifier == name {
+			return true
+		}
+	}
+	return false
+}

tools/analyzers/shadowpredecl/analyzer_test.go

@@ -0,0 +1,11 @@
+package shadowpredecl
+
+import (
+	"testing"
+
+	"golang.org/x/tools/go/analysis/analysistest"
+)
+
+func TestAnalyzer(t *testing.T) {
+	analysistest.Run(t, analysistest.TestData(), Analyzer)
+}

tools/analyzers/shadowpredecl/testdata/BUILD.bazel

@@ -0,0 +1,8 @@
+load("@prysm//tools/go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["shadow.go"],
+    importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/shadowpredecl/testdata",
+    visibility = ["//visibility:public"],
+)

tools/analyzers/shadowpredecl/testdata/shadow.go

@@ -0,0 +1,69 @@
+package testdata
+
+type len struct { // want "Type 'len' shadows a predeclared identifier with the same name. Choose another name."
+
+}
+
+type int interface { // want "Type 'int' shadows a predeclared identifier with the same name. Choose another name."
+
+}
+
+func Struct() {
+	type error struct { // want "Type 'error' shadows a predeclared identifier with the same name. Choose another name."
+		int int // No diagnostic because the name is always referenced indirectly through a struct variable.
+	}
+}
+
+func TypeAlias() {
+	type error string // want "Type 'error' shadows a predeclared identifier with the same name. Choose another name."
+}
+
+func UninitializedVarAndAssignments() {
+	var error int // want "Identifier 'error' shadows a predeclared identifier with the same name. Choose another name."
+	error = 1     // No diagnostic because the original declaration already triggered one.
+	if error == 0 {
+	}
+}
+
+func InitializedVar() {
+	error := 0 // want "Identifier 'error' shadows a predeclared identifier with the same name. Choose another name."
+	if error == 0 {
+	}
+}
+
+func FirstInVarList() {
+	error, x := 0, 1 // want "Identifier 'error' shadows a predeclared identifier with the same name. Choose another name."
+	if error == x {
+	}
+}
+
+func SecondInVarList() {
+	x, error := 0, 1 // want "Identifier 'error' shadows a predeclared identifier with the same name. Choose another name."
+	if error == x {
+	}
+}
+
+func Const() {
+	const error = 0 // want "Identifier 'error' shadows a predeclared identifier with the same name. Choose another name."
+}
+
+// Test function and parameter names.
+func error(len int) { // want "Function 'error' shadows a predeclared identifier with the same name. Choose another name." "Identifier 'len' shadows a predeclared identifier with the same name. Choose another name."
+	if len == 0 {
+	}
+
+	// Test parameter in a new line.
+	f := func(
+		int string) { // want "Identifier 'int' shadows a predeclared identifier with the same name. Choose another name."
+	}
+
+	f("")
+}
+
+type receiver struct {
+}
+
+// Test receiver function.
+func (s *receiver) Receiver(len int) {
+
+}