Skip to content

Commit ab5f671

Browse files
aschackmullaschackmull
authored
Merge pull request #20449 from aschackmull/csharp/nullguard-pattern
C#: Bugfix for nullguards for complex patterns.
2 parents 05d5c1d + afc98ca commit ab5f671

File tree

6 files changed

+77
-6
lines changed

6 files changed

+77
-6
lines changed

‎csharp/ql/lib/semmle/code/csharp/controlflow/Guards.qll‎

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,29 @@ module AbstractValues {
273273

274274
private import AbstractValues
275275

276+
/** Gets the value resulting from matching `null` against `pat`. */
277+
private boolean patternMatchesNull(PatternExpr pat) {
278+
pat instanceof NullLiteral and result = true
279+
or
280+
not pat instanceof NullLiteral and
281+
not pat instanceof NotPatternExpr and
282+
not pat instanceof OrPatternExpr and
283+
not pat instanceof AndPatternExpr and
284+
result = false
285+
or
286+
result = patternMatchesNull(pat.(NotPatternExpr).getPattern()).booleanNot()
287+
or
288+
exists(OrPatternExpr ope | pat = ope |
289+
result =
290+
patternMatchesNull(ope.getLeftOperand()).booleanOr(patternMatchesNull(ope.getRightOperand()))
291+
)
292+
or
293+
exists(AndPatternExpr ape | pat = ape |
294+
result =
295+
patternMatchesNull(ape.getLeftOperand()).booleanAnd(patternMatchesNull(ape.getRightOperand()))
296+
)
297+
}
298+
276299
pragma[nomagic]
277300
private predicate typePattern(PatternMatch pm, TypePatternExpr tpe, Type t) {
278301
tpe = pm.getPattern() and
@@ -362,8 +385,7 @@ class DereferenceableExpr extends Expr {
362385
isNull = branch
363386
or
364387
// E.g. `x is string` or `x is ""`
365-
not pm.getPattern() instanceof NullLiteral and
366-
branch = true and
388+
branch.booleanNot() = patternMatchesNull(pm.getPattern()) and
367389
isNull = false
368390
or
369391
exists(TypePatternExpr tpe |

‎csharp/ql/src/change-notes/2025-09-17-nullguard-pattern.md‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,4 @@
1+
---
2+
category: minorAnalysis
3+
---
4+
* The modeling of null guards based on complex pattern expressions has been improved, which in turn improves the query `cs/dereferenced-value-may-be-null` by removing false positives.

‎csharp/ql/test/query-tests/Nullness/E.cs‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -432,7 +432,7 @@ static bool Ex44(int? i, IEnumerable<int> @is)
432432
return @is.Any();
433433
}
434434

435-
static void Ex45(string s)
435+
static void Ex45(string s) // $ Source[cs/dereferenced-value-may-be-null]
436436
{
437437
if (s is null)
438438
{
@@ -441,7 +441,7 @@ static void Ex45(string s)
441441

442442
if (s is not not null)
443443
{
444-
s.ToString(); // $ MISSING: Alert[cs/dereferenced-value-is-always-null]
444+
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null] MISSING: Alert[cs/dereferenced-value-is-always-null]
445445
}
446446

447447
if (s is not null)
@@ -453,6 +453,15 @@ static void Ex45(string s)
453453
{
454454
s.ToString(); // GOOD
455455
}
456+
457+
if (s is not object)
458+
{
459+
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
460+
}
461+
else
462+
{
463+
s.ToString(); // GOOD
464+
}
456465
}
457466
}
458467

‎csharp/ql/test/query-tests/Nullness/Implications.expected‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1302,9 +1302,10 @@
13021302
| E.cs:432:16:432:24 | call to method Any<Int32> | true | E.cs:432:16:432:18 | access to parameter is | non-empty |
13031303
| E.cs:437:13:437:21 | ... is ... | false | E.cs:437:13:437:13 | access to parameter s | non-null |
13041304
| E.cs:437:13:437:21 | ... is ... | true | E.cs:437:13:437:13 | access to parameter s | null |
1305-
| E.cs:442:13:442:29 | ... is ... | true | E.cs:442:13:442:13 | access to parameter s | non-null |
1305+
| E.cs:442:13:442:29 | ... is ... | false | E.cs:442:13:442:13 | access to parameter s | non-null |
13061306
| E.cs:447:13:447:25 | ... is ... | true | E.cs:447:13:447:13 | access to parameter s | non-null |
13071307
| E.cs:452:13:452:23 | ... is ... | true | E.cs:452:13:452:13 | access to parameter s | non-null |
1308+
| E.cs:457:13:457:27 | ... is ... | false | E.cs:457:13:457:13 | access to parameter s | non-null |
13081309
| F.cs:8:9:8:9 | access to local variable o | non-null | F.cs:7:20:7:23 | null | non-null |
13091310
| F.cs:8:9:8:9 | access to local variable o | null | F.cs:7:20:7:23 | null | null |
13101311
| F.cs:14:9:14:9 | access to local variable o | non-null | F.cs:13:21:13:24 | null | non-null |

‎csharp/ql/test/query-tests/Nullness/NullCheck.expected‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -300,9 +300,10 @@
300300
| E.cs:429:13:429:22 | access to property HasValue | E.cs:429:13:429:13 | access to parameter i | true | false |
301301
| E.cs:437:13:437:21 | ... is ... | E.cs:437:13:437:13 | access to parameter s | false | false |
302302
| E.cs:437:13:437:21 | ... is ... | E.cs:437:13:437:13 | access to parameter s | true | true |
303-
| E.cs:442:13:442:29 | ... is ... | E.cs:442:13:442:13 | access to parameter s | true | false |
303+
| E.cs:442:13:442:29 | ... is ... | E.cs:442:13:442:13 | access to parameter s | false | false |
304304
| E.cs:447:13:447:25 | ... is ... | E.cs:447:13:447:13 | access to parameter s | true | false |
305305
| E.cs:452:13:452:23 | ... is ... | E.cs:452:13:452:13 | access to parameter s | true | false |
306+
| E.cs:457:13:457:27 | ... is ... | E.cs:457:13:457:13 | access to parameter s | false | false |
306307
| Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | Forwarding.cs:9:14:9:14 | access to local variable s | false | false |
307308
| Forwarding.cs:14:13:14:32 | call to method IsNotNullOrEmpty | Forwarding.cs:14:13:14:13 | access to local variable s | true | false |
308309
| Forwarding.cs:19:14:19:23 | call to method IsNull | Forwarding.cs:19:14:19:14 | access to local variable s | false | false |

‎csharp/ql/test/query-tests/Nullness/NullMaybe.expected‎

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -83,6 +83,8 @@
8383
| E.cs:417:34:417:34 | access to parameter i | E.cs:417:24:417:40 | SSA capture def(i) | E.cs:417:34:417:34 | access to parameter i | Variable $@ may be null at this access because it has a nullable type. | E.cs:415:27:415:27 | i | i | E.cs:415:27:415:27 | i | this |
8484
| E.cs:423:38:423:38 | access to parameter i | E.cs:423:28:423:44 | SSA capture def(i) | E.cs:423:38:423:38 | access to parameter i | Variable $@ may be null at this access because it has a nullable type. | E.cs:420:27:420:27 | i | i | E.cs:420:27:420:27 | i | this |
8585
| E.cs:430:39:430:39 | access to parameter i | E.cs:430:29:430:45 | SSA capture def(i) | E.cs:430:39:430:39 | access to parameter i | Variable $@ may be null at this access because it has a nullable type. | E.cs:427:27:427:27 | i | i | E.cs:427:27:427:27 | i | this |
86+
| E.cs:444:13:444:13 | access to parameter s | E.cs:435:29:435:29 | SSA param(s) | E.cs:444:13:444:13 | access to parameter s | Variable $@ may be null at this access as suggested by $@ null check. | E.cs:435:29:435:29 | s | s | E.cs:437:13:437:21 | ... is ... | this |
87+
| E.cs:459:13:459:13 | access to parameter s | E.cs:435:29:435:29 | SSA param(s) | E.cs:459:13:459:13 | access to parameter s | Variable $@ may be null at this access as suggested by $@ null check. | E.cs:435:29:435:29 | s | s | E.cs:437:13:437:21 | ... is ... | this |
8688
| GuardedString.cs:35:31:35:31 | access to local variable s | GuardedString.cs:7:16:7:32 | SSA def(s) | GuardedString.cs:35:31:35:31 | access to local variable s | Variable $@ may be null at this access because of $@ assignment. | GuardedString.cs:7:16:7:16 | s | s | GuardedString.cs:7:16:7:32 | String s = ... | this |
8789
| NullMaybeBad.cs:7:27:7:27 | access to parameter o | NullMaybeBad.cs:13:17:13:20 | null | NullMaybeBad.cs:7:27:7:27 | access to parameter o | Variable $@ may be null at this access because of $@ null argument. | NullMaybeBad.cs:5:25:5:25 | o | o | NullMaybeBad.cs:13:17:13:20 | null | this |
8890
| Params.cs:14:17:14:20 | access to parameter args | Params.cs:20:12:20:15 | null | Params.cs:14:17:14:20 | access to parameter args | Variable $@ may be null at this access because of $@ null argument. | Params.cs:12:36:12:39 | args | args | Params.cs:20:12:20:15 | null | this |
@@ -445,7 +447,23 @@ edges
445447
| E.cs:423:28:423:44 | SSA capture def(i) | E.cs:423:38:423:38 | access to parameter i |
446448
| E.cs:430:29:430:45 | SSA capture def(i) | E.cs:430:39:430:39 | access to parameter i |
447449
| E.cs:435:29:435:29 | SSA param(s) | E.cs:437:13:437:21 | [true] ... is ... |
450+
| E.cs:437:13:437:21 | [true] ... is ... | E.cs:438:9:440:9 | {...} |
448451
| E.cs:437:13:437:21 | [true] ... is ... | E.cs:439:13:439:13 | access to parameter s |
452+
| E.cs:438:9:440:9 | {...} | E.cs:442:9:445:9 | if (...) ... |
453+
| E.cs:442:9:445:9 | if (...) ... | E.cs:442:22:442:29 | [no-match] not ... |
454+
| E.cs:442:13:442:29 | [true] ... is ... | E.cs:443:9:445:9 | {...} |
455+
| E.cs:442:13:442:29 | [true] ... is ... | E.cs:444:13:444:13 | access to parameter s |
456+
| E.cs:442:18:442:29 | [match] not ... | E.cs:442:13:442:29 | [true] ... is ... |
457+
| E.cs:442:22:442:29 | [no-match] not ... | E.cs:442:18:442:29 | [match] not ... |
458+
| E.cs:443:9:445:9 | {...} | E.cs:447:9:450:9 | if (...) ... |
459+
| E.cs:447:9:450:9 | if (...) ... | E.cs:447:18:447:25 | [no-match] not ... |
460+
| E.cs:447:13:447:25 | [false] ... is ... | E.cs:452:9:455:9 | if (...) ... |
461+
| E.cs:447:18:447:25 | [no-match] not ... | E.cs:447:13:447:25 | [false] ... is ... |
462+
| E.cs:452:9:455:9 | if (...) ... | E.cs:452:13:452:23 | [false] ... is ... |
463+
| E.cs:452:13:452:23 | [false] ... is ... | E.cs:457:9:464:9 | if (...) ... |
464+
| E.cs:457:9:464:9 | if (...) ... | E.cs:457:18:457:27 | [match] not ... |
465+
| E.cs:457:13:457:27 | [true] ... is ... | E.cs:459:13:459:13 | access to parameter s |
466+
| E.cs:457:18:457:27 | [match] not ... | E.cs:457:13:457:27 | [true] ... is ... |
449467
| F.cs:7:16:7:23 | SSA def(o) | F.cs:8:9:8:9 | access to local variable o |
450468
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:9:13:9:30 | [false] !... |
451469
| Forwarding.cs:9:13:9:30 | [false] !... | Forwarding.cs:14:9:17:9 | if (...) ... |
@@ -894,7 +912,23 @@ nodes
894912
| E.cs:430:39:430:39 | access to parameter i |
895913
| E.cs:435:29:435:29 | SSA param(s) |
896914
| E.cs:437:13:437:21 | [true] ... is ... |
915+
| E.cs:438:9:440:9 | {...} |
897916
| E.cs:439:13:439:13 | access to parameter s |
917+
| E.cs:442:9:445:9 | if (...) ... |
918+
| E.cs:442:13:442:29 | [true] ... is ... |
919+
| E.cs:442:18:442:29 | [match] not ... |
920+
| E.cs:442:22:442:29 | [no-match] not ... |
921+
| E.cs:443:9:445:9 | {...} |
922+
| E.cs:444:13:444:13 | access to parameter s |
923+
| E.cs:447:9:450:9 | if (...) ... |
924+
| E.cs:447:13:447:25 | [false] ... is ... |
925+
| E.cs:447:18:447:25 | [no-match] not ... |
926+
| E.cs:452:9:455:9 | if (...) ... |
927+
| E.cs:452:13:452:23 | [false] ... is ... |
928+
| E.cs:457:9:464:9 | if (...) ... |
929+
| E.cs:457:13:457:27 | [true] ... is ... |
930+
| E.cs:457:18:457:27 | [match] not ... |
931+
| E.cs:459:13:459:13 | access to parameter s |
898932
| F.cs:7:16:7:23 | SSA def(o) |
899933
| F.cs:8:9:8:9 | access to local variable o |
900934
| Forwarding.cs:7:16:7:23 | SSA def(s) |

0 commit comments

Comments
 (0)