Есть ли ошибки в вашей IDE? Проверка AvalonStudio с помощью PVS-Studio

Разработчики по всему миру каждый день используют свои любимые IDE для создания программного обеспечения. Сегодня мы проверим одну из них и посмотрим на наиболее интересные найденные ошибки.

Введение

Современный разработчик использует множество инструментов для достижения успеха. К ним относятся IDE (интегрированная среда разработки). Они содержат целый набор инструментов, облегчающих жизнь. Современная среда разработки включает в себя текстовый редактор, компилятор или интерпретатор, отладчик и так далее.

В этой статье я проверяю одну из IDE с открытым исходным кодом. Для проверки проекта я использую статический анализатор кода PVS-Studio. Это аналитическое решение позволяет искать ошибки и потенциальные уязвимости в коде на C, C++, C# и Java.

Теперь позвольте представить проект, который мы будем анализировать.

AvalonStudio — это кроссплатформенная IDE, написанная на C#. Эта среда в первую очередь ориентирована на разработку для Embedded C, C++, .NET Core, Avalonia и TypeScript. AvalonStudio основана на Avalonia UI, который мы проверили ранее.

Проверить исходный код AvalonStudio не составило труда, так как он доступен на GitHub. Я выбрал самые интересные ошибки, которые нашел анализатор :). Приятного чтения!

Проверка

В этой статье каждая проблема представлена с предупреждением анализатора, фрагментом кода, описанием и авторским мнением. Если в статье есть моменты, с которыми вы не согласны, я буду рад обсудить их в комментариях.

Выпуск 1

private async void Timer_Tick(object sender, EventArgs e)
{
  ....  
  if (AssociatedObject.IsPointerOver)
  {
    var mouseDevice = (popup.GetVisualRoot() as IInputRoot)?.MouseDevice;
    lastPoint = mouseDevice.GetPosition(AssociatedObject);
    popup.Open();
  }
}

V3105 Переменная ‘mouseDevice’ была использована после того, как ей был присвоен оператор null-conditional. Возможно возникновение NullReferenceException. TooltipBehavior.cs 173

Анализатор заметил что-то подозрительное в преобразовании через оператор as. Оператор null-conditional проверяет результат на наличие null. Это означает, что разработчик предполагал, что преобразование через as может вернуть null. Разработчик защитил код оператором ‘?’ от NullReferenceException только один раз, но в дальнейшем исключение может быть выброшено.

Проблема 2

Следующее предупреждение не указывает на явную ошибку, но тем не менее фрагмент кода выглядит подозрительно:

private static Signature BuildSignature(IMethodSymbol symbol)
{
  ....
  var returnTypeInfo = CheckForStaticExtension.GetReturnType(symbol);
  if(returnTypeInfo.HasValue)
  {
    if(returnTypeInfo.Value.inbuilt)
    {
      signature.BuiltInReturnType = returnTypeInfo.Value.name;
    }
    else
    {
      signature.ReturnType = returnTypeInfo.Value.name;
    }
  }
  signature.Parameters = symbol.Parameters.Select(parameter =>
  {
    var info = CheckForStaticExtension.GetReturnType(parameter);
    ....
    if(info.HasValue)
    {
      if(info.Value.inbuilt)
      {
        result.BuiltInType = info.Value.name;   // <=
      }
      else
      {
        result.BuiltInType = info.Value.name;   // <=
      }
    }
    ....
  }).ToList();
  ....
}

V3004 Оператор ‘then’ эквивалентен оператору ‘else’. InvocationContext.cs 400

PVS-Studio обнаружил, что ветви then и else оператора if эквивалентны. В то же время, если подняться чуть выше по коду, то можно заметить, что есть похожий фрагмент, но ветви разные. Возможно, одна из ветвей является лишней. Кстати, возможно, что на месте BuiltInType должно быть другое свойство. Например, аналогичное свойство Type, которое уже существует. Разработчик мог использовать автодополнение и не заметить, что автодополненный код был неправильным. Кстати, VS2022 тоже предложила мне неправильную подсказку. Как говорится: «Доверяй, но проверяй».

Проблема 3

Я отформатировал следующий код, потому что он был неструктурированным и трудночитаемым, но оставил логику без изменений.

Вот как код выглядит в редакторе. Если код отформатировать немного лучше, ошибка станет гораздо более очевидной.

private bool CursorIsValidDeclaration(ClangCursor c)
{
  var result = false;

  if (  (c.Kind == NClang.CursorKind.FunctionDeclaration)  // <=
      || c.Kind == NClang.CursorKind.CXXMethod 
      || c.Kind == NClang.CursorKind.Constructor 
      || c.Kind == NClang.CursorKind.Destructor 
      || c.Kind == NClang.CursorKind.FunctionDeclaration)  // <=
  {
    result = true;
  }
  return result;
}

V3001 Слева и справа от оператора ‘||’ имеются одинаковые подвыражения ‘c.Kind == NClang.CursorKind.FunctionDeclaration’. CPlusPlusLanguageService.cs 1275

Такие ошибки допускаются из-за невнимательности. Одно из сравнений может быть лишним. Или может оказаться, что вместо одного из элементов FunctionDeclaration должно быть что-то другое :).

Задача 4

public override void Render(DrawingContext context)
{
  ....
  foreach (var breakPoint in _manager?.OfType<Breakpoint>().Where(....))
  {
    ....
  }
  ....
}

V3153 Перечисление результата оператора доступа с нулевым условием может привести к NullReferenceException. BreakPointMargin.cs 46

Анализатор обнаружил, что результат оператора ‘?’ разыменовывается немедленно. Такая проверка на нуль внутри foreach не имеет смысла. NullReferenceException все равно настигает разработчика, когда поток выполнения достигает нулевой ссылки :).

В общем, такое поведение неочевидно для многих программистов. Мы даже написали целую статью на эту тему: «Использование оператора ?. в foreach: защита от неработающего NullReferenceException».

Проблема 5

public async Task<(....)> LoadProject(....)
{
  ....
  return await Task.Run(() =>
  {
    ....
    if (   loadData.CscCommandLine != null                            // <=
        && loadData.CscCommandLine.Count > 0)
    {
      ....
      return (projectInfo, projectReferences, loadData.TargetPath);
    }
    else
    {
      ....
      return (projectInfo, projectReferences, loadData?.TargetPath);  // <=
    }
  });
}

V3095 Объект ‘loadData’ использовался до того, как был проверен на null. Проверьте строки: 233, 262. MSBuildHost.cs 233

Эта ошибка кажется довольно простой. Если переменная loadData потенциально равна null в ветви else, то эта переменная также может быть равна null в том же условии if. За исключением того, что нет проверки loadData для null-условия, что означает, что может быть выброшено исключение. И да, loadData никак не изменяется в else. Скорее всего, разработчик пропустил оператор ‘?’ в операторе if. Хотя, возможно, что этот оператор в return не нужен и его следует убрать, чтобы не путать разработчиков.

Выпуск 6

public override void Render(DrawingContext context)
{
  if (_lastFrame != null && !(   _lastFrame.Width == 0 
                              || _lastFrame.Width == 0))
  {
    ....
  }
  base.Render(context);
}

V3001 Слева и справа от оператора ‘||’ имеются одинаковые подвыражения ‘_lastFrame.Width == 0’. RemoteWidget.cs 308

Анализатор обнаружил два одинаковых подвыражения слева и справа от оператора ‘||’. Думаю, что исправить такую ошибку довольно просто: нужно изменить одно из свойств Width на Height. Это свойство существует и используется далее в методе.

Проблема 7

public GlobalRunSpec(....)
{
  ....
  if (specialEntry.Value != null)
  {
    ....
  }
  RunSpec spec = new(specialOps,
                     specialVariables ?? variables,
                     specialEntry.Value.VariableSetup.FallbackFormat);
  ....
}

V3125 Объект ‘specialEntry.Value’ использовался после того, как он был проверен на null. Проверьте строки: 92, 86. GlobalRunSpec.cs 92

PVS-Studio обнаружил что-то подозрительное. Объект specialEntry.Value проверяется на null, а затем используется без соответствующей проверки. Внутри оператора if значение specialEntry.Value не меняется.

Выпуск 8

private static StyledText InfoTextFromCursor(ClangCursor cursor)
{
  ....
  if (cursor.ResultType != null)
  {
    result.Append(cursor.ResultType.Spelling + " ",
                  IsBuiltInType(cursor.ResultType) ? theme.Keyword 
                                                   : theme.Type);
  }
  else if (cursor.CursorType != null)
  {
    switch (kind)
    {
      ....
    }
    result.Append(cursor.CursorType.Spelling + " ",
                  IsBuiltInType(cursor.ResultType) ? theme.Keyword   // <=
                                                   : theme.Type);
  }
  ....
}

V3022 Выражение ‘IsBuiltInType(cursor.ResultType)’ всегда ложно. CPlusPlusLanguageService.cs 1105

Анализатор ожидает, что IsBuiltInType(cursor.ResultType) всегда будет возвращать false. Если *метод IsBuiltInType вызывается в ветви else, то *cursor.ResultType *имеет значение *null. Метод IsBuiltInType содержит проверку, которая гарантирует, что если параметр равен null, метод вернет false.

private static bool IsBuiltInType(ClangType cursor)
{
  var result = false;
  if (cursor != null && ....)
  {
    return true;
  }
  return result;
}

Получается, что IsBuiltInType(cursor.ResultType) всегда возвращает false.

Хм, почему так происходит? Здесь явная проблема копи-паста. Разработчик просто скопировал аналогичный фрагмент кода сверху — но забыл изменить cursor.ResultType на cursor.CursorType.

Проблема 9

private int BuildCompletionsForMarkupExtension(....)
{
  ....
  if (t.SupportCtorArgument == MetadataTypeCtorArgument.HintValues)
  {
    ....
  }
  else if (attribName.Contains("."))
  {
    if (t.SupportCtorArgument != MetadataTypeCtorArgument.Type)
    {
      ....
      if (   mType != null 
          && t.SupportCtorArgument ==
             MetadataTypeCtorArgument.HintValues)        // <=
      {
        var hints = FilterHintValues(....);
        completions.AddRange(hints.Select(.....));
      }
      ....
    }  
  }
}

V3022 Выражение ‘mType != null && t.SupportCtorArgument == MetadataTypeCtorArgument.HintValues’ всегда ложно. CompletionEngine.cs 601

Свойство SupportCtorArgument сравнивается со значением перечисления. Это выражение находится в ветви else оператора if. SupportCtorArgument — это обычное автосвойство. Если бы свойство было равно MetadataTypeCtorArgument.HintValues, то выполнение кода не дошло бы до этой ветви else. Соответственно, выражение в условии всегда будет false, и код, находящийся в ветке then, никогда не будет выполнен.

Выпуск 10

private void RegisterLanguageService(ISourceFile sourceFile)
{
  ....
  var languageServiceProvider = IoC.Get<IStudio>()
                                   .LanguageServiceProviders
                                   .FirstOrDefault(....)?.Value;

  LanguageService = languageServiceProvider.CreateLanguageService();  // <=

  if (LanguageService != null)
  {
    ....
  }
}

V3105 Переменная ‘languageServiceProvider’ была использована после того, как ей был присвоен оператор null-conditional. Возможно возникновение NullReferenceException. CodeEditorViewModel.cs 309

Анализатор показывает, что происходит разыменование переменной, значение которой было получено с помощью оператора null-conditional.

И как мы видим, оператор ‘?’ действительно используется для вычисления значения переменной languageServiceProvider. В следующей строке эта переменная разыменовывается без предварительной проверки. Такой код может сгенерировать исключение NullReferenceException *. Один из способов исправить код — использовать ‘?’ при вызове *CreateLanguageService.

Проблема 11

public void SetCursorPosition(int column, int row)
{
  ....
  if (LeftAndRightMarginEnabled)
  {
    if (CursorState.OriginMode && CursorState.CurrentColumn < LeftMargin)
      CursorState.CurrentColumn = LeftMargin;
    if (CursorState.CurrentColumn >= RightMargin)
      RightMargin = RightMargin;                   // <=
  }
  ....
}

V3005 Переменная ‘RightMargin’ присваивается самой себе. VirtualTerminalController.cs 1446

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

Выпуск 12

Следующий триггер интригующий и несколько непонятный.

public class ColorScheme
{
  private static List<ColorScheme> s_colorSchemes =
    new List<ColorScheme>();
  private static Dictionary<string, ColorScheme> s_colorSchemeIDs =
    new Dictionary<string, ColorScheme>();
  private static readonly ColorScheme DefaultColorScheme = 
    ColorScheme.SolarizedLight;
  ....
  public static readonly ColorScheme SolarizedLight = new ColorScheme
  {
    ....
  };
}

Попробуйте найти ошибку среди этих строк. Задача максимально упрощена, так как за «….» я спрятал почти 200 строк кода, которые не нужны для обнаружения ошибки. Нашли что-нибудь?

Давайте удалим еще немного кода и добавим сообщение анализатора.

public class ColorScheme
{
  private static readonly ColorScheme DefaultColorScheme = 
    ColorScheme.SolarizedLight;
  ....
  public static readonly ColorScheme SolarizedLight = new ColorScheme
  {
    ....
  };
}

V3070 Неинициализированная переменная ‘SolarizedLight’ используется при инициализации переменной ‘DefaultColorScheme’. ColorScheme.cs 32

Думаю, сообщение анализатора все проясняет. Дело в том, что поле DefaultColorScheme будет инициализировано не тем значением, которое ожидал разработчик. В момент инициализации DefaultColorScheme поле SolarizedLight будет иметь значение по умолчанию null. Получается, что поле DefaultColorScheme также будет установлено в нулевое значение. Другие примеры вы можете найти в документации к этой диагностике. Ссылка находится выше, в сообщении анализатора.

Заключение

Анализатор PVS-Studio нашел несколько интересных ошибок в AvalonStudio. Я надеюсь, что эта статья поможет улучшить этот проект. Стоит отметить, что IDE с открытым исходным кодом в наше время довольно мало. Я надеюсь, что в будущем их будет больше — чтобы каждый разработчик мог выбрать среду разработки по своему вкусу. Они могли бы даже участвовать в разработке инструмента, который они используют.

Вы также можете улучшить свой проект простым и бесплатным способом. Все, что вам нужно сделать, это проверить свой код с помощью PVS-Studio. А пробный ключ, который вы можете получить на нашем сайте, поможет вам в этом :).