# Файловый менеджер Files: ошибки по папочкам Files — это сторонний файловый менеджер для Windows\. Он сделан для того, чтобы быть лучшим открытым файловым менеджером для ОС, который поддерживается сообществом\. А в этой статье мы взглянем на ошибки в исходном коде Files и внесём свой вклад в open source сообщество\. ![1295_Files_ru/image1.png](https://import.viva64.com/docx/blog/1295_Files_ru/image1.png) Прежде чем мы перейдём непосредственно к анализу проекта, предлагаю поближе с ним познакомиться\. На момент проверки Files имеет на GitHub чуть больше 39 тысяч звёзд\. Это достаточно большой показатель\. Files ориентирован на удобство пользователя, организацию файлов и папок и богатый функционал с глубокими интеграциями\. Вот пример интерфейса с [сайта](https://files.community/) проекта: ![1295_Files_ru/image2.png](https://import.viva64.com/docx/blog/1295_Files_ru/image2.png) Как мне кажется, он имеет более современный дизайн по сравнению с проводником в Windows 10 и 11\. При этом в Files есть поддержка палитры команд, интеграция с Git, удобные интеграции с облачными хранилищами и многое другое\. Вы можете гибко настроить его под себя, изменяя дизайн, переназначая сочетания клавиш и создавая свои команды\. Ранее я не использовал Files, но после знакомства с ним точно дам шанс этому проекту\. Хотя тяжело перестать машинально использовать встроенный проводник Windows после стольких лет\. Перейдём к проверке исходного кода\. Он доступен по [ссылке](https://github.com/files-community/Files)\. Проверялся следующий [коммит](https://github.com/files-community/Files/commit/a6b39a9fca99436b3e39d30d026fc6139459248f)\. Для проверки использовался статический анализатор PVS\-Studio\. Он может анализировать код, написанный на C, C\+\+, C\# и Java\. А Files как раз написан на C\#\. Кстати, этот проект уже перешёл с формата решений \.sln на \.slnx\. Если вам интересно, чем он лучше прошлого формата, или вы вообще не слышали про \.slnx, то пригашаю почитать эту [статью](https://pvs-studio.ru/ru/blog/posts/csharp/1265/)\. PVS\-Studio уже поддерживает новый формат решений, так что проблем это не добавляет\. Конечно, мы не будем рассматривать все ошибки, которые обнаружил анализатор\. В рамках статьи смотреть десятки похожих предупреждений было бы неинтересно\. Всего PVS\-Studio выдал около 250 предупреждений\. И, как я говорил, мы внесём свой вклад в open source сообщество, сообщив разработчикам о найденных потенциальных проблемах\. А теперь поехали\! ## Ошибки в коде **Issue 1** ```cpp protected void ChangeMode(OmnibarMode? oldMode, OmnibarMode newMode) { .... var modeSeparatorWidth = itemCount is not 0 or 1 ? _modesHostGrid.Children[1] is FrameworkElement frameworkElement ? frameworkElement.ActualWidth : 0 : 0; .... } ``` Предупреждение PVS\-Studio: [V3207](https://pvs-studio.ru/ru/docs/warnings/v3207/) \[CWE\-670\] The 'not 0 or 1' logical pattern may not work as expected\. The 'not' pattern is matched only to the first expression from the 'or' pattern\. Files\.App\.Controls Omnibar\.cs 149 А начнём мы с одной интересной и в то же время лёгкой ошибки\. В этом коде нас интересует часть `itemCount is not 0 or 1`\. Уже догадались? Этот паттерн избыточен\. Его вторая часть ни на что не влияет\. Люди привыкли, что когда мы в речи говорим "x это не 0 или 1", то имеем в виду, что x это не 0 и не 1\. Но в C\# приоритет операций другой\. `x is not 0 or 1` на самом деле `x is (not 0) or 1`\. Подобные ошибки могут приводить не просто к избыточности, но и к ошибкам по типу NullReferenceException: `list is not null or list.Count == 0`\. По этой проблеме даже была [встреча](https://github.com/dotnet/csharplang/blob/main/meetings/2024/LDM-2024-11-04.md#not-and-or-patterns), на которой обсуждались возможные пути решения\. Исходя из обсуждения, вероятно, в будущем эта ситуация будет отлавливаться или компилятором, или встроенным статическим анализом как предупреждение\. **Issue 2** ```cpp private void SetPathData(string pathData, FrameworkElement element) { .... if (GetTemplateChild(LayerPathPart) is Path path) { path.Data = geometry; path.Width = element.Width; path.Width = element.Height; } } ``` Предупреждение PVS\-Studio: [V3127](https://pvs-studio.ru/ru/docs/warnings/v3127/) \[CWE\-682\] Two similar code fragments were found\. Perhaps, this is a typo and 'Height' variable should be used instead of 'Width' Files\.App\.Controls ThemedIconLayer\.cs 122 Часто встречаемая copy\-paste ошибка\. Вместо `path.Height` присваивание происходит дважды в `path.Width`\. **Issue 3** ```cpp private void UpdateRadii(double newRadius, bool isTrack) { double valueRingThickness = _valueRingThickness; double trackRingThickness = _trackRingThickness; // Limit the Thickness values to no more than 1/5 of the container size if (isTrack) trackRingThickness = newRadius > (AdjustedSize / 5) ? (AdjustedSize / 5) : newRadius; else valueRingThickness = newRadius > (AdjustedSize / 5) ? (AdjustedSize / 5) : newRadius; // If both Rings have Equal thickness, use 0; // otherwise, use the larger thickness to adjust the size double check = (AdjustedSize / 2) – (_thicknessCheck is ThicknessCheck.Equal ? 0 : _largerThickness / 2); double minSize = 4; _sharedRadius = check <= minSize ? minSize : check; } ``` Предупреждения PVS\-Studio: [V3137](https://pvs-studio.ru/ru/docs/warnings/v3137/) \[CWE\-563\] The 'trackRingThickness' variable is assigned but is not used by the end of the function\. Files\.App\.Controls StorageRing\.cs 211 [V3137](https://pvs-studio.ru/ru/docs/warnings/v3137/) \[CWE\-563\] The 'valueRingThickness' variable is assigned but is not used by the end of the function\. Files\.App\.Controls StorageRing\.cs 213 Анализатор обнаружил, что две локальные переменные не используются после присваивания им значений\. Значения из полей `_valueRingThickness` и `_trackRingThickness` записываются в локальные переменные `trackRingThickness` и `valueRingThickness`\. Далее они, в зависимости от условия, изменяются, но позже в методе не используются\. Вполне вероятно, что локальные переменные должны были использоваться в дальнейших операциях\. **Issue 4** ```cpp public Task ExecuteAsync(object? parameter = null) { if (context.HasSelection && context?.SelectedItem?.ItemPath is not null) ExecuteShellCommand(context.SelectedItem.ItemPath); else if (context?.Folder?.ItemPath is not null) ExecuteShellCommand(context.Folder.ItemPath); return Task.CompletedTask; } ``` Предупреждение PVS\-Studio: [V3042](https://pvs-studio.ru/ru/docs/warnings/v3042/) \[CWE\-476\] Possible NullReferenceException\. The '?\.' and '\.' operators are used for accessing members of the 'context' object Files\.App OpenClassicPropertiesAction\.cs 40 Привычная для C\# разработчиков ошибка — NullReferenceException\. В условии `if` разыменовывается `context` без проверки на `null`, хотя далее в этом уже условии и в `else if` есть проверка с помощью `?.`\. **Issue 5** ```cpp public async Task ExecuteAsync(object? parameter = null) { if (context.SelectedItems.Count > 0) { foreach (ListedItem listedItem in context.ShellPage?.SlimContentPage.SelectedItems) { .... } } } ``` Предупреждение PVS\-Studio: [V3153](https://pvs-studio.ru/ru/docs/warnings/v3153/) \[CWE\-476\] Enumerating the result of null\-conditional access operator can lead to NullReferenceException\. Files\.App UnpinFromStartAction\.cs 32 Ещё один потенциальный NRE\. В цикле `foreach` перечисляемая коллекция получается с использованием оператора `?.`\. Не все разработчики знают, что `foreach` не работает с `null`\. Этот тип ошибки достаточно часто встречается\. На эту тему есть даже отдельная статья: "[Использование оператора ?\. в foreach: защита от NullReferenceException, которая не работает](https://pvs-studio.ru/ru/blog/posts/csharp/0832/)"\. ![1295_Files_ru/image3.png](https://import.viva64.com/docx/blog/1295_Files_ru/image3.png) **Issue 6** ```cpp private void Window_Activated(object sender, WindowActivatedEventArgs args) { AppModel.IsMainWindowClosed = false; // TODO(s): Is this code still needed? if (args.WindowActivationState != WindowActivationState.CodeActivated || args.WindowActivationState != WindowActivationState.PointerActivated) return; ApplicationData.Current.LocalSettings.Values["INSTANCE_ACTIVE"] = -Environment.ProcessId; } ``` Предупреждение PVS\-Studio: [V3022](https://pvs-studio.ru/ru/docs/warnings/v3022/) \[CWE\-571\] Expression is always true\. Probably the '&&' operator should be used here\. Files\.App App\.xaml\.cs 181 Анализатор обнаружил условие оператора `if`, которое всегда `true`\. `args.WindowActivationState` сравнивается с элементами перечисления\. Из\-за ошибочного использования оператора `||`, вместо `&&` в условии проверяется, что значение из свойства `WindowActivationState` не равно или `WindowActivationState.CodeActivated`, или `WindowActivationState.PointerActivated`\. Оно всегда не равно чему\-то одному\. Выливается это ещё и в недостижимость кода, который идёт после `if`: [V3142](https://pvs-studio.ru/ru/docs/warnings/v3142/) \[CWE\-561\] Unreachable code detected\. It is possible that an error is present\. Files\.App App\.xaml\.cs 185 Кстати, можно заметить комментарий "Этот код всё ещё нужен?"\. Но он и в прошлом не был нужен, и в будущем не нужен, т\.к\. содержит ошибку\. Если разработчики хотели сделать недостижимый код, то можно просто поставить безусловный return\. **Issue 7** ```cpp public AccessControlEntry(bool isFolder, string ownerSid, AccessControlEntryType type, AccessMaskFlags accessMaskFlags, bool isInherited, AccessControlEntryFlags inheritanceFlags) { AccessMaskItems = SecurityAdvancedAccessControlItemFactory .Initialize(this, AreAdvancedPermissionsShown, IsInherited, IsFolder); ChangeAccessControlTypeCommand = new RelayCommand(x => { //AccessControlType = Enum.Parse(x); }); ChangeInheritanceFlagsCommand = new RelayCommand(x => { //var parts = x.Split(','); //InheritanceFlags = Enum.Parse(parts[0]); }); IsFolder = isFolder; Principal = new(ownerSid); AccessControlType = type; AccessMaskFlags = accessMaskFlags; IsInherited = isInherited; AccessControlEntryFlags = inheritanceFlags; .... } ``` Видите ошибку? А их тут целых две: * [V3128](https://pvs-studio.ru/ru/docs/warnings/v3128/) \[CWE\-665\] The 'IsFolder' property is used before it is initialized in constructor\. Files\.App AccessControlEntry\.cs 337 * [V3128](https://pvs-studio.ru/ru/docs/warnings/v3128/) \[CWE\-665\] The 'IsInherited' property is used before it is initialized in constructor\. Files\.App AccessControlEntry\.cs 337 Здесь свойства `IsFolder` и `IsInherited` используются до того, как произойдёт их инициализация в этом же конструкторе с помощью соответствующих им аргументов\. Выходит, что `IsFolder` и `IsInherited` всегда передаются в метод `SecurityAdvancedAccessControlItemFactory.Initialize` с дефолтным значением `false`\. **Issue 8** ```cpp protected override async IAsyncEnumerable GetProviders() { .... var reader = cmdConnection.ExecuteReader(); while (reader.Read()) { var connection = ( ConnectionType: reader["conn_type"]?.ToString(), HostName: reader["host_name"]?.ToString() ); connections.Add(reader["id"]?.ToString(), connection); } .... } ``` Предупреждение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) \[CWE\-690\] The result of null\-conditional operator is passed as the first argument to the 'Add' method and is not expected to be null\. Files\.App SynologyDriveCloudDetector\.cs 52 У анализатора есть информация о том, в какой библиотечный метод можно передавать `null`, а в какой нельзя\. Мы называем эту технологию аннотированием методов\. Из коробки в анализаторе есть информацию, например, о системных библиотеках, Unity, ASP\.NET Core и т\.д\. Подробнее про технологии статического анализа вы можете прочитать в [этой статье](https://pvs-studio.ru/ru/blog/posts/0908/)\. Так, метод `Add` — это экземплярный метод для `Dictionary`\. При передаче `null` первым аргументом, т\.е\. в качестве ключа словаря, будет выброшено исключение `ArgumentNullException`\. В качестве первого аргумента идёт выражение с использованием `?.`\. Не могу сказать, насколько это возможная ситуация, но ошибка есть ошибка\. **Issue 9** ```cpp public void Log(....) { if (exception is null || exception.Data[Mechanism.HandledKey] is false || logLevel <= LogLevel.Information) return; var generalSettingsService = Ioc.Default.GetRequiredService(); var level = logLevel switch { LogLevel.Debug => SentryLevel.Debug, LogLevel.Information => SentryLevel.Info, LogLevel.Warning => SentryLevel.Warning, LogLevel.Error => SentryLevel.Error, LogLevel.Critical => SentryLevel.Fatal, _ => SentryLevel.Debug }; } ``` Предупреждение PVS\-Studio: [V3202](https://pvs-studio.ru/ru/docs/warnings/v3202/) \[CWE\-561\] Unreachable code detected\. The 'case' value is out of range of the match expression\. Files\.App SentryLogger\.cs 35 Ещё немного недостижимого кода\. В `switch` соотносится значение `logLevel` и элементов перечисления `LogLevel`\. Но обратите внимание, что перед `switch` есть проверка: если `logLevel <= LogLevel.Information`, то `return`\. Взглянем на определение перечисления LogLevel: ```cpp public enum LogLevel { Trace = 0, Debug = 1, Information = 2, .... } ``` Т\.е\., исходя из условия, `Trace`, `Debug` и `Information` не доходят до `switch`\. Но как раз\-таки `Debug` и `Information` мы видим в первые двух выражениях в `switch`\. **Issue 10** ```cpp private async void Omnibar_QuerySubmitted(....) { .... if (action is not null) { var overload = action.GetOverloads().FirstOrDefault(); await overload?.InvokeAsync(actionInstance.Context); } .... } ``` Предупреждение PVS\-Studio: [V3168](https://pvs-studio.ru/ru/docs/warnings/v3168/) \[CWE\-476\] Awaiting on the 'overload?\.InvokeAsync\(actionInstance\.Context\)' expression with potential null value can lead to NullReferenceException\. Files\.App NavigationToolbar\.xaml\.cs 214 Ещё один потенциальный NRE, но теперь не из\-за `foreach` или метода, который не работает с `null`\. Здесь используется оператор `await` с выражением, значением которого может быть `null`\. Анализатор предполагает это из\-за оператора `?.`\. **Issue 11** ```cpp private void ReportProgress(StatusCenterItemProgressModel value) { switch (value.TotalSize, value.ItemsCount) { // In progress, displaying items count & processed size case (not 0, not 0): ProgressPercentage = Math.Clamp((int)(value.ProcessedSize * 100.0 / value.TotalSize), 0, 100); SpeedText = $"{value.ProcessingSizeSpeed.ToSizeString()}/s"; point = new((float)(value.ProcessedSize * 100.0 / value.TotalSize), (float)value.ProcessingSizeSpeed); break; // In progress, displaying processed size case (not 0, _): ProgressPercentage = Math.Clamp((int)(value.ProcessedSize * 100.0 / value.TotalSize), 0, 100); SpeedText = $"{value.ProcessingSizeSpeed.ToSizeString()}/s"; point = new((float)(value.ProcessedSize * 100.0 / value.TotalSize), (float)value.ProcessingSizeSpeed); break; // In progress, displaying items count case (_, not 0): ProgressPercentage = Math.Clamp((int)(value.ProcessedItemsCount * 100.0 / value.ItemsCount), 0, 100); SpeedText = $"{value.ProcessingItemsCountSpeed:0} items/s"; point = new((float)(value.ProcessedItemsCount * 100.0 / value.ItemsCount), (float)value.ProcessingItemsCountSpeed); break; default: .... } .... } ``` Предупреждение PVS\-Studio: [V3139](https://pvs-studio.ru/ru/docs/warnings/v3139/) Two or more case\-branches perform the same actions\. Files\.App StatusCenterItem\.cs 342 Анализатор обнаружил ситуацию, когда в `switch` первые два `case` содержат одинаковые фрагменты кода\. Здесь эти фрагменты достаточно большие, так что вряд ли их написали просто так вместо того, чтобы объединить оба `case`: ```cpp case (not 0, not 0): case (not 0, _): ``` Вероятно, они должны немного отличаться\. Но чем? Давайте посмотрим на комментарии к `case`: ```cpp // Set speed text and percentage switch (value.TotalSize, value.ItemsCount) { // In progress, displaying items count & processed size case (not 0, not 0): .... // In progress, displaying processed size case (not 0, _): .... // In progress, displaying items count case (_, not 0): .... default: .... } ``` Читаем и видим: * в первом случае в прогресс отображается количество элементов и обработанный размер чего\-то; * во втором случае в прогресс отображается обработанный размер чего\-то; * в третьем случае в прогресс отображается количество элементов\. Достаточно непросто искать ошибки в проекте, когда ты не являешься его разработчиком, но можно заметить закономерность: * в первом `case` используется только свойство `ProcessedSize`; * во втором `case` используется только свойство `ProcessedSize`; * в третьем `case` используется только свойство `ProcessedItemsCount`\. Выходит, что в первом `case` не хватает обработки для отображения `ProcessedItemsCount`\. Поздравляю, мы добрались до проблемы\. ![1295_Files_ru/image4.png](https://import.viva64.com/docx/blog/1295_Files_ru/image4.png) Рисунок N4 – Источник: сериал "Шерлок" **Issue 12** ```cpp public bool LoadTagsList => SelectedItem?.HasTags ?? false && PreviewPaneState is PreviewPaneStates.NoPreviewAvailable || PreviewPaneState is PreviewPaneStates.PreviewAndDetailsAvailable; ``` Предупреждение PVS\-Studio: [V3177](https://pvs-studio.ru/ru/docs/warnings/v3177/) \[CWE\-783\] The 'false' literal belongs to the '\|\|' operator with a higher priority\. It is possible the literal was intended to belong to '??' operator instead\. Files\.App InfoPaneViewModel\.cs 122 Ещё одна не самая очевидная ошибка\. Здесь всё дело в приоритете операций\. Т\.к\. приоритет оператора '??' ниже, чем у '&&', сначала будет выполнена проверка выражения `false && PreviewPaneState is PreviewPaneStates.NoPreviewAvailable`, которая всегда возвращает `false`\. Сомнительно, что именно этого хотел добиться разработчик\. **Issue 13** ```cpp private static string FormatEncodingBitrate(object input) { // For cases when multiple files are selected and it has a string value if (input.GetType() != typeof(uint)) return input?.ToString() ?? string.Empty; .... } ``` Предупреждение PVS\-Studio: [V3095](https://pvs-studio.ru/ru/docs/warnings/v3095/) \[CWE\-476\] The 'input' object was used before it was verified against null\. Check lines: 375, 376\. Files\.App FileProperty\.cs 375 Анализатор сигнализирует о том, что `input` сначала используется без проверки на `null`, а после с проверкой\. Не первый раз встречаю паттерн, когда объект не проверяют на равенство `null` при вызове метода `GetType()`\. Может, кто\-то думает, что он работает как метод расширения, но нет\. При вызове получим `NullReferenceException`\. **Issue 14** ```cpp private void ColumnViewBase_KeyUp(object sender, ....) { var shPage = ActiveColumnShellPage as ColumnShellPage; if (shPage?.SlimContentPage?.SelectedItem?.PrimaryItemAttribute is not StorageItemTypes.Folder) CloseUnnecessaryColumns(shPage?.ColumnParams); // <= } private void CloseUnnecessaryColumns(ColumnParam column) { if (string.IsNullOrEmpty(column.NavPathParam)) return; .... } ``` Предупреждение PVS\-Studio: [V3105](https://pvs-studio.ru/ru/docs/warnings/v3105/) \[CWE\-690\] The result of null\-conditional operator is dereferenced inside the 'CloseUnnecessaryColumns' method\. NullReferenceException is possible\. Inspect the first argument 'shPage?\.ColumnParams'\. Files\.App ColumnsLayoutPage\.xaml\.cs 287 Просто пример работы межпроцедурного анализа\. Анализатор увидел, что в метод `CloseUnnecessaryColumns` передаётся значение выражения `shPage?.ColumnParams`, которое может быть `null`\. В методе `CloseUnnecessaryColumns` параметр сразу разыменовывается, что может привести к NRE\. При этом условие в `if` не спасает от проблемы\. Условие в `if` будет равно `true`, если знаечние переменной `shPage` будет равно `null`\. **Issue 15** ```cpp protected override void OnNavigatedTo(NavigationEventArgs eventArgs) { .... if (FolderSettings?.ColumnsViewModel is not null) // <= { ColumnsViewModel.DateCreatedColumn.Update( FolderSettings.ColumnsViewModel.DateCreatedColumn); ColumnsViewModel.DateDeletedColumn.Update( FolderSettings.ColumnsViewModel.DateDeletedColumn); ColumnsViewModel.DateModifiedColumn.Update( FolderSettings.ColumnsViewModel.DateModifiedColumn); .... } ParentShellPageInstance.ShellViewModel.EnabledGitProperties = GetEnabledGitProperties(ColumnsViewModel); FolderSettings.LayoutModeChangeRequested += // <= FolderSettings_LayoutModeChangeRequested; .... } ``` Предупреждение PVS\-Studio: [V3125](https://pvs-studio.ru/ru/docs/warnings/v3125/) \[CWE\-476\] The 'FolderSettings' object was used after it was verified against null\. Check lines: 174, 148\. Files\.App DetailsLayoutPage\.xaml\.cs 174 Проблема похожа на те, что мы рассматривали ранее\. Свойство `FolderSettings` проверяется на равенство `null` в условии `if` и далее используется в then\-ветви\. Но после `if` используется уже без проверки\. ## Заключение Вот мы и рассмотрели часть ошибок, найденных в Files\. Разработчики создают действительно классный продукт, который хочется использовать\. Если вам интересно, что ещё нашёл PVS\-Studio в этом проекте, или вы хотите проверить свой собственный проект, то скачать и попробовать анализатор можно по [ссылке](https://pvs-studio.ru/ru/pvs-studio/try-free/)\. Удачи\!