# Check of Roslyn Analyzers

Many may have noticed that the PVS\-Studio C\# analyser uses Roslyn \(\.NET compiler platform\) to obtain input data\. Therefore, when we came across the project "Roslyn Analyzers" from Microsoft, checking it with PVS\-Studio became inevitable\. The project is an extension for Visual Studio, contains analytics of errors, style, and code complexity\. Knowing the features of Roslyn allowed us to better understand, what Roslyn Analyzers' developers wanted to implement\. So in our opinion, the check turned out to be quite engaging for our team\.

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

## Introduction

The source code of Roslyn Analyzers can be downloaded from this [repository](https://github.com/dotnet/roslyn-analyzers)\. The repository also contains usage guidelines and a full description of its functionality\. To check the code I used the PVS\-Studio static code analyser, version 7\.03\.

This article is not intended to compare analysers\. Well, for a [number of reasons](https://pvs-studio.com/en/blog/posts/0637/), we don't even want to write such articles\. Both analysers are good in their own way and find different errors\. So this is the article on the errors found in Roslyn Analyzers\.

At the same time, we checked the code of PVS\-Studio using Roslyn Analyzers\. Nothing remarkable was found, so there is nothing to write on this topic\. Of the useful, we only had recommendations to replace_ _the equality operator \(\=\=\) with _Equals_\. In addition, we found several false positives and added exceptions to our analyser for similar patterns\.

I think I must note the high quality of the Roslyn Analyzers' code\. The PVS\-Studio analyser issued only 31 warnings \(of High certainty level\) and 67 warnings \(Medium certainty level\) for its code per 400,000 lines of code\.

It may be hard to read an article without previous experience of working with Roslyn\. So I'll be doing small italic inserts explaining the platform features\. Skip these places if you understand the code\. If you want to understand the essence of Roslyn in depth, you're welcome to read the article: [Introduction to Roslyn](https://pvs-studio.com/en/blog/posts/csharp/0399/)\. Some of the inserts are copied right from this article\.

## Errors

**PVS\-Studio warning:** [V3127](https://pvs-studio.com/en/docs/warnings/v3127/) Two similar code fragments were found\. Perhaps, this is a typo and 'leadingTrivia' variable should be used instead of 'trailingTrivia' UseLiteralsWhereAppropriate\.Fixer\.cs 76

```cpp
private async Task<Document> ToConstantDeclarationAsync(...)
{
  ....
  if (leadingTrivia.Count == 0 && trailingTrivia.Count == 0)
  {
     leadingTrivia = leadingTrivia.AddRange(modifier.LeadingTrivia);
     trailingTrivia = trailingTrivia.AddRange(modifier.TrailingTrivia);
  }
  else
  {
     trailingTrivia = trailingTrivia.AddRange(modifier.LeadingTrivia);  // <=
     trailingTrivia = trailingTrivia.AddRange(modifier.TrailingTrivia); // <=
     ....//here Trivia and trailingTrivia are handled
  }
....
}
```

_Trivia \(additional syntax information\) are those elements of the tree, which won't be compiled into IL\-code\. These include elements of formatting \(spaces, line feed characters\),comments, preprocessor directives\.  Are located in the tree with connection to other nods\. The binding can be before the node \- LeadingTrivia, or after \- TrailingTrivia\._

This code checks the number of elements in _leadingTrivia_ and _trailingTrivia_ arrays\. If there are no elements \- they are added in local _leadingTrivia_ and _trailingTrivia_ arrays\._ _If there are elements in arrays \- they are all added only in _trailingTrivia_ \(which was noticed by our analyser\)\. 

Perhaps, in the _else _branch the code author copied handling of the _trailingTrivia _array, but forgot to change the array for _leadingTrivia_, the same as it was made in another _if_ branch\.

On the other hand, in doing so, both lines of code would be the same and could be removed from the condition\. So it's not very clear, but something's wrong with the code\.

**PVS\-Studio warning:** [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) There are identical sub\-expressions 'data1\.IsReachableBlockData' to the left and to the right of the '\=\=' operator\. AnalysisEntityBasedPredicateAnalysisData\.cs 39

```cpp
protected AnalysisEntityBasedPredicateAnalysisData(....)
  : base(....)
{
  Debug.Assert(data1.IsReachableBlockData == data1.IsReachableBlockData);
  ....
}
```

Here in the condition the variable is compared to itself, which clearly doesn't make sense\. In any case, in addition to editing this code, I suggest that developers of Roslyn Analyzers implement an analogue of our [V3001](https://pvs-studio.com/en/docs/warnings/v3001/) diagnostic \(on the comparison of identical sub\-expressions\)\. 

**PVS\-Studio warning:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference of method return value\. Consider inspecting: GetCandidateReferencedSymbols\(\.\.\.\)\. SyntaxNodeHelper\.cs 78

```cpp
public static IEnumerable<IMethodSymbol> GetCandidateCalleeMethodSymbols(
  SyntaxNode node, SemanticModel semanticModel)
{
  foreach (ISymbol symbol in GetCandidateReferencedSymbols(
                              node, semanticModel))
  {
     if (symbol != null && symbol.Kind == SymbolKind.Method)
     {
        yield return (IMethodSymbol)symbol;
     }
  }
}
```

If we consider the method _GetCandidateReferencedSymbols_, we can see that it can return the _null_ value:

```cpp
public static IEnumerable<ISymbol> GetCandidateReferencedSymbols(
  SyntaxNode node, SemanticModel semanticModel)
{
  if (node == null)
  {
     return null;
  }
  return semanticModel.GetSymbolInfo(node).CandidateSymbols;
}
```

_ISymbol is the base interface of the symbol, which provides methods that are common for all the objects, regardless of what they are \- fields, properties or_ _something_ _else\._ 

Indeed, if the _node_ isn't assigned, _null_ can get into the enumeration, resulting in _NullReferenceException_\. The code can be fixed either by throwing an exception right from the method _GetCandidateReferencedSymbols_, or by adding a check after getting a value from it\. I suggest that we choose the second, safer way:

```cpp
public static IEnumerable<IMethodSymbol> GetCandidateCalleeMethodSymbols(
  SyntaxNode node, SemanticModel semanticModel)
{
  var candidateReferencedSymbols = GetCandidateReferencedSymbols(...);
  if(candidateReferencedSymbols != null)
  {
    foreach (ISymbol symbol in candidateReferencedSymbols)
    {
       if (symbol != null && symbol.Kind == SymbolKind.Method)
          yield return (IMethodSymbol)symbol;
    }
  }
}
```

**PVS\-Studio warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'valueClauseName' object was used after it was verified against null\. Check lines: 2320, 2318\. DiagnosticAnalyzer\.cs 2320

```cpp
private SuppDiagReturnSymbolInfo SuppDiagReturnSymbol(....)
{
  ....
  var valueClauseName = valueClauseMemberAccess.Name as IdentifierNameSyntax;
  if (valueClauseName == null 
      || valueClauseName.Identifier.Text != "Create")
     {
     ReportDiagnostic(context, 
       SuppDiagReturnValueRule, 
       valueClauseName.GetLocation(),                  // <=
       propertyDeclaration.Identifier.Text); 
     return result;
     }
  ....
}
```

_MemberAccessExpressionSyntax is a class that reflects access to a method, property or a field of a certain element\. The class has two properties: Expression \(left part\) and Name \(right part\)\._

The analyser noticed dereference right after checking for _null_\. The best option is to get _NullReferenceException_\. But those who are familiar with Roslyn might ask: what's the error? For trivial examples of fields or properties, _Name _will definitely always be _IdentifierNameSyntax_\. As soon as the generic method is called, the type will become _GenericNameSyntax_, which cannot be cast to _IdentifierNameSyntax_\. I'm not sure if this method can handle the call of the generic method, but I would foresee this case if I were at the developers' place\. 

**PVS\-Studio warning:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'oldIdName'\. CodeFixProvider\.cs 1476

It's a pretty large method\. No panic\. You can scroll it, I'll describe the important points below\.

```cpp
private async Task<Document> IdDeclTypeAsync(....)
{
  ....
  ExpressionSyntax oldIdName = null;
  foreach (MemberDeclarationSyntax memberSyntax in members)
  {
     var fieldDeclaration = memberSyntax as FieldDeclarationSyntax;
     if (fieldDeclaration == null)
       continue;
     if (fieldDeclaration.Declaration.Type is IdentifierNameSyntax fieldType
         && fieldType.Identifier.Text == "DiagnosticDescriptor")
     {
       ....
       for (int i = 0; i < ruleArgumentList.Arguments.Count; i++)
       {
         ArgumentSyntax currentArg = ruleArgumentList.Arguments[i];
         string currentArgName = currentArg.NameColon.Name.Identifier.Text;
         if (currentArgName == "id")
         {
           oldIdName = currentArg.Expression;
           break;
         }
       }
       continue;
    }
    ....
  }
  var newRule = rule.ReplaceNode(oldIdName.Ancestors()   // <=
                    .OfType<ArgumentSyntax>()
                    .First(), newArg);
  ...
}
```

So what's going on here: _oldIdName_ is initialized by a null reference\. The following conditions must be met to assign an object to _oldIdName_:

1. in the considered method there are declarations of a field named _DiagnosticDescriptor_;
1. an object created through a constructor is assigned to this field;
1. The constructor has a parameter named _id_\.

If the conditions are not favourable, _NullReferenceException _will be thrown when attempting to obtain _Ancestors\._ That is, either the method crashes when calling it or the developer is confident that a declaration of this field will be in the method\. For example, these conditions have been checked earlier\. Or this is the method created by a code generator\. In any case, this code is quite vulnerable to changes\. 

Ways to remedy this situations depend on what function had to the executed\. It's worth adding the _oldIdName_ check and exit, or, for example, throw an exception\.

**PVS\-Studio warning:** [V3095](https://pvs-studio.com/en/docs/warnings/v3095/) The 'rule' object was used before it was verified against null\. Check lines: 2180, 2181\. CodeFixProvider\.cs 2180

```cpp
internal static string GetFirstRuleName(ClassDeclarationSyntax declaration)
{
  SyntaxList<MemberDeclarationSyntax> members = declaration.Members;
  FieldDeclarationSyntax rule = null;

  foreach (MemberDeclarationSyntax member in members)
  {
     rule = member as FieldDeclarationSyntax;
     var ruleType = rule.Declaration.Type as IdentifierNameSyntax; // <=
     if (rule != null 
         && ruleType != null 
         && ruleType.Identifier.Text == "DiagnosticDescriptor")
       {break;}
     rule = null;
  }
  ....
}
```

_ClassDeclarationSyntax is a class presentation in Roslyn\. The property Members contains nodes of all class elements \(field, property, methods, other classes and structures\)\._

I even double\-checked the behavior of _Members _when I saw this code\. The developer was confident, that the first declaration would be a field's declaration\. But in _Members, _elements are written in the order of their declaration in the class\. The order of declarations doesn't change\. So may be we'll try to get the declaration type from a non\-existent field\. In this case, _NullRefenceException_ will be thrown\. The developer was aware that there might not be a field and added the check\.\.\. but later than it should be\. 

When editing the code, I'd rewrite the method using _Linq_\.

```cpp
internal static string GetFirstRuleName(ClassDeclarationSyntax declaration)
{
  SyntaxList<MemberDeclarationSyntax> members = declaration.Members;
  FieldDeclarationSyntax rule = 
    members.OfType<FieldDeclarationSyntax>()
      .FirstOrDefault(x =>(x.Declaration.Type as IdentifierNameSyntax)?
        .Identifier.Text == "DiagnosticDescriptor");
  ....
}
```

It looks a bit worse, but conveys the essence better\.

**PVS\-Studio warning:** [V3137](https://pvs-studio.com/en/docs/warnings/v3137/) The 'sourceOrigins' variable is assigned but is not used by the end of the function\. TaintedDataAnalysis\.TaintedDataOperationVisitor\.cs 328

```cpp
public override TaintedDataAbstractValue VisitArrayInitializer(
  IArrayInitializerOperation operation,
  object argument)
{
  HashSet<SymbolAccess> sourceOrigins = null;
  ...
  if (baseAbstractValue.Kind == TaintedDataAbstractValueKind.Tainted)
  {
     sourceOrigins = new HashSet<SymbolAccess>(...);
  }
  ....
}
```

Actually, there is nothing to add to the message of the analyser\. The field is really no longer used below in the method\. No conditional compilation directives, no returns by _ref_\. Not a single reference\.\.\. it is not clear what this creature is for\.

**PVS\-Studio warning:** [V3080](https://pvs-studio.com/en/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'methodDeclaration'\. DiagnosticAnalyzer\.cs 506

```cpp
private bool CheckIfStatementAnalysis(...
  IMethodSymbol analysisMethodSymbol)
{
  var methodDeclaration = AnalysisGetStatements(analysisMethodSymbol)
                           as MethodDeclarationSyntax;
  var body = methodDeclaration.Body as BlockSyntax;
  if (body == null)
  { return false; }
  ....
}
```

The analyser warns that the _AnalysisGetStatements _method can return _null_\. Let's have a look at it\.

```cpp
private MethodDeclarationSyntax AnalysisGetStatements(
   IMethodSymbol
   analysisMethodSymbol)
{
  MethodDeclarationSyntax result = null;
  if (analysisMethodSymbol == null)
  {
     return result;
  }
  var methodDeclaration = analysisMethodSymbol
                            .DeclaringSyntaxReferences[0]
                            .GetSyntax() as MethodDeclarationSyntax;
  if (methodDeclaration == null)
  {
     return result;
  }
  return methodDeclaration;
}
```

_MethodDeclarationSyntax is a representation of a method declaration in Roslyn\. Although it is not essential here \- just for the sake of satisfying possible curiosity\._

If I get it right, a new entity is created here\. The value of this variable doesn't change, but the variable is returned from the function twice\. There is a feeling that the code is not finished\.

**PVS\-Studio warning:** [V3125](https://pvs-studio.com/en/docs/warnings/v3125/) The 'ifStatement' object was used after it was verified against null\. Check lines: 788, 773\. CodeFixProvider\.cs 788

```cpp
private async Task<Document> TriviaCountIncorrectAsync(
  MethodDeclarationSyntax declaration)
{
  SyntaxGenerator generator = SyntaxGenerator.GetGenerator(document);
  ....
  var ifStatement = declaration.Body.Statements[2] as IfStatementSyntax;
  if (ifStatement != null)
  {
    ....
  }
  ....
  var oldBlock = ifStatement.Statement as BlockSyntax;
  ....
}
```

_IfStatementSyntax is a representation of if condition in Roslyn\.  I'll highlight two properties \- Condition, Statement\. They contain representations of entry conditions and executable code when executing the condition\._

_If in Statement the code is in curly braces \{\}, the type this node will be BlockSyntax\. This way, it's possible to get an array of expressions from it through the property Statements\._

The analyzer triggered for _ifStatement _dereference without a check\. Note that the needed check took place earlier along the code\. I'd say, it's quite dangerous to cast the _IfStatementSyntax\.Statement _ type to _BlockSyntax_ without checking\. The fact of the matter is that the condition can be written in two ways: 

```cpp
if (true)
{
  var A = b;
}
```

or as follows: 

```cpp
if (true)
   var A = b;
```

When omitting curly brackets, _Statement _won't be of the _BlockSyntax _type, it'll be _ExpressionStatementSyntax_\. 

On the other hand, getting _ifStatement _looks as follows: _declaration\.Body\.Statements\[2\], _without checking the length of the_ Statements _array_\._ So developers are sure that there will be a condition\. Perhaps, the clue to this method is in getting _generator_, even though it has nothing to do with _ifStatement_\. Anyway, I think that the check is necessary, at least for a more meaningful exception\.

**PVS\-Studio warning:** [V3139](https://pvs-studio.com/en/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. CodeMetricsAnalyzer\.cs 251

```cpp
static bool isApplicableByDefault(string ruleId, SymbolKind symbolKind)
{
  switch (ruleId)
  {
     ....
     case CA1505RuleId:
       switch (symbolKind)
       {
          case SymbolKind.NamedType:
          case SymbolKind.Method:
          case SymbolKind.Field:
          case SymbolKind.Property:
          case SymbolKind.Event:
            return true;
          default:
            return false;
       }
     case CA1506RuleId:
       switch (symbolKind)
       {
          case SymbolKind.NamedType:
          case SymbolKind.Method:
          case SymbolKind.Field:
          case SymbolKind.Property:
          case SymbolKind.Event:
            return true;
          default:
            return false;
       }
     default:
       throw new NotImplementedException();
  }
}
```

Perhaps different behavior was meant for 1505 and 1506 rules\. This way, we found a real error\. But there is a chance that it's made intentionally to change the behavior later\. Or may be the developer forgot for a second that conditions could be grouped\. 

Let's suggest that the code works correctly and the analyser complains only about the code style\. Though we don't have diagnostics for bad style\. This way, the best option to get rid of a warning, and a Copy\-Paste error in code is to group the conditions:

```cpp
static bool isApplicableByDefault(string ruleId, SymbolKind symbolKind)
{
  switch (ruleId)
  {
     ....
     case CA1505RuleId:
     case CA1506RuleId:
       switch (symbolKind)
       {
          case SymbolKind.NamedType:
          case SymbolKind.Method:
          case SymbolKind.Field:
          case SymbolKind.Property:
          case SymbolKind.Event:
            return true;
          default:
            return false;
        }
     default:
       throw new NotImplementedException();
  }
}
```

**PVS\-Studio warning:** [V3105](https://pvs-studio.com/en/docs/warnings/v3105/) The 'lastField' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. UseLiteralsWhereAppropriate\.cs 63

A curious case: in fact, this warning is a false positive, but when delving into the code I found another potential error\.

```cpp
public override void Initialize(AnalysisContext analysisContext)
{
  var fieldInitializer = saContext.Operation as IFieldInitializerOperation;
  analysisContext.RegisterOperationAction(saContext =>
     { 
       var lastField = fieldInitializer?.InitializedFields.LastOrDefault();
       var fieldInitializerValue = fieldInitializer?.Value;
       if (fieldInitializerValue == null || lastField.IsConst ...)
          return;
     }
  ....
}
```

_IFieldInitializerOperation interface of a field declaration\. InitializedFields enables to get all declarations in case of overriding the field in an derived class, for example\. Very rarely an array can be empty and most likely it is a compilation error\._

This code checks condition in a way, which is tricky for our analyser in terms of its current level of development\. The connection between _lastField_ and _fieldInitializerValue_ isn't obvious to the analyser and the warning is incorrect\.

The check _fieldInitializerValue \=\= null _checks _lastField _as well\.  Since we've initiated the check \- let's pay attention to the call _LastOrDefault_\. For reference types, the method might return _null_\. The type _InitializedFields_ \- _ImmutableArray<IFieldSymbol\>_\. A developer uses the _LastOrDefault_ method\. But in case if the list of initialized fields doesn't contain a single character, we'll get a general exception _NullReferenceException_\. I suggest using _Last_ to get a more meaningful exception\.  

## Briefly about the Tests

Roslyn Analyzers takes a curious approach to unit tests\. Methods store long string literals, which contain classes for checking a certain diagnostic\. I think, writing such code isn't convenient, since IntelliSence doesn't work inside literals\.

I would suggest our approach instead: creating classes for each diagnostic rule\. Further these classes are added in resources as files and are retrieved in tests for using specific diagnostics\.

We have at least two classes for each diagnostic, with false and correct warnings \(yes, special hodgie code is written there\)\. No, we don't have vacancies of hodgie coders :\)\. Unit tests traverse files by certain rules and notify if errors were found in false ones and there are no errors in good ones\. When analyzing our unit test base, we can get more than 10,000 warnings\. Sure, Roslyn Analyzers' tests might be located in a separate repository\. It is also possible that a fundamentally different approach is used there\. I haven't studied the insights of Roslyn Analyzers in more details\.

## Conclusions

At the moment, Roslyn Analyzers isn't the largest project from all open source static code analysers\. One of the main goals of the project is usage of its diagnostics for writing own ones\. In this regard, its high code quality gets even more important\. I hope our article helped to make the project a little better\.  

For those who are choosing what static analyser to use for own project, I'd suggest using several ones\. Various analysers complement each other\. If the price of making an error in your project is high, its better to be insured by all possible means\. However, we shouldn't forget that analysers should be up\-to\-date\. Adding outdated analysers in a project can make it even worse, as it can give a false sense of security\.