Skip to content

Commit 8cdddbe

Browse files
committed
Fix: false positive from deferred funcs (#24)
- this adds more liberal checking of deferred functions to look for any usage of End/RecordError/SetStatus.
1 parent 926dd06 commit 8cdddbe

File tree

3 files changed

+79
-2
lines changed

3 files changed

+79
-2
lines changed

spancheck.go

Lines changed: 29 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -360,18 +360,19 @@ func usesCall(
360360
startSpanMatchers []spanStartMatcher,
361361
depth int,
362362
) bool {
363-
if depth > 1 { // for perf reasons, do not dive too deep thru func literals, just one level deep check.
363+
if depth > 1 { // for perf reasons, do not dive too deep thru func literals, just two levels deep.
364364
return false
365365
}
366366

367+
cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
368+
367369
found, reAssigned := false, false
368370
for _, subStmt := range stmts {
369371
stack := []ast.Node{}
370372
ast.Inspect(subStmt, func(n ast.Node) bool {
371373
switch n := n.(type) {
372374
case *ast.FuncLit:
373375
if len(stack) > 0 {
374-
cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
375376
g := cfgs.FuncLit(n)
376377
if g != nil && len(g.Blocks) > 0 {
377378
return usesCall(pass, g.Blocks[0].Nodes, sv, selName, ignoreCheckSig, startSpanMatchers, depth+1)
@@ -387,6 +388,32 @@ func usesCall(
387388
return false
388389
}
389390
}
391+
case *ast.DeferStmt:
392+
if n.Call == nil {
393+
break
394+
}
395+
396+
f, ok := n.Call.Fun.(*ast.FuncLit)
397+
if !ok {
398+
break
399+
}
400+
401+
if g := cfgs.FuncLit(f); g != nil && len(g.Blocks) > 0 {
402+
for _, b := range g.Blocks {
403+
if usesCall(
404+
pass,
405+
b.Nodes,
406+
sv,
407+
selName,
408+
ignoreCheckSig,
409+
startSpanMatchers,
410+
depth+1,
411+
) {
412+
found = true
413+
return false
414+
}
415+
}
416+
}
390417
case nil:
391418
if len(stack) > 0 {
392419
stack = stack[:len(stack)-1] // pop

testdata/disableerrorchecks/disable_error_checks.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -56,3 +56,15 @@ func _() error {
5656
}
5757

5858
func recordErr(span trace.Span, err error) {}
59+
60+
// https://github.com/jjti/go-spancheck/issues/24
61+
func _() (err error) {
62+
_, span := otel.Tracer("foo").Start(context.Background(), "bar")
63+
defer func() {
64+
recordErr(span, err)
65+
66+
span.End()
67+
}()
68+
69+
return errors.New("test")
70+
}

testdata/enableall/enable_all.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,3 +249,41 @@ func _() error {
249249

250250
// return errors.New("test")
251251
// }
252+
253+
// https://github.com/jjti/go-spancheck/issues/24
254+
func _() (err error) {
255+
_, span := otel.Tracer("foo").Start(context.Background(), "bar")
256+
defer func() {
257+
if err != nil {
258+
span.RecordError(err)
259+
span.SetStatus(codes.Error, "test")
260+
}
261+
262+
span.End()
263+
}()
264+
265+
return errors.New("test")
266+
}
267+
268+
func _() (err error) {
269+
_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
270+
defer func() {
271+
if true {
272+
span.End()
273+
}
274+
span.RecordError(err)
275+
}()
276+
277+
return errors.New("test") // want "return can be reached without calling span.SetStatus"
278+
}
279+
280+
func _() (err error) {
281+
_, span := otel.Tracer("foo").Start(context.Background(), "bar")
282+
defer func() {
283+
span.RecordError(err)
284+
span.SetStatus(codes.Error, "test")
285+
span.End()
286+
}()
287+
288+
return errors.New("test")
289+
}

0 commit comments

Comments
 (0)