From 6228b3cd9f3f333b74fc8ae1d10d70252a8e782f Mon Sep 17 00:00:00 2001 From: rkapka Date: Fri, 21 Aug 2020 18:48:27 +0200 Subject: [PATCH] Identical expression comparison analyzer (#7066) * static analyzer with tests * Merge branch 'origin-master' into identical-expression-comparison-analyzer * resolve analyzer errors * Merge branch 'origin-master' into identical-expression-comparison-analyzer * change excluded file in nogo_config * remove tests from bazel file * exclude test file explicitly * extracted common code to helper file * Revert "extracted common code to helper file" This reverts commit e9ccea73604c920451975214d0305e6d16943b3c. * Merge refs/heads/master into identical-expression-comparison-analyzer * Merge refs/heads/master into identical-expression-comparison-analyzer --- BUILD.bazel | 1 + nogo_config.json | 7 ++ tools/analyzers/comparesame/BUILD.bazel | 28 ++++++++ tools/analyzers/comparesame/analyzer.go | 65 +++++++++++++++++++ tools/analyzers/comparesame/analyzer_test.go | 11 ++++ .../comparesame/testdata/BUILD.bazel | 8 +++ .../comparesame/testdata/compare_len.go | 37 +++++++++++ 7 files changed, 157 insertions(+) create mode 100644 tools/analyzers/comparesame/BUILD.bazel create mode 100644 tools/analyzers/comparesame/analyzer.go create mode 100644 tools/analyzers/comparesame/analyzer_test.go create mode 100644 tools/analyzers/comparesame/testdata/BUILD.bazel create mode 100644 tools/analyzers/comparesame/testdata/compare_len.go diff --git a/BUILD.bazel b/BUILD.bazel index da095f7ba..ba7026975 100644 --- a/BUILD.bazel +++ b/BUILD.bazel @@ -108,6 +108,7 @@ nogo( "//tools/analyzers/cryptorand:go_tool_library", "//tools/analyzers/errcheck:go_tool_library", "//tools/analyzers/featureconfig:go_tool_library", + "//tools/analyzers/comparesame:go_tool_library", ] + select({ # nogo checks that fail with coverage enabled. ":coverage_enabled": [], diff --git a/nogo_config.json b/nogo_config.json index ca1a2b869..ed02a84de 100644 --- a/nogo_config.json +++ b/nogo_config.json @@ -109,5 +109,12 @@ "shared/rand/rand\\.go": "Abstracts CSPRNGs for common use", "shared/aggregation/testing/bitlistutils.go": "Test-only package" } + }, + "comparesame": { + "exclude_files": { + "external/.*": "Third party code", + "rules_go_work-.*": "Third party code", + "tools/analyzers/comparesame/testdata/compare_len.go": "Analyzer testdata has to break rules" + } } } diff --git a/tools/analyzers/comparesame/BUILD.bazel b/tools/analyzers/comparesame/BUILD.bazel new file mode 100644 index 000000000..f92915767 --- /dev/null +++ b/tools/analyzers/comparesame/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/comparesame", + 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/comparesame", + 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/comparesame/analyzer.go b/tools/analyzers/comparesame/analyzer.go new file mode 100644 index 000000000..a29ec2ece --- /dev/null +++ b/tools/analyzers/comparesame/analyzer.go @@ -0,0 +1,65 @@ +package comparesame + +import ( + "bytes" + "errors" + "go/ast" + "go/printer" + "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 comparison (==, !=, >=, <=, >, <) of identical boolean expressions." + +const messageTemplate = "Boolean expression has identical expressions on both sides. The result is always %v." + +// Analyzer runs static analysis. +var Analyzer = &analysis.Analyzer{ + Name: "comparesame", + 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.BinaryExpr)(nil), + } + + inspect.Preorder(nodeFilter, func(node ast.Node) { + expr, ok := node.(*ast.BinaryExpr) + if !ok { + return + } + + switch expr.Op { + case token.EQL, token.NEQ, token.GEQ, token.LEQ, token.GTR, token.LSS: + var xBuf, yBuf bytes.Buffer + if err := printer.Fprint(&xBuf, pass.Fset, expr.X); err != nil { + pass.Reportf(expr.X.Pos(), err.Error()) + } + if err := printer.Fprint(&yBuf, pass.Fset, expr.Y); err != nil { + pass.Reportf(expr.Y.Pos(), err.Error()) + } + if xBuf.String() == yBuf.String() { + switch expr.Op { + case token.EQL, token.NEQ, token.GEQ, token.LEQ: + pass.Reportf(expr.OpPos, messageTemplate, true) + case token.GTR, token.LSS: + pass.Reportf(expr.OpPos, messageTemplate, false) + } + } + } + }) + + return nil, nil +} diff --git a/tools/analyzers/comparesame/analyzer_test.go b/tools/analyzers/comparesame/analyzer_test.go new file mode 100644 index 000000000..d51cee9cb --- /dev/null +++ b/tools/analyzers/comparesame/analyzer_test.go @@ -0,0 +1,11 @@ +package comparesame + +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/comparesame/testdata/BUILD.bazel b/tools/analyzers/comparesame/testdata/BUILD.bazel new file mode 100644 index 000000000..833040e68 --- /dev/null +++ b/tools/analyzers/comparesame/testdata/BUILD.bazel @@ -0,0 +1,8 @@ +load("@prysm//tools/go:def.bzl", "go_library") + +go_library( + name = "go_default_library", + srcs = ["compare_len.go"], + importpath = "github.com/prysmaticlabs/prysm/tools/analyzers/comparesame/testdata", + visibility = ["//visibility:public"], +) diff --git a/tools/analyzers/comparesame/testdata/compare_len.go b/tools/analyzers/comparesame/testdata/compare_len.go new file mode 100644 index 000000000..49cae86b1 --- /dev/null +++ b/tools/analyzers/comparesame/testdata/compare_len.go @@ -0,0 +1,37 @@ +package testdata + +func Equal() { + x := []string{"a"} + if len(x) == len(x) { // want "Boolean expression has identical expressions on both sides. The result is always true." + } +} + +func NotEqual() { + x := []string{"a"} + if len(x) != len(x) { // want "Boolean expression has identical expressions on both sides. The result is always true." + } +} + +func GreaterThanOrEqual() { + x := []string{"a"} + if len(x) >= len(x) { // want "Boolean expression has identical expressions on both sides. The result is always true." + } +} + +func LessThanOrEqual() { + x := []string{"a"} + if len(x) <= len(x) { // want "Boolean expression has identical expressions on both sides. The result is always true." + } +} + +func GreaterThan() { + x := []string{"a"} + if len(x) > len(x) { // want "Boolean expression has identical expressions on both sides. The result is always false." + } +} + +func LessThan() { + x := []string{"a"} + if len(x) < len(x) { // want "Boolean expression has identical expressions on both sides. The result is always false." + } +}