Skip to content

Commit

Permalink
Fix: false positive from deferred funcs (#24)
Browse files Browse the repository at this point in the history
- this adds more liberal checking of deferred functions to look for
  any usage of End/RecordError/SetStatus.
  • Loading branch information
jjti committed Nov 30, 2024
1 parent 926dd06 commit 8cdddbe
Show file tree
Hide file tree
Showing 3 changed files with 79 additions and 2 deletions.
31 changes: 29 additions & 2 deletions spancheck.go
Original file line number Diff line number Diff line change
Expand Up @@ -360,18 +360,19 @@ func usesCall(
startSpanMatchers []spanStartMatcher,
depth int,
) bool {
if depth > 1 { // for perf reasons, do not dive too deep thru func literals, just one level deep check.
if depth > 1 { // for perf reasons, do not dive too deep thru func literals, just two levels deep.
return false
}

cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)

found, reAssigned := false, false
for _, subStmt := range stmts {
stack := []ast.Node{}
ast.Inspect(subStmt, func(n ast.Node) bool {
switch n := n.(type) {
case *ast.FuncLit:
if len(stack) > 0 {
cfgs := pass.ResultOf[ctrlflow.Analyzer].(*ctrlflow.CFGs)
g := cfgs.FuncLit(n)
if g != nil && len(g.Blocks) > 0 {
return usesCall(pass, g.Blocks[0].Nodes, sv, selName, ignoreCheckSig, startSpanMatchers, depth+1)
Expand All @@ -387,6 +388,32 @@ func usesCall(
return false
}
}
case *ast.DeferStmt:
if n.Call == nil {
break
}

f, ok := n.Call.Fun.(*ast.FuncLit)
if !ok {
break
}

if g := cfgs.FuncLit(f); g != nil && len(g.Blocks) > 0 {
for _, b := range g.Blocks {
if usesCall(
pass,
b.Nodes,
sv,
selName,
ignoreCheckSig,
startSpanMatchers,
depth+1,
) {
found = true
return false
}
}
}
case nil:
if len(stack) > 0 {
stack = stack[:len(stack)-1] // pop
Expand Down
12 changes: 12 additions & 0 deletions testdata/disableerrorchecks/disable_error_checks.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,15 @@ func _() error {
}

func recordErr(span trace.Span, err error) {}

// https://github.com/jjti/go-spancheck/issues/24
func _() (err error) {
_, span := otel.Tracer("foo").Start(context.Background(), "bar")
defer func() {
recordErr(span, err)

span.End()
}()

return errors.New("test")
}
38 changes: 38 additions & 0 deletions testdata/enableall/enable_all.go
Original file line number Diff line number Diff line change
Expand Up @@ -249,3 +249,41 @@ func _() error {

// return errors.New("test")
// }

// https://github.com/jjti/go-spancheck/issues/24
func _() (err error) {
_, span := otel.Tracer("foo").Start(context.Background(), "bar")
defer func() {
if err != nil {
span.RecordError(err)
span.SetStatus(codes.Error, "test")
}

span.End()
}()

return errors.New("test")
}

func _() (err error) {
_, span := otel.Tracer("foo").Start(context.Background(), "bar") // want "span.SetStatus is not called on all paths"
defer func() {
if true {
span.End()
}
span.RecordError(err)
}()

return errors.New("test") // want "return can be reached without calling span.SetStatus"
}

func _() (err error) {
_, span := otel.Tracer("foo").Start(context.Background(), "bar")
defer func() {
span.RecordError(err)
span.SetStatus(codes.Error, "test")
span.End()
}()

return errors.New("test")
}

0 comments on commit 8cdddbe

Please sign in to comment.