Have you ever wanted to get rid of the null reference dereferencing problem? If so, then using Nullable Reference types is not your choice. I wonder why? This is what will be discussed today.
We warned and it happened. About a year ago, my colleagues wrote an article warning that introducing Nullable Reference types would not protect against null reference dereferencing. We now have real confirmation of our words, which was found in the depths of Roslyn.
Nullable Reference types
The very idea of adding Nullable Reference (hereinafter - NR) types seems interesting to me, since the problem associated with dereferencing null references is relevant to this day. The implementation of the protection against dereferencing is extremely unreliable. As planned by the creators assume the value null may only those variables whose type is marked with a "?". For example, a variable of type string? says that it can contain null , of type string - on the contrary.
However, nobody prohibits us from passing null to non-nullable reference variables anyway.(hereinafter - NNR) types, because they are not implemented at the level of IL code. The static analyzer built into the compiler is responsible for this limitation. Therefore, this innovation is rather advisory in nature. Here's a simple example to show how it works:
#nullable enable
object? nullable = null;
object nonNullable = nullable;
var deref = nonNullable.ToString();As we can see, the type of nonNullable is specified as NNR, but we can safely pass null there . Of course, we will get a warning about converting "Converting null literal or possible null value to non-nullable type." However, this can be circumvented by adding a little aggression:
#nullable enable
object? nullable = null;
object nonNullable = nullable!; // <=
var deref = nonNullable.ToString();One exclamation point and there are no warnings. If one of you is a gourmet, then another option is available:
#nullable enable
object nonNullable = null!;
var deref = nonNullable.ToString();Well, one more example. We create two simple console projects. In the first, we write:
namespace NullableTests
{
public static class Tester
{
public static string RetNull() => null;
}
}In the second, we write:
#nullable enable
namespace ConsoleApp1
{
class Program
{
static void Main(string[] args)
{
string? nullOrNotNull = NullableTests.Tester.RetNull();
System.Console.WriteLine(nullOrNotNull.Length);
}
}
}Hover over nullOrNotNull and see the following message:
We are told that the string cannot be null here . However, we understand that it will be null here . We start the project and get an exception:
Of course, these are just synthetic examples, the purpose of which is to show that this introduction does not guarantee you protection against null reference dereferencing. If you thought that synthetics are boring, and where there are real examples at all, then I ask you not to worry, then all this will be.
NR types have another problem - it is not clear whether they are included or not. For example, the solution has two projects. One is marked up with this syntax, and the other is not. Having entered a project with NR types, you can decide that once only one is marked, then all are marked. However, this will not be the case. It turns out that you need to look every time if the nullable context is included in the project or file. Otherwise, you might mistakenly think that the normal reference type is NNR.
How the evidence was found
When developing new diagnostics in the PVS-Studio analyzer, we always test them on our base of real projects. It helps in various aspects. For instance:
- see "live" at the quality of the warnings received;
- get rid of some of the false positives;
- find interesting points in the code, which you can then talk about;
- etc.
One of the new V3156 diagnostics found places where exceptions can be thrown due to potential null . The wording of the diagnostic rule is: "The argument of the method is not expected to be null". Its essence is that the method does not expect null , in value can be passed as an argument to null . This can lead, for example, to an exception or incorrect execution of the called method. You can read more about this diagnostic rule here .
Proofs here
So we got to the main part of this article. Here you will see real code fragments from the Roslyn project, for which the diagnostics issued warnings. Their main meaning is that either the NNR type is passed null , or there is no check for the value of the NR type. All of this can lead to an exception being thrown.
Example 1
private static Dictionary<object, SourceLabelSymbol>
BuildLabelsByValue(ImmutableArray<LabelSymbol> labels)
{
....
object key;
var constantValue = label.SwitchCaseLabelConstant;
if ((object)constantValue != null && !constantValue.IsBad)
{
key = KeyForConstant(constantValue);
}
else if (labelKind == SyntaxKind.DefaultSwitchLabel)
{
key = s_defaultKey;
}
else
{
key = label.IdentifierNodeOrToken.AsNode();
}
if (!map.ContainsKey(key)) // <=
{
map.Add(key, label);
}
....
}V3156 The first argument of the 'ContainsKey' method is not expected to be null. Potential null value: key. SwitchBinder.cs 121 The
message states that key is potential null . Let's see where this variable can get such a value. Let's check the KeyForConstant method first :
protected static object KeyForConstant(ConstantValue constantValue)
{
Debug.Assert((object)constantValue != null);
return constantValue.IsNull ? s_nullKey : constantValue.Value;
}
private static readonly object s_nullKey = new object();Since s_nullKey is not null , let 's see what constantValue.Value returns :
public object? Value
{
get
{
switch (this.Discriminator)
{
case ConstantValueTypeDiscriminator.Bad: return null; // <=
case ConstantValueTypeDiscriminator.Null: return null; // <=
case ConstantValueTypeDiscriminator.SByte: return Boxes.Box(SByteValue);
case ConstantValueTypeDiscriminator.Byte: return Boxes.Box(ByteValue);
case ConstantValueTypeDiscriminator.Int16: return Boxes.Box(Int16Value);
....
default: throw ExceptionUtilities.UnexpectedValue(this.Discriminator);
}
}
}There are two null literals here, but in this case we will not go into any case with them. This is due to the IsBad and IsNull checks . However, I would like to draw your attention to the return type of this property. It is an NR type, but the KeyForConstant method already returns an NNR type. It turns out that, in general, the KeyForConstant method can return null . Another source that can return null is the AsNode method :
public SyntaxNode? AsNode()
{
if (_token != null)
{
return null;
}
return _nodeOrParent;
}Again, please pay attention to the return type of the method - it is an NR type. It turns out that when we say that null can be returned from the method , this does not affect anything. The interesting thing is that the compiler does not swear at converting from NR to NNR here:
Example 2
private SyntaxNode CopyAnnotationsTo(SyntaxNode sourceTreeRoot,
SyntaxNode destTreeRoot)
{
var nodeOrTokenMap = new Dictionary<SyntaxNodeOrToken,
SyntaxNodeOrToken>();
....
if (sourceTreeNodeOrTokenEnumerator.Current.IsNode)
{
var oldNode = destTreeNodeOrTokenEnumerator.Current.AsNode();
var newNode = sourceTreeNodeOrTokenEnumerator.Current.AsNode()
.CopyAnnotationsTo(oldNode);
nodeOrTokenMap.Add(oldNode, newNode); // <=
}
....
}V3156 The first argument of the 'Add' method is not expected to be null. Potential null value: oldNode. SyntaxAnnotationTests.cs 439
Another example with the AsNode function described above. Only this time oldNode will be of type NR. Whereas the above key was of type NNR.
By the way, I cannot but share with you an interesting observation. As I described above, when developing diagnostics, we test it on different projects. When checking the positives of this rule, a curious moment was noticed. About 70% of all warnings were issued to methods of the Dictionary class . Moreover, most of them fell on the TryGetValue method... Perhaps this is due to the fact that subconsciously we do not expect exceptions from a method that contains the word try . So check your code for this pattern to see if you find something similar.
Example 3
private static SymbolTreeInfo TryReadSymbolTreeInfo(
ObjectReader reader,
Checksum checksum,
Func<string, ImmutableArray<Node>,
Task<SpellChecker>> createSpellCheckerTask)
{
....
var typeName = reader.ReadString();
var valueCount = reader.ReadInt32();
for (var j = 0; j < valueCount; j++)
{
var containerName = reader.ReadString();
var name = reader.ReadString();
simpleTypeNameToExtensionMethodMap.Add(typeName, // <=
new ExtensionMethodInfo(containerName, name));
}
....
}V3156 The first argument of the 'Add' method is passed as an argument to the 'TryGetValue' method and is not expected to be null. Potential null value: typeName. SymbolTreeInfo_Serialization.cs 255
The analyzer says that the problem lies in the typeName . Let's first make sure that this argument is indeed potential null . We look at ReadString :
public string ReadString() => ReadStringValue();So, look at ReadStringValue :
private string ReadStringValue()
{
var kind = (EncodingKind)_reader.ReadByte();
return kind == EncodingKind.Null ? null : ReadStringValue(kind);
}Great, now let's refresh our memory by looking at where our variable was passed to:
simpleTypeNameToExtensionMethodMap.Add(typeName, // <=
new ExtensionMethodInfo(containerName,
name));I think it's time to go inside the Add method :
public bool Add(K k, V v)
{
ValueSet updated;
if (_dictionary.TryGetValue(k, out ValueSet set)) // <=
{
....
}
....
}Indeed, if null is passed to the Add method as the first argument , then we will get an ArgumentNullException . By the way, it is interesting that if we hover the cursor over typeName in Visual Studio , we will see that its type is string? :
In this case, the return type of the method is simply string :
In this case, if you further create a variable of type NNR and assign it typeName , then no error will be displayed.
Let's try to drop Roslyn
Not for malice, but for fun, I suggest trying to reproduce one of the examples shown.
Test 1
Let's take the example described under number 3:
private static SymbolTreeInfo TryReadSymbolTreeInfo(
ObjectReader reader,
Checksum checksum,
Func<string, ImmutableArray<Node>,
Task<SpellChecker>> createSpellCheckerTask)
{
....
var typeName = reader.ReadString();
var valueCount = reader.ReadInt32();
for (var j = 0; j < valueCount; j++)
{
var containerName = reader.ReadString();
var name = reader.ReadString();
simpleTypeNameToExtensionMethodMap.Add(typeName, // <=
new ExtensionMethodInfo(containerName, name));
}
....
}To reproduce it, you need to call the TryReadSymbolTreeInfo method , but it is private . It's good that the class with it has a ReadSymbolTreeInfo_ForTestingPurposesOnly method , which is already internal :
internal static SymbolTreeInfo ReadSymbolTreeInfo_ForTestingPurposesOnly(
ObjectReader reader,
Checksum checksum)
{
return TryReadSymbolTreeInfo(reader, checksum,
(names, nodes) => Task.FromResult(
new SpellChecker(checksum,
nodes.Select(n => new StringSlice(names,
n.NameSpan)))));
}It is very pleasant that we are directly offered to test the TryReadSymbolTreeInfo method . Therefore, let's create our class side by side and write the following code:
public class CheckNNR
{
public static void Start()
{
using var stream = new MemoryStream();
using var writer = new BinaryWriter(stream);
writer.Write((byte)170);
writer.Write((byte)9);
writer.Write((byte)0);
writer.Write(0);
writer.Write(0);
writer.Write(1);
writer.Write((byte)0);
writer.Write(1);
writer.Write((byte)0);
writer.Write((byte)0);
stream.Position = 0;
using var reader = ObjectReader.TryGetReader(stream);
var checksum = Checksum.Create("val");
SymbolTreeInfo.ReadSymbolTreeInfo_ForTestingPurposesOnly(reader, checksum);
}
}Now we collect Roslyn , create a simple console application, connect all the necessary dll files and write the following code:
static void Main(string[] args)
{
CheckNNR.Start();
}We launch, we reach the required place and see:
Next, go to the Add method and get the expected exception:
Let me remind you that the ReadString method returns an NNR type, which, by design, cannot contain null . This example once again confirms the relevance of the PVS-Studio diagnostic rules for searching for dereferencing of null references.
Test 2
Well, since we have already started to reproduce examples, why not reproduce one more. This example will not be related to NR types. However, it was found by the same V3156 diagnostics, and I wanted to tell you about it. Here is the code:
public SyntaxToken GenerateUniqueName(SemanticModel semanticModel,
SyntaxNode location,
SyntaxNode containerOpt,
string baseName,
CancellationToken cancellationToken)
{
return GenerateUniqueName(semanticModel,
location,
containerOpt,
baseName,
filter: null,
usedNames: null, // <=
cancellationToken);
}V3156 The sixth argument of the 'GenerateUniqueName' method is passed as an argument to the 'Concat' method and is not expected to be null. Potential null value: null. AbstractSemanticFactsService.cs 24
I'll be honest: when making this diagnostics, I didn't really expect any positives on the straight line null . After all, it is rather strange to send null to a method that will throw an exception because of this. Although I have seen places where this was justified (for example, with the Expression class ), but now is not about that.
Therefore, I was very intrigued when I saw this warning. Let's see what happens in the GenerateUniqueName method .
public SyntaxToken GenerateUniqueName(SemanticModel semanticModel,
SyntaxNode location,
SyntaxNode containerOpt,
string baseName,
Func<ISymbol, bool> filter,
IEnumerable<string> usedNames,
CancellationToken cancellationToken)
{
var container = containerOpt ?? location
.AncestorsAndSelf()
.FirstOrDefault(a => SyntaxFacts.IsExecutableBlock(a)
|| SyntaxFacts.IsMethodBody(a));
var candidates = GetCollidableSymbols(semanticModel,
location,
container,
cancellationToken);
var filteredCandidates = filter != null ? candidates.Where(filter)
: candidates;
return GenerateUniqueName(baseName,
filteredCandidates.Select(s => s.Name)
.Concat(usedNames)); // <=
}We see that there is only one exit from the method, no exceptions are thrown, and no goto . In other words, nothing prevents you from passing usedNames to the Concat method and getting an ArgumentNullException .
But these are all words, let's do it. To do this, look where you can call this method. The method itself is in the AbstractSemanticFactsService class . The class is abstract, so for convenience, let's take the CSharpSemanticFactsService class , which inherits from it. In the file of this class, we will create our own, which will call the GenerateUniqueName method . It looks like this:
public class DropRoslyn
{
private const string ProgramText =
@"using System;
using System.Collections.Generic;
using System.Text
namespace HelloWorld
{
class Program
{
static void Main(string[] args)
{
Console.WriteLine(""Hello, World!"");
}
}
}";
public void Drop()
{
var tree = CSharpSyntaxTree.ParseText(ProgramText);
var instance = CSharpSemanticFactsService.Instance;
var compilation = CSharpCompilation
.Create("Hello World")
.AddReferences(MetadataReference
.CreateFromFile(typeof(string)
.Assembly
.Location))
.AddSyntaxTrees(tree);
var semanticModel = compilation.GetSemanticModel(tree);
var syntaxNode1 = tree.GetRoot();
var syntaxNode2 = tree.GetRoot();
var baseName = "baseName";
var cancellationToken = new CancellationToken();
instance.GenerateUniqueName(semanticModel,
syntaxNode1,
syntaxNode2,
baseName,
cancellationToken);
}
}Now we collect Roslyn, create a simple console application, connect all the necessary dll files and write the following code:
class Program
{
static void Main(string[] args)
{
DropRoslyn dropRoslyn = new DropRoslyn();
dropRoslyn.Drop();
}
}We launch the application and get the following:
This is misleading
Let's say we agree with the nullable concept. It turns out that if we see an NR type, then we believe that it can contain a potential null . However, sometimes you can see situations when the compiler tells us otherwise. Therefore, here will be considered a few cases where the use of this concept is not intuitive.
Case 1
internal override IEnumerable<SyntaxToken>? TryGetActiveTokens(SyntaxNode node)
{
....
var bodyTokens = SyntaxUtilities
.TryGetMethodDeclarationBody(node)
?.DescendantTokens();
if (node.IsKind(SyntaxKind.ConstructorDeclaration,
out ConstructorDeclarationSyntax? ctor))
{
if (ctor.Initializer != null)
{
bodyTokens = ctor.Initializer
.DescendantTokens()
.Concat(bodyTokens); // <=
}
}
return bodyTokens;
}V3156 The first argument of the 'Concat' method is not expected to be null. Potential null value: bodyTokens. CSharpEditAndContinueAnalyzer.cs 219 Let's take a
look at why bodyTokens is potential null and see the null conditional operator:
var bodyTokens = SyntaxUtilities
.TryGetMethodDeclarationBody(node)
?.DescendantTokens(); // <=If we go into the TryGetMethodDeclarationBody method , we will see that it can return null . However, it is relatively large, so I leave a link to it if you want to see for yourself. With bodyTokens everything is clear, but I want to draw attention to the ctor argument :
if (node.IsKind(SyntaxKind.ConstructorDeclaration,
out ConstructorDeclarationSyntax? ctor))As we can see, its type is set to NR. In this case, the line below is dereferencing:
if (ctor.Initializer != null)This combination is a little alarming. However, you might say that, most likely, if IsKind returns true , then ctor is definitely not null . The way it is:
public static bool IsKind<TNode>(
[NotNullWhen(returnValue: true)] this SyntaxNode? node, // <=
SyntaxKind kind,
[NotNullWhen(returnValue: true)] out TNode? result) // <=
where TNode : SyntaxNode
{
if (node.IsKind(kind))
{
result = (TNode)node;
return true;
}
result = null;
return false;
}Here, special attributes are used that indicate at which output value the parameters will not be null . We can verify this by looking at the logic of the IsKind method . It turns out that inside the condition, the type of ctor must be NNR. The compiler understands this and says that the ctor inside the condition will not be null . However, in order to understand this for us, we must go into the IsKind method and notice the attribute there. Otherwise, it looks like dereferencing an NR variable without checking for null . You can try to add some clarity like this:
if (node.IsKind(SyntaxKind.ConstructorDeclaration,
out ConstructorDeclarationSyntax? ctor))
{
if (ctor!.Initializer != null) // <=
{
....
}
}Case 2
public TextSpan GetReferenceEditSpan(InlineRenameLocation location,
string triggerText,
CancellationToken cancellationToken)
{
var searchName = this.RenameSymbol.Name;
if (_isRenamingAttributePrefix)
{
searchName = GetWithoutAttributeSuffix(this.RenameSymbol.Name);
}
var index = triggerText.LastIndexOf(searchName, // <=
StringComparison.Ordinal);
....
}V3156 The first argument of the 'LastIndexOf' method is not expected to be null. Potential null value: searchName. AbstractEditorInlineRenameService.SymbolRenameInfo.cs 126
We are interested in the searchName variable . null can be written to it after calling the GetWithoutAttributeSuffix method , but it's not that simple. Let's see what happens in it:
private string GetWithoutAttributeSuffix(string value)
=> value.GetWithoutAttributeSuffix(isCaseSensitive:
_document.GetRequiredLanguageService<ISyntaxFactsService>()
.IsCaseSensitive)!;Let's go deeper:
internal static string? GetWithoutAttributeSuffix(
this string name,
bool isCaseSensitive)
{
return TryGetWithoutAttributeSuffix(name, isCaseSensitive, out var result)
? result : null;
}It turns out that the TryGetWithoutAttributeSuffix method will return either result or null . And the method returns an NR type. However, going back a step, we notice that the type of the method suddenly changed to NNR. This happens because of the hidden sign "!":
_document.GetRequiredLanguageService<ISyntaxFactsService>()
.IsCaseSensitive)!; // <=By the way, it is rather difficult to notice it in Visual Studio:
By supplying it, the developer tells us that the method will never return null . Although, looking at the previous examples and going into the TryGetWithoutAttributeSuffix method , personally I cannot be sure about this:
internal static bool TryGetWithoutAttributeSuffix(
this string name,
bool isCaseSensitive,
[NotNullWhen(returnValue: true)] out string? result)
{
if (name.HasAttributeSuffix(isCaseSensitive))
{
result = name.Substring(0, name.Length - AttributeSuffix.Length);
return true;
}
result = null;
return false;
}Output
Finally, I want to say that trying to save us the unnecessary null checks is a great idea. However, NR types are rather advisory in nature, because no one strictly forbids us to pass null to an NNR type. That is why the corresponding PVS-Studio rules remain relevant. For example, such as V3080 or V3156 .
All the best and thank you for your attention.
If you want to share this article with an English-speaking audience, please use the translation link: Nikolay Mironov. Nullable Reference will not protect you, and here is the proof .