Go: Improve precision of go/unhandled-writable-file-close#21940
Open
owen-mc wants to merge 9 commits into
Open
Go: Improve precision of go/unhandled-writable-file-close#21940owen-mc wants to merge 9 commits into
go/unhandled-writable-file-close#21940owen-mc wants to merge 9 commits into
Conversation
There are paths to the exit of the function which go through the defer statement and paths which don't, so we add an optional call to the deferred function. This causes FPs in the query as it stands.
Correct the doc for unhandledCall (it also matches expression statements where the result is discarded) and remove a stale commented-out line in isWritableFileHandle.
A deferred Close runs at function exit, but the CFG splices it in at the exit node where it can be reached along paths that never execute Sync. The previous dominance check therefore produced a false positive when a statement followed the if-block that registered the defer (e.g. deferredCloseWithSync2). For deferred closes, require instead that a handled Sync post-dominates the point where the defer is registered, which guarantees Sync runs before Close on every path on which Close is registered. Non-deferred closes keep the existing dominance check.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR improves the Go query go/unhandled-writable-file-close to reduce false positives when a deferred Close is guaranteed (on all relevant paths) to be preceded by a handled Sync on the same file handle.
Changes:
- Refines the query logic to treat deferred
Closedifferently from non-deferredClosewhen reasoning about whether a handledSyncprecedes it. - Extends the query test suite with an additional “previously spurious” scenario and updates expected results accordingly.
- Adds a Go change note documenting the precision improvement.
Show a summary per file
| File | Description |
|---|---|
| go/ql/src/InconsistentCode/UnhandledCloseWritableHandle.ql | Updates query/control-flow reasoning to reduce false positives for deferred closes preceded by handled syncs. |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/tests.go | Adds a new regression scenario marked as previously spurious. |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/UnhandledCloseWritableHandle.expected | Updates expected results and records fixed spurious expectations. |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer.ql | Adds a CFG-dumping debug query (currently missing a matching expected artifact). |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer2.ql | Adds a CFG-dumping query variant for the same functions. |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer2.expected | Stores expected output for the CFG dump. |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer3.ql | Adds another CFG-dumping query variant (near-duplicate of DebugDefer2). |
| go/ql/test/query-tests/InconsistentCode/UnhandledCloseWritableHandle/DebugDefer3.expected | Stores expected output for the CFG dump. |
| go/ql/src/change-notes/2026-06-04-unhandled-writable-file-close.md | Documents the precision improvement in change notes. |
Copilot's findings
- Files reviewed: 9/9 changed files
- Comments generated: 3
94b75a5 to
14e3ee2
Compare
Contributor
Author
|
DCA showed no significant performance impact and no result changes. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The query
go/unhandled-writable-file-close("Writable file handle closed without error handling") now produces fewer false positives. A deferred call toClosethat is preceded on every execution path by a handled call toSyncon the same file handle is no longer flagged.