# Copy\-paste на большом экране: разбор ошибок и странных мест Radarr Писать одно и то же — невероятно муторное занятие\! Поэтому программисты регулярно используют copy\-paste, но, помимо экономии времени, также растет и количество ошибок\. Чтобы в этом убедиться, мы разберём ошибки и странные места проекта Radarr\. ![1232_Radarr_errors_ru/image1.png](https://import.viva64.com/docx/blog/1232_Radarr_errors_ru/image1.png) ## Введение [Radarr](https://radarr.video/) — менеджер фильмов для пользователей Usenet и BitTorrent с открытым исходным кодом\. Проект поддерживает мониторинг RSS\-фидов для того, чтобы скачивать фильмы и обновлять их по мере появления нужных версий и разрешений\. Проект достаточно хорошо написан и содержит не так много ошибок\. Но тем не менее есть существенное количество слабых мест, которые появились из\-за комбинации невнимательности и использовании copy\-paste\. Для проведения анализа использовался исходный код, доступный на GitHub по [ссылке](https://github.com/Radarr/Radarr/releases/tag/v5.18.2.9651)\. Перейдём к рассмотрению обнаруженных анализатором ошибок\! ## Проверяем проект ### Ошибка из\-за упаковки **Фрагмент кода 1** ```cpp public bool Equals(OsPath other, bool ignoreTrailingSlash) { if (ReferenceEquals(other, null)) { return false; } .... } ``` Предупреждение PVS\-Studio: [V3161](https://pvs-studio.ru/ru/docs/warnings/v3161/) Comparing value type variables with 'ReferenceEquals' is incorrect because 'other' will be boxed\. OsPath\.cs 379 В этом случае выполняется сравнение объекта значимого типа по ссылке\. Метод `ReferenceEquals` принимает параметр типа `Object`, и при передаче значимого типа будет произведена упаковка\. Созданная в куче ссылка не будет равна никакой другой, и из\-за этого метод всегда будет возвращать `false`\. Приведу пример, который иллюстрирует, почему важно помнить nullable value types: ```cpp static void NullableTest() { int? a = null; object aObj = a; int? b = new int?(); object bObj = b; Console.WriteLine(Object.ReferenceEquals(aObj, bObj)); // True or False? } ``` Предлагаю вам внимательно изучить фрагмент кода выше и ответить, что будет выведено в консоль\. Для тех, кто хочет себя проверить или более подробно изучить этот вопрос, оставляю [ссылку на статью](https://pvs-studio.ru/ru/blog/posts/csharp/0772/)\. **Фрагмент кода 2** ```cpp switch (format.ToUpperInvariant()) { case "L": // Long format var stringBuilder = new StringBuilder(); stringBuilder.AppendLine("Guid: " + Guid ?? "Empty"); .... default: return ToString(); } ``` Предупреждение PVS\-Studio: [V3123](https://pvs-studio.ru/ru/docs/warnings/v3123/) Perhaps the '??' operator works in a different way than it was expected\. Its priority is lower than priority of other operators in its left part\. ReleaseInfo\.cs 87 На первый взгляд этот фрагмент кода тоже не вызывает подозрений, но автор забыл о том, что операторы `??` и `?:` имеют более низкий приоритет по сравнению с оператором `+`\. Таким образом, выражение `"Guid: " + Guid` будет отрабатывать первым, и даже если переменная `Guid` равна `null`, то `?? "Empty"` всё равно не будет выполнена\. Ситуация, когда программист один раз ошибся и потом преумножил ошибку копипастом, достаточно распространена даже в больших проектах\. Так, например, в проекте Azure SDK для \.NET можно было встретить следующие фрагменты кода: ```cpp public struct BlobSasBuilder : IEquatable { .... public bool Equals(BlobSasBuilder other) => BlobName == other.BlobName && CacheControl == other.CacheControl && BlobContainerName == other.BlobContainerName && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding && // <= ContentLanguage == other.ContentEncoding && // <= .... } ``` Предупреждение PVS\-Studio: [V3112](https://pvs-studio.ru/ru/docs/warnings/v3112/) An abnormality within similar comparisons\. It is possible that a typo is present inside the expression 'ContentLanguage \=\= other\.ContentEncoding'\. BlobSasBuilder\.cs 410 И точно такая же ошибка встречается в другом месте: ```cpp public struct FileSasBuilder : IEquatable { .... public bool Equals(FileSasBuilder other) => CacheControl == other.CacheControl && ContentDisposition == other.ContentDisposition && ContentEncoding == other.ContentEncoding // <= && ContentLanguage == other.ContentEncoding // <= .... } ``` [V3112](https://pvs-studio.ru/ru/docs/warnings/v3112/) An abnormality within similar comparisons\. It is possible that a typo is present inside the expression 'ContentLanguage \=\= other\.ContentEncoding'\. FileSasBuilder\.cs 265 Можете более подробно почитать про проверку Azure SDK для \.NET в этой [статье](https://pvs-studio.ru/ru/blog/posts/csharp/0692/)\. Такую базовую вещь можно случайно забыть и написать ошибочный код, подобный приведенному выше\. Встроенные в IDE анализаторы не способны выявить подобные паттерны, и проблема останется незамеченной\. Но в этой ситуации выручают инструменты статического анализа\. **Фрагмент кода 3** ```cpp var qualityComparer = new QualityModelComparer(qualityProfile); var qualityCompare = qualityComparer.Compare(newQuality?.Quality, // <= currentQuality.Quality); var downloadPropersAndRepacks = _configService.DownloadPropersAndRepacks; if (qualityCompare > 0 && QualityCutoffNotMet(qualityProfile, currentQuality, newQuality)) { _logger.Debug("New item has a better quality. Existing: {0}. New: {1}", currentQuality, newQuality); return UpgradeableRejectReason.None; } ``` Предупреждение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) The result of null\-conditional operator is dereferenced inside the 'Compare' method\. NullReferenceException is possible\. Inspect the first argument 'newQuality?\.Quality'\. UpgradableSpecification\.cs 34 Анализатор сообщает, что возможно возникновение `NullReferenceException` при выполнении программы\. В коде мы передали поле переменной без проверки её на `null`\. Ключевым моментом является то, что значение переменной вычисляется с помощью выражения, где используется оператор `null-conditional`\. Также в методе `Compare` не производится проверка аргумента на `null`, значит, при попытке обратиться к полю `Quality`, может возникнуть `NullReferenceException`\. Рассмотрим пример: ```cpp void UsersProcessing(Users users) { .... LogUserNames(users?.GetUsersCollection()); } void LogUserNames(IEnumerable usersList) { foreach (var user in usersList) { .... } } ``` Переменная 'usersList' передаётся в качестве аргумента методу 'LogUserNames'\. Она может иметь значение 'null', так как оно получено с помощью оператора null\-conditional\. Внутри 'LogUserNames' производится обход переданной коллекции\. Для этого используется 'foreach' и, следовательно, у коллекции будет вызван метод 'GetEnumerator'\. Если 'userList' имеет значение 'null', то будет выброшено исключение типа 'NullReferenceException'\. Вариант с исправлениями может выглядеть следующим образом: ```cpp void UsersProcessing(Users users) { .... LogUserNames( users?.GetUsersCollection() ?? Enumerable.Empty()); } void LogUserNames(IEnumerable usersList) { foreach (var user in usersList) { .... } } ``` Результат выполнения 'users?\.GetUsersCollection\(\)' присваивается переменной 'usersList'\. Если значение этой переменной 'null', то в метод 'LogUserNames' будет передаваться пустая коллекция\. Это поможет избежать исключения типа 'NullReferenceException' при обходе 'usersList' в 'foreach'\. > Подобный паттерн ошибок часто встречается при использовании \`foreach\`\\\. Об этом у нас есть \[статья\]\(https://pvs\-studio\.ru/ru/blog/posts/csharp/0832/\)\\\. **Фрагмент кода 4** ```cpp if (resource.Languages != null) { // Don't allow user to set movieFile with 'Any' or 'Original' language movieFile.Languages = resource.Languages .Where(l => l != Language.Any || l != Language.Original || l != null) .ToList(); } ``` Предупреждение PVS\-Studio: [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always true if it is evaluated: l \!\= null\. MovieFileController\.cs 122 Здесь явно намудрили с условием\. Из комментария понятно, что нельзя позволить пользователю установить язык на `Any` или `Original`, но этому условию будут удовлетворять вообще все элементы\. Допустим, на проверку поступает элемент, равный `Language.Any`\. Первую проверку он завалит, но вторую пройдёт\. Аналогично дело обстоит с элементами, равными `Language.Original`\. А элемент, не равный `Language.Any` и `Language.Original`, удовлетворит любое из условий\. Делаем вывод, что программист ошибся и должен был написать `&&` вместо `||`\. **Фрагмент кода 5** ```cpp var rootFolder = Path.GetDirectoryName(path); var movie = _movieService.GetMovie(movieId); var languageParse = LanguageParser.ParseLanguages(path); if ( languageParse.Count <= 1 && languageParse.First() == Language.Unknown // <= && movie != null) { } ``` Предупреждение PVS\-Studio: [V3179](https://pvs-studio.ru/ru/docs/warnings/v3179/) Calling the 'First' method for potentially empty collection 'languageParse' may result in exception\. ManualImportService\.cs 107 Анализатор выдал предупреждение с низким уровнем важности, но тем не менее мне эта ситуация кажется интересной\. В условии `if` проверяется, что коллекция `languageParse` имеет один элемент или меньше\. Если получится, что у `languageParse` не будет элементов вовсе, то при попытке получить первый элемент с помощью функции `First` программа получит исключение\. Подозрительное место в этой ситуации не является существенным, поскольку очень маловероятно, что у `languageParse` не будет элементов из\-за предшествующего кода\. ### Коварный \!\= **Фрагмент кода 6** ```cpp public virtual bool IsServiceRunning(string name) { _logger.Debug("Checking if '{0}' service is running", name); .... return service != null && ( service.Status != ServiceControllerStatus.Stopped || service.Status == ServiceControllerStatus.StopPending || service.Status == ServiceControllerStatus.Paused || service.Status == ServiceControllerStatus.PausePending); } ``` Предупреждение PVS\-Studio: [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: service\.Status \=\= ServiceControllerStatus\.StopPending\. ServiceProvider\.cs 55 Анализатор сообщает, что второе условие будет всегда ложным, если до него дойдёт дело\. Так и есть: если первая проверка `service.Status` окажется ложной, а это произойдёт только если `service.Status == ServiceControllerStatus.Stopped`, то и все остальные проверки будут ложными\. Возможно, программист опечатался и хотел использовать `==` вместо `!=` в условии `service.Status != ServiceControllerStatus.Stopped` ### Ошибки и подозрительные места, вызванные copy\-paste **Фрагмент кода 7** ```cpp switch (torrent.Status) { .... case "error": status = DownloadItemStatus.Failed; break; case "complete": status = DownloadItemStatus.Completed; break; case "removed": status = DownloadItemStatus.Failed; break; } ``` Предупреждение PVS\-Studio: [V3139](https://pvs-studio.ru/ru/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. Aria2\.cs 118 Здесь анализатор обращает наше внимание на то, что две или более ветви `case` выполняют одно и то же присваивание\. Без знания того, какой функционал закладывался в код, сложно понять, есть ли здесь ошибка\. В пользу того, что перед нами может быть ошибка, скажу, что у перечисления `DownloadItemStatus` есть неиспользованный элемент `Warning`, который, возможно, подходит по смыслу\. ### Тест на внимательность **Фрагмент кода 8** Разработчик оставил простенькую ошибку для внимательных, поэтому предлагаю вам попробовать самостоятельно найти опечатку: ```cpp switch (Settings.TraktListType) { case (int)TraktPopularListType.Trending: link += "movies/trending"; break; case (int)TraktPopularListType.Popular: link += "movies/popular"; break; case (int)TraktPopularListType.Anticipated: link += "movies/anticipated"; break; case (int)TraktPopularListType.BoxOffice: link += "movies/boxoffice"; break; case (int)TraktPopularListType.TopWatchedByWeek: link += "movies/watched/weekly"; break; case (int)TraktPopularListType.TopWatchedByMonth: link += "movies/watched/monthly"; break; case (int)TraktPopularListType.TopWatchedByYear: link += "movies/watched/yearly"; break; case (int)TraktPopularListType.TopWatchedByAllTime: link += "movies/watched/all"; break; case (int)TraktPopularListType.RecommendedByWeek: link += "movies/recommended/weekly"; break; case (int)TraktPopularListType.RecommendedByMonth: link += "movies/recommended/monthly"; break; case (int)TraktPopularListType.RecommendedByYear: link += "movies/recommended/yearly"; break; case (int)TraktPopularListType.RecommendedByAllTime: link += "movies/recommended/yearly"; break; } ```
Кто справился и нашёл ошибку, тот молодец\\\. А вот где она на самом деле: Предупреждение PVS\-Studio: V3139 Two or more case\-branches perform the same actions\. TraktPopularRequestGenerator\.cs 65 Здесь проблема схожа с той, что была в предыдущем фрагменте кода\. Эта ошибка, на мой взгляд, более очевидная и, скорее всего, допущена из\-за невнимательности при использовании copy\-paste\. По смыслу понятно, что переменная `link` в последнем `case` должна принимать значение `movies/recommended/all` вместо `movies/recommended/yearly`\. На это нам намекает код выше\. Справиться с такой ошибкой иногда и код ревью не поможет: у всех глаза раньше отвалятся, а у статического анализатора ничего не отвалится, и сделает он это практически моментально :\)
**Фрагмент кода 9** ```cpp public override bool Enable => OnGrab || OnDownload ||(OnDownload && OnUpgrade) || OnRename || OnMovieAdded || OnMovieDelete || OnMovieFileDelete ||(OnMovieFileDelete && OnMovieFileDeleteForUpgrade) || OnHealthIssue || OnHealthRestored || OnApplicationUpdate || OnManualInteractionRequired; ``` Предупреждение PVS\-Studio: [V3017](https://pvs-studio.ru/ru/docs/warnings/v3017/) A pattern was detected: \(OnDownload\) \|\| \(\(OnDownload\) && \.\.\.\)\. The expression is excessive or contains a logical error\. NotificationDefinition\.cs 62 Тот самый случай, когда в одной строке допущены сразу две ошибки\. Надеюсь, всем понятно, что не надо дублировать условия и нужно стараться писать код так, чтобы его можно было легко прочитать\. А если неочевидно, то поясню: условие `OnDownload ||(OnDownload && OnUpgrade)` является излишним, поскольку ему достаточно, чтобы `OnDownload` имело значение `true`\. Аналогичным образом дела обстоят с условиями `OnMovieFileDelete || (OnMovieFileDelete && OnMovieFileDeleteForUpgrade)`\. **Фрагмент кода 10** ```cpp if (importList == null || !definition.Enable) { _logger.Debug("Import List {0} ({1}) is not enabled, skipping.", importList.Name, // <= importList.Definition.Name); // <= return result; } ``` Предупреждение PVS\-Studio: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) The 'importList' object was used after it was verified against null\. Check lines: 147, 145\. FetchAndParseImportListService\.cs 147 Здесь произошла ошибка по невнимательности\. Очевидно, что разработчик хотел сделать проверку на `null`\. Но что\-то пошло не так, и, если `importList` равен `null`, будет обращение по нулевой ссылке в местах, отмеченных `// <=`, что вызовет ошибку\. **Фрагмент кода 11** ```cpp if (mediaInfo.VideoHdrFormatCompatibility .IsNotNullOrWhiteSpace()) { if (mediaInfo.VideoHdrFormatCompatibility .ContainsIgnoreCase("HLG")) { return HdrFormat.Hlg10; } if (mediaInfo.VideoHdrFormatCompatibility .ContainsIgnoreCase("dolby")) { return HdrFormat.DolbyVision; } if (mediaInfo.VideoHdrFormatCompatibility // <= .ContainsIgnoreCase("dolby")) { return HdrFormat.DolbyVision; // <= } if (mediaInfo.VideoHdrFormatCompatibility .ContainsIgnoreCase("hdr10+")) { return HdrFormat.Hdr10Plus; } if (mediaInfo.VideoHdrFormatCompatibility .ContainsIgnoreCase("hdr10")) { return HdrFormat.Hdr10; } } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'mediaInfo\.VideoHdrFormatCompatibility\.ContainsIgnoreCase\("dolby"\)' is always false\. 199\_mediainfo\_to\_ffmpeg\.cs 351 Здесь также явно прослеживается ошибка, допущенная из\-за невнимательного копипаста\. Оператор `if` с идентичным условием можно смело убрать, поскольку он никогда не выполнит свою роль\. Я предполагаю, что программист забыл изменить параметр внутри метода `mediaInfo.VideoHdrFormatCompability.ContainsIgnoreCase` и возвращаемый элемент `HdrFormat`\. На эту мысль меня натолкнул тот факт, что в перечислении `HdrFormat` ещё есть `DolbyVisionHdr10`, ` DolbyVisionSdr`, ` DolbyVisionHlg` и `DolbyVisionHdr10Plus`\. **Фрагмент кода 12** ```cpp if (videoFormat.ContainsIgnoreCase("MPEG-4 Visual")) { m.VideoFormat = "mpeg4"; .... return; } if ( videoFormat.ContainsIgnoreCase("MPEG-4 Visual") // <= || videoFormat.ContainsIgnoreCase("mp4v")) { m.VideoFormat = "mpeg4"; .... return; } ``` Предупреждение PVS\-Studio: [V3063](https://pvs-studio.ru/ru/docs/warnings/v3063/) A part of conditional expression is always false if it is evaluated: videoFormat\.ContainsIgnoreCase\("MPEG\-4 Visual"\)\. 199\_mediainfo\_to\_ffmpeg\.cs 205 Первая часть условия, отмеченного оператором `if`, является всегда ложной, поскольку, если условие верное, программа остановится ещё на первом `if`\. Исходя из значения, которое присваивается переменной `m.VideoFormat`, я бы предположил, что это ошибка копирования\. И условие должно проверять, соответствует ли формат видео `mp4v`, и, соответственно, в переменную `m.VideoFormat` нужно записывать `mp4v`\. Опять же, без знания, как должна работать программа, нельзя однозначно говорить, что перед нами ошибка\. Скорее это просто странность программы, о которой нужно сообщить разработчикам\. **Фрагмент кода 13** ```cpp if (Type == TokenType.UnknownSymbol) // <= { Value = Buffer[Index].ToString(); Index++; return Type; } .... if ( end >= Buffer.Length || Buffer[end] == ',' || Buffer[end] == ')' || Buffer[end] == '(' || char.IsWhiteSpace(Buffer[end])) { Index = end; } else if (Type == TokenType.UnknownSymbol) // <= { Value = Buffer[Index].ToString(); Index++; return Type; } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) Expression 'Type \=\= TokenType\.UnknownSymbol' is always false\. SqliteSyntaxReader\.cs 166 Здесь нижнее выражение `Type == TokenType.UnknownSymbol` будет всегда ложным, потому что сверху есть точно такое же условие, внутри которого находится `return`\. Если условие будет `true`, то выполнение программы в контексте этого метода закончится в теле первого `if`\. ## Заключение Подводя итог, можно заявить, что проект содержит не так уж и много ошибок, хоть и рассмотрели мы далеко не все\. Зачастую это были простые опечатки, которые допустили из\-за невнимательности\. Но на примере этого приложения можно сделать выводы о том, что даже такие мелкие вещи, как пренебрежение приоритетом операций, порой доставляют большие проблемы даже в таких крупных проектах, как Azure\. Также следует помнить о том, что чем больше проект, тем больше ответственность, а также возрастает и плотность ошибок\. Поэтому лучше всего отлавливать их на ранних стадиях разработки с помощью статического анализатора\. Если вы хотите писать безопасный код и не боятся опечаток, то можете попробовать анализатор PVS\-Studio по [ссылке](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Удачи в ваших проектах, и не забывайте проверять свой код\!