fix: missed where opporunity false positive

[baywet]

fixes #7936

[Copilot]

🟡 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.

[Copilot]

🟡 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

[Copilot]

🟡 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

[Copilot]

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

[Copilot]

🔵 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

[michaelnebel]

Thank you very much for the contribution! We really appreciate it!
I have added a couple of comments.

[github-actions]

QHelp previews:

csharp/ql/src/Linq/MissedWhereOpportunity.qhelp

Missed opportunity to use Where

Programmers sometimes need to iterate over a filtered version of a sequence, rather than the sequence itself. For example, you might want to print out only the numbers in the range [1,10] that are even. One standard way of doing this is to write a loop that iterates over the whole sequence, testing the variable each iteration to determine whether or not it is even. This is often written using either if(!condition(var)) continue; as the initial statement in the loop, or by enclosing the entire loop body with if(condition(var)).

This recommendation does not apply when the matching branch exits the loop without continuing to later iterations, such as with return, yield break, or throw. In those cases the loop is searching for a terminal condition rather than filtering the remaining loop body.

Recommendation

This pattern works well and is also available as the Where method in LINQ in C# 3.5 and above. It is better to use a library method in preference to writing your own pattern unless you have a specific need for a custom version. In particular, this makes the code easier to read by expressing the intent better and by reducing the nesting depth of the code.

Example

This example shows two ways of iterating over a series of integers and only performing an action on the even ones.

class MissedWhereOpportunity
{
    public static void Main(string[] args)
    {
        List<int> lst = Enumerable.Range(1, 10).ToList();

        foreach (int i in lst)
        {
            if (i % 2 != 0)
                continue;
            Console.WriteLine(i);
            Console.WriteLine((i / 2));
        }

        foreach (int i in lst)
        {
            if (i % 2 == 0)
            {
                Console.WriteLine(i);
                Console.WriteLine((i / 2));
            }
        }
    }
}

This is far better expressed using the Where method.

class MissedWhereOpportunityFix
{
    public static void Main(string[] args)
    {
        List<int> lst = Enumerable.Range(1, 10).ToList();

        foreach (int i in lst.Where(e => e % 2 == 0))
        {
            Console.WriteLine(i);
            Console.WriteLine((i / 2));
        }
    }
}

The following example should not use Where, because the matching branch exits the method or iterator instead of continuing with filtered loop work.

class MissedWhereOpportunityGood
{
    public int? FindFirstEven(System.Collections.Generic.IEnumerable<int> values)
    {
        foreach (int value in values)
        {
            if (value % 2 == 0)
                return value;
        }

        return null;
    }
}

References

• MSDN: Enumerable.Where Method.

[michaelnebel]

Great, thank you for addressing the comments!
I triggered the CI as well and have a couple of extra asks 😄

• In my review I overlooked a minor thing (see comment for removal of exists).
• The PR needs a change-note. An example of a change note can be seen here. The documentation for change-notes can be found here.

Let me know, if you need any assistance.

In the meantime, I will start a DCA run (this is an automated test for running the query against a range of repositories to inspect the changes to alerts/performance) and review the results.

[Copilot]

🟢 Approval recommended

The implementation addresses the reported false positive and includes comprehensive regression coverage.

Review details

• Files reviewed: 5/5 changed files
• Comments generated: 0 new
• Review effort level: Balanced

[michaelnebel]

The automated test (DCA) has completed and it shows that we remove approximately 5k alerts across the repositories we test against; I spot checked around 20 of the removals - and they all look good to me 👍 Also, performance doesn't appear to be affected 👍
When the PR is in its final approved state, I will run DCA once more.
I believe that only a change note is missing.

[baywet]

@michaelnebel I've pushed the change note right after you left your last comment, this should be good to go now.

Let me know if you have any additional comments or questions.

[michaelnebel]

@michaelnebel I've pushed the change note right after you left your last comment, this should be good to go now.

Let me know if you have any additional comments or questions.

Excellent, thank you!
If the CI reports green, I will run one more DCA test as well - and then most likely it is ready to be merged 😄

[michaelnebel]

Excellent! Thank you very much! This removes a large category of false positives!
Running DCA once again and if that doesn't show any surprises, then the PR can be merged.

[michaelnebel]

DCA looks good; Merging now.

[baywet]

Thanks for all the help @michaelnebel !!
It's my first time contributing to this repository so I do have a couple of questions about the releasing process: when will the false positives start disappearing from production repositories?

I suspect it'll require a new codeql run on main?

For that I suspect it'll require the run uses the new rules?

Are the rules shipped along with the core engine? or are they live?

[michaelnebel]

Thanks for all the help @michaelnebel !! It's my first time contributing to this repository so I do have a couple of questions about the releasing process: when will the false positives start disappearing from production repositories?

I suspect it'll require a new codeql run on main?

For that I suspect it'll require the run uses the new rules?

Are the rules shipped along with the core engine? or are they live?

Happy to help - and thank you very much for the contribution!

Now that the changes are merged to main, a new version of CodeQL needs to be released (including the C# library and query pack) for the changes to be included (and made generally available). I suspect this will happen with CodeQL 2.27.0, which is in a couple of weeks.