Единороги врываются в RTS: анализируем исходный код OpenRA

• OpenRA
• Лишние срабатывания
• Результаты проверки
  • Бессмысленные условия
  • Недостижимый код
  • Неинициализированная переменная в конструкторе
  • Потенциальная опечатка
  • True, true and nothing but true
  • Лишняя проверка на null?
  • Possible NullReferenceException
  • Быть может, всё-таки OrDefault?
  • Потерянное присваивание
  • Проверка default-значения
• Заключение

Данная статья посвящена проверке проекта OpenRA с помощью статического анализатора PVS-Studio. Что такое OpenRA? Это игровой движок с открытым исходным кодом, предназначенный для создания стратегий в реальном времени. В статье рассказывается о том, как проводился анализ, какие особенности самого проекта были обнаружены и какие интересные срабатывания выдал PVS-Studio. Ну и, конечно же, здесь будут рассмотрены некоторые особенности анализатора, которые позволили сделать процесс проверки проекта более комфортным.

OpenRA

Проект, выбранный для проверки, представляет собой игровой движок для RTS в стиле таких игр, как Command & Conquer: Red Alert. Более подробную информацию можно найти на сайте. Исходный код написан на C# и доступен для просмотра и использования в репозитории.

OpenRA был выбран для проверки по 3 причинам. Во-первых, он, судя по всему, представляет интерес для многих людей. Во всяком случае, это касается обитателей GitHub, так как репозиторий собрал более 8 тысяч звёзд. Во-вторых, кодовая база OpenRA насчитывает 1285 файлов. Обычно такого количества вполне достаточно, чтобы надеяться найти в них интересные срабатывания. Ну и в-третьих... Игровые движки – это круто :)

Лишние срабатывания

Я анализировал OpenRA с помощью PVS-Studio и поначалу был воодушевлён результатами:

Я решил, что среди такого количества High-предупреждений уж точно смогу найти целую уйму самых разных срабатываний. И, конечно же, на их основе напишу самую крутую и интересную статью :) Но не тут-то было!

Стоило лишь взглянуть на сами предупреждения, и всё сразу встало на свои места. 1277 из 1306 предупреждений уровня High были связаны с диагностикой V3144. Она выдаёт сообщения вида "This file is marked with copyleft license, which requires you to open the derived source code". Более подробно данная диагностика описана здесь.

Очевидно, что срабатывания подобного плана меня совершенно не интересовали, ведь OpenRA и так является open-source проектом. Поэтому их необходимо было скрыть, чтобы они не мешали просмотру остальной части лога. Так как я пользовался плагином для Visual Studio, то сделать это было легко. Нужно было просто кликнуть правой кнопкой по одному из срабатываний V3144 и в открывшемся меню выбрать "Hide all V3144 errors".

Также можно выбрать, какие предупреждения будут отображены в логе, перейдя в раздел "Detectable Errors (C#)" в опциях анализатора.

Для того, чтобы перейти к ним, используя плагин для Visual Studio 2019, нужно кликнуть в верхнем меню Extensions->PVS-Studio->Options.

Результаты проверки

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

Тем не менее среди них удалось найти интересные моменты.

Бессмысленные условия

Достаточно многие срабатывания указывали на лишние проверки. Подобное может свидетельствовать об ошибке, ведь намеренно такой код обычно люди не пишут. Однако в OpenRA довольно часто всё выглядит так, будто эти ненужные условия добавлены специально. Например:

public virtual void Tick()
{
  ....

  Active = !Disabled && Instances.Any(i => !i.IsTraitPaused);
  if (!Active)
    return;

  if (Active)
  {
    ....
  }
}

Предупреждение анализатора: V3022 Expression 'Active' is always true. SupportPowerManager.cs 206

PVS-Studio вполне справедливо подмечает, что вторая проверка бессмысленна, ведь если Active будет false, то до неё выполнение не дойдёт. Может быть тут и правда ошибка, но я думаю, что это написано намеренно. Зачем? Ну а почему бы и нет?

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

Давайте рассмотрим ещё одну проверку "на всякий случай":

Pair<string, bool>[] MakeComponents(string text)
{
  ....

  if (highlightStart > 0 && highlightEnd > highlightStart)  // <=
  {
    if (highlightStart > 0)                                 // <=
    {
      // Normal line segment before highlight
      var lineNormal = line.Substring(0, highlightStart);
      components.Add(Pair.New(lineNormal, false));
    }
  
    // Highlight line segment
    var lineHighlight = line.Substring(
      highlightStart + 1, 
      highlightEnd - highlightStart – 1
    );
    components.Add(Pair.New(lineHighlight, true));
    line = line.Substring(highlightEnd + 1);
  }
  else
  {
    // Final normal line segment
    components.Add(Pair.New(line, false));
    break;
  }
  ....
}

Предупреждение анализатора: V3022 Expression 'highlightStart > 0' is always true. LabelWithHighlightWidget.cs 54

Опять же, очевидно, что повторная проверка полностью лишена смысла. Значение highlightStart проверяется дважды, причём в соседних строчках. Ошибка? Возможно, в одном из условий выбраны не те переменные для проверки. Так или иначе, сложно сказать наверняка, в чём тут дело. Ясно одно – код надо изучить и поправить или оставить пояснение, если дополнительная проверка всё-таки зачем-то нужна.

Вот ещё один подобный момент:

public static void ButtonPrompt(....)
{
  ....
  var cancelButton = prompt.GetOrNull<ButtonWidget>(
    "CANCEL_BUTTON"
  );
  ....

  if (onCancel != null && cancelButton != null)
  {
    cancelButton.Visible = true;
    cancelButton.Bounds.Y += headerHeight;
    cancelButton.OnClick = () =>
    {
      Ui.CloseWindow();
      if (onCancel != null)
        onCancel();
    };

    if (!string.IsNullOrEmpty(cancelText) && cancelButton != null)
      cancelButton.GetText = () => cancelText;
  }
  ....
}

Предупреждение анализатора: V3063 A part of conditional expression is always true if it is evaluated: cancelButton != null. ConfirmationDialogs.cs 78

cancelButton действительно может быть null, ведь в эту переменную записывается значение, возвращаемое методом GetOrNull. Однако логично посчитать, что в теле условного оператора cancelButton никаким образом не превратится в null. Тем не менее проверка всё равно есть. Если не обратить внимание на внешнее условие, то вообще получается странная ситуация: сначала производится обращение к свойствам переменной, а потом разработчик решил убедиться – всё-таки null там или нет? :)

Сначала я предположил, что в проекте, возможно, используется какая-то специфичная логика, связанная с перегрузкой оператора "==". На мой взгляд, реализовывать в проекте что-то подобное для ссылочных типов – спорная идея. Всё же непривычное поведение усложняет понимание кода другими разработчиками. При этом мне сложно представить ситуацию, когда без таких хитростей нельзя обойтись. Хотя вполне вероятно, что в каком-то специфичном случае это было бы удобным решением.

В игровом движке Unity, например, оператор "==" переопределён для класса UnityEngine.Object. В официальной документации, доступной по ссылке, показано, что сравнение экземпляров этого класса с null работает не так, как обычно. Что ж, наверняка у разработчиков были причины для реализации подобной необычной логики.

В OpenRA я чего-то такого не нашёл :). Так что если в рассмотренных ранее проверках на null и есть какой-то смысл, то состоит он в чём-то другом.

PVS-Studio смог обнаружить ещё несколько аналогичных моментов, но ни к чему перечислять здесь их все. Всё же скучновато смотреть одни и те же срабатывания. К счастью (или нет) анализатор смог отыскать и другие странности.

Недостижимый код

void IResolveOrder.ResolveOrder(Actor self, Order order)
{
  ....
  if (!order.Queued || currentTransform == null)
    return;
  
  if (!order.Queued && currentTransform.NextActivity != null)
    currentTransform.NextActivity.Cancel(self);

  ....
}

Предупреждение анализатора: V3022 Expression '!order.Queued && currentTransform.NextActivity != null' is always false. TransformsIntoTransforms.cs 44

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

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

Неинициализированная переменная в конструкторе

public class CursorSequence
{
  ....
  public readonly ISpriteFrame[] Frames;

  public CursorSequence(
    FrameCache cache, 
    string name, 
    string cursorSrc, 
    string palette, 
    MiniYaml info
  )
  {
    var d = info.ToDictionary();

    Start = Exts.ParseIntegerInvariant(d["Start"].Value);
    Palette = palette;
    Name = name;

    if (
      (d.ContainsKey("Length") && d["Length"].Value == "*") || 
      (d.ContainsKey("End") && d["End"].Value == "*")
    ) 
      Length = Frames.Length - Start;
    else if (d.ContainsKey("Length"))
      Length = Exts.ParseIntegerInvariant(d["Length"].Value);
    else if (d.ContainsKey("End"))
      Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
    else
      Length = 1;

    Frames = cache[cursorSrc]
      .Skip(Start)
      .Take(Length)
      .ToArray();

    ....
  }
}

Предупреждение анализатора: V3128 The 'Frames' field is used before it is initialized in constructor. CursorSequence.cs 35

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

(d.ContainsKey("Length") && d["Length"].Value == "*") || 
(d.ContainsKey("End") && d["End"].Value == "*")

будет истинным.

Трудно судить о том, как нужно поправить код, чтобы всё было хорошо. Я могу лишь предположить, что функция должна выглядеть как-то так:

public CursorSequence(....)
{
  var d = info.ToDictionary();

  Start = Exts.ParseIntegerInvariant(d["Start"].Value);
  Palette = palette;
  Name = name;
  ISpriteFrame[] currentCache = cache[cursorSrc];
    
  if (
    (d.ContainsKey("Length") && d["Length"].Value == "*") || 
    (d.ContainsKey("End") && d["End"].Value == "*")
  ) 
    Length = currentCache.Length - Start;
  else if (d.ContainsKey("Length"))
    Length = Exts.ParseIntegerInvariant(d["Length"].Value);
  else if (d.ContainsKey("End"))
    Length = Exts.ParseIntegerInvariant(d["End"].Value) - Start;
  else
    Length = 1;

  Frames = currentCache
    .Skip(Start)
    .Take(Length)
    .ToArray();

  ....
}

В данном варианте указанная проблема отсутствует, однако сказать, насколько он соответствует изначальной задумке, сможет только разработчик.

Потенциальная опечатка

public void Resize(int width, int height)
{
  var oldMapTiles = Tiles;
  var oldMapResources = Resources;
  var oldMapHeight = Height;
  var oldMapRamp = Ramp;
  var newSize = new Size(width, height);

  ....
  Tiles = CellLayer.Resize(oldMapTiles, newSize, oldMapTiles[MPos.Zero]);
  Resources = CellLayer.Resize(
    oldMapResources,
    newSize,
    oldMapResources[MPos.Zero]
  );
  Height = CellLayer.Resize(oldMapHeight, newSize, oldMapHeight[MPos.Zero]);
  Ramp = CellLayer.Resize(oldMapRamp, newSize, oldMapHeight[MPos.Zero]);  
  ....
}

Предупреждение анализатора: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'oldMapRamp' variable should be used instead of 'oldMapHeight' Map.cs 964

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

CellLayer.Resize(oldMapTiles,     newSize, oldMapTiles[MPos.Zero]);
CellLayer.Resize(oldMapResources, newSize, oldMapResources[MPos.Zero]);
CellLayer.Resize(oldMapHeight,    newSize, oldMapHeight[MPos.Zero]);
CellLayer.Resize(oldMapRamp,      newSize, oldMapHeight[MPos.Zero]);

Достаточно странно, что в последнем вызове производится передача oldMapHeight, а не oldMapRamp. Конечно, далеко не все подобные случаи действительно являются ошибками. Вполне возможно, что тут всё написано правильно. Но согласитесь, что выглядит это место необычно. Я склоняюсь к тому, что здесь действительно допущена ошибка.

Примечание коллеги - Андрея Карпова. А я не вижу в данном коде ничего странного :). Это же классическая ошибка последней строки!

Если же ошибки тут всё-таки нет, то стоит добавить какое-нибудь пояснение. Ведь если момент похож на ошибку, то его обязательно кому-нибудь захочется исправить.

True, true and nothing but true

В проекте нашлись весьма своеобразные методы, возвращаемое значение которых имеет тип bool. Их своеобразность заключается в том, что при любых условиях они возвращают true. Например:

static bool State(
  S server, 
  Connection conn, 
  Session.Client client, 
  string s
)
{
  var state = Session.ClientState.Invalid;
  if (!Enum<Session.ClientState>.TryParse(s, false, out state))
  {
    server.SendOrderTo(conn, "Message", "Malformed state command");
    return true;
  }

  client.State = state;

  Log.Write(
    "server", 
    "Player @{0} is {1}",
    conn.Socket.RemoteEndPoint, 
    client.State
  );

  server.SyncLobbyClients();

  CheckAutoStart(server);

  return true;
}

Предупреждение анализатора: V3009 It's odd that this method always returns one and the same value of 'true'. LobbyCommands.cs 123

Всё ли в порядке в этом коде? Закралась ли тут ошибка? Выглядит крайне странно. Почему разработчик не использовал void?

Неудивительно, что анализатор считает такое место странным, но всё же придётся признать, что у программиста на самом деле была причина написать именно так. Какая?

Я решил посмотреть, где вызывается этот метод и используется ли его возвращаемое always true значение. Оказалось, что на него присутствует лишь одна-единственная ссылка в том же классе – в словаре commandHandlers, который имеет тип

IDictionary<string, Func<S, Connection, Session.Client, string, bool>>

При инициализации в него добавляются значения

{"state", State},
{"startgame", StartGame},
{"slot", Slot},
{"allow_spectators", AllowSpectators}

и т.д.

Перед нами представлен редкий (мне хочется в это верить) случай, когда статическая типизация создаёт нам проблемы. Ведь сделать словарь, в котором значениями будут функции с различными сигнатурами... как минимум проблематично. commandHandlers используется лишь в методе InterpretCommand:

public bool InterpretCommand(
  S server, Connection conn, Session.Client client, string cmd
)
{
  if (
    server == null || 
    conn == null || 
    client == null || 
    !ValidateCommand(server, conn, client, cmd)
  )  return false;

  var cmdName = cmd.Split(' ').First();
  var cmdValue = cmd.Split(' ').Skip(1).JoinWith(" ");

  Func<S, Connection, Session.Client, string, bool> a;
  if (!commandHandlers.TryGetValue(cmdName, out a))
    return false;

  return a(server, conn, client, cmdValue);
}

Судя по всему, целью разработчика была универсальная возможность сопоставления строкам тех или иных выполняемых операций. Я думаю, что выбранный им способ далеко не единственный, однако предложить что-то более удобное/правильное в такой ситуации не так уж просто. Особенно, если не использовать какой-нибудь dynamic или что-то подобное. Если у вас есть идеи на этот счёт, оставляйте комментарии. Мне было бы интересно посмотреть на различные варианты решения данной задачи :).

Получается, что предупреждения, связанные с always true методами в этом классе, скорее всего ложные. И всё же... Пугает вот это вот "скорее всего" :) Нужно быть осторожным с такими штуками, ведь среди них и правда может оказаться ошибка.

Все подобные срабатывания стоит тщательно проверить, а затем уже пометить при необходимости как ложные. Делается это достаточно просто. Нужно оставить в том месте, на которое указывает анализатор, специальный комментарий:

static bool State(....) //-V3009

Есть и другой способ: можно выделить срабатывания, которые необходимо пометить как ложные, и в контекстном меню кликнуть на "Mark selected messages as False Alarms".

Подробнее эту тему можно изучить в документации.

Лишняя проверка на null?

static bool SyncLobby(....)
{
  if (!client.IsAdmin)
  {
    server.SendOrderTo(conn, "Message", "Only the host can set lobby info");
    return true;
  }

  var lobbyInfo = Session.Deserialize(s); 
  if (lobbyInfo == null)                    // <=
  {
    server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
    return true;
  }

  server.LobbyInfo = lobbyInfo;

  server.SyncLobbyInfo();

  return true;
}

Предупреждение анализатора: V3022 Expression 'lobbyInfo == null' is always false. LobbyCommands.cs 851

Ещё один метод, всегда возвращающий true. Однако на этот раз мы разбираем срабатывание другого типа. Изучать такие штуки нужно достаточно внимательно, потому что далеко не факт, что это просто избыточный код. Но обо всём по порядку.

Метод Deserialize никогда не возвращает null – в этом можно легко убедиться, взглянув на его код:

public static Session Deserialize(string data)
{
  try
  {
    var session = new Session();
    ....
    return session;
  }
  catch (YamlException)
  {
    throw new YamlException(....);
  }
  catch (InvalidOperationException)
  {
    throw new YamlException(....);
  }
}

Для удобства чтения я сократил исходный код метода. Полностью его можно увидеть, перейдя по ссылке. Ну или поверьте мне на слово, что переменная session здесь ни при каких обстоятельствах не превращается в null.

Что же мы видим в нижней части? Deserialize не возвращает null, если что-то пошло не так, он бросает исключения. Разработчик, написавший после вызова проверку на null, думал иначе, судя по всему. Скорее всего в исключительной ситуации метод SyncLobby должен выполнять код, который сейчас выполняется... да никогда он не выполняется, ведь lobbyInfo никогда не null:

if (lobbyInfo == null)
{
  server.SendOrderTo(conn, "Message", "Invalid Lobby Info Sent");
  return true;
}

Полагаю, что вместо этой "лишней" проверки всё-таки нужно использовать try-catch. Ну или зайти с другой стороны и написать какой-нибудь TryDeserialize, который в случае исключительной ситуации будет возвращать null.

Possible NullReferenceException

public ConnectionSwitchModLogic(....)
{
  ....
  var logo = panel.GetOrNull<RGBASpriteWidget>("MOD_ICON");
  if (logo != null)
  {
    logo.GetSprite = () =>
    {
      ....
    };
  }

  if (logo != null && mod.Icon == null)                    // <=
  {
    // Hide the logo and center just the text
    if (title != null)
    title.Bounds.X = logo.Bounds.Left;

    if (version != null)
      version.Bounds.X = logo.Bounds.X;
    width -= logo.Bounds.Width;
  }
  else
  {
    // Add an equal logo margin on the right of the text
    width += logo.Bounds.Width;                           // <=
  }
  ....
}

Предупреждение анализатора: V3125 The 'logo' object was used after it was verified against null. Check lines: 236, 222. ConnectionLogic.cs 236

Что-то мне подсказывает, что здесь стопроцентно допущена ошибка. Перед нами уже точно не "лишние" проверки, ведь метод GetOrNull, скорее всего, действительно может вернуть нулевую ссылку. Что же будет, если logo будет null? Обращение к свойству Bounds приведёт к выбрасыванию исключения, что явно не входило в планы разработчика.

Возможно, фрагмент нужно переписать как-то так:

if (logo != null)
{
  if (mod.Icon == null)
  {
    // Hide the logo and center just the text
    if (title != null)
    title.Bounds.X = logo.Bounds.Left;

    if (version != null)
      version.Bounds.X = logo.Bounds.X;
    width -= logo.Bounds.Width;
  }
  else
  {
    // Add an equal logo margin on the right of the text
    width += logo.Bounds.Width;
  }
}

Данный вариант достаточно прост для восприятия, хотя дополнительная вложенность выглядит не слишком здорово. В качестве более ёмкого решения можно использовать null-conditional operator:

// Add an equal logo margin on the right of the text
width += logo?.Bounds.Width ?? 0; // <=

Отмечу, что верхний вариант исправления мне нравится больше. Читать его приятно и никаких вопросов при этом не возникает. Но некоторые разработчики достаточно высоко ценят краткость, поэтому второй вариант я тоже решил привести :).

Быть может, всё-таки OrDefault?

public MapEditorLogic(....)
{
  var editorViewport = widget.Get<EditorViewportControllerWidget>("MAP_EDITOR");

  var gridButton = widget.GetOrNull<ButtonWidget>("GRID_BUTTON");
  var terrainGeometryTrait = world.WorldActor.Trait<TerrainGeometryOverlay>();

  if (gridButton != null && terrainGeometryTrait != null) // <=
  {
    ....
  }

  var copypasteButton = widget.GetOrNull<ButtonWidget>("COPYPASTE_BUTTON");
  if (copypasteButton != null)
  {
    ....
  }

  var copyFilterDropdown = widget.Get<DropDownButtonWidget>(....);
  copyFilterDropdown.OnMouseDown = _ =>
  {
    copyFilterDropdown.RemovePanel();
    copyFilterDropdown.AttachPanel(CreateCategoriesPanel());
  };

  var coordinateLabel = widget.GetOrNull<LabelWidget>("COORDINATE_LABEL");
  if (coordinateLabel != null)
  {
    ....
  }

  ....
}

Предупреждение анализатора: V3063 A part of conditional expression is always true if it is evaluated: terrainGeometryTrait != null. MapEditorLogic.cs 35

Давайте проанализируем данный фрагмент. Можно обратить внимание, что каждый раз, когда используется метод GetOrNull класса Widget, производится проверка на равенство null. В то же время, если используется Get, то проверки нет. Это логично – метод Get не возвращает null:

public T Get<T>(string id) where T : Widget
{
  var t = GetOrNull<T>(id);
  if (t == null)
    throw new InvalidOperationException(....);
  return t;
}

Если элемент не найден, то выбрасывается исключение – это разумное поведение. И в то же время логичным вариантом будет проверять значения, возвращаемые методом GetOrNull, на равенство нулевой ссылке.

В коде, приведённом выше, на равенство null проверяется значение, возвращённое методом Trait. На самом деле внутри метода Trait и вызывается Get класса TraitDictionary:

public T Trait<T>()
{
  return World.TraitDict.Get<T>(this);
}

Может ли быть такое, что этот Get ведёт себя не так, как рассмотренный ранее? Всё же классы разные. Давайте же проверим:

public T Get<T>(Actor actor)
{
  CheckDestroyed(actor);
  return InnerGet<T>().Get(actor);
}

Метод InnerGet возвращает экземпляр TraitContainer<T>. Реализация Get в этом классе очень напоминает Get класса Widget:

public T Get(Actor actor)
{
  var result = GetOrDefault(actor);
  if (result == null)
    throw new InvalidOperationException(....);
  return result;
}

Главное сходство состоит именно в том, что и здесь никогда не возвращается null. В случае, если что-то пошло не так, аналогично выбрасывается InvalidOperationException. Следовательно, метод Trait ведёт себя так же.

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

В этом месте кажется более подходящим вызов какого-нибудь TraitOrNull. Однако такого метода нет :). Зато есть TraitOrDefault, который и является аналогом GetOrNull для данного случая.

Есть ещё один подобный момент, связанный уже методом Get:

public AssetBrowserLogic(....)
{
  ....
  frameSlider = panel.Get<SliderWidget>("FRAME_SLIDER");
  if (frameSlider != null)
  {
    ....
  }
  ....
}

Предупреждение анализатора: V3022 Expression 'frameSlider != null' is always true. AssetBrowserLogic.cs 128

Как и в коде, рассмотренном ранее, здесь что-то не в порядке. Либо проверка действительно лишняя, либо вместо Get всё-таки нужно вызывать GetOrNull.

Потерянное присваивание

public SpawnSelectorTooltipLogic(....)
{
  ....
  var textWidth = ownerFont.Measure(labelText).X;
  if (textWidth != cachedWidth)
  {
    label.Bounds.Width = textWidth;
    widget.Bounds.Width = 2 * label.Bounds.X + textWidth; // <=
  }

  widget.Bounds.Width = Math.Max(                         // <=
    teamWidth + 2 * labelMargin, 
    label.Bounds.Right + labelMargin
  );
  team.Bounds.Width = widget.Bounds.Width;
  ....
}

Предупреждение анализатора: V3008 The 'widget.Bounds.Width' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 78, 75. SpawnSelectorTooltipLogic.cs 78

Похоже, что в случае истинности условия textWidth != cachedWidth в widget.Bounds.Width должно записываться некоторое специфичное для данного случая значение. Однако присваивание, производимое ниже вне зависимости от истинности данного условия, лишает строку

widget.Bounds.Width = 2 * label.Bounds.X + textWidth;

всякого смысла. Вполне вероятно, что здесь просто забыли поставить else:

if (textWidth != cachedWidth)
{
  label.Bounds.Width = textWidth;
  widget.Bounds.Width = 2 * label.Bounds.X + textWidth;
}
else
{
  widget.Bounds.Width = Math.Max(
    teamWidth + 2 * labelMargin,
    label.Bounds.Right + labelMargin
  );
}

Проверка default-значения

public void DisguiseAs(Actor target)
{
  ....
  var tooltip = target.TraitsImplementing<ITooltip>().FirstOrDefault();
  AsPlayer = tooltip.Owner;
  AsActor = target.Info;
  AsTooltipInfo = tooltip.TooltipInfo;
  ....
}

Предупреждение анализатора: V3146 Possible null dereference of 'tooltip'. The 'FirstOrDefault' can return default null value. Disguise.cs 192

В каких случаях обычно используется FirstOrDefault вместо First? Если выборка пуста, то First выбросит InvalidOperationException. FirstOrDefault же не выбросит исключение, а вернёт null для ссылочного типа.

В проекте интерфейс ITooltip реализуют различные классы. Таким образом, если target.TraitsImplementing<ITooltip>() вернёт пустую выборку, в tooltip будет записан null. Обращение к свойствам этого объекта, которое производится далее, приведёт к NullReferenceException.

В случаях, когда разработчик уверен, что выборка не будет пустой, правильнее будет использовать First. Если же такой уверенности нет, то стоит проверять значение, возвращаемое FirstOrDefault. Довольно странно, что здесь этого нет. Ведь значения, возвращаемые рассмотренным ранее методом GetOrNull, всегда проверялись. Отчего же тут этого не сделали?

Да кто его знает... А, точно! Наверняка на эти вопросы может ответить разработчик. В конце концов ему этот код и править :)

Заключение

OpenRA так или иначе оказался проектом, который было приятно и интересно проверять. Разработчики проделали большую работу и при этом не забывали и о том, что исходник должен быть удобен для изучения. Конечно, и тут найдутся разные... спорные моменты, но куда ж без них :)

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

Очевидно, что куда лучше обнаружить ошибку, чем её последствия. Можно для этого тратить часы на перепроверку огромного количества новых исходников вручную. Ну и старые заодно глянуть – вдруг ранее не заметили какую-нибудь оплошность? Да, ревью действительно полезны, но если просматривать приходится большое количество кода, то со временем перестаёшь замечать некоторые вещи. Да и тратится на это много времени и сил.

Статический анализ как раз и является удобным дополнением к другим способам проверки качества исходного кода, таких как code-review. PVS-Studio найдёт "простые" (а иногда и не только) ошибки вместо разработчика, позволяя людям сосредоточиться на более серьёзных вопросах.

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

На самом деле не нужно верить мне на слово - куда лучше будет убедиться в правдивости вышесказанного самостоятельно. По ссылке можно загрузить анализатор и получить триальный ключ. Куда уж проще? :)

Ну а на этом всё. Спасибо за внимание! Желаю вам чистого кода и такого же чистого лога ошибок!