Skip to content

fix: missed where opporunity false positive - #22484

Open
baywet wants to merge 5 commits into
github:mainfrom
baywet:fix/csharp-where-opportunity-false
Open

fix: missed where opporunity false positive#22484
baywet wants to merge 5 commits into
github:mainfrom
baywet:fix/csharp-where-opportunity-false

Conversation

@baywet

@baywet baywet commented Sep 1, 2026

Copy link
Copy Markdown

fixes #7936

Copilot AI balanced review requested due to automatic review settings September 1, 2026 14:16
@baywet
baywet requested a review from a team as a code owner September 1, 2026 14:16

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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, and throw terminal 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.

Comment thread csharp/ql/lib/Linq/Helpers.qll
Comment thread csharp/ql/src/Linq/MissedWhereOpportunity.qhelp Outdated
baywet and others added 2 commits September 1, 2026 10:23
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread csharp/ql/lib/Linq/Helpers.qll
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 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

Comment thread csharp/ql/lib/Linq/Helpers.qll

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🔵 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; } } leaves is.getThen().stripSingletonBlocks() as a UsingStmt, so none of these cases match and the original false positive remains; lock and try/finally have 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missed opportunity to use Where - false positive

2 participants