Александр Сеничкин

30 Авг 2019

Теги:

#CSharp

Проверяем Roslyn analyzers

30 Авг 2019

Автор: Александр Сеничкин

Введение
Ошибки
Немножко о тестах
Выводы

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

Введение

Исходный код "Roslyn Analyzers" можно скачать из этого репозитория. В репозитории также можно найти руководство по использованию и полное описание функционала. Для проверки же кода я использовал статический анализатор кода PVS-Studio версии 7.03.

Эта статья не имеет целью сравнение анализаторов. И вообще по ряду причин мы не хотим писать подобные статьи. Оба анализатора по-своему хороши и находят разные ошибки. Перед вами статья о найденных ошибках в "Roslyn analyzers".

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

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

Возможно, вам будет тяжело читать статью без опыта работы с Roslyn. Поэтому я буду делать небольшие вставки курсивом, поясняющие особенности работы платформы. Пропускайте эти места, если код вам понятен. Если же вы захотите глубже понять сущности в Roslyn – можете прочесть статью: введение в Roslyn. Часть вставок скопированы как раз из этой статьи.

Ошибки

Предупреждение PVS-Studio: 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

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 There are identical sub-expressions 'data1.IsReachableBlockData' to the left and to the right of the '==' operator. AnalysisEntityBasedPredicateAnalysisData.cs 39

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

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

Предупреждение PVS-Studio: V3080 Possible null dereference of method return value. Consider inspecting: GetCandidateReferencedSymbols(...). SyntaxNodeHelper.cs 78

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:

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

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

Действительно, если node не назначен,в перебор может попасть null, что приведёт к NullReferenceException. Исправить код можно либо выбрасывая исключение сразу из метода GetCandidateReferencedSymbols, либо добавив проверку после получения значения из него. Предлагаю пойти вторым, более безопасным путём:

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 The 'valueClauseName' object was used after it was verified against null. Check lines: 2320, 2318. DiagnosticAnalyzer.cs 2320

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 Possible null dereference. Consider inspecting 'oldIdName'. CodeFixProvider.cs 1476

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

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 некоторого объекта должны быть соблюдены следующие условия:

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

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

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

Предупреждение PVS-Studio: V3095 The 'rule' object was used before it was verified against null. Check lines: 2180, 2181. CodeFixProvider.cs 2180

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.

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 The 'sourceOrigins' variable is assigned but is not used by the end of the function. TaintedDataAnalysis.TaintedDataOperationVisitor.cs 328

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 Possible null dereference. Consider inspecting 'methodDeclaration'. DiagnosticAnalyzer.cs 506

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

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

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 The 'ifStatement' object was used after it was verified against null. Check lines: 788, 773. CodeFixProvider.cs 788

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 без проверки. Дело в том, что можно записать условие двумя способами:

if (true)
{
  var A = b;
}

Или так:

if (true)
   var A = b;

При записи без фигурных скобок Statement не будет типа BlockSyntax, это будет ExpressionStatementSyntax.

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

Предупреждение PVS-Studio: V3139 Two or more case-branches perform the same actions. CodeMetricsAnalyzer.cs 251

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 планировалось разное поведение, тогда мы нашли настоящую ошибку. Но, возможно, это сделано на будущее, чтобы изменить поведение позднее. А также вероятно, что разработчик на секунду забыл, что условия можно группировать.

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

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 The 'lastField' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. UseLiteralsWhereAppropriate.cs 63

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

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" не самый большой из статических анализаторов с открытым кодом. Одна из целей проекта – использование в качестве примера для написания собственных диагностик. Тем важнее для него качество кода и, надеюсь, что наша статья сделает проект чуть-чуть лучше.

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

Напомню, что скачать и попробовать PVS-Studio может любой желающий. Всем чистого кода!

#CSharp