Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUG] False positive in defer including check for named error return #24

Closed
stefanvanburen opened this issue Nov 8, 2024 · 3 comments
Closed
Assignees
Labels
bug Something isn't working

Comments

@stefanvanburen
Copy link

Describe the bug
False positive with defer including check for named error return.

To Reproduce

func _(ctx context.Context) (retErr error) {
	ctx, span := otel.Tracer("foo").Start(ctx, "bar")
    defer func() {
		if retErr != nil {
			span.SetStatus(codes.Error, retErr.Error())
        	span.RecordError(retErr)
		}
		span.End() // always ended
	}()

    if err := subTask(ctx); err != nil {
        return err
    }
}

Expected behavior
No spancheck lint on the ended span, since it's always ended.

Additional context
This is a somewhat common pattern that I've seen used to always capture the error from any returned error in a function, that seems to trip up spancheck - it should be able to tell that span.End() is unconditionally ended, despite not being the first statement in the defer?

@stefanvanburen stefanvanburen added the bug Something isn't working label Nov 8, 2024
@jjti
Copy link
Owner

jjti commented Nov 8, 2024

I feared the day this bug report would arrive... I've hit this myself, honestly, and also like that approach of using named errors to set the status and error on the span. I gave a fix 30m a bit ago, but haven't properly fixed it. I have a long flight coming up that might be an ideal time...

@jjti jjti self-assigned this Nov 8, 2024
@chancez
Copy link

chancez commented Nov 26, 2024

Just wanted to note that this seems to been coming up if there's any conditionals in the defer, just in case that helps.

jjti added a commit that referenced this issue Nov 30, 2024
- this adds more liberal checking of deferred functions to look for
  any usage of End/RecordError/SetStatus.
@jjti
Copy link
Owner

jjti commented Nov 30, 2024

@stefanvanburen @chancez I think this is fixed now. Added a test case here well showing it not reporting missing calls: v0.6.3...v0.6.4

It'll take a bit before it's merged and released in golangci-lint (assuming that's how you're using the linter). Please report here and re-open this issue if it persists after picking up this latest version tho, v0.6.4

@jjti jjti closed this as completed Nov 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

3 participants