PascalABC.NET, повторная проверка

Приветствуем всех любителей чистого кода. Сегодня у нас на разборе проект PascalABC.NET. Ранее мы уже искали ошибки в этом проекте при помощи сразу двух инструментов статического анализа, а именно плагинов для SonarQube: SonarC# и PVS-Studio. Этому была посвящена статья в далёком 2017 году. В этот раз мы решили ограничиться только C# анализатором PVS-Studio последней актуальной версии и посмотреть, какие новые ошибки можно найти в PascalABC.NET сегодня. Тем более что за это время наш анализатор стал гораздо более продвинутым и обзавелся новыми возможностями поиска ошибок и потенциальных уязвимостей в коде.

Введение

В своё время проект PascalABC.NET запомнился мне интересным общением с разработчиками, с которыми мы случайно пересеклись на одной из конференций сразу после выхода статьи "Проверяем проект PascalABC.NET с помощью плагинов для SonarQube: SonarC# и PVS-Studio" в 2017 году. Выглядело так, будто мы специально подстроили такую ситуацию: написали статью про ошибки в проекте и поехали на конференцию, чтобы обсудить это с авторами. Конечно, такого плана не было, это совпадение. Но получилось забавно. После того случая я несколько раз подумывал о том, чтобы написать статью о повторной проверке этого проекта. Но всё не доходили руки. И вот, наконец, пришло время.

PascalABC.NET представляет собой современную реализацию языка Pascal на платформе .NET. На сайте проекта можно ознакомиться с его подробным описанием, а также удостовериться, что проект развивается. Последняя версия 3.8.1 датирована августом 2021 года. Хорошие новости, так как повторная проверка "заброшенного" авторами проекта лишена смысла. Этот факт дополнительно мотивировал меня на написание этой статьи. Ведь если проект развивается, то в нём будут исправлены старые ошибки и, конечно, добавлены новые.

Для анализа я взял исходный код с GitHub от 10.12.2021. За время написания и публикации статьи в код могут быть внесены изменения, прошу учитывать этот факт в случае самостоятельной проверки исходников PascalABC.NET. Кстати, для этого вы можете легко запросить пробную версию анализатора PVS-Studio. Также не забудьте про новую возможность "Best warnings", которая позволит сразу ознакомиться с наиболее интересными ошибками, что немаловажно при работе с таким большим проектом.

К моему большому сожалению, множество ошибок, которые я обнаружил в PascalABC.NET в 2017 году, так и не были исправлены к настоящему времени. После публикации статьи о проверке проекта мы всегда сообщаем разработчикам о проблемах в коде, но внесение правок остаётся на их совести. Поэтому в этот раз я проделал довольно много дополнительной работы, чтобы исключить из отчёта ранее найденные ошибки. Несмотря на это, при повторной проверке удалось обнаружить несколько новых интересных ошибок, о которых я расскажу далее.

Ошибки

Начну с классических ошибок типа copy-paste. Невероятно, но программисты раз за разом допускают эти досадные ошибки, что точно не оставит инструменты типа PVS-Studio без работы. К тому же подобные ошибки хорошо демонстрируют важное преимущество технологии статического анализа: внимательность, недоступную человеку в силу усталости и прочих причин.

V3001 There are identical sub-expressions to the left and to the right of the '||' operator. NETGenerator.cs 461

public class CompilerOptions
{
  public enum PlatformTarget { x64, x86, AnyCPU,
    dotnet5win, dotnet5linux, dotnet5macos };
  ....
}
....
bool IsDotnet5()
{
  return 
    comp_opt.platformtarget ==
      CompilerOptions.PlatformTarget.dotnet5win || 
    comp_opt.platformtarget ==
      CompilerOptions.PlatformTarget.dotnet5linux || 
    comp_opt.platformtarget ==
      CompilerOptions.PlatformTarget.dotnet5linux;
}

В методе IsDotnet5() производят повторное сравнение со значением перечисления CompilerOptions.PlatformTarget.dotnet5linux. Еcли взглянуть на объявление перечисления PlatformTarget, то можно предположить, что код метода должен выглядеть так:

bool IsDotnet5()
{
  return 
    comp_opt.platformtarget ==
      CompilerOptions.PlatformTarget.dotnet5win || 
    comp_opt.platformtarget ==
      CompilerOptions.PlatformTarget.dotnet5linux || 
    comp_opt.platformtarget ==
      CompilerOptions.PlatformTarget.dotnet5macos;
}

Хочу обратить внимание, что код отформатирован для более удобного восприятия. В первоначальном варианте всё выражение return записано в одну строку.

V3001 There are identical sub-expressions 'ctn2.compiled_type == TypeFactory.ObjectType' to the left and to the right of the '||' operator. NETGenerator.cs 8518

private void AssignToDereferenceNode(....)
{
  ....
  if (.... && (ctn2.compiled_type == TypeFactory.ObjectType ||
      (ctn2.compiled_type == TypeFactory.ObjectType ||
       ctn2.compiled_type.IsInterface)))
  ....
}

Здесь используют сравнение одной и той же переменной со значением TypeFactory.ObjectType. Код снова отформатирован для удобства. В первоначальном варианте выражение if записано в одну строку. Думаю, в таком коде человеку довольно сложно заметить проблемы. О вариантах исправления ошибки в данном случае судить трудно, так как класс TypeFactory содержит большое количество полей.

V3001 There are identical sub-expressions 'SK == SymKind.field' to the left and to the right of the '||' operator. LightScopeHelperClasses.cs 30

public enum SymKind { var, field, param, procname, funcname,
                      classname, recordname, interfacename };
....
public class SymInfoSyntax
{
  public override string ToString()
  {
    ....
    if (SK == SymKind.var || 
        SK == SymKind.field || 
        SK == SymKind.field || 
        SK == SymKind.param)
    ....
  }
  ....
}

Одно из сравнений SK == SymKind.field ошибочно и должно содержать другое значение перечисления SymKind. Вероятно, автор кода смог бы дать точный ответ в данной ситуации.

V3004 [CWE-691] The 'then' statement is equivalent to the 'else' statement. SymbolTable.cs 870

private Scope FindClassScope(Scope scope)
{
  while (scope != null && !(scope is ClassScope))
      if(scope is ClassMethodScope)
        scope = scope.TopScope;
      else
        scope = scope.TopScope;
  return scope;
}

Другой паттерн ошибки, но снова copy-paste: оба блока кода оператора if идентичны. Здесь также нужен автор кода для анализа и исправления.

V3005 The 'e' variable is assigned to itself. generics.cs 430

public static type_node determine_type(....)
{
  ....
  try
  {
    return ....;
  }
  catch(Exception e)
  {
    e = e;
  }
  ....
}

Какой-то парадоксальный код. Здесь возможен как copy-paste, так и осмысленная попытка заглушить предупреждение о неиспользуемой переменной. Или это последствия рефакторинга, и ранее, например, была некая внешняя относительно блока catch переменная e, которую затем удалили. В любом случае, код выглядит небрежно.

Помимо ошибок copy-paste, в коде PascalABC.NET я обнаружил и другие проблемы.

V3022 [CWE-570] Expression 't != null' is always false. Visitor.cs 598

public void prepare_collection(....)
{
  myTreeNode t;
  ....
  if (t == null)
  {
    ....
    if (t != null)
      t.Nodes.Add(tn);
    else
      nodes.Add(tn);
    ....
  }
  ....
}

Последствия рефакторинга, излишняя перестраховка или простая невнимательность. В результате then-ветка t.Nodes.Add(tn) блока if никогда не будет выполнена. Код необходимо исправить.

V3027 [CWE-476] The variable 'fn.return_value_type' was utilized in the logical expression before it was verified against null in the same logical expression. NetHelper.cs 1109

private static function_node get_conversion(....)
{
  ....
  function_node fn = si.sym_info as function_node;
  if (.... || fn.return_value_type.original_generic == to || ....
      && fn.return_value_type != null && ....)
  {
    return fn;
  }
  ....
}

Переменную fn.return_value_type разыменовывают без проверки на равенство null. Автор предполагал, что эта переменная может иметь значение null, так как далее он это явно проверяет.

V3032 [CWE-835] Waiting on this expression is unreliable, as compiler may optimize some of the variables. Use volatile variable(s) or synchronization primitives to avoid this. RemoteCompiler.cs 407

CompilerState compilerState = CompilerState.Reloading;
....
public string Compile()
{
  ....
  compilerState = CompilerState.CompilationStarting;
  ....
  while (compilerState != CompilerState.Ready)
    Thread.Sleep(5);
  ....
}

Интересная ошибка, связанная с особенностями работы компилятора. Проблема может проявить себя в Release-версии: цикл while станет вечным в результате оптимизации. Про особенности возникновения данной ошибки и варианты исправления предлагаю почитать в документации V3032.

V3043 [CWE-483] The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. Compiler.cs 2196

public string Compile()
{
  ....
  int n = 1;
  try
  {
    n = 2;
    ....
    if (File.Exists(pdb_file_name))
      File.Delete(pdb_file_name);
      n = 5;
    ....
  }
  ....
}

Может показаться, что выражение n = 5 относится к блоку if, но это не так. Код просто небрежно отформатирован. Это предупреждение я привожу здесь для примера. Довольно редкая ошибка, которая в данном случае не приводит к проблемам, но так бывает не всегда. На нашем сайте есть раздел, где выписаны ошибки в проектах, про которые мы ранее писали статьи. В том числе там можно найти и обнаруженные ошибки V3043. Одна из них – похожая, но более опасная ошибка из проекта PascalABC.NET, которую я выписал туда при первой проверке проекта в 2017 году. Если интересно, можете пройти по ссылке и ознакомиться с этой ошибкой (после перехода пролистайте немного вниз до проекта PascalABC.NET).

Перед описанием следующей ошибки предлагаю изучить фрагмент кода и попробовать самостоятельно идентифицировать проблему:

public static typed_expression
  GetTempFunctionNodeForTypeInference(....)
{
  ....
  for (int i = 0; i < def.formal_parameters.params_list.Count; i++)
  { 
    ....
    for (int j = 0;
      j < def.formal_parameters.params_list[i].idents.idents.Count;
      j++)
    {
      var new_param = new common_parameter(....,
        visitor.get_location(
          def.formal_parameters.params_list[i].idents.idents[0]));
      ....
    }
  }
  ....
}

Заметили ошибку? Скажу честно, даже увидев предупреждение анализатора, я не сразу понял, в чём тут дело. И да, код был отформатирован для удобства. Первоначальный вариант гораздо менее читабелен. Вот предупреждение анализатора: V3102 Suspicious access to element of 'def.formal_parameters.params_list[i].idents.idents' object by a constant index inside a loop. LambdaHelper.cs 402

Обратите внимание на вычисление значения переменной new_param. На всех итерациях вложенного цикла используют доступ к нулевому элементу списка def.formal_parameters.params_list[i].idents.idents[0]. Всё указывает на то, что вместо 0 там должен быть использован индекс j.

И последняя интересная ошибка, о которой хотелось бы рассказать.

V3146 [CWE-476] Possible null dereference. The 'symbolInfo.FirstOrDefault()' can return default null value. SystemLibInitializer.cs 112

public class SymbolInfo
{
  ....
}
....
List<TreeConverter.SymbolInfo> symbolInfo = null;
....
public List<TreeConverter.SymbolInfo> SymbolInfo
{
  get
  {
    if (symbolInfo != null && ....)
    {
      if (symbolInfo.FirstOrDefault().sym_info is common_type_node)
        ....
    }
  }
}

Обратите внимание на условие второго блока if. Ссылку symbolInfo проверили на равенство null ранее, здесь вопросов нет. Однако забыли, что метод FirstOrDefault() может вернуть значение по умолчанию (null) для типа SymbolInfo в случае, если список symbolInfo не будет содержать элементов. Это создаст проблемы при доступе к свойству sym_info по нулевой ссылке.

Заключение

Статья получилась небольшая. Но это не значит, что проект PascalABC.NET содержит мало ошибок. Просто значительную их часть я описал ранее в статье от 2017 года. И большинство этих ошибок не были исправлены авторами. При последней проверке на первом уровне критичности High анализатор выдал 400 предупреждений. А на уровне Medium — 1364 предупреждения. Среди них есть множество однотипных, о которых нет смысла рассказывать подробно. Читатель может убедиться в этом сам, если проверит проект PascalABC.NET при помощи PVS-Studio и поищет в коде ошибки, которые я описал в этой и предыдущей статьях.

Вообще, проблема несвоевременной правки ошибок в коде открытых проектов достаточно распространена. Этой теме посвящена недавняя заметка моего коллеги Андрея Карпова "1000 глаз, которые не хотят проверять код открытых проектов".

Также хочу заметить, что в ходе проделанной работы я убедился в неудобстве и малой эффективности использования анализатора время от времени. Действительно сложно и неблагодарно выискивать реальные ошибки среди тысяч предупреждений. И это на фоне того, что старые ошибки не правят и никак не заглушают предупреждения анализатора. Не думаю, что какой-то программист согласится заниматься такой работой. Я его понимаю.

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

Напомню, что современные статические анализаторы, в том числе и PVS-Studio, обладают массой возможностей для удобной работы с большими проектами. Особенно на стадии внедрения при большой кодовой базе. Мы рекомендуем в таком случае использовать режим подавления всех старых предупреждений и работу только с новыми, выдаваемыми для нового кода (инкрементальный режим). Старые ошибки можно исправлять по мере сил, при этом они не будут отображаться в отчёте анализатора. С особенностями данных режимов работы можно ознакомиться в статьях "Подавление сообщений анализатора (отключение выдачи предупреждений на существующий код)" и "Режим инкрементального анализа PVS-Studio".

На этом хочу завершить своё исследование и по традиции пожелать всем безбажного кода. Удачи.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Sergey Khrenov. Re-checking PascalABC.NET.