Skip to content

C#: Improve cs/dereference-* queries and add to the Code Quality suite. #19589

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.

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
merged 8 commits into from
Jun 11, 2025
Merged
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
@@ -2,6 +2,8 @@ ql/csharp/ql/src/API Abuse/CallToGCCollect.ql
ql/csharp/ql/src/API Abuse/FormatInvalid.ql
ql/csharp/ql/src/API Abuse/NoDisposeCallOnLocalIDisposable.ql
ql/csharp/ql/src/Bad Practices/Control-Flow/ConstantCondition.ql
ql/csharp/ql/src/CSI/NullAlways.ql
ql/csharp/ql/src/CSI/NullMaybe.ql
ql/csharp/ql/src/Dead Code/DeadStoreOfLocal.ql
ql/csharp/ql/src/Language Abuse/MissedReadonlyOpportunity.ql
ql/csharp/ql/src/Likely Bugs/Collections/ContainerLengthCmpOffByOne.ql
9 changes: 7 additions & 2 deletions csharp/ql/lib/semmle/code/csharp/dataflow/Nullness.qll
Original file line number Diff line number Diff line change
@@ -544,8 +544,13 @@ class Dereference extends G::DereferenceableExpr {
p.hasExtensionMethodModifier() and
not emc.isConditional()
|
p.fromSource() // assume all non-source extension methods perform a dereference
implies
// Assume all non-source extension methods on
// (1) nullable types are null-safe
// (2) non-nullable types are doing a dereference.
p.fromLibrary() and
not p.getAnnotatedType().isNullableRefType()
or
p.fromSource() and
exists(
Ssa::ImplicitParameterDefinition def,
AssignableDefinitions::ImplicitParameterDefinition pdef
Original file line number Diff line number Diff line change
@@ -41,9 +41,7 @@ class SystemDiagnosticsDebugClass extends SystemDiagnosticsClass {
/** Gets an `Assert(bool, ...)` method. */
Method getAssertMethod() {
result.getDeclaringType() = this and
result.hasName("Assert") and
result.getParameter(0).getType() instanceof BoolType and
result.getReturnType() instanceof VoidType
result.hasName("Assert")
}
}

1 change: 1 addition & 0 deletions csharp/ql/src/CSI/NullAlways.ql
Original file line number Diff line number Diff line change
@@ -9,6 +9,7 @@
* correctness
* exceptions
* external/cwe/cwe-476
* quality
*/

import csharp
1 change: 1 addition & 0 deletions csharp/ql/src/CSI/NullMaybe.ql
Original file line number Diff line number Diff line change
@@ -10,6 +10,7 @@
* correctness
* exceptions
* external/cwe/cwe-476
* quality
*/

import csharp
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
---
category: minorAnalysis
---
* The queries `cs/dereferenced-value-is-always-null` and `cs/dereferenced-value-may-be-null` have been improved to reduce false positives. The queries no longer assume that expressions are dereferenced when passed as the receiver (`this` parameter) to extension methods where that parameter is a nullable type.
14 changes: 7 additions & 7 deletions csharp/ql/test/query-tests/Nullness/A.cs
Original file line number Diff line number Diff line change
@@ -5,7 +5,7 @@ class A
public void Lock()
{
object synchronizedAlways = null;
lock (synchronizedAlways) // BAD (always)
lock (synchronizedAlways) // $ Alert[cs/dereferenced-value-is-always-null]
{
synchronizedAlways.GetHashCode(); // GOOD
}
@@ -14,7 +14,7 @@ public void Lock()
public void ArrayAssignTest()
{
int[] arrayNull = null;
arrayNull[0] = 10; // BAD (always)
arrayNull[0] = 10; // $ Alert[cs/dereferenced-value-is-always-null]

int[] arrayOk;
arrayOk = new int[10];
@@ -28,10 +28,10 @@ public void Access()
object methodAccess = null;
object methodCall = null;

Console.WriteLine(arrayAccess[1]); // BAD (always)
Console.WriteLine(fieldAccess.Length); // BAD (always)
Func<String> tmp = methodAccess.ToString; // BAD (always)
Console.WriteLine(methodCall.ToString()); // BAD (always)
Console.WriteLine(arrayAccess[1]); // $ Alert[cs/dereferenced-value-is-always-null]
Console.WriteLine(fieldAccess.Length); // $ Alert[cs/dereferenced-value-is-always-null]
Func<String> tmp = methodAccess.ToString; // $ Alert[cs/dereferenced-value-is-always-null]
Console.WriteLine(methodCall.ToString()); // $ Alert[cs/dereferenced-value-is-always-null]

Console.WriteLine(arrayAccess[1]); // GOOD
Console.WriteLine(fieldAccess.Length); // GOOD
@@ -47,7 +47,7 @@ public void OutOrRef()

object varRef = null;
TestMethod2(ref varRef);
varRef.ToString(); // BAD (always)
varRef.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

varRef = null;
TestMethod3(ref varRef);
10 changes: 5 additions & 5 deletions csharp/ql/test/query-tests/Nullness/Assert.cs
Original file line number Diff line number Diff line change
@@ -12,23 +12,23 @@ void Fn(bool b)

s = b ? null : "";
Assert.IsNull(s);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsNotNull(s);
Console.WriteLine(s.Length); // GOOD

s = b ? null : "";
Assert.IsTrue(s == null);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsTrue(s != null);
Console.WriteLine(s.Length); // GOOD

s = b ? null : "";
Assert.IsFalse(s != null);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsFalse(s == null);
@@ -44,10 +44,10 @@ void Fn(bool b)

s = b ? null : "";
Assert.IsTrue(s == null && b);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]

s = b ? null : "";
Assert.IsFalse(s != null || !b);
Console.WriteLine(s.Length); // BAD (always)
Console.WriteLine(s.Length); // $ Alert[cs/dereferenced-value-is-always-null]
}
}
4 changes: 2 additions & 2 deletions csharp/ql/test/query-tests/Nullness/B.cs
Original file line number Diff line number Diff line change
@@ -10,7 +10,7 @@ public void OperatorCall()
B neqCallAlways = null;

if (eqCallAlways == null)
eqCallAlways.ToString(); // BAD (always)
eqCallAlways.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

if (b2 != null)
b2.ToString(); // GOOD
@@ -21,7 +21,7 @@ public void OperatorCall()

if (neqCallAlways != null) { }
else
neqCallAlways.ToString(); // BAD (always)
neqCallAlways.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}

public static bool operator ==(B b1, B b2)
58 changes: 29 additions & 29 deletions csharp/ql/test/query-tests/Nullness/C.cs
Original file line number Diff line number Diff line change
@@ -15,7 +15,7 @@ public void NotTest()

if (!(o != null))
{
o.GetHashCode(); // BAD (always)
o.GetHashCode(); // $ Alert[cs/dereferenced-value-is-always-null]
}
}

@@ -39,7 +39,7 @@ public void AssertTest()
{
var s = Maybe() ? null : "";
Debug.Assert(s == null);
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

s = Maybe() ? null : "";
Debug.Assert(s != null);
@@ -50,22 +50,22 @@ public void AssertNullTest()
{
var o1 = new object();
AssertNull(o1);
o1.ToString(); // BAD (always) (false negative)
o1.ToString(); // $ MISSING: Alert[cs/dereferenced-value-is-always-null]

var o2 = Maybe() ? null : "";
Assert.IsNull(o2);
o2.ToString(); // BAD (always)
o2.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}

public void AssertNotNullTest()
{
var o1 = Maybe() ? null : new object();
var o1 = Maybe() ? null : new object(); // $ Source[cs/dereferenced-value-may-be-null]
AssertNonNull(o1);
o1.ToString(); // GOOD (false positive)
o1.ToString(); // $ SPURIOUS (false positive): Alert[cs/dereferenced-value-may-be-null]

var o2 = Maybe() ? null : new object();
var o2 = Maybe() ? null : new object(); // $ Source[cs/dereferenced-value-may-be-null]
AssertNonNull(o1);
o2.ToString(); // BAD (maybe)
o2.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]

var o3 = Maybe() ? null : new object();
Assert.IsNotNull(o3);
@@ -91,16 +91,16 @@ public void InstanceOf()

public void Lock()
{
var o = Maybe() ? null : new object();
lock (o) // BAD (maybe)
var o = Maybe() ? null : new object(); // $ Source[cs/dereferenced-value-may-be-null]
lock (o) // $ Alert[cs/dereferenced-value-may-be-null]
o.ToString(); // GOOD
}

public void Foreach(IEnumerable<int> list)
{
if (Maybe())
list = null;
foreach (var x in list) // BAD (maybe)
list = null; // $ Source[cs/dereferenced-value-may-be-null]
foreach (var x in list) // $ Alert[cs/dereferenced-value-may-be-null]
{
x.ToString(); // GOOD
list.ToString(); // GOOD
@@ -159,23 +159,23 @@ public void DoWhile()
s = null;
do
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
s = null;
}
while (s != null);

s = null;
do
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}
while (s != null);

s = "";
do
{
s.ToString(); // BAD (maybe)
s = null;
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
s = null; // $ Source[cs/dereferenced-value-may-be-null]
}
while (true);
}
@@ -193,15 +193,15 @@ public void While()
s = null;
while (b)
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
s = null;
}

s = "";
while (true)
{
s.ToString(); // BAD (maybe)
s = null;
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
s = null; // $ Source[cs/dereferenced-value-may-be-null]
}
}

@@ -215,12 +215,12 @@ public void If()
}

if (s == null)
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

s = "";
if (s != null && s.Length % 2 == 0)
s = null;
s.ToString(); // BAD (maybe)
s = null; // $ Source[cs/dereferenced-value-may-be-null]
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
}

public void For()
@@ -230,23 +230,23 @@ public void For()
{
s.ToString(); // GOOD
}
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]

for (s = null; s == null; s = null)
{
s.ToString(); // BAD (always)
s.ToString(); // $ Alert[cs/dereferenced-value-is-always-null]
}

for (s = ""; ; s = null)
for (s = ""; ; s = null) // $ Source[cs/dereferenced-value-may-be-null]
{
s.ToString(); // BAD (maybe)
s.ToString(); // $ Alert[cs/dereferenced-value-may-be-null]
}
}

public void ArrayAssignTest()
{
int[] a = null;
a[0] = 10; // BAD (always)
a[0] = 10; // $ Alert[cs/dereferenced-value-is-always-null]

a = new int[10];
a[0] = 42; // GOOD
@@ -257,8 +257,8 @@ public void Access()
int[] ia = null;
string[] sa = null;

ia[1] = 0; // BAD (always)
var temp = sa.Length; // BAD (always)
ia[1] = 0; // $ Alert[cs/dereferenced-value-is-always-null]
var temp = sa.Length; // $ Alert[cs/dereferenced-value-is-always-null]

ia[1] = 0; // BAD (always), but not first
temp = sa.Length; // BAD (always), but not first
Loading
Oops, something went wrong.