Зачем разработчикам Discord.NET нужен статический анализатор? / Habr

Discord.NET – библиотека, написанная на C#, которая используется для взаимодействия с Discord API. Сможет ли PVS-Studio помочь её разработчикам? Узнаете в статье!

Введение

Discord.NET может быть полезна при создании любых приложений, использующих Discord API. Наиболее часто с её помощью разрабатывают Discord ботов.

Гуляя по GitHub, мы нашли репозиторий этого проекта и подумали – а почему бы не проверить его качество с помощью статического анализатора? Быть может, PVS-Studio позволит исправить какие-нибудь проблемы, которые не удалось заметить разработчикам? Что ж, давайте узнаем!

В этой статье был проверен исходный код проекта, соответствующий этому коммиту.

Ошибочный сдвиг

Issue 1

public enum GuildFeature : long
{
  None = 0,
  AnimatedBanner = 1 << 0,
  AnimatedIcon = 1 << 1,
  Banner = 1 << 2,
  ....
  TextInVoiceEnabled = 1 << 32,
  ThreadsEnabled = 1 << 33,
  ThreadsEnabledTesting = 1 << 34,
  ....
  VIPRegions = 1 << 40,
  WelcomeScreenEnabled = 1 << 41,
}

Предупреждение PVS-Studio: V3134 Shift by 32 bits is greater than the size of 'Int32' type of expression '1'. GuildFeature.cs 147

Хочу обратить ваше внимание на базовый тип перечисления: им выступает long. Следовательно, каждому из элементов GuildFeature будет соответствовать значение этого типа. Сами значения будут получены путём сдвига 1 на различное количество бит.

В данном примере сдвиг производится на числа, лежащие в диапазоне от 0 до 41. Для значения типа int сдвиг на 32 бита эквивалентен его отсутствию, сдвиг на 33 бита – то же самое, что и на 1, и так далее. Начиная с TextInVoiceEnabled, значения элементов перечисления начинают повторяться. При этом названия элементов с совпадающими значениями никак не связаны семантически.

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

Есть как минимум два варианта, почему разработчик мог допустить ошибку. Либо он не знал, что числовые литералы по умолчанию имеют тип int, либо ожидал, что в результате сдвига вернётся значение типа long.

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

public enum MyEnum
{
  Elem1 = ....,
  Elem2 = Elem1
}

Бессмысленный вызов 'Concat'

Issue 2

public static async Task<RestGuildUser> AddGuildUserAsync(....)
{
  ....
  if (args.Roles.IsSpecified)
  {
    var ids = args.Roles.Value.Select(r => r.Id);

    if (args.RoleIds.IsSpecified)
      args.RoleIds.Value.Concat(ids);                  // <=
    else
      args.RoleIds = Optional.Create(ids);
  }
  ....
}

Предупреждение PVS-Studio: V3010 The return value of function 'Concat' is required to be utilized. GuildHelper.cs 431

Анализатор сообщает о том, что возвращаемое значение метода не используется, поэтому вызов не имеет смысла. Так ли это?

В данном случае Concat – метод расширения из System.Linq. Он позволяет получить перечисление, содержащее элементы двух коллекций. Возможно, разработчик полагал, что результат выполнения Concat изменит состояние RoleIds.Value, но это не так. Concat лишь возвращает результат объединения коллекций, не меняя их. Подобные ошибки не раз встречались при проверке проектов – если интересно, то посмотреть их можно по ссылке.

Беспорядок в аргументах

Issue 3

async Task<IUserMessage> IDiscordInteraction
                         .FollowupWithFileAsync(string filePath,
                                                string text,
                                                string fileName,
                                                ....)
  => await FollowupWithFileAsync(filePath,
                                 text,                        // <=
                                 filename,                    // <=
                                 ....).ConfigureAwait(false);

Предупреждение PVS-Studio: V3066 Possible incorrect order of arguments passed to 'FollowupWithFileAsync' method: 'text' and 'fileName'. RestInteraction.cs 434

Для разбора данного срабатывания стоит рассмотреть определение используемой перегрузки метода FollowupWithFileAsync:

/// <summary>
///     Sends a followup message for this interaction.
/// </summary>
/// <param name="text">The text of the message to be sent.</param>
/// <param name="filePath">The file to upload.</param>
/// <param name="fileName">The file name of the attachment.</param>
....
public abstract Task<RestFollowupMessage>
                    FollowupWithFileAsync(string filePath,
                                          string fileName = null,  // <=
                                          string text = null,      // <=
                                          ....);

Исходя из описания этого метода, параметр text содержит текст отправляемого сообщения, а fileName – имя файла вложения. Обратив внимание на место вызова, можно заметить, что последовательность передаваемых аргументов не соответствует ожидаемой. Сложно представить ситуацию, когда вместо текста некоторого сообщения требуется передавать имя файла и наоборот. Тем более для данного метода есть ряд перегрузок, где второй аргумент это text. Возможно, именно этот фактор вызвал путаницу при передаче аргументов.

Issue 4

public async Task<InviteMetadata>
            CreateChannelInviteAsync(ulong channelId,
                                     CreateChannelInviteParams args,
                                     RequestOptions options = null)
{
  ....
  if (args.TargetType.Value == TargetUserType.Stream)
    Preconditions.GreaterThan(args.TargetUserId, 0,
                              nameof(args.TargetUserId));        // <=

  if (args.TargetType.Value == TargetUserType.EmbeddedApplication)
    Preconditions.GreaterThan(args.TargetApplicationId, 0,
                              nameof(args.TargetUserId));        // <=
  ....
}

Предупреждение PVS-Studio: V3127 Two similar code fragments were found. Perhaps, this is a typo and 'TargetApplicationId' variable should be used instead of 'TargetUserId' DiscordRestApiClient.cs 1759

Анализатор обнаружил участок кода, в котором содержится опечатка. Рассмотрим вызовы GreaterThan. В первом из них в качестве первого аргумента передаётся args.TargetUserId, а в качестве третьего – nameof(args.TargetUserId). Во втором же вызове первый аргумент – args.TargetApplicationId, а третий – опять nameof(args.TargetUserId). Выглядит достаточно странно, что третий аргумент одинаков в обоих вызовах.

Из сигнатуры метода видно, что третий параметр – имя проверяемой переменной. Странно, что оно одно и то же для разных объектов.

public static void GreaterThan(Optional<ulong> obj,
                               ulong value,
                               string name,
                               string msg = null)

Исправленное условие будет выглядеть следующим образом:

if (args.TargetType.Value == TargetUserType.EmbeddedApplication)
  Preconditions.GreaterThan(args.TargetApplicationId, 0,
                            nameof(args.TargetApplicationId));

Конструктор с подвохом

Issue 5, 6

public class ThreadUpdateAuditLogData : IAuditLogData
{
  private ThreadUpdateAuditLogData(IThreadChannel thread,
                                   ThreadType type,
                                   ThreadInfo before,
                                   ThreadInfo after)
  {
    Thread = thread;
    ThreadType = type;
    Before = before;
    After = After;
  }
  ....
}

Здесь PVS-Studio выдаёт сразу два предупреждения:

V3117 Constructor parameter 'after' is not used. ThreadUpdateAuditLogData.cs 13
V3005 The 'After' variable is assigned to itself. ThreadUpdateAuditLogData.cs 18

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

Ошибки с null

Issue 7, 8

internal SocketResolvableData(DiscordSocketClient discord,
                              ulong? guildId,
                              T model)
{
  var guild = guildId.HasValue ? discord.GetGuild(guildId.Value) : null;
  ....
  if (resolved.Members.IsSpecified && guild != null)           // <=
  {
    ....
    var user = guild.AddOrUpdateUser(member.Value);
    ....
  }

  if (resolved.Roles.IsSpecified)
  {
    foreach (var role in resolved.Roles.Value)
    {
      var socketRole = guild.AddOrUpdateRole(role.Value);      // <=
      ....
    }
  }

  if (resolved.Messages.IsSpecified)
  {
    foreach (var msg in resolved.Messages.Value)
    {
      ....
      if (guild != null)                                       // <=
      {
        if (msg.Value.WebhookId.IsSpecified)
          ....
        else
          author = guild.GetUser(msg.Value.Author.Value.Id);
      }
      else
        ....
    }
  }
  ....
}

Снова пара предупреждений на один фрагмент кода:

V3125 The 'guild' object was used after it was verified against null. Check lines: 76, 62. SocketResolvableData.cs 76
V3095 The 'guild' object was used before it was verified against null. Check lines: 76, 88. SocketResolvableData.cs 76

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

Issue 9

internal class NullableComponentConverter<T> : ComponentTypeConverter<T>
{
  ....

  public NullableComponentConverter(InteractionService interactionService,
                                    IServiceProvider services)
  {
    var type = Nullable.GetUnderlyingType(typeof(T));

    if (type is null)
      throw new ArgumentException($"No type {nameof(TypeConverter)}" +
                                  $"is defined for this {type.FullName}",  // <=
                                  "type");

    _typeConverter = interactionService
                       .GetComponentTypeConverter(type, services);
  }
  ....
}

Предупреждение PVS-Studio: V3080 Possible null dereference. Consider inspecting 'type'. NullableComponentConverter.cs 15

Анализатор сообщает о возможном разыменовании нулевой ссылки. В условии переменная type проверяется на null, после чего в then-ветви производится обращение к свойству FullName этой переменой. Очевидно, что подобное обращение приведет к выбрасыванию исключения NullReferenceException.

Для исправления ошибки следует заменить type.FullName на typeof(T).FullName.

Issue 10

public sealed class BuildOverrides
{
  private static Assembly
                 _overrideDomain_Resolving(AssemblyLoadContext arg1,
                                           AssemblyName arg2)
  {
    var v = _loadedOverrides
      .FirstOrDefault(x => 
        x.Value.Any(x =>
           x.Assembly.FullName == arg1.Assemblies
                                      .FirstOrDefault().FullName)); // <=

     return GetDependencyAsync(v.Key.Id, $"{arg2}").GetAwaiter()
                                                   .GetResult();
  }
}

Предупреждение PVS-Studio: V3146 Possible null dereference. The 'FirstOrDefault' can return default null value. BuildOverrides.cs 254

FirstOrDefault вернёт первый элемент Assemblies или значение по умолчанию, если элементов нет. Данная коллекция хранит объекты ссылочного типа, следовательно, значением по умолчанию будет null. Если разработчик подразумевал, что Assemblies может не иметь ни одного элемента, то непонятно, почему нет проверки на null перед обращением к FullName. Если же коллекция точно не будет пуста, стоит использовать First, а не FirstOrDefault. Тогда и код не будет вызывать лишних вопросов.

Issue 11

internal void Update(ClientState state, Model model)
{
  var roles = 
       new ConcurrentDictionary<ulong, SocketRole>
           (ConcurrentHashSet.DefaultConcurrencyLevel,
           (int)(model.Roles.Length * 1.05));           // <=
  if (model.Roles != null)                              // <=
  {
    for (int i = 0; i < model.Roles.Length; i++)
    {
      var role = SocketRole.Create(this, state, model.Roles[i]);
      roles.TryAdd(role.Id, role);
    }
  }
}

Предупреждение PVS-Studio: V3095 The 'model.Roles' object was used before it was verified against null. Check lines: 534, 535. SocketGuild.cs 534

Еще одно интересное срабатывание, связанное с потенциальным разыменованием нулевой ссылки. Сначала производится обращение к свойству model.Roles.Length, а затем model.Roles проверяется на null. Скорее всего, разработчик предполагал, что model.Roles может иметь значение null, именно поэтому написал проверку. Выглядит странно, что это свойство проверяется лишь во втором случае.

Выражение, которое всегда ложно

Issue 12

public IEnumerable<CommandMatch> GetCommands(....)
{
  ....
  int nextSegment = NextSegment(text, index, service._separatorChar);
  ....
  if (visitChildren)
  {
    ....
    if (nextSegment != -1)
    {
      name = text.Substring(index, nextSegment - index);
      if (_nodes.TryGetValue(name, out nextNode))
      {
        foreach (var cmd in
                   nextNode.GetCommands(service,
                                        nextSegment == -1 ? "" : text, // <=
                                        nextSegment + 1,
                                        false))
          yield return cmd;
      }
    }
  }
}

Предупреждение PVS-Studio: V3022 Expression 'nextSegment == -1' is always false. CommandMapNode.cs 109

Обратите внимание на второй if в этом фрагменте кода, после чего посмотрите на выражение nextSegment == -1 ? "" : text. Результатом данного условия всегда будет false. В данном примере нет ошибки, лишь излишний код, но его также стоит избегать.

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

Заключение

PVS-Studio удалось найти ряд подозрительных моментов в коде Discord.NET. Большая часть из них связана с потенциальным разыменованием нулевой ссылки. Мне кажется, что разработчикам стоит обратить на это внимание. Впрочем, как и на остальные срабатывания, описанные в этой статье :).

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

Сможет ли анализатор помочь вам? Предлагаю проверить это :). И пишите в комментариях – нашёл ли анализатор странности в вашем коде?

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Panevin. How can a static analyzer help Discord.NET developers?.