# Checking the Roslyn Source Code

Once in a while we go back to the projects that we have previously checked using PVS\-Studio, which results in their descriptions in various articles\. Two reasons make these comebacks exciting for us\.  Firstly, the opportunity to assess the progress of our analyzer\. Secondly, monitoring the feedback of the project's authors to our article and the report of errors, which we usually provide them with\. Of course, errors can be corrected without our participation\. However, it is always nice when our efforts help to make a project better\. Roslyn was no exception\. The previous article about this project check dates back to December 23, 2015\. It's quite a long time, in the view of the progress that our analyzer has made since that time\. Since the C\# core of the PVS\-Studio analyzer is based on Roslyn, it gives us additional interest in this project\. As a result, we're as keen as mustard about the code quality of this project\. Now let's test it once again and find out some new and interesting issues \(but let's hope that nothing significant\) that PVS\-Studio will be able to find\.

![0622_Roslyn/image1.png](https://import.viva64.com/docx/blog/0622_Roslyn/image1.png)

Many of our readers are likely to be well aware of Roslyn \(or \.NET Compiler Platform\)\. In short, it is a set of open source compilers and an API for code analysis of C\# and Visual Basic \.NET languages from Microsoft\. The source code of the project is available on [GitHub](https://github.com/dotnet/roslyn)\. 

I won't give a detailed description of this platform and I'll recommend checking out the article by my colleague Sergey Vasiliev "[Introduction to Roslyn and its use in program development](https://pvs-studio.com/en/blog/posts/csharp/0399/)" to all interested readers\. From this article, you can find out not only about the features of the Roslyn's architecture, but how exactly we use this platform\.

As I mentioned earlier, it has been more than 3 years since my colleague Andrey Karpov wrote the last article about the Roslyn check "[New Year PVS\-Studio 6\.00 Release: Scanning Roslyn](https://pvs-studio.com/en/blog/posts/csharp/0363/)"\. Since then the C\#  PVS\-Studio analyzer had got many new features\. Actually, Andrey's article was a test case, as at that time the C\# analyzer was just added in PVS\-Studio\. Despite this, we managed to detect errors in the Roslyn project, which was certainly of high\-quality\.  So what has changed in the analyzer for C\# code by this moment that will allow us to perform a more in\-depth analysis?

Since then, both the core and the infrastructure have been developing\. We added support for Visual Studio 2017 and Roslyn 2\.0, and a deep integration with MSBuild\. The article by my colleague Paul Eremeev "[Support of Visual Studio 2017 and Roslyn 2\.0 in PVS\-Studio: sometimes it's not that easy to use ready\-made solutions as it may seem](https://pvs-studio.com/en/blog/posts/csharp/0503/)" describes our approach to integration with MSBuild and reasons for this decision\. 

At the moment we are actively working on moving to Roslyn 3\.0 in the same way as we initially supported Visual Studio 2017\. It requires usage of our own toolset, included in the PVS\-Studio distributive as a "stub", which is an empty MSBuild\.exe file\. Despite the fact that it looks like a "crutch" \(MSBuild API is not very friendly for reusing in third\-party projects due to low libraries portability\), such approach has already helped us to relatively seamlessly overcome multiple Roslyn updates in terms of Visual Studio 2017\. Until now it was helping \(even with some challenges\) to pass through the Visual Studio 2019 update and maintain full backward compatibility and performance for systems with older MSBuild versions\.

The analyzer core has also undergone a number of improvements\. One of the main features is a complete interprocedural analysis with consideration of input and output methods' values, evaluating \(depending on these parameters\) reachability of the execution branches and return points\.   

We are on our way to completing the task of monitoring parameters inside the methods \(for example, potentially dangerous dereferences\) along with saving their auto\-annotations\. For a diagnostic that uses data\-flow mechanism, this will allow taking into account dangerous situations, occurring when passing a parameter in a method\. Before this, when analyzing such dangerous places, a warning wasn't generated, as we couldn't know about all possible input values in such a method\. Now we can detect danger, as in all the places of calling this method, these input parameters will be taken into account\.

Note: you can read about basic analyzer mechanisms, such as data\-flow and others in the article "[Technologies used in the PVS\-Studio code analyzer for finding bugs and potential vulnerabilities](https://pvs-studio.com/en/blog/posts/cpp/0592/)"\.

Interprocedural analysis in PVS\-Studio C\# is limited neither by input parameters, nor the depth\. The only limitation is virtual methods in classes, open for inheritance, as well as getting into recursion \(the analysis stops when it stumbles upon a repeated call of the already evaluated method\)\. In doing so, the recursive method itself will be eventually evaluated assuming that the return value of its recursion is unknown\.

Another great new feature in the C\# analyzer has become taking into account possible dereference of a potentially null pointer\. Before that, the analyzer complained of a possible null reference exception, being ensured that in all execution branches the variable value will be null\. Of course, it was wrong in some cases, that's why the [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) diagnostic had been previously called potential null reference\. 

Now the analyzer is aware of the fact that the variable could be null in one of the execution branches \(for example, under a certain _if_ condition\)\.  If it notices access to such a variable without a check, it will issue the V3080 warning, but at a lower level of certainty, than if it sees null in all branches\. Along with the improved interprocedural analysis, such a mechanism allows finding errors that are very difficult to detect\. Here is an example \- imagine a long chain of method calls, the last of which is unfamiliar to you\. Under certain circumstances, it returns null in the _catch_ block, but you haven't protected from this, as you simple haven't known\. In this case, the analyzer only complains, when it exactly sees null assignment\. In our view, it qualitatively distinguishes our approach from such feature of C\# 8\.0 as nullable type reference, which, in fact, confines to setting checks for null for each method\. However, we suggest the alternative \- to perform checks only in places where null can really occur, and our analyzer can now search for such cases\.

So, let's not delay the main point for too long and go to blame\-storming \- analyzing the results of the Roslyn check\. First, let's consider the errors, found due to the features, described above\.   In sum, there were quite a lot of warnings for the Roslyn code this time\.  I think it is related to the fact that the platform is very actively evolving \(at this point the codebase is about 2 770 000 lines excluding empty\), and we haven't analyzed this project for long\. Nevertheless, there are not so many critical errors, whereas they are of the most interest for the article\. As usual, I excluded tests from the check, there are quite a lot of them in Roslyn\.

I will start with V3080 errors of the Medium level of certainty, in which the analyzer has detected a possible access by null reference, but not in all possible cases \(code branches\)\.

**Possible null dereference \- Medium**

V3080 Possible null dereference\. Consider inspecting 'current'\. CSharpSyntaxTreeFactoryService\.PositionalSyntaxReference\.cs 70

```cpp
private SyntaxNode GetNode(SyntaxNode root)
{
  var current = root;
  ....
  while (current.FullSpan.Contains(....))     // <=
  {
    ....
    var nodeOrToken = current.ChildThatContainsPosition(....);
    ....
    current = nodeOrToken.AsNode();           // <=
  }
  ....
}

public SyntaxNode AsNode()
{
  if (_token != null)
  {
    return null;
  }
  
  return _nodeOrParent;
}
```

Let's consider the method _GetNode_\. The analyzer suggests the access by null reference is possible in the condition of the _while _block_\. _ The variable is assigned a value in the body of the _while _block, which is a result of the _AsNode _method\. In some cases, this value will be _null_\. A good example of interprocedural analysis in action\.

Now let's consider a similar case, in which the interprocedural analysis was carried out via two method calls\.

V3080 Possible null dereference\. Consider inspecting 'directory'\. CommonCommandLineParser\.cs 911

```cpp
private IEnumerable<CommandLineSourceFile>
  ExpandFileNamePattern(string path, string baseDirectory, ....)
{
    string directory = PathUtilities.GetDirectoryName(path);
    ....
    var resolvedDirectoryPath = (directory.Length == 0) ?  // <=
      baseDirectory : 
      FileUtilities.ResolveRelativePath(directory, baseDirectory);
    ....
}

public static string GetDirectoryName(string path)
{
    return GetDirectoryName(path, IsUnixLikePlatform);
}

internal static string GetDirectoryName(string path, bool isUnixLike)
{
  if (path != null)
  {
    ....
  }
  
  return null;
}
```

The _directory _variable in the body of the _ExpandFileNamePattern _method gets the value from the method _GetDirectoryName\(string\)_\. That, in turn, returns the result of the overloaded method _GetDirectoryName \(string, bool\) _whose value can be _null_\. Since the variable _directory_ is used without a preliminary check for null in the body of the method _ExpandFileNamePattern_ \- we can proclaim the analyzer correct about issuing the warning\. This is a potentially unsafe construction\.

Another code fragment with the V3080 error, more precisely, with two errors, issued for a single line of code\. The interprocedural analysis wasn't needed here\.

V3080 Possible null dereference\. Consider inspecting 'spanStartLocation'\. TestWorkspace\.cs 574

V3080 Possible null dereference\. Consider inspecting 'spanEndLocationExclusive'\. TestWorkspace\.cs 574

```cpp
private void MapMarkupSpans(....)
{
  ....
  foreach (....)
  {
    ....
    foreach (....)
    {
      ....
      int? spanStartLocation = null;
      int? spanEndLocationExclusive = null;
  
      foreach (....)
      {
        if (....)
        {
          if (spanStartLocation == null &&
              positionInMarkup <= markupSpanStart && ....)
          {
            ....
            spanStartLocation = ....;
          }
  
          if (spanEndLocationExclusive == null &&
              positionInMarkup <= markupSpanEndExclusive && ....)
          {
            ....
            spanEndLocationExclusive = ....;
            break;
          }
          ....
        }
        ....
      }
  
      tempMappedMarkupSpans[key].
        Add(new TextSpan(
          spanStartLocation.Value,            // <=
          spanEndLocationExclusive.Value -    // <=
            spanStartLocation.Value));
    }
  }
  ....
}
```

The variables _spanStartLocation _and _spanEndLocationExclusive _are of the _nullable int _ type and are initialized by _null_\. Further along the code they can be assigned values, but only under certain conditions\. In some cases, their value remains _null_\. After that, these variables are accessed by reference without preliminary check for null, which the analyzer indicates\.

The Roslyn code contains quite a lot of such errors, more than 100\. Often the pattern of these errors is the same\. There is some kind of a general method, which potentially returns _null_\. The result of this method is used in many places, sometimes through dozens of intermediate method calls or additional checks\. It is important to understand that these errors are not fatal, but they can potentially lead to access by null reference\. While detecting of such errors is quite challenging\. That's why in some cases one should consider code refactoring, in which case if _null_ returns, the method will throw an exception\. Otherwise, you can secure your code only with general checks which is quite tiring and sometimes unreliable\. Anyway, it's clear that each specific case requires a solution based on project specifications\. 

Note\. It so happens, that at a given point there are no such cases \(input data\), when the method returns _null_ and there is no actual error\. However, such code is still not reliable, because everything can change when introducing some code changes\.

In order to drop the [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) subject, let's look at obvious errors of High certainty level, when the access by null reference is the likeliest or even inevitable\.

**Possible null dereference \- High**

V3080 Possible null dereference\. Consider inspecting 'collectionType\.Type'\. AbstractConvertForToForEachCodeRefactoringProvider\.cs 137

```cpp
public override async Task 
ComputeRefactoringsAsync(CodeRefactoringContext context)
{
  ....
  var collectionType = semanticModel.GetTypeInfo(....);
  if (collectionType.Type == null && 
      collectionType.Type.TypeKind == TypeKind.Error)
  {
    return;
  }
  ....  
}
```

Due to the typo in the condition \(_&& _is used instead of the operator_ \|\|_\), the code works differently than intended and the access to the _collectionType\.Type _variable will be executed when it is _null_\. The condition should be corrected as follows:

```cpp
if (collectionType.Type == null || 
    collectionType.Type.TypeKind == TypeKind.Error)
  ....
```

By the way, things may unfold in another way: in the first part of the condition the operators _\=\=_ and _\!\=_ are messed up_\._ Then the correct code would look like this:

```cpp
if (collectionType.Type != null &&
    collectionType.Type.TypeKind == TypeKind.Error)
  ....
```

This version of the code is less logical, but also corrects the error\. The final solution rests for project's authors to decide\. 

Another similar error\.

V3080 Possible null dereference\. Consider inspecting 'action'\. TextViewWindow\_InProc\.cs 372

```cpp
private Func<IWpfTextView, Task> GetLightBulbApplicationAction(....)
{
  ....
  if (action == null)
  {
    throw new
      InvalidOperationException(
        $"Unable to find FixAll in {fixAllScope.ToString()}
        code fix for suggested action '{action.DisplayText}'.");
  }
  ....
}
```

The error is made when generating the message for the exception\. It is followed by the attempt to access the _action\.DisplayText _property via the _action_ variable, which is known to be _null_\.

Here comes the last V3080 error of the High level\.

V3080 Possible null dereference\. Consider inspecting 'type'\. ObjectFormatterHelpers\.cs 91

```cpp
private static bool IsApplicableAttribute(
  TypeInfo type, 
  TypeInfo targetType, 
  string targetTypeName)
{
  return type != null && AreEquivalent(targetType, type)
    || targetTypeName != null && type.FullName == targetTypeName;
}
```

The method is quite small, so I cite it entirely\. The condition in the _return_ block is incorrect\.  In some cases, when accessing _type\.FullName_, an exception may occur\. I'll use parentheses to make it clear \(they won't change the behavior\):

```cpp
return (type != null && AreEquivalent(targetType, type))
    || (targetTypeName != null && type.FullName == targetTypeName);
```

According to the operations precedence, the code will work exactly like this\. In case if the _type _variable is _null_, we'll fall in the else\-check, where we'll use the _type _null reference, having checked the variable _targetTypeName _for _null_\. Code might be fixed, for example, as follows:

```cpp
return type != null && 
  (AreEquivalent(targetType, type) || 
  targetTypeName != null && type.FullName == targetTypeName);
```

I think, it's enough for reviewing V3080 errors\. Now it's high time to see other interesting stuff that the PVS\-Studio analyzer managed to find\.

**Typo**

[V3005](https://pvs-studio.com/en/docs/warnings/v3005/) The 'SourceCodeKind' variable is assigned to itself\. DynamicFileInfo\.cs 17

```cpp
internal sealed class DynamicFileInfo
{
  ....
  public DynamicFileInfo(
    string filePath,
    SourceCodeKind sourceCodeKind,
    TextLoader textLoader,
    IDocumentServiceProvider documentServiceProvider)
  {
    FilePath = filePath;
    SourceCodeKind = SourceCodeKind;  // <=
    TextLoader = textLoader;
    DocumentServiceProvider = documentServiceProvider;
  }
  ....
}
```

Due to failing variables naming, a typo was made in the constructor of the _DynamicFileInfo _class\. The _SourceCodeKind _field is assigned its own value instead of using the parameter _sourceCodeKind_\. To minimize the likelihood of such errors, we recommend that you use the underscore prefix to the parameter names in such cases\. Here is an example of a corrected version of the code:

```cpp
public DynamicFileInfo(
  string _filePath,
  SourceCodeKind _sourceCodeKind,
  TextLoader _textLoader,
  IDocumentServiceProvider _documentServiceProvider)
{
  FilePath = _filePath;
  SourceCodeKind = _sourceCodeKind;
  TextLoader = _textLoader;
  DocumentServiceProvider = _documentServiceProvider;
}
```

**Inadvertence**

[V3006](https://pvs-studio.com/en/docs/warnings/v3006/) The object was created but it is not being used\. The 'throw' keyword could be missing: throw new InvalidOperationException\(FOO\)\. ProjectBuildManager\.cs 61

```cpp
~ProjectBuildManager()
{
  if (_batchBuildStarted)
  {
    new InvalidOperationException("ProjectBuilderManager.Stop()
                                   not called.");
  }
}
```

Under a certain condition, the destructor must throw an exception, but it's not happening while the exception object is simply created\. The _throw_ keyword was missed\. Here is the corrected version of the code:

```cpp
~ProjectBuildManager()
{
  if (_batchBuildStarted)
  {
    throw new InvalidOperationException("ProjectBuilderManager.Stop()
                                         not called.");
  }
}
```

The issue with destructors in C\# and throwing exceptions from them is a topic for another discussion, which is beyond the scope of this article\.

**When the result is not important**

Methods, which received the same value in all cases, triggered a certain number of [V3009](https://pvs-studio.com/en/docs/warnings/v3009/) warnings\. In some cases it can be not critical or the return value is simply not checked in the calling code\. I skipped such warnings\. But a few code snippets seemed suspicious\. Here is one of them:

V3009 It's odd that this method always returns one and the same value of 'true'\. GoToDefinitionCommandHandler\.cs 62

```cpp
internal bool TryExecuteCommand(....)
{
  ....
  using (context.OperationContext.AddScope(....))
  {
    if (....)
    {
      return true;
    }  
  }
  ....
  return true;
}
```

The method _TryExecuteCommand _returns nothing but _true_\.  In doing so, in the calling code the returned value is involved in some checks\.

```cpp
public bool ExecuteCommand(....)
{
  ....
  if (caretPos.HasValue && TryExecuteCommand(....))
  {
    ....
  }
  ....
}
```

It's hard to say exactly to what extent such behaviour is dangerous\. But if the result is not needed, maybe the type of the return value should be changed to void and one should make small edits in the calling method\. This will make the code more readable and secure\.

Similar analyzer warnings: 

* V3009 It's odd that this method always returns one and the same value of 'true'\. CommentUncommentSelectionCommandHandler\.cs 86
* V3009 It's odd that this method always returns one and the same value of 'true'\. RenameTrackingTaggerProvider\.RenameTrackingCommitter\.cs 99
* V3009 It's odd that this method always returns one and the same value of 'true'\. JsonRpcClient\.cs 138
* V3009 It's odd that this method always returns one and the same value of 'true'\. AbstractFormatEngine\.OperationApplier\.cs 164
* V3009 It's odd that this method always returns one and the same value of 'false'\. TriviaDataFactory\.CodeShapeAnalyzer\.cs 254
* V3009 It's odd that this method always returns one and the same value of 'true'\. ObjectList\.cs 173
* V3009 It's odd that this method always returns one and the same value of 'true'\. ObjectList\.cs 249

**Checked the wrong thing**

[V3019](https://pvs-studio.com/en/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'value', 'valueToSerialize'\. RoamingVisualStudioProfileOptionPersister\.cs 277

```cpp
public bool TryPersist(OptionKey optionKey, object value)
{
  ....
  var valueToSerialize = value as NamingStylePreferences;
  if (value != null)
  {
      value = valueToSerialize.CreateXElement().ToString();
  }
  ....
}
```

The _value _variable is cast to the type _NamingStylePreferences_\.  The problem is in the check that follows this\. Even if the _value_ variable was non null, it doesn't guarantee that type casting was successful and _valueToSerialize_ doesn't contain  _null_\. Possible throwing of the exception _NullReferenceException_\. The code needs to be corrected as follows:

```cpp
var valueToSerialize = value as NamingStylePreferences;
if (valueToSerialize != null)
{
  value = valueToSerialize.CreateXElement().ToString();
}
```

Another similar bug:

V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'columnState', 'columnState2'\. StreamingFindUsagesPresenter\.cs 181

```cpp
private void SetDefinitionGroupingPriority(....)
{
  ....
  foreach (var columnState in ....)
  {
    var columnState2 = columnState as ColumnState2;
    if (columnState?.Name ==                        // <=
        StandardTableColumnDefinitions2.Definition)
    {
      newColumns.Add(new ColumnState2(
        columnState2.Name,                          // <=
        ....));
    }
    ....
  }
  ....
}
```

The _columnState_ variable is cast to the type _ColumnState2_\. However, the operation result, which is the variable _columnState2,_ is not checked for _null _further\. Instead, the _columnState_ variable is checked using the conditional _null _operator\. Why is this code dangerous? Just like in the previous example, casting with the _as_ operator may fail and the variable will be _null _which will result in an exception\.  By the way, a typo may be to blame here\. Take a look at the condition in the _if_ block\. 

Perhaps, instead of _columnState?\.Name _the author wanted to write _columnState2?\.Name_\. It is very likely, considering rather faulty variable names _columnState _and_ columnState2\._

**Redundant checks**

Quite a large number of warnings \(more than 100\) were issued on noncritical, but potentially unsafe constructions related to redundant checks\. For example, this is one of them\.

[V3022](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'navInfo \=\= null' is always false\. AbstractSyncClassViewCommandHandler\.cs 101

```cpp
public bool ExecuteCommand(....)
{
  ....
  IVsNavInfo navInfo = null;
  if (symbol != null)
  {
    navInfo = libraryService.NavInfoFactory.CreateForSymbol(....);
  }
  
  if (navInfo == null)
  {
    navInfo = libraryService.NavInfoFactory.CreateForProject(....);
  }
  
  if (navInfo == null)    // <=
  {
    return true;
  }  
  ....
}

public IVsNavInfo CreateForSymbol(....)
{
  ....
  return null;
}

public IVsNavInfo CreateForProject(....)
{
  return new NavInfo(....);
}
```

May be there is no actual bug here\. It's just a good reason to demonstrate "interprocedural analysis \+ dataflow analysis" working in a tow\. The analyzer suggests the second check _navInfo \=\= null_ is redundant\. Indeed, before it the value assigned to _navInfo _will be obtained from the method _libraryService\.NavInfoFactory\.CreateForProject_, which will construct and return a new object of the _NavInfo _class\.  No way it will return _null_\. Here the question arises, why didn't the analyzer issue a warning for the first check _navInfo \=\= null_? There are some reasons\. Firstly, if the _symbol _variable is _null_, the _navInfo _value will remain a null reference as well\. Secondly, even if _navInfo _gets the value from the method _ibraryService\.NavInfoFactory\.CreateForSymbol_, this value can also be _null_\. Thus, the first check _navInfo \=\= null_ is really needed\.

**Insufficient checks**

Now the reverse situation from the discussed above\. Several [V3042](https://pvs-studio.com/en/docs/warnings/v3042/) warnings were triggered for the code, in which access by null reference is possible\. Even one or two small checks could have fixed everything\.

Let's consider another interesting code fragment, which has two such errors\.

V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'receiver' object Binder\_Expressions\.cs 7770

V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'receiver' object Binder\_Expressions\.cs 7776

```cpp
private BoundExpression GetReceiverForConditionalBinding(
  ExpressionSyntax binding,
  DiagnosticBag diagnostics)
{
  ....
  BoundExpression receiver = this.ConditionalReceiverExpression;
  if (receiver?.Syntax !=                                // <=
    GetConditionalReceiverSyntax(conditionalAccessNode))
  {
    receiver = BindConditionalAccessReceiver(conditionalAccessNode,
                                             diagnostics);
  }
  
  var receiverType = receiver.Type;                      // <=
  if (receiverType?.IsNullableType() == true)
  {
    ....
  }

  receiver = new BoundConditionalReceiver(receiver.Syntax, 0,  // <=
    receiverType ?? CreateErrorType(), 
    hasErrors: receiver.HasErrors)                             // <=
    { WasCompilerGenerated = true };
  
  return receiver; 
}
```

The _receiver _variable may be null\. The author of the code knows about this, as he uses the conditional _null _operator in the condition of the _if _block to access _receiver?\.Syntax_\. Further the _receiver _variable is used without any checks to access _receiver\.Type_, _receiver\.Syntax _and _receiver\.HasErrors_\. These errors must be corrected:

```cpp
private BoundExpression GetReceiverForConditionalBinding(
  ExpressionSyntax binding,
  DiagnosticBag diagnostics)
{
  ....
  BoundExpression receiver = this.ConditionalReceiverExpression;
  if (receiver?.Syntax !=
    GetConditionalReceiverSyntax(conditionalAccessNode))
  {
    receiver = BindConditionalAccessReceiver(conditionalAccessNode,
                                             diagnostics);
  }
  
  var receiverType = receiver?.Type;
  if (receiverType?.IsNullableType() == true)
  {
    ....
  }

  receiver = new BoundConditionalReceiver(receiver?.Syntax, 0,
    receiverType ?? CreateErrorType(), 
    hasErrors: receiver?.HasErrors)
    { WasCompilerGenerated = true };
  
  return receiver; 
}
```

We also have to be sure that the constructor supports getting _null_ values for its parameters or we need to perform additional refactoring\.

Other similar errors:

* V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'containingType' object SyntaxGeneratorExtensions\_Negate\.cs 240
* V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions\.cs 349
* V3042 Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions\.cs 349

**Error in the condition**

[V3057](https://pvs-studio.com/en/docs/warnings/v3057/) The 'Substring' function could receive the '\-1' value while non\-negative value is expected\. Inspect the second argument\. CommonCommandLineParser\.cs 109

```cpp
internal static bool TryParseOption(....)
{
  ....
  if (colon >= 0)
  {
      name = arg.Substring(1, colon - 1);
      value = arg.Substring(colon + 1);
  }
  ....
}
```

In case if the _colon _variable is 0, which is fine according to the condition in the code, the _Substring_ method will throw an exception\. This has to be fixed:

```cpp
if (colon > 0)
```

**Possible typo**

[V3065](https://pvs-studio.com/en/docs/warnings/v3065/) Parameter 't2' is not utilized inside method's body\. CSharpCodeGenerationHelpers\.cs 84

```cpp
private static TypeDeclarationSyntax
  ReplaceUnterminatedConstructs(....)
{
  ....
  var updatedToken = lastToken.ReplaceTrivia(lastToken.TrailingTrivia,
    (t1, t2) =>
    {
      if (t1.Kind() == SyntaxKind.MultiLineCommentTrivia)
      {
        var text = t1.ToString();
        ....
      }
      else if (t1.Kind() == SyntaxKind.SkippedTokensTrivia)
      {
        return ReplaceUnterminatedConstructs(t1);
      }
      return t1;
    });
  ....
}
```

The lambda expression accepts two parameters: t1 and t2\. However, only t1 is used\. It looks suspicious, taking into account the fact how easy it is to make a mistake when using variables with such names\.

**Inadvertence**

[V3083](https://pvs-studio.com/en/docs/warnings/v3083/) Unsafe invocation of event 'TagsChanged', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. PreviewUpdater\.Tagger\.cs 37

```cpp
public void OnTextBufferChanged()
{
  if (PreviewUpdater.SpanToShow != default)
  {
    if (TagsChanged != null)
    {
        var span = _textBuffer.CurrentSnapshot.GetFullSpan();
        TagsChanged(this, new SnapshotSpanEventArgs(span));  // <=
    }
  }
}
```

The _TagsChanged _event is invoked in an unsafe way\. Between checking for _null _and invoking the event, someone may unsubscribe from it, then an exception will be thrown\. Furthermore, other operations are performed in the body of the _if_  block right before invoking the event\. I called this error "Inadvertence", because this event is handled more carefully in other places, as follows:

```cpp
private void OnTrackingSpansChanged(bool leafChanged)
{
  var handler = TagsChanged;
  if (handler != null)
  {
      var snapshot = _buffer.CurrentSnapshot;
      handler(this,
              new SnapshotSpanEventArgs(snapshot.GetFullSpan()));
  }
}
```

Usage of an additional _handler _variable prevents the problem\. In the method _OnTextBufferChanged,_ one has to make edits in order to safely handle the event\.

**Intersecting ranges**

[V3092](https://pvs-studio.com/en/docs/warnings/v3092/) Range intersections are possible within conditional expressions\. Example: if \(A \> 0 && A < 5\) \{ \.\.\. \} else if \(A \> 3 && A < 9\) \{ \.\.\. \}\. ILBuilderEmit\.cs 677

```cpp
internal void EmitLongConstant(long value)
{
  if (value >= int.MinValue && value <= int.MaxValue)
  {
    ....
  }
  else if (value >= uint.MinValue && value <= uint.MaxValue)
  {
    ....
  }
  else
  {
    ....
  }
}
```

For better understanding, let me rewrite this code, changing the names of the constants with their actual values:

```cpp
internal void EmitLongConstant(long value)
{
  if (value >= -2147483648 && value <= 2147483648)
  {
    ....
  }
  else if (value >= 0 && value <= 4294967295)
  {
    ....
  }
  else
  {
    ....
  }
}
```

Probably, there is no real error, but the condition looks strange\. Its second part \(_else if_\)  will be executed only for the range from 2147483648 \+ 1 to 4294967295\.

Another couple of similar warnings:

* V3092 Range intersections are possible within conditional expressions\. Example: if \(A \> 0 && A < 5\) \{ \.\.\. \} else if \(A \> 3 && A < 9\) \{ \.\.\. \}\. LocalRewriter\_Literal\.cs 109
* V3092 Range intersections are possible within conditional expressions\. Example: if \(A \> 0 && A < 5\) \{ \.\.\. \} else if \(A \> 3 && A < 9\) \{ \.\.\. \}\. LocalRewriter\_Literal\.cs 66

**More about checks for null \(or lack of them\)**

A couple of [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) errors on the check of a variable for null right after its usage\. The first is ambiguous, let's consider the code\.

V3095 The 'displayName' object was used before it was verified against null\. Check lines: 498, 503\. FusionAssemblyIdentity\.cs 498

```cpp
internal static IAssemblyName ToAssemblyNameObject(string displayName)
{
  if (displayName.IndexOf('\0') >= 0)
  {
      return null;
  }
  
  Debug.Assert(displayName != null);
  ....
}
```

It is assumed that the reference_ displayName_ can be null\. For this, the check _Debug\.Assert _was performed\.  It is not clear why it goes after using a string\. It also has to be taken into account that for configurations different from Debug, the compiler will remove _Debug\.Assert _at all_\. _ Does it mean that getting a null reference is possible only for Debug? If it is not so, why did the author make the check of_ string\.IsNullOrEmpty\(string\)_, for example\. It is the question to authors of the code\.

The following error is more evident\.

V3095 The 'scriptArgsOpt' object was used before it was verified against null\. Check lines: 321, 325\. CommonCommandLineParser\.cs 321

```cpp
internal void FlattenArgs(...., List<string> scriptArgsOpt, ....)
{
  ....
  while (args.Count > 0)
  {
    ....
    if (parsingScriptArgs)
    {
        scriptArgsOpt.Add(arg);  // <=
        continue;
    }
    
    if (scriptArgsOpt != null)
    {
      ....
    }
    ....
  }
}
```

I think this code doesn't need any explanations\. Let me give you the fixed version:

```cpp
internal void FlattenArgs(...., List<string> scriptArgsOpt, ....)
{
  ....
  while (args.Count > 0)
  {
    ....
    if (parsingScriptArgs)
    {
        scriptArgsOpt?.Add(arg);
        continue;
    }
    
    if (scriptArgsOpt != null)
    {
      ....
    }
    ....
  }
}
```

In Roslyn code, there were 15 more similar errors:

* V3095 The 'LocalFunctions' object was used before it was verified against null\. Check lines: 289, 317\. ControlFlowGraphBuilder\.RegionBuilder\.cs 289
* V3095 The 'resolution\.OverloadResolutionResult' object was used before it was verified against null\. Check lines: 579, 588\. Binder\_Invocation\.cs 579
* V3095 The 'resolution\.MethodGroup' object was used before it was verified against null\. Check lines: 592, 621\. Binder\_Invocation\.cs 592
* V3095 The 'touchedFilesLogger' object was used before it was verified against null\. Check lines: 111, 126\. CSharpCompiler\.cs 111
* V3095 The 'newExceptionRegionsOpt' object was used before it was verified against null\. Check lines: 736, 743\. AbstractEditAndContinueAnalyzer\.cs 736
* V3095 The 'symbol' object was used before it was verified against null\. Check lines: 422, 427\. AbstractGenerateConstructorService\.Editor\.cs 422
* V3095 The '\_state\.BaseTypeOrInterfaceOpt' object was used before it was verified against null\. Check lines: 132, 140\. AbstractGenerateTypeService\.GenerateNamedType\.cs 132
* V3095 The 'element' object was used before it was verified against null\. Check lines: 232, 233\. ProjectUtil\.cs 232
* V3095 The 'languages' object was used before it was verified against null\. Check lines: 22, 28\. ExportCodeCleanupProvider\.cs 22
* V3095 The 'memberType' object was used before it was verified against null\. Check lines: 183, 184\. SyntaxGeneratorExtensions\_CreateGetHashCodeMethod\.cs 183
* V3095 The 'validTypeDeclarations' object was used before it was verified against null\. Check lines: 223, 228\. SyntaxTreeExtensions\.cs 223
* V3095 The 'text' object was used before it was verified against null\. Check lines: 376, 385\. MSBuildWorkspace\.cs 376
* V3095 The 'nameOrMemberAccessExpression' object was used before it was verified against null\. Check lines: 206, 223\. CSharpGenerateTypeService\.cs 206
* V3095 The 'simpleName' object was used before it was verified against null\. Check lines: 83, 85\. CSharpGenerateMethodService\.cs 83
* V3095 The 'option' object was used before it was verified against null\. Check lines: 23, 28\. OptionKey\.cs 23

Let's consider [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) errors\. Here the conditional _null _operator is used when initializing the variable, but further the variable is used without checks for _null_\. 

Two warnings indicate the following error:

V3105 The 'documentId' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. CodeLensReferencesService\.cs 138

V3105 The 'documentId' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. CodeLensReferencesService\.cs 139

```cpp
private static async Task<ReferenceLocationDescriptor>
  GetDescriptorOfEnclosingSymbolAsync(....)
{
  ....
  var documentId = solution.GetDocument(location.SourceTree)?.Id;
  
  return new ReferenceLocationDescriptor(
    ....
    documentId.ProjectId.Id,
    documentId.Id,
    ....);
}
```

The _documentId_ variable can be initialized by _null_\. As a result, creating an object _ReferenceLocationDescriptor_ will result in throwing an exception\. The code must be fixed:

```cpp
return new ReferenceLocationDescriptor(
  ....
  documentId?.ProjectId.Id,
  documentId?.Id,
  ....);
```

Developers should also cover the possibility of variables, passed to a constructor, to be _null\._

Other similar errors in the code:

* V3105 The 'symbol' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. SymbolFinder\_Hierarchy\.cs 44
* V3105 The 'symbol' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. SymbolFinder\_Hierarchy\.cs 51

**Priorities and parentheses**

[V3123](https://pvs-studio.com/en/docs/warnings/v3123/) Perhaps the '?:' operator works in a different way than it was expected\. Its priority is lower than the priority of other operators in its condition\. Edit\.cs 70

```cpp
public bool Equals(Edit<TNode> other)
{
  return _kind == other._kind
    && (_oldNode == null) ? other._oldNode == null :
        _oldNode.Equals(other._oldNode)
    && (_newNode == null) ? other._newNode == null :
       _newNode.Equals(other._newNode);
}
```

The condition in the return block is evaluated not as the developer intended\. It was assumed that the first condition will be _\_kind \=\= other\.\_kin_d, \(this is why after this condition there is a line break\), and after that the blocks of conditions with the operator "_?_" will be evaluated in sequence\. In fact, the first condition is  _\_kind \=\= other\.\_kind && \(\_oldNode \=\= null\)_\. This is due to the fact that the operator _&&_ has a higher priority than operator "_?_"\. To fix this, a developer should take all expressions of the operator "_?_"  in parentheses:

```cpp
return _kind == other._kind
    && ((_oldNode == null) ? other._oldNode == null :
        _oldNode.Equals(other._oldNode))
    && ((_newNode == null) ? other._newNode == null :
       _newNode.Equals(other._newNode));
```

That concludes my description of the errors found\.

**Conclusion** 

Despite the large number of errors, which I managed to find, in terms of the size of the Roslyn project code \(2 770 000 lines\), it is not too much\. As Andrey wrote in a previous article, I am also ready to acknowledge the high quality of this project\.

I'd like to note that such occasional code checks have nothing to do with the methodology of static analysis and are almost unhelpful\. Static analysis should be applied regularly, and not on a case\-by\-case basis\. This way, many errors will be corrected at the earliest stages, and hence the cost of fixing them will be ten times less\. This idea is set forth in more detail in this little [note](https://pvs-studio.com/en/blog/posts/0534/), please, check it out\.

You can check yourself some errors both in this project and in another\. To do this, you just need to [download](https://pvs-studio.com/en/pvs-studio/download/) and try our analyzer\.