Static analyzer for httputil.HandleError calls (#16134)

**What type of PR is this?**

Tooling

**What does the PR do?**

Every call to `httputil.HandleError` must be followed by a `return`
statement. It's easy to miss this during reviews, so having a static
analyzer that enforces this will make our life easier.

**Acknowledgements**

- [x] I have read
[CONTRIBUTING.md](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md).
- [x] I have included a uniquely named [changelog fragment
file](https://github.com/prysmaticlabs/prysm/blob/develop/CONTRIBUTING.md#maintaining-changelogmd).
- [x] I have added a description with sufficient context for reviewers
to understand this PR.
- [x] I have tested that my changes work as expected and I added a
testing plan to the PR description (if applicable).

---------

Co-authored-by: james-prysm <90280386+james-prysm@users.noreply.github.com>

BUILD.bazel

@@ -193,6 +193,7 @@ nogo(
         "//tools/analyzers/featureconfig:go_default_library",
         "//tools/analyzers/gocognit:go_default_library",
         "//tools/analyzers/ineffassign:go_default_library",
+        "//tools/analyzers/httperror:go_default_library",
         "//tools/analyzers/interfacechecker:go_default_library",
         "//tools/analyzers/logcapitalization:go_default_library",
         "//tools/analyzers/logruswitherror:go_default_library",

beacon-chain/rpc/eth/light-client/handlers.go

@@ -116,6 +116,7 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R
 		for _, update := range updates {
 			if ctx.Err() != nil {
 				httputil.HandleError(w, "Context error: "+ctx.Err().Error(), http.StatusInternalServerError)
+				return
 			}
 
 			updateSlot := update.AttestedHeader().Beacon().Slot
@@ -131,12 +132,15 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R
 			chunkLength = ssz.MarshalUint64(chunkLength, uint64(len(updateSSZ)+4))
 			if _, err := w.Write(chunkLength); err != nil {
 				httputil.HandleError(w, "Could not write chunk length: "+err.Error(), http.StatusInternalServerError)
+				return
 			}
 			if _, err := w.Write(updateEntry.ForkDigest[:]); err != nil {
 				httputil.HandleError(w, "Could not write fork digest: "+err.Error(), http.StatusInternalServerError)
+				return
 			}
 			if _, err := w.Write(updateSSZ); err != nil {
 				httputil.HandleError(w, "Could not write update SSZ: "+err.Error(), http.StatusInternalServerError)
+				return
 			}
 		}
 	} else {
@@ -145,6 +149,7 @@ func (s *Server) GetLightClientUpdatesByRange(w http.ResponseWriter, req *http.R
 		for _, update := range updates {
 			if ctx.Err() != nil {
 				httputil.HandleError(w, "Context error: "+ctx.Err().Error(), http.StatusInternalServerError)
+				return
 			}
 
 			updateJson, err := structs.LightClientUpdateFromConsensus(update)

beacon-chain/rpc/eth/node/handlers.go

@@ -132,6 +132,7 @@ func (s *Server) GetHealth(w http.ResponseWriter, r *http.Request) {
 	optimistic, err := s.OptimisticModeFetcher.IsOptimistic(ctx)
 	if err != nil {
 		httputil.HandleError(w, "Could not check optimistic status: "+err.Error(), http.StatusInternalServerError)
+		return
 	}
 	if s.SyncChecker.Synced() && !optimistic {
 		return

beacon-chain/rpc/eth/validator/handlers.go

@@ -1103,7 +1103,8 @@ func (s *Server) GetProposerDuties(w http.ResponseWriter, r *http.Request) {
 		httputil.HandleError(w, "Could not check optimistic status: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
-	if !sortProposerDuties(w, duties) {
+	if err = sortProposerDuties(duties); err != nil {
+		httputil.HandleError(w, "Could not sort proposer duties: "+err.Error(), http.StatusInternalServerError)
 		return
 	}
 
@@ -1447,22 +1448,20 @@ func syncCommitteeDutiesAndVals(
 	return duties, vals, nil
 }
 
-func sortProposerDuties(w http.ResponseWriter, duties []*structs.ProposerDuty) bool {
+func sortProposerDuties(duties []*structs.ProposerDuty) error {
-	ok := true
+	var err error
 	sort.Slice(duties, func(i, j int) bool {
-		si, err := strconv.ParseUint(duties[i].Slot, 10, 64)
+		si, parseErr := strconv.ParseUint(duties[i].Slot, 10, 64)
-		if err != nil {
+		if parseErr != nil {
-			httputil.HandleError(w, "Could not parse slot: "+err.Error(), http.StatusInternalServerError)
+			err = errors.Wrap(parseErr, "could not parse slot")
-			ok = false
 			return false
 		}
-		sj, err := strconv.ParseUint(duties[j].Slot, 10, 64)
+		sj, parseErr := strconv.ParseUint(duties[j].Slot, 10, 64)
-		if err != nil {
+		if parseErr != nil {
-			httputil.HandleError(w, "Could not parse slot: "+err.Error(), http.StatusInternalServerError)
+			err = errors.Wrap(parseErr, "could not parse slot")
-			ok = false
 			return false
 		}
 		return si < sj
 	})
-	return ok
+	return err
 }

changelog/radek_httperror-analyzer.md

@@ -0,0 +1,3 @@
+### Added
+
+- Static analyzer that ensures each `httputil.HandleError` call is followed by a `return` statement.

tools/analyzers/httperror/BUILD.bazel

@@ -0,0 +1,13 @@
+load("@prysm//tools/go:def.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = ["analyzer.go"],
+    importpath = "github.com/OffchainLabs/prysm/v7/tools/analyzers/httperror",
+    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",
+    ],
+)

tools/analyzers/httperror/analyzer.go

@@ -0,0 +1,163 @@
+package httperror
+
+import (
+	"go/ast"
+
+	"golang.org/x/tools/go/analysis"
+	"golang.org/x/tools/go/analysis/passes/inspect"
+	"golang.org/x/tools/go/ast/inspector"
+)
+
+var Analyzer = &analysis.Analyzer{
+	Name: "httperror",
+	Doc:  "Ensures calls to httputil.HandleError are immediately followed by a return statement.",
+	Requires: []*analysis.Analyzer{
+		inspect.Analyzer,
+	},
+}
+
+func Run(pass *analysis.Pass) (any, error) {
+	ins := pass.ResultOf[inspect.Analyzer].(*inspector.Inspector)
+
+	ins.Preorder([]ast.Node{(*ast.FuncDecl)(nil)}, func(n ast.Node) {
+		fn := n.(*ast.FuncDecl)
+		if fn.Body == nil {
+			return
+		}
+		// top-level: there is no next statement after the function body
+		checkBlock(pass, fn, fn.Body, nil)
+	})
+
+	return nil, nil
+}
+
+func init() {
+	Analyzer.Run = Run
+}
+
+// checkBlock inspects all statements inside block.
+// nextAfter is the statement that comes immediately *after* the statement that contains this block,
+// at the enclosing lexical level (or nil if none).
+// Example: for `if ... { ... } ; return`, when checking the if's body the nextAfter will be the top-level return stmt.
+func checkBlock(pass *analysis.Pass, fn *ast.FuncDecl, block *ast.BlockStmt, nextAfter ast.Stmt) {
+	stmts := block.List
+	for i, stmt := range stmts {
+		// compute the next statement after this statement at the same lexical (ancestor) level:
+		// if there is a sibling, use it; otherwise propagate the nextAfter we were given.
+		var nextForThisStmt ast.Stmt
+		if i+1 < len(stmts) {
+			nextForThisStmt = stmts[i+1]
+		} else {
+			nextForThisStmt = nextAfter
+		}
+
+		// Recurse into nested blocks BEFORE checking this stmt's own expr,
+		// but we must pass nextForThisStmt to nested blocks so nested HandleError
+		// will see the correct "next statement after this statement".
+		switch s := stmt.(type) {
+		case *ast.IfStmt:
+			// pass what's next after the whole if-statement down into its bodies
+			if s.Init != nil {
+				// init is a statement (rare), treat it as contained in s; it should use next being the if's body first stmt,
+				// but for our purposes we don't need to inspect s.Init specially beyond nested calls.
+				// We'll just check it with nextForThisStmt as well.
+				checkBlock(pass, fn, &ast.BlockStmt{List: []ast.Stmt{s.Init}}, nextForThisStmt)
+			}
+			if s.Body != nil {
+				checkBlock(pass, fn, s.Body, nextForThisStmt)
+			}
+			if s.Else != nil {
+				switch els := s.Else.(type) {
+				case *ast.BlockStmt:
+					checkBlock(pass, fn, els, nextForThisStmt)
+				case *ast.IfStmt:
+					// else-if: its body will receive the same nextForThisStmt
+					checkBlock(pass, fn, els.Body, nextForThisStmt)
+				}
+			}
+
+		case *ast.ForStmt:
+			if s.Init != nil {
+				checkBlock(pass, fn, &ast.BlockStmt{List: []ast.Stmt{s.Init}}, nextForThisStmt)
+			}
+			if s.Body != nil {
+				checkBlock(pass, fn, s.Body, nextForThisStmt)
+			}
+			if s.Post != nil {
+				checkBlock(pass, fn, &ast.BlockStmt{List: []ast.Stmt{s.Post}}, nextForThisStmt)
+			}
+
+		case *ast.RangeStmt:
+			if s.Body != nil {
+				checkBlock(pass, fn, s.Body, nextForThisStmt)
+			}
+
+		case *ast.BlockStmt:
+			// nested block (e.g. anonymous block) — propagate nextForThisStmt
+			checkBlock(pass, fn, s, nextForThisStmt)
+		}
+
+		// Now check the current statement itself: is it (or does it contain) a direct call to httputil.HandleError?
+		// We only consider ExprStmt that are direct CallExpr to httputil.HandleError.
+		call := findHandleErrorCall(stmt)
+		if call == nil {
+			continue
+		}
+
+		// Determine the actual "next statement after this call" in lexical function order:
+		// - If there is a sibling in the same block after this stmt, that's next.
+		// - Otherwise, next is nextForThisStmt (propagated from ancestor).
+		var nextStmt ast.Stmt
+		if i+1 < len(stmts) {
+			nextStmt = stmts[i+1]
+		} else {
+			nextStmt = nextAfter
+		}
+
+		// If there is a next statement and it's a return -> OK
+		if nextStmt != nil {
+			if _, ok := nextStmt.(*ast.ReturnStmt); ok {
+				// immediately followed (in lexical order) by a return at some nesting level -> OK
+				continue
+			}
+			// otherwise it's not a return (even if it's an if/for etc) -> violation
+			pass.Reportf(stmt.Pos(), "call to httputil.HandleError must be immediately followed by a return statement")
+			continue
+		}
+
+		// If nextStmt == nil, this call is lexically the last statement in the function.
+		// That is allowed only if the function has no result values.
+		if fn.Type.Results == nil || len(fn.Type.Results.List) == 0 {
+			// void function: allowed
+			continue
+		}
+
+		// Non-void function and it's the last statement → violation
+		pass.Reportf(stmt.Pos(), "call to httputil.HandleError must be followed by return because function has return values")
+	}
+}
+
+// findHandleErrorCall returns the call expression if stmt is a direct call to httputil.HandleError(...),
+// otherwise nil. We only match direct ExprStmt -> CallExpr -> SelectorExpr where selector is httputil.HandleError.
+func findHandleErrorCall(stmt ast.Stmt) *ast.CallExpr {
+	es, ok := stmt.(*ast.ExprStmt)
+	if !ok {
+		return nil
+	}
+	call, ok := es.X.(*ast.CallExpr)
+	if !ok {
+		return nil
+	}
+	sel, ok := call.Fun.(*ast.SelectorExpr)
+	if !ok {
+		return nil
+	}
+	pkgIdent, ok := sel.X.(*ast.Ident)
+	if !ok {
+		return nil
+	}
+	if pkgIdent.Name == "httputil" && sel.Sel.Name == "HandleError" {
+		return call
+	}
+	return nil
+}