Skip to content

Conversation

@aschackmull
Copy link
Contributor

This PR adds the following pattern to the set of implications in the Guards library.

b = x == v1 || g;
if (b) {
  if (x == v2) {
    // if v1 and v2 are disjoint then g must hold here
  }
}

This pattern was observed in several nullness false positives for C#, which now ought to be fixed by this change.

@aschackmull aschackmull requested a review from a team as a code owner November 12, 2025 13:20
Copilot AI review requested due to automatic review settings November 12, 2025 13:20
@aschackmull aschackmull requested a review from a team as a code owner November 12, 2025 13:20
@github-actions github-actions bot added the Java label Nov 12, 2025
Copilot finished reviewing on behalf of aschackmull November 12, 2025 13:22
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR extends the Guards library to support disjunctive implications, enabling analysis of patterns where a disjunction constrains one alternative, allowing the other alternative to be inferred. The key pattern is: if b = x == v1 || g and b is true, then within a nested condition where x == v2 (with v1 and v2 disjoint), g must hold.

  • Adds a new DisjunctiveGuard module to compute disjunctive guard implications
  • Integrates the new disjunctive analysis into the existing guard implication transitive closure
  • Adds test case demonstrating the fix for nullness false positives in Java

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
shared/controlflow/codeql/controlflow/Guards.qll Implements the disjunctive guard implication logic and integrates it into the guard control flow analysis
java/ql/test/query-tests/Nullness/C.java Adds test case ex19 demonstrating the disjunctive nullness pattern that should now be handled correctly

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@aschackmull
Copy link
Contributor Author

Dca:

  • C++: uneventful
  • Java: A clean improvement. 2 new TPs for java/useless-null-check, 2 FPs removed for java/dereferenced-value-may-be-null
  • C#: Looks like a clean improvement as well.
    • cs/constant-condition, many new results, spot-checking indicates that they're all TPs.
    • cs/inefficient-containskey, one new TP.
    • cs/dereferenced-value-may-be-null, 23 FPs removed.

@aschackmull
Copy link
Contributor Author

There's a C# qltest that's weird - I strongly suspect that it's splitting which confuses things. Luckily that doesn't seem to affect actual query results, so I guess we can just accept the qltest weirdness for now.

Copy link
Contributor

@hvitved hvitved left a comment

Choose a reason for hiding this comment

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

QL changes LGTM.

@aschackmull aschackmull requested a review from a team as a code owner November 14, 2025 10:38
@github-actions github-actions bot added the C# label Nov 14, 2025
@aschackmull
Copy link
Contributor Author

I've added a commit accepting the qltest.

@aschackmull
Copy link
Contributor Author

I've also added a change note for C#, but not for the other languages as the possible changes there seemed too minor.

@aschackmull aschackmull merged commit 1c93710 into github:main Nov 14, 2025
47 checks passed
@aschackmull aschackmull deleted the guards/disjunctive-implication branch November 14, 2025 14:44
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.

2 participants