# Проверяем Roslyn analyzers

Многие могли заметить, что C\# анализатор PVS\-Studio использует Roslyn \(\.NET compiler platform\) для получения входных данных\. Поэтому, когда мы встретили проект "Roslyn analyzers" от компании Microsoft, его проверка с помощью PVS\-Studio стала неизбежным событием\. Проект представляет собой расширение для Visual Studio, содержит аналитики ошибок, стиля и сложности кода\. Знание особенностей Roslyn позволило нам лучше понять, что хотели сделать разработчики "Roslyn analyzers"\. Поэтому, на наш взгляд, проверка получилась довольно занятной для нашей команды\.

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

## Введение

Исходный код "Roslyn Analyzers" можно скачать из этого [репозитория](https://github.com/dotnet/roslyn-analyzers)\. В репозитории также можно найти руководство по использованию и полное описание функционала\. Для проверки же кода я использовал статический анализатор кода PVS\-Studio версии 7\.03\.

Эта статья не имеет целью сравнение анализаторов\. И вообще [по ряду причин](https://pvs-studio.ru/ru/blog/posts/0637/) мы не хотим писать подобные статьи\. Оба анализатора по\-своему хороши и находят разные ошибки\. Перед вами статья о найденных ошибках в "Roslyn analyzers"\.

Заодно мы проверили и код нашего анализатора PVS\-Studio с помощью "Roslyn Analyzers"\. Ничего примечательного не нашлось, так что написать на эту тему нечего\. Из полезного нам показались только рекомендации по замене оператора равенства \(\=\=\) на _Equals_\. Кроме того, мы нашли несколько ложных срабатываний и добавили в наш анализатор исключения на подобные паттерны\.

Считаю обязательным отметить высокое качество кода "Roslyn Analyzers"\. Анализатор PVS\-Studio выдал на его код всего 31 предупреждение \(достоверности уровня high\) и 67 предупреждений \(достоверности уровня medium\) на примерно 400 тысяч строк кода\.

Возможно, вам будет тяжело читать статью без опыта работы с Roslyn\. Поэтому я буду делать небольшие вставки курсивом, поясняющие особенности работы платформы\. Пропускайте эти места, если код вам понятен\. Если же вы захотите глубже понять сущности в Roslyn – можете прочесть статью: [введение в Roslyn](https://pvs-studio.ru/ru/blog/posts/csharp/0399/)\. Часть вставок скопированы как раз из этой статьи\.

## Ошибки

**Предупреждение PVS\-Studio**: [V3127](https://pvs-studio.ru/ru/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); // <=
     .... //здесь проводится работа с leadingTrivia и trailingTrivia
  }
....
}
```

_Trivia \- \(дополнительная синтаксическая информация\) – это те элементы дерева, которые не будут скомпилированы в IL\-код\. К таким элементам относятся элементы форматирования \(пробелы, символы перевода строки\), комментарии, директивы препроцессора\. Располагаются в дереве с привязкой к другим нодам\. Привязка может быть до ноды – LeadingTrivia, или после – TrailingTrivia\._

В данном коде проверяется количество элементов в массивах _leadingTrivia_ и _trailingTrivia_\. Если элементов нет \- они добавляются в локальные массивы _leadingTrivia_ и _trailingTrivia\._ Если же в массивах были элементы \- они все добавляются только в _trailingTrivia_ \(на что обратил внимание наш анализатор\)\. 

Возможно, в ветке _else_ скопировали обработку массива _trailingTrivia_, но забыли поменять массив на _leadingTrivia_, как это было сделано в другой ветке _if_\.

С другой стороны, тогда обе строки кода были бы идентичны и их можно было бы вынести из условия\. В общем не понятно, но с кодом явно что\-то не так\.

**Предупреждение PVS\-Studio**: [V3001](https://pvs-studio.ru/ru/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);
  ....
}
```

Здесь в условии переменная сравнивается сама с собой, что явно не имеет смысла\. В любом случае, кроме правки данного кода предлагаю разработчикам "Roslyn Analyzers" реализовать аналог нашей диагностики [V3001](https://pvs-studio.ru/ru/docs/warnings/v3001/) \(о сравнении идентичных подвыражений\)\. 

**Предупреждение PVS\-Studio**: [V3080](https://pvs-studio.ru/ru/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;
     }
  }
}
```

Если посмотреть на метод _GetCandidateReferencedSymbols,_ то видно, что он может вернуть значение _null_:

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

_ISymbol – базовый интерфейс символа, предоставляет методы и свойства, общие для всех объектов, независимо от того, чем они являются – полем, свойством или чем\-то ещё\._

Действительно, если _node_ не назначен,в перебор может попасть _null_, что приведёт к _NullReferenceException_\. Исправить код можно либо выбрасывая исключение сразу из метода _GetCandidateReferencedSymbols_, либо добавив проверку после получения значения из него\. Предлагаю пойти вторым, более безопасным путём:

```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**: [V3125](https://pvs-studio.ru/ru/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 – класс отражающий доступ к методу, свойству или полю некоторого элемента\. У класса есть два свойства: Expression \(левая часть\) и Name \(правая часть\)\._

Анализатор увидел разыменование сразу после проверки на _null_\. Самый верный способ получить _NullReferenceException_\. Но знакомые с Roslyn спросят: а в чём ошибка? Для тривиальных полей или свойств, _Name_ действительно всегда будет _IdentifierNameSyntax_\. Но как только появится вызов generic метода, тип станет _GenericNameSyntax_, который не приводится к _IdentifierNameSyntax_\. Не знаю, может ли этот метод получить в обработку вызов generic метода, но я бы на месте разработчиков предусмотрел и этот случай\. 

**Предупреждение PVS\-Studio**: [V3080](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'oldIdName'\. CodeFixProvider\.cs 1476

Довольно большой метод\. Без паники\. Можете его промотать, я опишу в конце важные моменты\.

```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);
  ...
}
```

Итак, что здесь происходит: _oldIdName_ инициализируется нулевой ссылкой\. Для присвоения в _oldIdName_ некоторого объекта должны быть соблюдены следующие условия:

1. в рассматриваемом методе есть объявления поля с именем _DiagnosticDescriptor_;
1. этому полю присваивается объект, созданный через конструктор;
1. у конструктора есть параметр с именем _id_\.

Если условия не сойдутся, то при попытке получения _Ancestors _будет выброшен _NullReferenceException_\. То есть либо метод иногда падает при вызове, либо разработчик точно уверен, что объявление такого поля будет в методе\. Например, эти же условия проверяются ранее\. Или именно такой метод создаётся кодогенератором\. В любом случае такой код довольно уязвим при изменениях\. 

Пути исправления ситуации зависят от того, какую функцию код должен был выполнять\. Стоит добавить проверку _oldIdName_ и выход, или, например, выброс исключения\.

**Предупреждение PVS\-Studio**: [V3095](https://pvs-studio.ru/ru/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 – представление класса в Roslyn\. Свойство Members содержит узлы всех элементов класса \(поля, свойства, методы, другие классы и структуры\)\._

Я даже перепроверил поведение _Members_, когда увидел этот код\. Разработчик был уверен, что первое же объявление будет объявлением поля\. Но в _Members_ элементы записываются в порядке записи в классе\. Порядок объявлений не меняется\. Значит, возможно, мы попытаемся получить тип декларации из несуществующего поля\. В этом случае будет выброшен _NullRefenceException_\. Разработчик понимал, что поля может не быть, поэтому добавил проверку\.\.\. но ниже, чем следовало бы\. 

При правке кода я бы переписал метод с использованием _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");
  ....
}
```

Выглядит немного хуже, зато лучше передаёт суть\.

**Предупреждение PVS\-Studio**: [V3137](https://pvs-studio.ru/ru/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>(...);
  }
  ....
}
```

Собственно, тут особо добавить к сообщению анализатора нечего\. Поле действительно больше не используется ниже в методе\. Нет директив условной компиляции, нет возврата по _ref_\. Ни одной ссылки\.\.\. для чего это создание – не понятно\.

**Предупреждение PVS\-Studio**: [V3080](https://pvs-studio.ru/ru/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; }
  ....
}
```

Анализатор предупреждает, что метод _AnalysisGetStatements_ может вернуть _null_\. Посмотрим на него:

```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 – представление объявления метода в Roslyn\. Хотя здесь это не существенно – просто ради удовлетворения возможного любопытства\._

Если я правильно понял, здесь создаётся новая сущность result\. Значение этой переменной не изменяется, зато она возвращается из функции целых два раза\. Есть ощущение, что код не дописан\.

**Предупреждение PVS\-Studio**: [V3125](https://pvs-studio.ru/ru/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 – представление условия if в Roslyn\. Среди свойств выделю два: Condition, Statement\. Содержат представления условий входа и исполняемый код при выполнении условия\._

_Если в Statement код в фигурных скобках \{\}, тип этого узла будет BlockSyntax\. Тогда у него можно будет получить массив выражений, через свойство Statements\._

Анализатор сработал на разыменование _ifStatement_ без проверки\. Отметим, что выше по коду нужная проверка была\. Я бы сказал, что довольно опасно приводить тип _IfStatementSyntax\.Statement _к _BlockSyntax_ без проверки\. Дело в том, что можно записать условие двумя способами: 

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

Или так:

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

При записи без фигурных скобок _Statement _не будет типа _BlockSyntax_, это будет _ExpressionStatementSyntax_\. 

С другой стороны, получение _ifStatement_ выглядит так: _declaration\.Body\.Statements\[2\],_ без проверки длины массива _Statements_\. Значит, разработчики точно уверены, что там будет условие\. Возможно, разгадка задачи данного метода в получении _generator_, хотя он никак не связан с _ifStatement_\. В любом случае я считаю, что проверка нужна, хотя бы для выброса более осмысленного исключения\.

**Предупреждение PVS\-Studio**: [V3139](https://pvs-studio.ru/ru/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();
  }
}
```

Возможно, для правил 1505 и 1506 планировалось разное поведение, тогда мы нашли настоящую ошибку\. Но, возможно, это сделано на будущее, чтобы изменить поведение позднее\. А также вероятно, что разработчик на секунду забыл, что условия можно группировать\. 

Предположим, что код работает корректно и анализатор просто ругается на стиль, хотя мы и не делаем диагностик для стиля\. Тогда лучший способ избавить себя от предупреждения, а код от копипасты – группировка условий:

```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**: [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) The 'lastField' variable was used after it was assigned through null\-conditional operator\. NullReferenceException is possible\. UseLiteralsWhereAppropriate\.cs 63

Любопытный случай: фактически, данное срабатывание ложное, но при изучении кода я нашел другую потенциальную ошибку\.

```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 интерфейс объявления поля\. InitializedFields позволяет получить все объявления, в случае переопределения поля, в классе потомка, например\. Очень редко массив может быть пустым и,скорее всего, это ошибка при компиляции\._

В данном коде сложная для текущего уровня развития нашего анализатора проверка условий\. Связь между _lastField_ и _fieldInitializerValue_ анализатору не видна, и предупреждение некорректно\.

Проверка _fieldInitializerValue \=\= null_ проверяет и _lastField_\. Но раз уж мы начали проверять \- обратим внимание на вызов _LastOrDefault_\. Для ссылочных типов метод может вернуть _null_\. Тип _InitializedFields_ – _ImmutableArray<IFieldSymbol\>_\. Если разработчик уверен, что в последовательности будет как минимум один элемент, лучше использовать _Last_\. Тогда будет выброшено более понятное исключение в случае, если в списке инициализированных полей не окажется ни одного символа\.

## Немножко о тестах

В "Roslyn Analyzers" используется любопытный подход к юнит тестам\. В методах сохранены длинные строковые литералы, в которых содержатся классы для проверки той или иной диагностики\. Мне кажется, писать такой код не очень удобно, ведь IntelliSence не работает внутри литералов\.

Вместо этого я бы предложил наш подход: создание классов для каждого диагностического правила\. Далее эти классы файлами добавляются в ресурсы и в тестах достаются для использования конкретных диагностик\.

У нас для каждой диагностики минимум два класса, с ложными и хорошими срабатываниями \(да, там написан специальный говнокод\)\. Нет, у нас нет вакансий говнокодеров :\)\. Юнит тесты проходят по файлам соответствующими правилами и сигналят, если ошибки нашлись в ложных, или если не нашлись в хороших\. При анализе нашей юнит тестовой базы можно получить более десяти тысяч срабатываний\. Конечно, тесты "Roslyn Analyzers" могут лежать в отдельном репозитории, или там использован принципиально другой подход\. Подробнее устройство "Roslyn Analyzers" я не изучал\.

## Выводы

На данный момент "Roslyn Analyzers" не самый большой из статических анализаторов с открытым кодом\. Одна из целей проекта – использование в качестве примера для написания собственных диагностик\. Тем важнее для него качество кода и, надеюсь, что наша статья сделает проект чуть\-чуть лучше\. 

Выбирающим, какой статический анализатор лучше использовать в своём проекте, я бы посоветовал использовать несколько\. Разные анализаторы дополняют друг друга, и если цена ошибки на вашем проекте велика,лучше подстраховаться всеми доступными способами\. Однако не стоит забывать, что анализаторы должны быть современными\. Добавление в проект устаревших анализаторов может сделать только хуже, ведь даст вам ложное ощущение безопасности\.

Напомню, что [скачать и попробовать](https://pvs-studio.ru/ru/pvs-studio/download/) PVS\-Studio может любой желающий\. Всем чистого кода\!