Skip to content

Commit 51d093a

Browse files
hvitvedhvitved
committed
C#: Address review comments
1 parent 946be96 commit 51d093a

File tree

10 files changed

+108
-5
lines changed

10 files changed

+108
-5
lines changed

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

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -661,11 +661,15 @@ module Internal {
661661
e1 = e2.(AssignExpr).getRValue()
662662
or
663663
e1 = e2.(Cast).getExpr()
664+
or
665+
e2 = e1.(NullCoalescingExpr).getAnOperand()
664666
}
665667

666668
/** Holds if expression `e3` is a `null` value whenever `e1` and `e2` are. */
667669
predicate nullValueImpliedBinary(Expr e1, Expr e2, Expr e3) {
668670
e3 = any(ConditionalExpr ce | e1 = ce.getThen() and e2 = ce.getElse())
671+
or
672+
e3 = any(NullCoalescingExpr nce | e1 = nce.getLeftOperand() and e2 = nce.getRightOperand())
669673
}
670674

671675
/** A callable that always returns a non-`null` value. */
@@ -684,11 +688,15 @@ module Internal {
684688
e instanceof ArrayCreation
685689
or
686690
e.hasValue() and
687-
not e instanceof NullLiteral and
688-
not e instanceof DefaultValueExpr
691+
exists(Expr stripped | stripped = e.stripCasts() |
692+
not stripped instanceof NullLiteral and
693+
not stripped instanceof DefaultValueExpr
694+
)
689695
or
690696
e instanceof ThisAccess
691697
or
698+
// "In string concatenation operations, the C# compiler treats a null string the same as an empty string."
699+
// (https://docs.microsoft.com/en-us/dotnet/csharp/how-to/concatenate-multiple-strings)
692700
e instanceof AddExpr and
693701
e.getType() instanceof StringType
694702
or
@@ -705,6 +713,8 @@ module Internal {
705713
e1 = e2.(CastExpr).getExpr()
706714
or
707715
e1 = e2.(AssignExpr).getRValue()
716+
or
717+
e1 = e2.(NullCoalescingExpr).getAnOperand()
708718
}
709719

710720
/**
@@ -1405,6 +1415,10 @@ module Internal {
14051415
not preControlsDirect(g2, any(PreBasicBlocks::PreBasicBlock bb | g1 = bb.getAnElement()),
14061416
v2)
14071417
)
1418+
or
1419+
g2 = g1.(NullCoalescingExpr).getAnOperand() and
1420+
v1.(NullValue).isNull() and
1421+
v2 = v1
14081422
}
14091423

14101424
cached

‎csharp/ql/src/semmle/code/csharp/dataflow/Nullness.qll‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,11 +35,17 @@ class MaybeNullExpr extends Expr {
3535
or
3636
this instanceof AsExpr
3737
or
38-
exists(MaybeNullExpr e | G::Internal::nullValueImpliedUnary(e, this))
38+
this.(AssignExpr).getRValue() instanceof MaybeNullExpr
3939
or
40-
exists(MaybeNullExpr e | G::Internal::nullValueImpliedBinary(e, _, this))
40+
this.(Cast).getExpr() instanceof MaybeNullExpr
4141
or
42-
exists(MaybeNullExpr e | G::Internal::nullValueImpliedBinary(_, e, this))
42+
this = any(ConditionalExpr ce |
43+
ce.getThen() instanceof MaybeNullExpr
44+
or
45+
ce.getElse() instanceof MaybeNullExpr
46+
)
47+
or
48+
this.(NullCoalescingExpr).getRightOperand() instanceof MaybeNullExpr
4349
}
4450
}
4551

‎csharp/ql/test/library-tests/cil/dataflow/Nullness.expected‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
alwaysNull
22
| dataflow.cs:70:21:70:35 | default(...) |
33
| dataflow.cs:74:21:74:34 | call to method NullFunction |
4+
| dataflow.cs:74:21:74:52 | ... ?? ... |
45
| dataflow.cs:74:39:74:52 | call to method IndirectNull |
56
| dataflow.cs:78:21:78:45 | call to method ReturnsNull |
67
| dataflow.cs:79:21:79:46 | call to method ReturnsNull2 |

‎csharp/ql/test/library-tests/controlflow/guards/Implications.expected‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -257,10 +257,14 @@
257257
| Guards.cs:108:27:108:36 | access to property Property | null | Guards.cs:106:22:106:25 | null | null |
258258
| Guards.cs:113:13:114:38 | String dummy = ... | non-null | Guards.cs:113:13:113:17 | access to local variable dummy | non-null |
259259
| Guards.cs:113:13:114:38 | String dummy = ... | null | Guards.cs:113:13:113:17 | access to local variable dummy | null |
260+
| Guards.cs:113:21:114:38 | ... ?? ... | null | Guards.cs:113:21:113:45 | access to field Field | null |
261+
| Guards.cs:113:21:114:38 | ... ?? ... | null | Guards.cs:114:14:114:38 | access to field Field | null |
260262
| Guards.cs:115:9:115:55 | ... = ... | non-null | Guards.cs:115:9:115:13 | access to local variable dummy | non-null |
261263
| Guards.cs:115:9:115:55 | ... = ... | non-null | Guards.cs:115:17:115:55 | ... ?? ... | non-null |
262264
| Guards.cs:115:9:115:55 | ... = ... | null | Guards.cs:115:9:115:13 | access to local variable dummy | null |
263265
| Guards.cs:115:9:115:55 | ... = ... | null | Guards.cs:115:17:115:55 | ... ?? ... | null |
266+
| Guards.cs:115:17:115:55 | ... ?? ... | null | Guards.cs:115:17:115:41 | access to field Field | null |
267+
| Guards.cs:115:17:115:55 | ... ?? ... | null | Guards.cs:115:46:115:55 | throw ... | null |
264268
| Guards.cs:117:9:117:25 | ... = ... | non-null | Guards.cs:117:9:117:18 | access to property Property | non-null |
265269
| Guards.cs:117:9:117:25 | ... = ... | non-null | Guards.cs:117:22:117:25 | null | non-null |
266270
| Guards.cs:117:9:117:25 | ... = ... | null | Guards.cs:117:9:117:18 | access to property Property | null |

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

Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -315,6 +315,33 @@ public void Ex26(E e)
315315

316316
public bool Field;
317317
string Make() => Field ? null : "";
318+
319+
static void Ex27(string s1, string s2)
320+
{
321+
if ((s1 ?? s2) is null)
322+
{
323+
s1.ToString(); // BAD (always)
324+
s2.ToString(); // BAD (always)
325+
}
326+
}
327+
328+
static void Ex28()
329+
{
330+
var x = (string)null ?? null;
331+
x.ToString(); // BAD (always)
332+
}
333+
334+
static void Ex29(string s)
335+
{
336+
var x = s ?? "";
337+
x.ToString(); // GOOD
338+
}
339+
340+
static void Ex30(string s, object o)
341+
{
342+
var x = s ?? o as string;
343+
x.ToString(); // BAD (maybe)
344+
}
318345
}
319346

320347
public static class Extensions

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -215,6 +215,7 @@
215215
| E.cs:274:17:274:25 | ... != ... | false | E.cs:274:22:274:25 | null | E.cs:274:17:274:17 | access to local variable o |
216216
| E.cs:293:13:293:24 | ... == ... | true | E.cs:293:15:293:19 | call to method M2 | E.cs:293:24:293:24 | (...) ... |
217217
| E.cs:293:13:293:24 | ... == ... | true | E.cs:293:24:293:24 | (...) ... | E.cs:293:15:293:19 | call to method M2 |
218+
| E.cs:321:13:321:30 | ... is ... | true | E.cs:321:14:321:21 | ... ?? ... | E.cs:321:27:321:30 | null |
218219
| Forwarding.cs:59:13:59:21 | ... == ... | true | Forwarding.cs:59:13:59:13 | access to parameter o | Forwarding.cs:59:18:59:21 | null |
219220
| Forwarding.cs:59:13:59:21 | ... == ... | true | Forwarding.cs:59:18:59:21 | null | Forwarding.cs:59:13:59:13 | access to parameter o |
220221
| Forwarding.cs:78:16:78:39 | call to method ReferenceEquals | true | Forwarding.cs:78:32:78:32 | access to parameter o | Forwarding.cs:78:35:78:38 | null |

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

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1488,6 +1488,30 @@
14881488
| E.cs:317:22:317:38 | ... ? ... : ... | non-null | E.cs:317:37:317:38 | "" | non-null |
14891489
| E.cs:317:22:317:38 | ... ? ... : ... | null | E.cs:317:22:317:26 | access to field Field | true |
14901490
| E.cs:317:22:317:38 | ... ? ... : ... | null | E.cs:317:30:317:33 | null | null |
1491+
| E.cs:321:13:321:30 | ... is ... | false | E.cs:321:14:321:21 | ... ?? ... | non-null |
1492+
| E.cs:321:13:321:30 | ... is ... | true | E.cs:321:14:321:21 | ... ?? ... | null |
1493+
| E.cs:321:14:321:21 | ... ?? ... | null | E.cs:321:14:321:15 | access to parameter s1 | null |
1494+
| E.cs:321:14:321:21 | ... ?? ... | null | E.cs:321:20:321:21 | access to parameter s2 | null |
1495+
| E.cs:330:13:330:36 | String x = ... | non-null | E.cs:330:13:330:13 | access to local variable x | non-null |
1496+
| E.cs:330:13:330:36 | String x = ... | null | E.cs:330:13:330:13 | access to local variable x | null |
1497+
| E.cs:330:17:330:28 | (...) ... | non-null | E.cs:330:25:330:28 | null | non-null |
1498+
| E.cs:330:17:330:28 | (...) ... | null | E.cs:330:25:330:28 | null | null |
1499+
| E.cs:330:17:330:36 | ... ?? ... | null | E.cs:330:17:330:28 | (...) ... | null |
1500+
| E.cs:330:17:330:36 | ... ?? ... | null | E.cs:330:33:330:36 | null | null |
1501+
| E.cs:331:9:331:9 | access to local variable x | non-null | E.cs:330:17:330:36 | ... ?? ... | non-null |
1502+
| E.cs:331:9:331:9 | access to local variable x | null | E.cs:330:17:330:36 | ... ?? ... | null |
1503+
| E.cs:336:13:336:23 | String x = ... | non-null | E.cs:336:13:336:13 | access to local variable x | non-null |
1504+
| E.cs:336:13:336:23 | String x = ... | null | E.cs:336:13:336:13 | access to local variable x | null |
1505+
| E.cs:336:17:336:23 | ... ?? ... | null | E.cs:336:17:336:17 | access to parameter s | null |
1506+
| E.cs:336:17:336:23 | ... ?? ... | null | E.cs:336:22:336:23 | "" | null |
1507+
| E.cs:337:9:337:9 | access to local variable x | non-null | E.cs:336:17:336:23 | ... ?? ... | non-null |
1508+
| E.cs:337:9:337:9 | access to local variable x | null | E.cs:336:17:336:23 | ... ?? ... | null |
1509+
| E.cs:342:13:342:32 | String x = ... | non-null | E.cs:342:13:342:13 | access to local variable x | non-null |
1510+
| E.cs:342:13:342:32 | String x = ... | null | E.cs:342:13:342:13 | access to local variable x | null |
1511+
| E.cs:342:17:342:32 | ... ?? ... | null | E.cs:342:17:342:17 | access to parameter s | null |
1512+
| E.cs:342:17:342:32 | ... ?? ... | null | E.cs:342:22:342:32 | ... as ... | null |
1513+
| E.cs:343:9:343:9 | access to local variable x | non-null | E.cs:342:17:342:32 | ... ?? ... | non-null |
1514+
| E.cs:343:9:343:9 | access to local variable x | null | E.cs:342:17:342:32 | ... ?? ... | null |
14911515
| Forwarding.cs:7:16:7:23 | String s = ... | non-null | Forwarding.cs:7:16:7:16 | access to local variable s | non-null |
14921516
| Forwarding.cs:7:16:7:23 | String s = ... | null | Forwarding.cs:7:16:7:16 | access to local variable s | null |
14931517
| Forwarding.cs:9:13:9:30 | !... | false | Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | true |

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

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,9 @@
3333
| E.cs:210:16:210:16 | access to parameter s | Variable $@ is always null here. | E.cs:206:28:206:28 | s | s |
3434
| E.cs:220:13:220:13 | access to local variable x | Variable $@ is always null here. | E.cs:215:13:215:13 | x | x |
3535
| E.cs:229:13:229:13 | access to local variable x | Variable $@ is always null here. | E.cs:225:13:225:13 | x | x |
36+
| E.cs:323:13:323:14 | access to parameter s1 | Variable $@ is always null here. | E.cs:319:29:319:30 | s1 | s1 |
37+
| E.cs:324:13:324:14 | access to parameter s2 | Variable $@ is always null here. | E.cs:319:40:319:41 | s2 | s2 |
38+
| E.cs:331:9:331:9 | access to local variable x | Variable $@ is always null here. | E.cs:330:13:330:13 | x | x |
3639
| Forwarding.cs:36:31:36:31 | access to local variable s | Variable $@ is always null here. | Forwarding.cs:7:16:7:16 | s | s |
3740
| Forwarding.cs:40:27:40:27 | access to local variable s | Variable $@ is always null here. | Forwarding.cs:7:16:7:16 | s | s |
3841
| NullAlwaysBad.cs:9:30:9:30 | access to parameter s | Variable $@ is always null here. | NullAlwaysBad.cs:7:29:7:29 | s | s |

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,15 @@
208208
| E.cs:306:31:306:31 | access to field l | E.cs:306:31:306:31 | access to field l | null | true |
209209
| E.cs:309:13:309:22 | access to property HasValue | E.cs:309:13:309:13 | access to field l | false | true |
210210
| E.cs:309:13:309:22 | access to property HasValue | E.cs:309:13:309:13 | access to field l | true | false |
211+
| E.cs:321:13:321:30 | ... is ... | E.cs:321:14:321:21 | ... ?? ... | false | false |
212+
| E.cs:321:13:321:30 | ... is ... | E.cs:321:14:321:21 | ... ?? ... | true | true |
213+
| E.cs:321:14:321:15 | access to parameter s1 | E.cs:321:14:321:15 | access to parameter s1 | non-null | false |
214+
| E.cs:321:14:321:15 | access to parameter s1 | E.cs:321:14:321:15 | access to parameter s1 | null | true |
215+
| E.cs:330:17:330:28 | (...) ... | E.cs:330:17:330:28 | (...) ... | null | true |
216+
| E.cs:336:17:336:17 | access to parameter s | E.cs:336:17:336:17 | access to parameter s | non-null | false |
217+
| E.cs:336:17:336:17 | access to parameter s | E.cs:336:17:336:17 | access to parameter s | null | true |
218+
| E.cs:342:17:342:17 | access to parameter s | E.cs:342:17:342:17 | access to parameter s | non-null | false |
219+
| E.cs:342:17:342:17 | access to parameter s | E.cs:342:17:342:17 | access to parameter s | null | true |
211220
| Forwarding.cs:9:14:9:30 | call to method IsNullOrEmpty | Forwarding.cs:9:14:9:14 | access to local variable s | false | false |
212221
| Forwarding.cs:14:13:14:32 | call to method IsNotNullOrEmpty | Forwarding.cs:14:13:14:13 | access to local variable s | true | false |
213222
| 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: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,6 +348,14 @@ nodes
348348
| E.cs:285:9:285:9 | access to local variable o |
349349
| E.cs:301:13:301:27 | SSA def(s) |
350350
| E.cs:302:9:302:9 | access to local variable s |
351+
| E.cs:319:29:319:30 | SSA param(s1) |
352+
| E.cs:321:20:321:21 | access to parameter s2 |
353+
| E.cs:321:27:321:30 | null |
354+
| E.cs:323:13:323:14 | access to parameter s1 |
355+
| E.cs:330:13:330:36 | SSA def(x) |
356+
| E.cs:331:9:331:9 | access to local variable x |
357+
| E.cs:342:13:342:32 | SSA def(x) |
358+
| E.cs:343:9:343:9 | access to local variable x |
351359
| Forwarding.cs:7:16:7:23 | SSA def(s) |
352360
| Forwarding.cs:14:9:17:9 | if (...) ... |
353361
| Forwarding.cs:19:9:22:9 | if (...) ... |
@@ -678,6 +686,11 @@ edges
678686
| E.cs:283:13:283:22 | [b (line 279): false] SSA def(o) | E.cs:285:9:285:9 | access to local variable o |
679687
| E.cs:283:13:283:22 | [b (line 279): true] SSA def(o) | E.cs:285:9:285:9 | access to local variable o |
680688
| E.cs:301:13:301:27 | SSA def(s) | E.cs:302:9:302:9 | access to local variable s |
689+
| E.cs:319:29:319:30 | SSA param(s1) | E.cs:321:20:321:21 | access to parameter s2 |
690+
| E.cs:321:20:321:21 | access to parameter s2 | E.cs:321:27:321:30 | null |
691+
| E.cs:321:27:321:30 | null | E.cs:323:13:323:14 | access to parameter s1 |
692+
| E.cs:330:13:330:36 | SSA def(x) | E.cs:331:9:331:9 | access to local variable x |
693+
| E.cs:342:13:342:32 | SSA def(x) | E.cs:343:9:343:9 | access to local variable x |
681694
| Forwarding.cs:7:16:7:23 | SSA def(s) | Forwarding.cs:14:9:17:9 | if (...) ... |
682695
| Forwarding.cs:14:9:17:9 | if (...) ... | Forwarding.cs:19:9:22:9 | if (...) ... |
683696
| Forwarding.cs:19:9:22:9 | if (...) ... | Forwarding.cs:24:9:27:9 | if (...) ... |
@@ -778,6 +791,7 @@ edges
778791
| E.cs:285:9:285:9 | access to local variable o | E.cs:283:13:283:22 | [b (line 279): false] SSA def(o) | E.cs:285:9:285:9 | access to local variable o | Variable $@ may be null here as suggested by $@ null check. | E.cs:283:13:283:13 | o | o | E.cs:284:9:284:9 | access to local variable o | this |
779792
| E.cs:285:9:285:9 | access to local variable o | E.cs:283:13:283:22 | [b (line 279): true] SSA def(o) | E.cs:285:9:285:9 | access to local variable o | Variable $@ may be null here as suggested by $@ null check. | E.cs:283:13:283:13 | o | o | E.cs:284:9:284:9 | access to local variable o | this |
780793
| E.cs:302:9:302:9 | access to local variable s | E.cs:301:13:301:27 | SSA def(s) | E.cs:302:9:302:9 | access to local variable s | Variable $@ may be null here because of $@ assignment. | E.cs:301:13:301:13 | s | s | E.cs:301:13:301:27 | String s = ... | this |
794+
| E.cs:343:9:343:9 | access to local variable x | E.cs:342:13:342:32 | SSA def(x) | E.cs:343:9:343:9 | access to local variable x | Variable $@ may be null here because of $@ assignment. | E.cs:342:13:342:13 | x | x | E.cs:342:13:342:32 | String x = ... | this |
781795
| 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 here because of $@ assignment. | GuardedString.cs:7:16:7:16 | s | s | GuardedString.cs:7:16:7:32 | String s = ... | this |
782796
| 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 here because of $@ null argument. | NullMaybeBad.cs:5:25:5:25 | o | o | NullMaybeBad.cs:13:17:13:20 | null | this |
783797
| StringConcatenation.cs:16:17:16:17 | access to local variable s | StringConcatenation.cs:14:16:14:23 | SSA def(s) | StringConcatenation.cs:16:17:16:17 | access to local variable s | Variable $@ may be null here because of $@ assignment. | StringConcatenation.cs:14:16:14:16 | s | s | StringConcatenation.cs:14:16:14:23 | String s = ... | this |

0 commit comments

Comments
 (0)