Skip to content

C#: Bugfix for nullguards for complex patterns. #20449

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
aschackmull merged 3 commits into from
Sep 17, 2025
Merged

C#: Bugfix for nullguards for complex patterns. #20449

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
62c8f28
C#: Bugfix for nullguards for complex patterns.
aschackmull Sep 17, 2025
5e76d5f
C#: Rename predicate as per review, and fixup qltest.
aschackmull Sep 17, 2025
afc98ca
C#: Add change note.
aschackmull Sep 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -273,6 +273,31 @@ module AbstractValues {

private import AbstractValues

/** Gets the value resulting from matching `null` against `pat`. */
Comment thread
aschackmull marked this conversation as resolved.
private boolean patternContainsNull(PatternExpr pat) {
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel Sep 17, 2025

Choose a reason for hiding this comment

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

The name might be a bit misleading at the pattern can have "mentions" (and thus containing) null without matching null. Maybe rename to matchesNull.

Copy link
Copy Markdown
Contributor Author

@aschackmull aschackmull Sep 17, 2025

Choose a reason for hiding this comment

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

Yeah, I was thinking "contains" in the sense that the pattern represents a set of values. But yes, I'll change it, patternMatchesNull is better.

pat instanceof NullLiteral and result = true
or
not pat instanceof NullLiteral and
not pat instanceof NotPatternExpr and
not pat instanceof OrPatternExpr and
not pat instanceof AndPatternExpr and
result = false
or
result = patternContainsNull(pat.(NotPatternExpr).getPattern()).booleanNot()
or
exists(OrPatternExpr ope | pat = ope |
result =
patternContainsNull(ope.getLeftOperand())
.booleanOr(patternContainsNull(ope.getRightOperand()))
)
or
exists(AndPatternExpr ape | pat = ape |
result =
patternContainsNull(ape.getLeftOperand())
.booleanAnd(patternContainsNull(ape.getRightOperand()))
)
}

pragma[nomagic]
private predicate typePattern(PatternMatch pm, TypePatternExpr tpe, Type t) {
tpe = pm.getPattern() and
Expand Down Expand Up @@ -362,8 +387,7 @@ class DereferenceableExpr extends Expr {
isNull = branch
or
// E.g. `x is string` or `x is ""`
not pm.getPattern() instanceof NullLiteral and
branch = true and
branch.booleanNot() = patternContainsNull(pm.getPattern()) and
Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel Sep 17, 2025

Choose a reason for hiding this comment

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

Maybe consider elaborating the comment above.

Do we need the dual

// E.g. `x is null` or `x is null or ...`
branch = patternContainsNull(pm.getPattern()) and
isNull = true

Copy link
Copy Markdown
Contributor Author

@aschackmull aschackmull Sep 17, 2025

Choose a reason for hiding this comment

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

No. Just because a pattern will match null doesn't mean that it's the only value that it'll match, so we can't conclude that the value is null based on a match or no-match branch edge (unless the pattern is literally null). I guess we could add another case for x is not null, which does imply x == null in its false branch, but I think I'll postpone that particular tweak to my work on instantiating Guards, because then we can handle those cases in slightly more generality.

Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel Sep 17, 2025

Choose a reason for hiding this comment

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

Ah yes, that is of course correct.

There is some overlap in this disjunct with

// E.g. `x is null`
pm.getPattern() instanceof NullLiteral and
isNull = branch

should the above be changed to only handle isNull = branch = true as a special case?

Copy link
Copy Markdown
Contributor Author

@aschackmull aschackmull Sep 17, 2025

Choose a reason for hiding this comment

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

True, but probably doesn't matter. Regardless, this is just a stepping stone - I'm working on a shared Guards instantiation in a separate branch where this will be tweaked anyway.

Copy link
Copy Markdown
Contributor

@michaelnebel michaelnebel Sep 17, 2025

Choose a reason for hiding this comment

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

Excellent! Once again, thank you!

isNull = false
or
exists(TypePatternExpr tpe |
Expand Down
Loading