github / github/codeql

Potential false positives - cs/dereferenced-value-may-be-null - when working with C# 8 nullability feature

Open
#2,774 2 comments 0 reactions 0 assignees View on GitHub
C# question
Dominant language
CodeQL
Stars
10.1k
Forks
2.1k
Avg merge
2d 15h
Merged PRs (30d)
141

Description

The recently introduced nullability analysis feature in C# 8 is expressive enough to capture cases where a method that is known to not return can be annotated as such (for instance if it is throwing).
See the following for reference: https://github.com/dotnet/roslyn/blob/3cef228f7a9c1991fd8e1e3fa59403d2ae682a96/docs/features/nullable-reference-types.md

Specifically: `cs/dereferenced-value-may-be-null` (https://lgtm.com/rules/1506091056882/)

With that, certain lgtm alerts around nullability can be false positives. Once such example is the following:
https://lgtm.com/projects/g/dotnet/corefx/latest/files/src/System.Text.Json/src/System/Text/Json/Serialization/JsonPropertyInfoNotNullable.cs?sort=name&dir=ASC&mode=heatmap

Note that a null check already exists which calls a method that will throw.
https://github.com/dotnet/runtime/blob/c44edc19fc6c4792b002ac115e72d2f7805ade6d/src/libraries/System.Text.Json/src/System/Text/Json/ThrowHelper.Serialization.cs#L40-L47

The simplified pattern that we sometimes use in super perf sensitive code (where the readability trade off makes sense) is the following:
```C#
public int Foo(string s)
{
if (s == null)
{
CallThrowHelper(); // This method does not return;
}
return s.Length; // This is guaranteed to not be null.
}

[DoesNotReturn]
private void CallThrowHelper()
{
throw new ArgumentNullException();
}
```

Even outside of throwing, I can imagine this type of nullability analysis being leveraged by any C# developer and hence the lgtm alerts false positive rate might become higher.

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.