Skip to content

Commit b31cd8a

Browse files
calumgrantcalumgrant
authored
Merge pull request github#1982 from hvitved/csharp/null-maybe-dynamic
C#: Remove false positives from `cs/dereferenced-value-may-be-null`
2 parents 4780952 + fb68d83 commit b31cd8a

File tree

10 files changed

+161
-4
lines changed

10 files changed

+161
-4
lines changed

‎change-notes/1.23/analysis-csharp.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ The following changes in version 1.23 affect C# analysis in all applications.
1515

1616
| **Query** | **Expected impact** | **Change** |
1717
|------------------------------|------------------------|-----------------------------------|
18+
| Dereferenced variable may be null (`cs/dereferenced-value-may-be-null`) | Fewer false positive results | More `null` checks are now taken into account, including `null` checks for `dynamic` expressions and `null` checks such as `object alwaysNull = null; if (x != alwaysNull) ...`. |
1819

1920
## Removal of old queries
2021

‎csharp/ql/src/semmle/code/csharp/commons/ComparisonTest.qll

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,10 @@ private newtype TComparisonTest =
121121
)
122122
} or
123123
TComparisonOperatorCall(OperatorCall oc, ComparisonKind kind, Expr first, Expr second) {
124-
exists(Operator o | o = oc.getTarget() |
124+
exists(Operator o |
125+
o = oc.getTarget() or
126+
o.getName() = oc.(DynamicOperatorCall).getLateBoundTargetName()
127+
|
125128
o instanceof EQOperator and
126129
kind.isEquality() and
127130
first = oc.getArgument(0) and

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -277,11 +277,12 @@ class DereferenceableExpr extends Expr {
277277
private Expr getABooleanNullCheck(BooleanValue v, boolean isNull) {
278278
exists(boolean branch | branch = v.getValue() |
279279
// Comparison with `null`, for example `x != null`
280-
exists(ComparisonTest ct, ComparisonKind ck, NullLiteral nl |
280+
exists(ComparisonTest ct, ComparisonKind ck, Expr e |
281281
ct.getExpr() = result and
282282
ct.getAnArgument() = this and
283-
ct.getAnArgument() = nl and
284-
this != nl and
283+
ct.getAnArgument() = e and
284+
e = any(NullValue nv | nv.isNull()).getAnExpr() and
285+
this != e and
285286
ck = ct.getComparisonKind()
286287
|
287288
ck.isEquality() and isNull = branch

‎csharp/ql/test/library-tests/commons/ComparisonTest/ComparisonTest.cs

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,4 +106,16 @@ void M(int x, int y)
106106
b = x.CompareTo(y).CompareTo(0).CompareTo(1) == 0;
107107
}
108108
}
109+
110+
void DynamicComparisons(object o1, object o2)
111+
{
112+
dynamic d1 = o1;
113+
dynamic d2 = o2;
114+
var b = d1 == d2;
115+
b = d1 != d2;
116+
b = d1 > d2;
117+
b = d1 < d2;
118+
b = d1 >= d2;
119+
b = d1 <= d2;
120+
}
109121
}

‎csharp/ql/test/library-tests/commons/ComparisonTest/comparisonTest.expected

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -62,3 +62,9 @@
6262
| ComparisonTest.cs:106:17:106:61 | ... == ... | < | ComparisonTest.cs:106:42:106:42 | 0 | ComparisonTest.cs:106:17:106:30 | call to method CompareTo |
6363
| ComparisonTest.cs:106:17:106:61 | ... == ... | = | ComparisonTest.cs:106:17:106:43 | call to method CompareTo | ComparisonTest.cs:106:55:106:55 | 1 |
6464
| ComparisonTest.cs:106:17:106:61 | ... == ... | = | ComparisonTest.cs:106:17:106:56 | call to method CompareTo | ComparisonTest.cs:106:61:106:61 | 0 |
65+
| ComparisonTest.cs:114:17:114:24 | dynamic call to operator == | = | ComparisonTest.cs:114:17:114:18 | access to local variable d1 | ComparisonTest.cs:114:23:114:24 | access to local variable d2 |
66+
| ComparisonTest.cs:115:13:115:20 | dynamic call to operator != | != | ComparisonTest.cs:115:13:115:14 | access to local variable d1 | ComparisonTest.cs:115:19:115:20 | access to local variable d2 |
67+
| ComparisonTest.cs:116:13:116:19 | dynamic call to operator > | < | ComparisonTest.cs:116:18:116:19 | access to local variable d2 | ComparisonTest.cs:116:13:116:14 | access to local variable d1 |
68+
| ComparisonTest.cs:117:13:117:19 | dynamic call to operator < | < | ComparisonTest.cs:117:13:117:14 | access to local variable d1 | ComparisonTest.cs:117:18:117:19 | access to local variable d2 |
69+
| ComparisonTest.cs:118:13:118:20 | dynamic call to operator >= | <= | ComparisonTest.cs:118:19:118:20 | access to local variable d2 | ComparisonTest.cs:118:13:118:14 | access to local variable d1 |
70+
| ComparisonTest.cs:119:13:119:20 | dynamic call to operator <= | <= | ComparisonTest.cs:119:13:119:14 | access to local variable d1 | ComparisonTest.cs:119:19:119:20 | access to local variable d2 |

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

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -342,6 +342,26 @@ static void Ex30(string s, object o)
342342
var x = s ?? o as string;
343343
x.ToString(); // BAD (maybe)
344344
}
345+
346+
static void Ex31(string s, object o)
347+
{
348+
dynamic x = s ?? o as string;
349+
x.ToString(); // BAD (maybe)
350+
}
351+
352+
static void Ex32(string s, object o)
353+
{
354+
dynamic x = s ?? o as string;
355+
if (x != null)
356+
x.ToString(); // GOOD
357+
}
358+
359+
static void Ex33(string s, object o)
360+
{
361+
var x = s ?? o as string;
362+
if (x != (string)null)
363+
x.ToString(); // GOOD
364+
}
345365
}
346366

347367
public static class Extensions

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -216,6 +216,10 @@
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 |
218218
| E.cs:321:13:321:30 | ... is ... | true | E.cs:321:14:321:21 | ... ?? ... | E.cs:321:27:321:30 | null |
219+
| E.cs:355:13:355:21 | dynamic call to operator != | false | E.cs:355:13:355:13 | access to local variable x | E.cs:355:18:355:21 | null |
220+
| E.cs:355:13:355:21 | dynamic call to operator != | false | E.cs:355:18:355:21 | null | E.cs:355:13:355:13 | access to local variable x |
221+
| E.cs:362:13:362:29 | ... != ... | false | E.cs:362:13:362:13 | access to local variable x | E.cs:362:18:362:29 | (...) ... |
222+
| E.cs:362:13:362:29 | ... != ... | false | E.cs:362:18:362:29 | (...) ... | E.cs:362:13:362:13 | access to local variable x |
219223
| Forwarding.cs:59:13:59:21 | ... == ... | true | Forwarding.cs:59:13:59:13 | access to parameter o | Forwarding.cs:59:18:59:21 | null |
220224
| Forwarding.cs:59:13:59:21 | ... == ... | true | Forwarding.cs:59:18:59:21 | null | Forwarding.cs:59:13:59:13 | access to parameter o |
221225
| 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 |

0 commit comments

Comments
 (0)