Skip to content

C#: Instantiate shared Guards and shared ControlFlowReachability and replace nullness - #20558

Merged
aschackmull merged 17 commits into
github:mainfrom
aschackmull:csharp/guards3
Oct 23, 2025
Merged

C#: Instantiate shared Guards and shared ControlFlowReachability and replace nullness#20558
aschackmull merged 17 commits into
github:mainfrom
aschackmull:csharp/guards3

Conversation

@aschackmull

@aschackmullaschackmull commented Sep 30, 2025

Copy link
Copy Markdown
Contributor

This PR contains several changes that have been bunched together by necessity - commit-by-commit review is encouraged!

The shared Guards library is instantiated for C#. This is somewhat incompatible with splitting, so 3 categories of splitting are removed. This has some minor impact on a few queries, including actually some precision improvements (see #20430 and its corresponding dca run for an isolated measurement of the impact of disabling these 3 categories of splitting (and disregard the changes to cs/call-to-object-tostring - they have been addressed in a separate bugfix)). Beyond the misc. minor query impact of removing these splitting categories, we see major precision reduction in cs/constant-condition (lost TPs) and cs/dereferenced-value-may-be-null (gained FPs). These regressions are therefore addressed in this PR.

The shared ControlFlowReachability library is instantiated and used to re-implement cs/dereferenced-value-may-be-null. This yields a massive precision improvement with lots of FPs removed; we also appear to gain some new TPs. Spot-checking indicates that these changes are almost exclusively improvements.

A new shared query is introduced to cover the gap in cs/constant-condition, which covers the results lost from lack of splitting and much more - i.e. we get a lot more results. The C#-specific instantiation adds several exclusions to reduce FPs. Spot-checking appears to indicate generally good results, but there's certainly still a number of FPs, and I have not done any formal measurements of FP rates.

One thing that's explicitly excluded in this PR is the replacement of the existing Guards library with the new one - for now the two libraries live side-by-side. This cleanup is left as follow-up work.

(note: some intermediate commits contain temporary comparison predicates, which I've found useful to leave in the commit-history during development)

private module ControlFlowInput implements
InputSig<Location, ControlFlow::Node, ControlFlow::BasicBlock>
{
private import csharp as CS

Check warning

Code scanning / CodeQL

Names only differing by case Warning

CS is only different by casing from Cs that is used elsewhere for modules.
private module GuardsInput implements
SharedGuards::InputSig<Location, ControlFlow::Node, ControlFlow::BasicBlock>
{
private import csharp as CS

Check warning

Code scanning / CodeQL

Names only differing by case Warning

CS is only different by casing from Cs that is used elsewhere for modules.
@aschackmull
aschackmullforce-pushed the csharp/guards3 branch 4 times, most recently from 366f0b0 to 2372a9cCompareOctober 3, 2025 13:28
@aschackmull
aschackmull marked this pull request as ready for review October 3, 2025 14:30
@aschackmull
aschackmull requested review from a team as code ownersOctober 3, 2025 14:30
CopilotAI review requested due to automatic review settings October 3, 2025 14:30

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

Pull Request Overview

This PR instantiates the shared Guards and ControlFlowReachability libraries for C# and introduces a new shared constant condition query, along with several related changes:

  • Instantiates shared Guards library for C# and removes 3 splitting categories that are incompatible
  • Instantiates shared ControlFlowReachability library and uses it to re-implement nullness analysis
  • Introduces new shared ConstantCondition query to replace functionality lost from removed splitting

Reviewed Changes

Copilot reviewed 65 out of 70 changed files in this pull request and generated no comments.

Show a summary per file
FileDescription
shared/controlflow/codeql/controlflow/queries/ConstantCondition.qllNew shared query library for detecting constant conditions
shared/controlflow/codeql/controlflow/Guards.qllUpdates to boolean value handling and trivial guard filtering
shared/controlflow/codeql/controlflow/ControlFlow.qllMinor method name change from getAPhiInput to getAnInput
shared/controlflow/codeql/controlflow/Cfg.qllAdds NormalExitNode class definition
java/ql/lib/semmle/code/java/dataflow/internal/BaseSSA.qllAdds getAnInput method for compatibility
java/ql/lib/semmle/code/java/dataflow/SSA.qllAdds getAnInput method for compatibility
Multiple test .expected filesUpdates to test expectations reflecting improved precision and new query results
csharp/ql/test/query-tests/Nullness/E.csTest file updates showing improved null analysis precision
csharp/ql/test/query-tests/Nullness/D.csTest file updates showing improved null analysis precision
csharp/ql/test/library-tests/dataflow/ssa/Splitting.csRemoved file - splitting functionality no longer used
Comments suppressed due to low confidence (3)

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

Pull Request Overview

Copilot reviewed 65 out of 70 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

@hvitvedhvitved 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.

Looks really great, thanks a lot for taking on the task of sharing this code.

Comment threadcsharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll
Comment threadshared/controlflow/codeql/controlflow/Cfg.qll
Comment threadcsharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll Outdated
@hvitved

Copy link
Copy Markdown
Contributor

DCA reports a relative significant slowdown on DynamoDS___buildless; have you investigated why that is?

@aschackmull

Copy link
Copy Markdown
ContributorAuthor

DCA reports a relative significant slowdown on DynamoDS___buildless; have you investigated why that is?

I have not, I'll take a look.

@aschackmull
aschackmull requested a review from a team as a code ownerOctober 22, 2025 08:23
Comment threadcsharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll Outdated
@aschackmull

Copy link
Copy Markdown
ContributorAuthor

I have found and fixed the join-order problem in DynamoDS___buildless - it was a bit tricky but the end result worked out nicely without resorting to any abstraction-breaking or the like. I've verified locally that it fixes the issue, and it should be a strict improvement, so I don't expect any issues.

I've kicked off another dca run.

@aschackmull
aschackmull merged commit 8a3f62b into github:mainOct 23, 2025
50 of 51 checks passed
@aschackmull
aschackmull deleted the csharp/guards3 branch October 23, 2025 06:43
Sign up for freeto 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.

4 participants

@aschackmull@hvitved@github-advanced-security