Skip to content

chbatchclose: add support for defer on no-arg closure - #8

Merged
cyrilou242 merged 3 commits into
mainfrom
inspect-closures
May 22, 2026
Merged

chbatchclose: add support for defer on no-arg closure #8
cyrilou242 merged 3 commits into
mainfrom
inspect-closures

Conversation

@cyrilou242

@cyrilou242 cyrilou242 commented May 21, 2026

Copy link
Copy Markdown
Contributor

WHAT

fix #7

batch closed in a closure was not supported

defer func() {
  if err = batch.Close(); err != nil {
    // log error
  }
}()

resulting in false positives

this was a known limitation, see updated tests

out of scope:

  • support closures with parameters
  • defer closeUtil(batch) with closeUtil an util defined elsewhere - this is a legit pattern but will be implemented later, it's a bigger change

@cyrilou242 cyrilou242 changed the title inspect closures chbatchclose: add support for defer on no-arg closure May 21, 2026
}()
}

// invalid: deferred closure calls only Abort() (or other methods), not Close().

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: I understand that Close() is not called in this pattern. But this case looks like more restrictive. For example how do you know if Close() is not called in later defers? Plus when to call Abort() looks like out of scope for these linters? no? May be I'm missing something?

@cyrilou242 cyrilou242 May 22, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test is similar to this one line 23:

// invalid: defer Abort() after PrepareBatch
// defer Abort() can return an error that is ignored by defer. batch.Close should be used
func invalidDeferAbort() {
	batch, err := conn.PrepareBatch(ctx, "INSERT INTO t") //want `clickhouse Batch batch must be closed defensively with defer batch\.Close\(\) after successful instantiation`
	if err != nil {
		return
	}
	defer batch.Abort()
	_ = batch.Append(1)
	_ = batch.Send()
}

the intent of these tests is more to be used as a specification than as actual tests. the intent is to clarify that abort is not equivalent to Close.

For example how do you know if Close() is not called in later defers?

the linter is not particularly looking at Abort, it's just observing that there is no batch.Close(), neither in the defer, neither in the rest of the function

does this answer your question?

@kavirajk kavirajk left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. left one minor comment

@cyrilou242
cyrilou242 merged commit b85aeec into main May 22, 2026
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

errcheck and ch go linter conflict

2 participants