C#: Simplify the ConstantCondition query. by aschackmull · Pull Request #21599 · github/codeql

aschackmull · 2026-03-27T14:19:20Z

Several simplifications are made (motivated by the upcoming CFG switch):

Constant boolean conditions are restricted to comparisons - literal true/false are seldom interesting alerts and the special-casing for isNullOrEmpty called on constants that happens inside the CFG construction seems a bit too narrow to warrant attention, and it's not something that'll be preserved in the new CFG. This allows us to get rid of all the allow-listing that was due to the the CFG round-trip.
The constant nullness implementation is short-circuited instead of relying on the CFG round-trip.
Constant matching seemed to silly to keep - switching on constants or obviously disjoint types is the kind of thing that only really happens in silly synthetic test cases. And any real occurrences are better handled by an LLM anyway.

The ConstantCondition query was partially implemented inside the CFG (in Completion.qll). In particular, this included all of isConstantComparison from Constants.qll, which was the main implementation for the ConstantComparison query. But due to the CFG round-trip the ConstantCondition query only kept those results that were manifested as missing conditional CFG edges. Short-circuiting this round-trip means that the ConstantComparison query is made redundant.

aschackmull · 2026-03-30T08:36:34Z

I've tested the constant-type-check part of the query on MRVA - there were very few results, and those that I checked were FPs, so dropping that seems perfectly fine.

github-actions · 2026-03-30T11:52:04Z

QHelp previews:

Copilot

Pull request overview

Simplifies the C# cs/constant-condition query implementation ahead of an upcoming CFG change by removing reliance on the CFG “round-trip”, narrowing constant-boolean reporting to constant comparisons, and dropping constant pattern-matching reporting; as a consequence, the standalone cs/constant-comparison query becomes redundant and is removed.

Changes:

Reworks cs/constant-condition to use constant-comparison/nullness logic directly and removes constant-matching and literal true/false condition reporting.
Removes the cs/constant-comparison query from the repo and from the standard suites/tests.
Updates/reshapes query-tests and expected outputs to match the new alert surface.

Reviewed changes

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

Show a summary per file

File	Description
csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected	Updates standalone expected results to reflect removed constant-matching reporting.
csharp/ql/test/query-tests/standalone/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs	Updates inline expectation comment to document descoped behavior.
csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/options	Removes test harness configuration for the deleted ConstantComparison query.
csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.qlref	Removes qlref for the deleted ConstantComparison query.
csharp/ql/test/query-tests/Likely Bugs/ConstantComparison/ConstantComparison.expected	Removes expected results for the deleted ConstantComparison query.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantWhileCondition.cs	Adjusts test annotations to reflect no longer flagging literal `false` loop conditions.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs	Adjusts test annotations around `IsNullOrEmpty` constant cases.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIfCondition.cs	Adjusts test annotations and adds an unsigned-range constant comparison case.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantForCondition.cs	Adjusts test annotations for literal `false` loop conditions.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantDoCondition.cs	Updates annotations to expect alerts for constant comparison do/while conditions.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantConditionalExpressionCondition.cs	Adjusts conditional-expression annotations consistent with the new scope.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.expected	Updates expected results to include constant-comparison-derived findings and remove descoped ones.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs	Updates pattern-matching test annotations and keeps other cases aligned with new query behavior.
csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantComparison.cs	Retargets constant-comparison test cases to be consumed by ConstantCondition.
csharp/ql/src/Likely Bugs/ConstantComparison.ql	Deletes the now-redundant ConstantComparison query.
csharp/ql/src/Likely Bugs/ConstantComparison.qhelp	Deletes documentation for the removed ConstantComparison query.
csharp/ql/src/Likely Bugs/ConstantComparison.cs	Deletes the sample source for the removed ConstantComparison query.
csharp/ql/src/CSI/CompareIdenticalValues.ql	Updates overlap-avoidance logic now that ConstantComparison is removed/re-scoped.
csharp/ql/src/codeql-suites/csharp-security-and-quality.qls	Removes `cs/constant-comparison` from the standard suite.
csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql	Refactors ConstantCondition implementation: direct nullness checks, comparisons-only boolean conditions, drops constant matching.
csharp/ql/lib/semmle/code/csharp/commons/Constants.qll	Removes `isConstantCondition` predicate (no longer used).
csharp/ql/integration-tests/posix/query-suite/csharp-security-and-quality.qls.expected	Updates integration expected suite contents after query removal.
csharp/ql/integration-tests/posix/query-suite/csharp-code-quality.qls.expected	Updates integration expected suite contents after query removal.
csharp/ql/integration-tests/posix/query-suite/csharp-code-quality-extended.qls.expected	Updates integration expected suite contents after query removal.

Comments suppressed due to low confidence (3)

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs:76

case int _: is annotated as // Missing Alert, but constant pattern/matching reporting is being removed in this PR. Consider marking this as // Descoped/// GOOD to match the new intended behavior rather than implying a bug.

        switch ((object)s)
        {
            case int _: // Missing Alert
                break;

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs:16

The // Missing Alert annotations for string.IsNullOrEmpty(...) calls look intentional given the PR’s decision to stop special-casing IsNullOrEmpty on constants. Consider relabeling them as // Descoped (or similar) so the test doesn’t imply the query is unexpectedly failing.

This issue also appears on line 46 of the same file.

                if (string.IsNullOrEmpty(nameof(args))) // Missing Alert (always false)
                {
                }

                string? x = null;
                if (string.IsNullOrEmpty(x)) // Missing Alert (always true)
                {

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantIsNullOrEmpty.cs:54

Same as earlier in this file: these // Missing Alert notes appear to reflect intentional descoping of constant IsNullOrEmpty handling. Consider renaming to // Descoped/// GOOD for clarity.

                if (string.IsNullOrEmpty(null)) // Missing Alert
                {
                }

                if (string.IsNullOrEmpty("")) // Missing Alert
                {
                }

                if (string.IsNullOrEmpty(" ")) // Missing Alert

csharp/ql/src/CSI/CompareIdenticalValues.ql

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs

michaelnebel · 2026-03-31T08:57:48Z

csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql

+      stripped.getType() =
+        any(ValueType t |
+          not t instanceof NullableType and
+          // Extractor bug: the type of `x?.Length` is reported as `int`, but it should
+          // be `int?`
+          not getQualifier*(stripped).(QualifiableExpr).isConditional()
+        ) and
+      b = false
+      or
+      stripped instanceof NullLiteral and
+      b = true
+      or
+      stripped.hasValue() and
+      not stripped instanceof NullLiteral and
+      b = false


Would it be possible to introduce a helper predicate for this and re-use it in Completion.qll as well?

That's scheduled for deletion in an upcoming PR.

michaelnebel

Looks good to me!
One tiny nitpick related to code-sharing.

github-actions bot added the C# label Mar 27, 2026

aschackmull force-pushed the csharp/constantcondition-simplify branch from e30a08f to 2d39a2b Compare March 30, 2026 06:57

aschackmull force-pushed the csharp/constantcondition-simplify branch 2 times, most recently from 92225e6 to 371b9b8 Compare March 30, 2026 11:51

github-actions bot added the documentation label Mar 30, 2026

aschackmull force-pushed the csharp/constantcondition-simplify branch from 371b9b8 to 86cc188 Compare March 30, 2026 12:05

aschackmull marked this pull request as ready for review March 31, 2026 07:11

aschackmull requested a review from a team as a code owner March 31, 2026 07:11

Copilot AI review requested due to automatic review settings March 31, 2026 07:11

Copilot started reviewing on behalf of aschackmull March 31, 2026 07:11 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

csharp/ql/src/CSI/CompareIdenticalValues.ql Show resolved Hide resolved

csharp/ql/test/query-tests/Bad Practices/Control-Flow/ConstantCondition/ConstantCondition.cs Outdated Show resolved Hide resolved

aschackmull added 3 commits March 31, 2026 09:38

C#: Simplify the ConstantCondition query.

056be6d

C#: Remove redundant ConstantComparison.ql query.

2a54dce

C#: Add change note.

29500c7

aschackmull force-pushed the csharp/constantcondition-simplify branch from 86cc188 to 29500c7 Compare March 31, 2026 07:39

michaelnebel reviewed Mar 31, 2026

View reviewed changes

michaelnebel approved these changes Mar 31, 2026

View reviewed changes

aschackmull merged commit 2bde364 into github:main Mar 31, 2026
30 checks passed

aschackmull deleted the csharp/constantcondition-simplify branch March 31, 2026 09:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

C#: Simplify the ConstantCondition query.#21599

C#: Simplify the ConstantCondition query.#21599
aschackmull merged 3 commits intogithub:mainfrom
aschackmull:csharp/constantcondition-simplify

aschackmull commented Mar 27, 2026 •

edited

Loading

Uh oh!

aschackmull commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

michaelnebel Mar 31, 2026

Uh oh!

aschackmull Mar 31, 2026

Uh oh!

michaelnebel left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

aschackmull commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aschackmull commented Mar 30, 2026

Uh oh!

github-actions bot commented Mar 30, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

michaelnebel Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

aschackmull Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

michaelnebel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aschackmull commented Mar 27, 2026 •

edited

Loading