Add static analyzer to discourage use of panic() (#15075)

* Implement static analysis to prevent panics

* Add nopanic to nogo

* Fix violations and add exclusions

Fix violations and add exclusions for all

* Changelog fragment

* Use pass.Report instead of pass.Reportf

* Remove strings.ToLower for checking init method name

* Add exclusion for herumi init

* Move api/client/beacon template function to init and its own file

* Fix nopanic testcase
This commit is contained in:
Preston Van Loon
2025-03-19 13:04:15 -05:00
committed by GitHub
parent 16d5abd21b
commit 2aa52fb56a
74 changed files with 329 additions and 104 deletions

View File

@@ -0,0 +1,26 @@
load("@prysm//tools/go:def.bzl", "go_library", "go_test")
go_library(
name = "go_default_library",
srcs = ["analyzer.go"],
importpath = "github.com/prysmaticlabs/prysm/v5/tools/analyzers/nopanic",
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_test(
name = "go_default_test",
srcs = ["analyzer_test.go"],
data = glob(["testdata/**"]) + [
"@go_sdk//:files",
],
embed = [":go_default_library"],
deps = [
"//build/bazel:go_default_library",
"@org_golang_x_tools//go/analysis/analysistest:go_default_library",
],
)

View File

@@ -0,0 +1,86 @@
package nopanic
import (
"errors"
"go/ast"
"strings"
"golang.org/x/tools/go/analysis"
"golang.org/x/tools/go/analysis/passes/inspect"
"golang.org/x/tools/go/ast/inspector"
)
const Doc = "Tool to discourage the use of panic(), except in init functions"
var errNoPanic = errors.New("panic() should not be used, except in rare situations or init functions")
// Analyzer runs static analysis.
var Analyzer = &analysis.Analyzer{
Name: "nopanic",
Doc: Doc,
Requires: []*analysis.Analyzer{inspect.Analyzer},
Run: run,
}
func run(pass *analysis.Pass) (interface{}, error) {
inspection, ok := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
if !ok {
return nil, errors.New("analyzer is not type *inspector.Inspector")
}
nodeFilter := []ast.Node{
(*ast.CallExpr)(nil),
}
inspection.WithStack(nodeFilter, func(n ast.Node, push bool, stack []ast.Node) bool {
switch stmt := n.(type) {
case *ast.CallExpr:
if isPanic(stmt) && !hasExclusion(pass, stack) {
pass.Report(analysis.Diagnostic{
Pos: n.Pos(),
End: n.End(),
Message: errNoPanic.Error(),
})
return false
}
}
return true
})
return nil, nil
}
// isPanic returns true if the method name is exactly "panic", case insensitive.
func isPanic(call *ast.CallExpr) bool {
i, ok := call.Fun.(*ast.Ident)
return ok && strings.ToLower(i.Name) == "panic"
}
// hasExclusion looks at the ast stack and if any node in the stack has the magic words "lint:nopanic"
// then this node is considered excluded. This allows exclusions to be placed at the function or package level.
// This method also excludes init functions.
func hasExclusion(pass *analysis.Pass, stack []ast.Node) bool {
if len(stack) < 2 {
return false
}
// The first value in the stack is always the file, then the second value would be a package level function.
// Init functions are always package level.
if fd, ok := stack[1].(*ast.FuncDecl); ok && fd.Name.Name == "init" {
return true
}
// Build a comment map and scan the comments of this node stack.
cm := ast.NewCommentMap(pass.Fset, stack[0], stack[0].(*ast.File).Comments)
for _, n := range stack {
for _, cmt := range cm[n] {
for _, l := range cmt.List {
if strings.Contains(l.Text, "lint:nopanic") {
return true
}
}
}
}
return false
}

View File

@@ -0,0 +1,20 @@
package nopanic
import (
"testing"
"github.com/prysmaticlabs/prysm/v5/build/bazel"
"golang.org/x/tools/go/analysis/analysistest"
)
func init() {
if bazel.BuiltWithBazel() {
bazel.SetGoEnv()
}
}
func TestAnalyzer(t *testing.T) {
testdata := bazel.TestDataPath(t)
analysistest.TestData = func() string { return testdata }
analysistest.Run(t, testdata, Analyzer)
}

View File

@@ -0,0 +1,26 @@
package testdata
func A() error {
panic("feeling cute, let's panic!") // want "panic\\(\\) should not be used, except in rare situations or init functions"
}
func B(foo interface{}) error {
if foo == nil {
panic("impossible condition: foo is nil") //lint:nopanic -- This is validated by the caller.
}
if _, ok := foo.(string); !ok {
panic("foo should not be a string!!") // want "panic\\(\\) should not be used, except in rare situations or init functions"
}
return nil
}
//lint:nopanic -- This is method is really safe ;)
func C(foo interface{}) error {
if foo == nil {
panic("impossible condition: foo is nil")
}
return nil
}

View File

@@ -0,0 +1,7 @@
package testdata
func init() {
if false {
panic("this should never happen")
}
}

View File

@@ -0,0 +1,8 @@
// package testdata
//
// lint:nopanic -- This package is so safe that you can trust it...
package testdata
func dive() {
panic("BELLY FLOP!!!!")
}