From f946a157eefedfbb429a7a50d92f093b55ae6be5 Mon Sep 17 00:00:00 2001 From: AGMETEOR Date: Tue, 20 Apr 2021 11:03:18 +0300 Subject: [PATCH] passes/sigchanyzer: allow valid inlined unbuffered signal channel Permit signal.Notify(make(chan os.Signal)) which is valid code, given that the channel isn't read elsewhere, the fact that signals can be lost is unimportant. Updates golang/go#45043 Change-Id: Ie984dfeedbb4e1e33a29391c3abb1fc83299fed3 Reviewed-on: https://go-review.googlesource.com/c/tools/+/311729 Reviewed-by: Cuong Manh Le Reviewed-by: Emmanuel Odeke Trust: Cuong Manh Le Run-TryBot: Cuong Manh Le gopls-CI: kokoro TryBot-Result: Go Bot --- go/analysis/passes/sigchanyzer/sigchanyzer.go | 18 ++++++++++++++++++ .../passes/sigchanyzer/testdata/src/a/a.go | 16 ++++++++++++++++ .../sigchanyzer/testdata/src/a/a.go.golden | 16 ++++++++++++++++ 3 files changed, 50 insertions(+) diff --git a/go/analysis/passes/sigchanyzer/sigchanyzer.go b/go/analysis/passes/sigchanyzer/sigchanyzer.go index 3d89061d176..b00aa7e1440 100644 --- a/go/analysis/passes/sigchanyzer/sigchanyzer.go +++ b/go/analysis/passes/sigchanyzer/sigchanyzer.go @@ -49,6 +49,11 @@ func run(pass *analysis.Pass) (interface{}, error) { chanDecl = decl } case *ast.CallExpr: + // Only signal.Notify(make(chan os.Signal), os.Interrupt) is safe, + // conservatively treate others as not safe, see golang/go#45043 + if isBuiltinMake(pass.TypesInfo, arg) { + return + } chanDecl = arg } if chanDecl == nil || len(chanDecl.Args) != 1 { @@ -127,3 +132,16 @@ func findDecl(arg *ast.Ident) ast.Node { } return nil } + +func isBuiltinMake(info *types.Info, call *ast.CallExpr) bool { + typVal := info.Types[call.Fun] + if !typVal.IsBuiltin() { + return false + } + switch fun := call.Fun.(type) { + case *ast.Ident: + return info.ObjectOf(fun).Name() == "make" + default: + return false + } +} diff --git a/go/analysis/passes/sigchanyzer/testdata/src/a/a.go b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go index 277bf2054c0..34dee88c3a0 100644 --- a/go/analysis/passes/sigchanyzer/testdata/src/a/a.go +++ b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go @@ -36,3 +36,19 @@ func j() { f := signal.Notify f(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" } + +func k() { + signal.Notify(make(chan os.Signal), os.Interrupt) // ok +} + +func l() { + signal.Notify(make(chan os.Signal, 1), os.Interrupt) // ok +} + +func m() { + signal.Notify(make(chan ao.Signal, 1), os.Interrupt) // ok +} + +func n() { + signal.Notify(make(chan ao.Signal), os.Interrupt) // ok +} diff --git a/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden index e3702d72f72..1158ff5fd37 100644 --- a/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden +++ b/go/analysis/passes/sigchanyzer/testdata/src/a/a.go.golden @@ -36,3 +36,19 @@ func j() { f := signal.Notify f(c, os.Interrupt) // want "misuse of unbuffered os.Signal channel as argument to signal.Notify" } + +func k() { + signal.Notify(make(chan os.Signal), os.Interrupt) // ok +} + +func l() { + signal.Notify(make(chan os.Signal, 1), os.Interrupt) // ok +} + +func m() { + signal.Notify(make(chan ao.Signal, 1), os.Interrupt) // ok +} + +func n() { + signal.Notify(make(chan ao.Signal), os.Interrupt) // ok +}