Conversation
Contributor
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Unreachable empty statements can still prevent the first reportable statement from being selected.
Review effort: Balanced
Findings: None
What changed in this PR
Updates go/unreachable-statement to report the first non-allowlisted statement after an allowlisted unreachable prefix.
Changes:
- Adds recursive allowlisted-prefix handling.
- Adds regression coverage and updates expected results.
Review findings:
- Moderate: Unreachable empty statements still break traversal and can suppress an alert.
- Nit: Add coverage for multiple consecutive allowlisted statements.
- Nit: Update PR metadata to describe a missing-result/false-negative fix.
| File | Description |
|---|---|
go/ql/test/query-tests/RedundantCode/UnreachableStatement/UnreachableStatement.expected |
Records the new expected alert. |
go/ql/test/query-tests/RedundantCode/UnreachableStatement/main.go |
Adds regression coverage. |
go/ql/test/query-tests/RedundantCode/UnreachableStatement/CONSISTENCY/CfgConsistency.expected |
Updates CFG consistency expectations. |
go/ql/src/RedundantCode/UnreachableStatement.ql |
Refines unreachable-statement selection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
owen-mc
force-pushed
the
go/fix/unreachable-statement-allowlist
branch
from
September 25, 2026 15:37
4cf5529 to
77b370d
Compare
owen-mc
force-pushed
the
go/fix/unreachable-statement-allowlist
branch
from
September 28, 2026 09:43
77b370d to
31b8064
Compare
michaelnebel
left a comment
michaelnebel
left a comment
Contributor
There was a problem hiding this comment.
Cool to get this fixed.
Is there a DCA run?
This branch has not been deployed
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.
We deliberately only report the first unreachable statement in a run of them. We also have an allowlist of statements we won't report. But the two features were interacting badly - if the first unreachable statement in a run was in the allowlist them we weren't reporting anything. This PR fixes that, so we report the first unreachable statement in a run which isn't in the allowlist. A test has been added to demonstrate the bug and show that it is fixed.
This does not need a change note as it is fixing FPs that haven't been in any release.