Skip to content

Commit 76e2cb9

Browse files
authored
Gracefully handle a panicking analyzer (#4374)
**What type of PR is this?** Bug fix **What does this PR do? Why is it needed?** We previously didn't show any error when an analyzer panicked and Bazel would show a "mandatory output not created" error for the nogo action. **Which issues(s) does this PR fix?** **Other notes for review**
1 parent 050ff83 commit 76e2cb9

File tree

2 files changed

+74
-2
lines changed

2 files changed

+74
-2
lines changed

go/tools/builders/nogo_main.go

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,12 @@ func (act *action) execOnce() {
429429
act.pass = pass
430430

431431
var err error
432+
defer func() {
433+
if r := recover(); r != nil {
434+
// If the analyzer panics, we catch it here and return an error.
435+
act.err = fmt.Errorf("panic: %v", r)
436+
}
437+
}()
432438
if !act.pkg.illTyped || pass.Analyzer.RunDespiteErrors {
433439
act.result, err = pass.Analyzer.Run(pass)
434440
if err == nil {

tests/core/nogo/custom/custom_test.go

Lines changed: 68 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -40,6 +40,7 @@ nogo(
4040
":importfmt",
4141
":visibility",
4242
":cgogen",
43+
":panicalyzer",
4344
],
4445
# config = "",
4546
visibility = ["//visibility:public"],
@@ -80,6 +81,14 @@ go_library(
8081
visibility = ["//visibility:public"],
8182
)
8283
84+
go_library(
85+
name = "panicalyzer",
86+
srcs = ["panicalyzer.go"],
87+
importpath = "panicalyzer",
88+
deps = ["@org_golang_x_tools//go/analysis"],
89+
visibility = ["//visibility:public"],
90+
)
91+
8392
go_library(
8493
name = "has_errors",
8594
srcs = ["has_errors.go"],
@@ -130,6 +139,12 @@ go_binary(
130139
pure = "on",
131140
)
132141
142+
go_library(
143+
name = "panics",
144+
srcs = ["panics.go"],
145+
importpath = "panics",
146+
)
147+
133148
-- foofuncname.go --
134149
// foofuncname checks for functions named "Foo".
135150
// It has the same package name as another check to test the checks with
@@ -336,6 +351,40 @@ func run(pass *analysis.Pass) (interface{}, error) {
336351
return nil, nil
337352
}
338353
354+
-- panicalyzer.go --
355+
// panicalyzer panics on functions named "ShouldPanic".
356+
package panicalyzer
357+
358+
import (
359+
"go/ast"
360+
361+
"golang.org/x/tools/go/analysis"
362+
)
363+
364+
const doc = "panic on functions named \"ShouldPanic\""
365+
366+
var Analyzer = &analysis.Analyzer{
367+
Name: "panicalyzer",
368+
Run: run,
369+
Doc: doc,
370+
}
371+
372+
func run(pass *analysis.Pass) (interface{}, error) {
373+
for _, f := range pass.Files {
374+
ast.Inspect(f, func(n ast.Node) bool {
375+
switch n := n.(type) {
376+
case *ast.FuncDecl:
377+
if n.Name.Name == "ShouldPanic" {
378+
panic("function must not be named ShouldPanic")
379+
}
380+
return true
381+
}
382+
return true
383+
})
384+
}
385+
return nil, nil
386+
}
387+
339388
-- config.json --
340389
{
341390
"importfmt": {
@@ -368,6 +417,10 @@ func run(pass *analysis.Pass) (interface{}, error) {
368417
"foofuncname": {
369418
"description": "no exemptions since we know this check is 100% accurate, so override base config",
370419
"exclude_files": {}
420+
},
421+
"panicalyzer": {
422+
"description": "no exemptions",
423+
"exclude_files": {}
371424
}
372425
}
373426
@@ -402,6 +455,12 @@ func Foo() bool { // This should fail foofuncname
402455
return true
403456
}
404457
458+
-- panics.go --
459+
package panics
460+
461+
func ShouldPanic() {
462+
}
463+
405464
-- no_errors.go --
406465
// package noerrors contains no analyzer errors.
407466
package noerrors
@@ -459,7 +518,7 @@ func Test(t *testing.T) {
459518
desc, config, target string
460519
wantSuccess bool
461520
includes, excludes []string
462-
bazelArgs []string
521+
bazelArgs []string
463522
}{
464523
{
465524
desc: "default_config",
@@ -530,7 +589,7 @@ func Test(t *testing.T) {
530589
target: "//:type_check_fail",
531590
wantSuccess: false,
532591
includes: []string{
533-
"4 analyzers skipped due to type-checking error: type_check_fail.go:8:10:",
592+
"\\d analyzers skipped due to type-checking error: type_check_fail.go:8:10:",
534593
},
535594
// Ensure that nogo runs even though compilation fails
536595
bazelArgs: []string{"--keep_going"},
@@ -543,6 +602,13 @@ func Test(t *testing.T) {
543602
desc: "uses_cgo_clean",
544603
target: "//:uses_cgo_clean",
545604
wantSuccess: true,
605+
}, {
606+
desc: "panics",
607+
target: "//:panics",
608+
wantSuccess: false,
609+
includes: []string{
610+
"panic: function must not be named ShouldPanic",
611+
},
546612
},
547613
} {
548614
t.Run(test.desc, func(t *testing.T) {

0 commit comments

Comments
 (0)