# Зачем разработчикам Discord\.NET нужен статический анализатор? Discord\.NET – библиотека, написанная на C\#, которая используется для взаимодействия с Discord API\. Сможет ли PVS\-Studio помочь её разработчикам? Узнаете в статье\! ![0974_DiscordDotNet_ru/image1.png](https://import.viva64.com/docx/blog/0974_DiscordDotNet_ru/image1.png) ## Введение Discord\.NET может быть полезна при создании любых приложений, использующих Discord API\. Наиболее часто с её помощью разрабатывают Discord ботов\. Гуляя по [GitHub](https://github.com), мы нашли [репозиторий](https://github.com/discord-net/Discord.Net) этого проекта и подумали – а почему бы не проверить его качество с помощью статического анализатора? Быть может, PVS\-Studio позволит исправить какие\-нибудь проблемы, которые не удалось заметить разработчикам? Что ж, давайте узнаем\! В этой статье был проверен исходный код проекта, соответствующий [этому коммиту](https://github.com/discord-net/Discord.Net/tree/2c428600b00e4b3f966158203c564313e5fbfb0a)\. ## Ошибочный сдвиг **Issue 1** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/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_\. Если же несколько элементов перечисления и правда должны иметь одно и то же значение, то гораздо понятнее будет следующая запись: ```cpp public enum MyEnum { Elem1 = ...., Elem2 = Elem1 } ``` ## Бессмысленный вызов 'Concat' **Issue 2** ```cpp public static async Task 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](https://pvs-studio.ru/ru/docs/warnings/v3010/) The return value of function 'Concat' is required to be utilized\. GuildHelper\.cs 431 Анализатор сообщает о том, что возвращаемое значение метода не используется, поэтому вызов не имеет смысла\. Так ли это? В данном случае_ Concat_ – метод расширения из _System\.Linq_\. Он позволяет получить перечисление, содержащее элементы двух коллекций\. Возможно, разработчик полагал, что результат выполнения _Concat_ изменит состояние _RoleIds\.Value_, но это не так\. _Concat_ лишь возвращает результат объединения коллекций, не меняя их\. Подобные ошибки не раз встречались при проверке проектов – если интересно, то посмотреть их можно [по ссылке](https://pvs-studio.ru/ru/blog/examples/v3010/)\. ## Беспорядок в аргументах **Issue 3** ```cpp async Task IDiscordInteraction .FollowupWithFileAsync(string filePath, string text, string fileName, ....) => await FollowupWithFileAsync(filePath, text, // <= filename, // <= ....).ConfigureAwait(false); ``` Предупреждение PVS\-Studio: [V3066](https://pvs-studio.ru/ru/docs/warnings/v3066/) Possible incorrect order of arguments passed to 'FollowupWithFileAsync' method: 'text' and 'fileName'\. RestInteraction\.cs 434 Для разбора данного срабатывания стоит рассмотреть определение используемой перегрузки метода _FollowupWithFileAsync_: ```cpp /// /// Sends a followup message for this interaction. /// /// The text of the message to be sent. /// The file to upload. /// The file name of the attachment. .... public abstract Task FollowupWithFileAsync(string filePath, string fileName = null, // <= string text = null, // <= ....); ``` Исходя из описания этого метода, параметр _text_ содержит текст отправляемого сообщения, а _fileName_ – имя файла вложения\. Обратив внимание на место вызова, можно заметить, что последовательность передаваемых аргументов не соответствует ожидаемой\. Сложно представить ситуацию, когда вместо текста некоторого сообщения требуется передавать имя файла и наоборот\. Тем более для данного метода есть ряд перегрузок, где второй аргумент это _text_\. Возможно, именно этот фактор вызвал путаницу при передаче аргументов\. **Issue 4** ```cpp public async Task 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](https://pvs-studio.ru/ru/docs/warnings/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\)_\. Выглядит достаточно странно, что третий аргумент одинаков в обоих вызовах\. Из сигнатуры метода видно, что третий параметр – имя проверяемой переменной\. Странно, что оно одно и то же для разных объектов\. ```cpp public static void GreaterThan(Optional obj, ulong value, string name, string msg = null) ``` Исправленное условие будет выглядеть следующим образом: ```cpp if (args.TargetType.Value == TargetUserType.EmbeddedApplication) Preconditions.GreaterThan(args.TargetApplicationId, 0, nameof(args.TargetApplicationId)); ``` ## Конструктор с подвохом **Issue 5, 6** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3117/) Constructor parameter 'after' is not used\. ThreadUpdateAuditLogData\.cs 13 * [V3005](https://pvs-studio.ru/ru/docs/warnings/v3005/) The 'After' variable is assigned to itself\. ThreadUpdateAuditLogData\.cs 18 Оба предупреждения анализатора указывают на одну проблему\. Очевидно, что разработчик ошибся в присваивании значения _After_\. Вместо одного из параметров конструктора, свойству присваивается собственное значение\. Данная операция не имеет никакого смысла\. ## Ошибки с null **Issue 7, 8** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'guild' object was used after it was verified against null\. Check lines: 76, 62\. SocketResolvableData\.cs 76 * [V3095](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp internal class NullableComponentConverter : ComponentTypeConverter { .... 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](https://pvs-studio.ru/ru/docs/warnings/v3080/) Possible null dereference\. Consider inspecting 'type'\. NullableComponentConverter\.cs 15 Анализатор сообщает о возможном разыменовании нулевой ссылки\. В условии переменная _type_ проверяется на _null_, после чего в then\-ветви производится обращение к свойству _FullName_ этой переменой\. Очевидно, что подобное обращение приведет к выбрасыванию исключения _NullReferenceException_\. Для исправления ошибки следует заменить _type\.FullName_ на _typeof\(T\)\.FullName_\. **Issue 10** ```cpp 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](https://pvs-studio.ru/ru/docs/warnings/v3146/) Possible null dereference\. The 'FirstOrDefault' can return default null value\. BuildOverrides\.cs 254 _FirstOrDefault_ вернёт первый элемент _Assemblies_ или значение по умолчанию, если элементов нет\. Данная коллекция хранит объекты ссылочного типа, следовательно, значением по умолчанию будет _null_\. Если разработчик подразумевал, что _Assemblies_ может не иметь ни одного элемента, то непонятно, почему нет проверки на _null_ перед обращением к _FullName_\. Если же коллекция точно не будет пуста, стоит использовать _First_, а не _FirstOrDefault_\. Тогда и код не будет вызывать лишних вопросов\. **Issue 11** ```cpp internal void Update(ClientState state, Model model) { var roles = new ConcurrentDictionary (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](https://pvs-studio.ru/ru/docs/warnings/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** ```cpp public IEnumerable 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](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'nextSegment \=\= \-1' is always false\. CommandMapNode\.cs 109 Обратите внимание на второй _if_ в этом фрагменте кода, после чего посмотрите на выражение _nextSegment \=\= \-1 ? "" : text_\. Результатом данного условия всегда будет _false_\. В данном примере нет ошибки, лишь излишний код, но его также стоит избегать\. Кстати, далеко не всегда код, содержащий подобный паттерн ошибок, столь безобиден\. Если вы хотите убедиться в этом, рекомендую [ознакомиться с примерами ошибок](https://pvs-studio.ru/ru/blog/examples/v3022/), найденных с помощью этой диагностики\. ## Заключение PVS\-Studio удалось найти ряд подозрительных моментов в коде Discord\.NET\. Большая часть из них связана с потенциальным разыменованием нулевой ссылки\. Мне кажется, что разработчикам стоит обратить на это внимание\. Впрочем, как и на остальные срабатывания, описанные в этой статье :\)\. Статический анализатор позволяет экономить время и средства за счёт того, что ошибки будут найдены на этапе написания кода, а не на более поздних стадиях разработки\. Понятно, что статический анализ не идеален и найти все недостатки в проекте не сможет\. Тем не менее, подобные инструменты могут ускорить работу над проектом и сделать код более качественным\. Сможет ли анализатор помочь вам? Предлагаю [проверить это](https://pvs-studio.ru/ru/pvs-studio/try-free/) :\)\. И пишите в комментариях – нашёл ли анализатор странности в вашем коде?