fix: missed where opporunity false positive - #22484
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Recursive behavior needs test coverage, and the query help inaccurately states that every throw exits the callable.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Prevents false positives when a filtered branch terminates loop processing.
Changes:
- Detects
return,yield break, andthrowterminal branches. - Adds query tests and documentation examples.
File summaries
| File | Description |
|---|---|
Helpers.qll |
Adds terminal-branch detection. |
MissedWhereOpportunity.cs |
Tests terminal and yielding cases. |
MissedWhereOpportunity.expected |
Updates expected results. |
MissedWhereOpportunity.qhelp |
Documents excluded patterns. |
MissedWhereOpportunityGood.cs |
Adds valid documentation examples. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Direct break branches remain falsely reported despite matching the documented terminal-loop criterion.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The newly added BreakStmt behavior lacks regression coverage.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
There was a problem hiding this comment.
🔵 Needs a closer look
Terminal exits wrapped in constructs such as using, lock, or try/finally remain false positives.
Review details
Suppressed comments (1)
Previously missed (1) — in code that hasn't changed since the last review.
csharp/ql/lib/Linq/Helpers.qll:23
- This syntactic check still reports unconditional exits wrapped by other statements. For example,
if (condition) { using (resource) { return element; } }leavesis.getThen().stripSingletonBlocks()as aUsingStmt, so none of these cases match and the original false positive remains;lockandtry/finallyhave the same problem. Please determine normal completion from the control-flow graph (or handle all transparent statement wrappers) and add a regression case for a wrapped return.
private predicate terminatesCallable(Stmt s) {
- Files reviewed: 5/5 changed files
- Comments generated: 0 new
- Review effort level: Balanced
fixes #7936