15.07.2022 15:51
rip_m 0 1200 Источник

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

Введение

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

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

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

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

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

Проверка

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

Issue 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 The 'mouseDevice' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. TooltipBehavior.cs 173

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

Issue 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 The 'then' statement is equivalent to the 'else' statement. InvocationContext.cs 400

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

Issue 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 There are identical sub-expressions 'c.Kind == NClang.CursorKind.FunctionDeclaration' to the left and to the right of the '||' operator. CPlusPlusLanguageService.cs 1275

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

Issue 4

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

V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. BreakPointMargin.cs 46

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

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

Issue 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 The 'loadData' object was used before it was verified against null. Check lines: 233, 262. MSBuildHost.cs 233

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

Issue 6

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

V3001 There are identical sub-expressions '_lastFrame.Width == 0' to the left and to the right of the '||' operator. RemoteWidget.cs 308

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

Issue 7

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

V3125 The 'specialEntry.Value' object was used after it was verified against null. Check lines: 92, 86. GlobalRunSpec.cs 92

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

Issue 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 Expression 'IsBuiltInType(cursor.ResultType)' is always false. 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.

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

Issue 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 Expression 'mType != null && t.SupportCtorArgument == MetadataTypeCtorArgument.HintValues' is always false. CompletionEngine.cs 601

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

Issue 10

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

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

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

V3105 The 'languageServiceProvider' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. CodeEditorViewModel.cs 309

Анализатор указывает, что происходит разыменование переменной, значение которой получено с помощью null-условного оператора. И как мы видим, оператор '?.' действительно используется при вычислении значения переменной languageServiceProvider. На следующей строчке происходит разыменование этой переменной без проверки. Подобный код может генерировать исключение NullReferenceException. Возможно, стоит использовать '?.' при вызове CreateLanguageService.

Issue 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 The 'RightMargin' variable is assigned to itself. VirtualTerminalController.cs 1446

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

Issue 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 Uninitialized variable 'SolarizedLight' is used when initializing the 'DefaultColorScheme' variable. ColorScheme.cs 32

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

Заключение

Проверка AvalonStudio позволила найти несколько интересных ошибок. Я надеюсь, что эта статья внесёт свой маленький вклад в улучшения данного проекта. Стоит сказать, что IDE с открытым исходным кодом сейчас довольно мало. Может быть в будущем их станет больше. Так каждый разработчик сможет выбрать среду разработки себе по душе. И даже попробовать самостоятельно улучшить инструмент, которым пользуется.

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

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Artem Rovenskii. Any bugs in your IDE? Checking AvalonStudio with PVS-Studio.

Комментарии (0)