From aaa3abf630565a46cfd8a547ef5eaad0ebca26ff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rados=C5=82aw=20Kapka?= Date: Mon, 14 Sep 2020 12:49:15 +0200 Subject: [PATCH] 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 | 1 + nogo_config.json | 7 ++ shared/cmd/password_reader.go | 4 +- shared/event/feed_test.go | 8 +- shared/params/loader.go | 26 ++--- shared/testutil/state.go | 6 +- shared/trieutil/sparse_merkle_test.go | 8 +- slasher/db/kv/spanner_new.go | 6 +- tools/analyzers/shadowpredecl/BUILD.bazel | 28 +++++ tools/analyzers/shadowpredecl/analyzer.go | 107 ++++++++++++++++++ .../analyzers/shadowpredecl/analyzer_test.go | 11 ++ .../shadowpredecl/testdata/BUILD.bazel | 8 ++ .../shadowpredecl/testdata/shadow.go | 69 +++++++++++ 13 files changed, 260 insertions(+), 29 deletions(-) create mode 100644 tools/analyzers/shadowpredecl/BUILD.bazel create mode 100644 tools/analyzers/shadowpredecl/analyzer.go create mode 100644 tools/analyzers/shadowpredecl/analyzer_test.go create mode 100644 tools/analyzers/shadowpredecl/testdata/BUILD.bazel create mode 100644 tools/analyzers/shadowpredecl/testdata/shadow.go diff --git a/BUILD.bazel b/BUILD.bazel index ba7026975..38dd72874 100644 --- a/BUILD.bazel +++ b/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": [], diff --git a/nogo_config.json b/nogo_config.json index ed02a84de..73a4fccbd 100644 --- a/nogo_config.json +++ b/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" + } } } diff --git a/shared/cmd/password_reader.go b/shared/cmd/password_reader.go index a76f29c08..0e9d8593b 100644 --- a/shared/cmd/password_reader.go +++ b/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 } diff --git a/shared/event/feed_test.go b/shared/event/feed_test.go index 4c56fd9ba..a0c3fcf52 100644 --- a/shared/event/feed_test.go +++ b/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() diff --git a/shared/params/loader.go b/shared/params/loader.go index ae7071d9a..126c20045 100644 --- a/shared/params/loader.go +++ b/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.") diff --git a/shared/testutil/state.go b/shared/testutil/state.go index 6ff25932f..dd20deb8e 100644 --- a/shared/testutil/state.go +++ b/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 diff --git a/shared/trieutil/sparse_merkle_test.go b/shared/trieutil/sparse_merkle_test.go index 2b2acc6c3..440b5a2d8 100644 --- a/shared/trieutil/sparse_merkle_test.go +++ 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) { diff --git a/slasher/db/kv/spanner_new.go b/slasher/db/kv/spanner_new.go index f58a79d5b..a23c82c90 100644 --- a/slasher/db/kv/spanner_new.go +++ 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 } diff --git a/tools/analyzers/shadowpredecl/BUILD.bazel b/tools/analyzers/shadowpredecl/BUILD.bazel new file mode 100644 index 000000000..5372acc8d --- /dev/null +++ b/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 diff --git a/tools/analyzers/shadowpredecl/analyzer.go b/tools/analyzers/shadowpredecl/analyzer.go new file mode 100644 index 000000000..001a0810c --- /dev/null +++ b/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 +} diff --git a/tools/analyzers/shadowpredecl/analyzer_test.go b/tools/analyzers/shadowpredecl/analyzer_test.go new file mode 100644 index 000000000..270f6c5bf --- /dev/null +++ b/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) +} diff --git a/tools/analyzers/shadowpredecl/testdata/BUILD.bazel b/tools/analyzers/shadowpredecl/testdata/BUILD.bazel new file mode 100644 index 000000000..18d1b5bff --- /dev/null +++ b/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"], +) diff --git a/tools/analyzers/shadowpredecl/testdata/shadow.go b/tools/analyzers/shadowpredecl/testdata/shadow.go new file mode 100644 index 000000000..86302093d --- /dev/null +++ b/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) { + +}