Nullable Reference types in C# 8.0 and static analysis

It's not a secret that Microsoft has been working on the 8-th version of C# language for quite a while. The new language version (C# 8.0) is already available in the recent release of Visual Studio 2019, but it's still in beta. This new version is going to have a few features implemented in a somewhat non-obvious, or rather unexpected, way. Nullable Reference types are one of them. This feature is announced as a means to fight Null Reference Exceptions (NRE).

It's good to see the language evolve and acquire new features to help developers. By coincidence, some time ago, we significantly enhanced the ability of PVS-Studio's C# analyzer to detect NREs. And now we're wondering if static analyzers in general and PVS-Studio in particular should still bother to diagnose potential null dereferences since, at least in new code that will be making use of Nullable Reference, such dereferences will become «impossible»? Let's try to clear that up.

Pros and cons of the new feature

One reminder before we continue: the latest beta version of C# 8.0, available as of writing this post, has Nullable Reference types disabled by default, i.e. the behavior of reference types hasn't changed.

So what are exactly nullable reference types in C# 8.0 if we enable this option? They are basically the same good old reference types except that now you'll have to add '?' after the type name (for example, string?), similarly to Nullable<T>, i.e. nullable value types (for example, int?). Without the '?', our string type will now be interpreted as non-nullable reference, i.e. a reference type that can't be assigned null.

Null Reference Exception is one of the most vexing exceptions to get into your program because it doesn't say much about its source, especially if the throwing method contains a number of dereference operations in a row. The ability to prohibit null assignment to a variable of a reference type looks cool, but what about those cases where passing a null to a method has some execution logic depending on it? Instead of null, we could, of course, use a literal, a constant, or simply an «impossible» value that logically can't be assigned to the variable anywhere else. But this poses a risk of replacing a crash of the program with «silent», but incorrect execution, which is often worse than facing the error right away.

What about throwing an exception then? A meaningful exception thrown in a location where something went wrong is always better than an NRE somewhere up or down the stack. But it's only good in your own project, where you can correct the consumers by inserting a try-catch block and it's solely your responsibility. When developing a library using (non) Nullable Reference, we need to guarantee that a certain method always returns a value. After all, it's not always possible (or at least easy) even in your own code to replace the returning of null with exception throwing (since it may affect too much code).

Nullable Reference can be enabled either at the global project level by adding the NullableContextOptions property with the value enable, or at the file level by means of the preprocessor directive:

#nullable enable 
string cantBeNull = string.Empty;
string? canBeNull = null;
cantBeNull = canBeNull!;

Nullable Reference feature will make types more informative. The method signature gives you a clue about its behavior: if it has a null check or not, if it can return null or not. Now, when you try using a nullable reference variable without checking it, the compiler will issue a warning.

This is pretty convenient when using third-party libraries, but it also adds a risk of misleading library's user, as it's still possible to pass null using the new null-forgiving operator (!). That is, adding just one exclamation point can break all further assumptions about the interface using such variables:

#nullable enable 
String GetStr() { return _count > 0 ? _str : null!; }
String str = GetStr();
var len = str.Length;

Yes, you can argue that this is bad programming and nobody would write code like that for real, but as long as this can potentially be done, you can't feel safe relying only on the contract imposed by the interface of a given method (saying that it can't return null).

By the way, you could write the same code using several ! operators, as C# now allows you to do so (and such code is perfectly compilable):

cantBeNull = canBeNull!!!!!!!;

By writing this way we, so to say, stress the idea, «look, this may be null!!!» (we in our team, we call this «emotional» programming). In fact, when building the syntax tree, the compiler (from Roslyn) interprets the ! operator in the same way as it interprets regular parentheses, which means you can write as many !'s as you like — just like with parentheses. But if you write enough of them, you can «knock down» the compiler. Maybe this will get fixed in the final release of C# 8.0.

Similarly, you can circumvent the compiler warning when accessing a nullable reference variable without a check:

canBeNull!.ToString();

Let's add more emotions:

canBeNull!!!?.ToString();

You'll hardly ever see syntax like that in real code though. By writing the null-forgiving operator we tell the compiler, «This code is okay, check not needed.» By adding the Elvis operator we tell it, «Or maybe not; let's check it just in case.»

Now, you can reasonably ask why you still can have null assigned to variables of non-nullable reference types so easily if the very concept of these type implies that such variables can't have the value null? The answer is that «under the hood», at the IL code level, our non-nullable reference type is still… the good old «regular» reference type, and the entire nullability syntax is actually just an annotation for the compiler's built-in analyzer (which, we believe, isn't quite convenient to use, but I'll elaborate on that later). Personally, we don't find it a «neat» solution to include the new syntax as simply an annotation for a third-party tool (even built into the compiler) because the fact that this is just an annotation may not be obvious at all to the programmer, as this syntax is very similar to the syntax for nullable structs yet works in a totally different way.

Getting back to other ways of breaking Nullable Reference types. As of the moment of writing this article, when you have a solution comprised of several projects, passing a variable of a reference type, say, String from a method declared in one project to a method in another project that has the NullableContextOptions enabled will make the compiler assume it's dealing with a non-nullable String and the compiler will remain silent. And that's despite the tons of [Nullable(1)] attributes added to every field and method in the IL code when enabling Nullable References. These attributes, by the way, should be taken into account if you use reflection to handle the attributes and assume that the code contains only your custom ones.

Such a situation may cause additional trouble when adapting a large code base to the Nullable Reference style. This process will likely be running for a while, project by project. If you are careful, of course, you can gradually integrate the new feature, but if you already have a working project, any changes to it are dangerous and undesirable (if it works, don't touch it!). That's why we made sure that you don't have to modify your source code or mark it to detect potential NREs when using PVS-Studio analyzer. To check locations that could throw a NullReferenceException, simply run the analyzer and look for V3080 warnings. No need to change the project's properties or the source code. No need to add directives, attributes, or operators. No need to change legacy code.

When adding Nullable Reference support to PVS-Studio, we had to decide whether the analyzer should assume that variables of non-nullable reference types always have non-null values. After investigating the ways this guarantee could be broken, we decided that PVS-Studio shouldn't make such an assumption. After all, even if a project uses non-nullable reference types all the way through, the analyzer could add to this feature by detecting those specific situations where such variables could have the value null.

How PVS-Studio looks for Null Reference Exceptions

The dataflow mechanisms in PVS-Studio's C# analyzer track possible values of variables during the analysis process. This also includes interprocedural analysis, i.e. tracking down possible values returned by a method and its nested methods, and so on. In addition to that, PVS-Studio remembers variables that could be assigned null value. Whenever it sees such a variable being dereferenced without a check, whether it's in the current code under analysis, or inside a method invoked in this code, it will issue a V3080 warning about a potential Null Reference Exception.

The idea behind this diagnostic is to have the analyzer get angry only when it sees a null assignment. This is the principal difference of our diagnostic's behavior from that of the compiler's built-in analyzer handling Nullable Reference types. The built-in analyzer will point at each and every dereference of an unchecked nullable reference variable — given that it hasn't been misled by the use of the ! operator or even just a complicated check (it should be noted, however, that absolutely any static analyzer, PVS-Studio being no exception here, can be «misled» one way or another, especially if you are intent on doing so).

PVS-Studio, on the other hand, warns you only if it sees a null (whether within the local context or the context of an outside method). Even if the variable is of a non-nullable reference type, the analyzer will keep pointing at it if it sees a null assignment to that variable. This approach, we believe, is more appropriate (or at least more convenient for the user) since it doesn't demand «smearing» the entire code with null checks to track potential dereferences — after all, this option was available even before Nullable Reference were introduced, for example, through the use of contracts. What's more, the analyzer can now provide a better control over non-nullable reference variables themselves. If such a variable is used «fairly» and never gets assigned null, PVS-Studio won't say a word. If the variable is assigned null and then dereferenced without a prior check, PVS-Studio will issue a V3080 warning:

#nullable enable 
String GetStr() { return _count > 0 ? _str : null!; }
String str = GetStr();
var len = str.Length; <== V3080: Possible null dereference. 
                                 Consider inspecting 'str'

Now let's take a look at some examples demonstrating how this diagnostic is triggered by the code of Roslyn itself. We already checked this project recently, but this time we'll be looking only at potential Null Reference Exceptions not mentioned in the previous articles. We'll see how PVS-Studio detects potential NREs and how they can be fixed using the new Nullable Reference syntax.

V3080 [CWE-476] Possible null dereference inside method. Consider inspecting the 2nd argument: chainedTupleType. Microsoft.CodeAnalysis.CSharp TupleTypeSymbol.cs 244

NamedTypeSymbol chainedTupleType;
if (_underlyingType.Arity < TupleTypeSymbol.RestPosition)
  { ....  chainedTupleType = null; }
else { .... }
return Create(ConstructTupleUnderlyingType(firstTupleType,
  chainedTupleType, newElementTypes), elementNames: _elementNames);

As you can see, the chainedTupleType variable can be assigned the null value in one of the execution branches. It is then passed to the ConstructTupleUnderlyingType method and used there after a Debug.Assert check. It's a very common pattern in Roslyn, but keep in mind that Debug.Assert is removed in the release version. That's why the analyzer still considers the dereference inside the ConstructTupleUnderlyingType method dangerous. Here's the body of that method, where the dereference takes place:

internal static NamedTypeSymbol ConstructTupleUnderlyingType(
  NamedTypeSymbol firstTupleType, 
  NamedTypeSymbol chainedTupleTypeOpt, 
  ImmutableArray<TypeWithAnnotations> elementTypes)
{
  Debug.Assert
    (chainedTupleTypeOpt is null ==
     elementTypes.Length < RestPosition);
  ....
  while (loop > 0)
  {   
    ....
    currentSymbol = chainedTupleTypeOpt.Construct(chainedTypes);
    loop--;
  }
  return currentSymbol;
}

It's actually a matter of dispute whether the analyzer should take Asserts like that into account (some of our users want it to do so) — after all, the analyzer does take contracts from System.Diagnostics.Contracts into account. Here's one small real-life example from our experience of using Roslyn in our own analyzer. While adding support of the latest version of Visual Studio recently, we also updated Roslyn to its 3rd version. After that, PVS-Studio started crashing on certain code it had never crashed on before. The crash, accompanied by a Null Reference Exception, would occur not in our code but in the code of Roslyn. Debugging revealed that the code fragment where Roslyn was now crashing had that very kind of Debug.Assert based null check several lines higher — and that check obviously didn't help.

It's a graphic example of how you can get into trouble with Nullable Referencebecause of the compiler treating Debug.Assert as a reliable check in any configuration. That is, if you add #nullable enable and mark the chainedTupleTypeOpt argument as a nullable reference, the compiler won't issue any warning on the dereference inside the ConstructTupleUnderlyingType method.

Moving on to other examples of warnings by PVS-Studio.

V3080 Possible null dereference. Consider inspecting 'effectiveRuleset'. RuleSet.cs 146

var effectiveRuleset = 
  ruleSet.GetEffectiveRuleSet(includedRulesetPaths);
effectiveRuleset = 
  effectiveRuleset.WithEffectiveAction(ruleSetInclude.Action);

if (IsStricterThan(effectiveRuleset.GeneralDiagnosticOption, ....))
   effectiveGeneralOption = effectiveRuleset.GeneralDiagnosticOption;

This warning says that the call of the WithEffectiveAction method may return null, while the return value assigned to the variable effectiveRuleset is not checked before use (effectiveRuleset.GeneralDiagnosticOption). Here's the body of the WithEffectiveAction method:

public RuleSet WithEffectiveAction(ReportDiagnostic action)
{
  if (!_includes.IsEmpty)
    throw new ArgumentException(....);
  switch (action)
  {
    case ReportDiagnostic.Default:
      return this;
    case ReportDiagnostic.Suppress:
      return null;
    ....     
      return new RuleSet(....);
     default:
       return null;
   }
}

With Nullable Reference enabled for the method GetEffectiveRuleSet, we'll get two locations where the code's behavior has to be changed. Since the method shown above can throw an exception, it's logical to assume that the call to it is wrapped in a try-catch block and it would be correct to rewrite the method to throw an exception rather than return null. However, if you trace a few calls back, you'll see that the catching code is too far up to reliably predict the consequences. Let's take a look at the consumer of the effectiveRuleset variable, the IsStricterThan method:

private static bool 
  IsStricterThan(ReportDiagnostic action1, ReportDiagnostic action2)
{
  switch (action2)
  {
    case ReportDiagnostic.Suppress:
      ....;
    case ReportDiagnostic.Warn:
      return action1 == ReportDiagnostic.Error;
    case ReportDiagnostic.Error:
      return false;
    default:
      return false;
  }
}

As you can see, it's a simple switch statement choosing between two enumerations, with ReportDiagnostic.Default as the default value. So it would be best to rewrite the call as follows:

The signature of WithEffectiveAction will change:

#nullable enable
public RuleSet? WithEffectiveAction(ReportDiagnostic action)

This is what the call will look like:

RuleSet? effectiveRuleset = 
  ruleSet.GetEffectiveRuleSet(includedRulesetPaths);
effectiveRuleset = 
  effectiveRuleset?.WithEffectiveAction(ruleSetInclude.Action);

if (IsStricterThan(effectiveRuleset?.GeneralDiagnosticOption ?? 
                     ReportDiagnostic.Default,
                   effectiveGeneralOption))
   effectiveGeneralOption = effectiveRuleset.GeneralDiagnosticOption;

Since IsStricterThan only performs comparison, the condition can be rewritten — for example, like this:

if (effectiveRuleset == null || 
    IsStricterThan(effectiveRuleset.GeneralDiagnosticOption,
                   effectiveGeneralOption))

Next example.

V3080 Possible null dereference. Consider inspecting 'propertySymbol'. BinderFactory.BinderFactoryVisitor.cs 372

var propertySymbol = GetPropertySymbol(parent, resultBinder);
var accessor = propertySymbol.GetMethod;
if ((object)accessor != null)
  resultBinder = new InMethodBinder(accessor, resultBinder);

To fix this warning, we need to see what happens to the propertySymbol variable next.

private SourcePropertySymbol GetPropertySymbol(
  BasePropertyDeclarationSyntax basePropertyDeclarationSyntax,
  Binder outerBinder)
{
  ....
  NamedTypeSymbol container 
    = GetContainerType(outerBinder, basePropertyDeclarationSyntax);

  if ((object)container == null)
    return null;
  ....
  return (SourcePropertySymbol)GetMemberSymbol(propertyName,
    basePropertyDeclarationSyntax.Span, container,
    SymbolKind.Property);
}

The GetMemberSymbol method, too, can return null under certain conditions.

private Symbol GetMemberSymbol(
  string memberName, 
  TextSpan memberSpan, 
  NamedTypeSymbol container, 
  SymbolKind kind)
{
  foreach (Symbol sym in container.GetMembers(memberName))
  {
    if (sym.Kind != kind)
      continue;
    if (sym.Kind == SymbolKind.Method)
    {
      ....
      var implementation =
        ((MethodSymbol)sym).PartialImplementationPart;
      if ((object)implementation != null)
        if (InSpan(implementation.Locations[0],
            this.syntaxTree, memberSpan))
          return implementation;
    }
    else if (InSpan(sym.Locations, this.syntaxTree, memberSpan))
      return sym;
  }
  return null;
}

With nullable reference types enabled, the call will change to this:

#nullable enable
SourcePropertySymbol? propertySymbol 
  = GetPropertySymbol(parent, resultBinder);
MethodSymbol? accessor = propertySymbol?.GetMethod;
if ((object)accessor != null)
  resultBinder = new InMethodBinder(accessor, resultBinder);

It's pretty easy to fix when you know where to look. Static analysis can catch this potential error with no effort by collecting all possible values of the field from all the procedure call chains.

V3080 Possible null dereference. Consider inspecting 'simpleName'. CSharpCommandLineParser.cs 1556

string simpleName;
simpleName = PathUtilities.RemoveExtension(
  PathUtilities.GetFileName(sourceFiles.FirstOrDefault().Path));
outputFileName = simpleName + outputKind.GetDefaultExtension();
if (simpleName.Length == 0 && !outputKind.IsNetModule())
  ....

The problem is in the line with the simpleName.Length check. The variable simpleName results from executing a long series of methods and can be assigned null. By the way, if you are curious, you could look at the RemoveExtension method to see how it's different from Path.GetFileNameWithoutExtension. A simpleName != null check would be enough, but with non-nullable reference types, the code will change to something like this:

#nullable enable
public static string? RemoveExtension(string path) { .... }
string simpleName;

This is what the call might look like:

simpleName = PathUtilities.RemoveExtension(
  PathUtilities.GetFileName(sourceFiles.FirstOrDefault().Path)) ?? 
  String.Empty;

Conclusion

Nullable Reference types can be a great help when designing architecture from scratch, but reworking existing code may require a lot of time and care, as it may lead to a number of elusive bugs. This article doesn't aim to discourage you from using Nullable Reference types. We find this new feature generally useful even though the exact way it is implemented may be controversial.

However, always remember about the limitations of this approach and keep in mind that enabling Nullable Reference mode doesn't protect you from NREs and that, when misused, it could itself become the source of these errors. We recommend that you complement the Nullable Reference feature with a modern static analysis tool, such as PVS-Studio, that supports interprocedural analysis to protect your program from NREs. Each of these approaches — deep interprocedural analysis and annotating method signatures (which is in fact what Nullable Reference mode does) — have their pros and cons. The analyzer will provide you with a list of potentially dangerous locations and let you see the consequences of modifying existing code. If there is a null assignment somewhere, the analyzer will point at every consumer of the variable where it is dereferenced without a check.

You can check this project or your own projects for other defects — just download PVS-Studio and give it a try.

Автор: Paull

Источник