Фраза "вопросов нет" часто слышна в конце встречи или доклада. Чаще всего она означает, что всё прошло хорошо. Однако в мире программирования её буквальное значение обретает совершенно иной, даже тревожный оттенок. Когда речь заходит о разработке, отсутствие "вопросов" зачастую указывает не на ясность, а на скрытые проблемы. Давайте разберём, как их отсутствие может навредить качеству проекта.

Немного о проекте

Stability Matrix — это проект с открытым исходным кодом, который представляет собой удобный графический интерфейс для управления различными инструментами ИИ для генерации изображений, анимации и 3D. Он упрощает процесс управления моделями, выбора параметров и создания изображений, выступая в роли "панели управления" для модели Stable Diffusion. Проект разработан на языке C#, использует платформу .NET для обеспечения кроссплатформенности.

Подготовка к анализу проекта

Рассмотри сценарий использования плагина PVS-Studio в Visual Studio Code для Linux. Так как проект Stability Matrix кроссплатформенный, он поддерживается на системе Linux. Чтобы проанализировать проект C# (MSBuild) на Linux, нужно установить консольную утилиту pvs-studio-dotnet. Подробнее о неё написано в документации.

После установки pvs-studio-dotnet, в VS Code нужно установить плагин PVS-Studio. В разделе Extensions в поле поиска необходимо ввести PVS-Studio и установить расширение, нажав кнопку Install:

После установки расширения окно PVS-Studio появится в качестве одной из вкладок на нижней панели VS Code. Для запуска анализа необходимо нажать на кнопку Start analysis. В этом случае будет запущен анализ всего решения.

После первого запуска анализа в папке проекта автоматически появится файл [workspace folder]/.PVS-Studio/MSBuildAnalyzerConfig.jsonc. Его можно редактировать, чтобы изменить настройки анализа: они совпадают с теми, что доступны в параметрах консольной версии анализатора.

После окончания анализа проекта появятся срабатывания анализатора.

Давайте рассмотрим некоторые срабатывания в проекте, которые были найдены с помощью PVS-Studio.

Ошибки в конструкторе

Issue 1

private void DelayedClearProgress(TimeSpan delay)
{
  Task.Delay(delay).ContinueWith(_ =>
  {
    TotalProgress = new ProgressReport(0, 0);
  });
}

Предупреждение PVS-Studio: V3064 Division inside method by the 2nd argument '0' which has zero value. HuggingFacePageViewModel.cs 226

Анализатор предупреждает, что в методе может быть деление на ноль. Чтобы его увидеть, нужно перейти внутрь метода ProgressReport.

public ProgressReport(int current, int total, ....)
{
  if (current < 0)
    throw new ArgumentOutOfRangeException(nameof(current), 
                                          "Current progress cannot negative.");
  if (total < 0)
    throw new ArgumentOutOfRangeException(nameof(total), 
                                          "Total progress cannot be negative.");
  ....
  Progress = (double)current / total;
  ....
}

В конструктор структуры ProgressReport передаются два параметра со значениями 0. Поэтому параметры current и total равны 0. Автор проверяет эти значения, но не проверяет их равенство с нолём. Поэтому значения не попадут под условия. Затем при определении свойства Progress параметры делятся друг на друга. Такое деление на 0 приведёт к значению NaN, так как результат деления приводится к типу double.

Issue 2

private readonly IModelIndexService modelIndexService;

public InferenceFluxTextToImageViewModel(
                      IServiceManager<ViewModelBase> vmFactory,
                      IInferenceClientManager inferenceClientManager,
                      INotificationService notificationService,
                      ISettingsManager settingsManager,
                      RunningPackageService runningPackageService)
{
  this.notificationService = notificationService;
  this.modelIndexService = modelIndexService;  // <=

  SeedCardViewModel = vmFactory.Get<SeedCardViewModel>();
  SeedCardViewModel.GenerateNewSeed();

  ModelCardViewModel = vmFactory.Get<UnetModelCardViewModel>();
}

Предупреждение PVS-Studio: V3005 The 'this.modelIndexService' variable is assigned to itself. InferenceFluxTextToImageViewModel.cs 63

Значение поля приравнивается само к себе. Поле имеет свойство readonly, оно может изменяться только в этом участке кода. Но из-за приравнивания к самому себе значение поля будет всегда null.

Судя по коду, автор этого не планировал. Ошибка из-за невнимательности. В конструкторе этого класса не объявлен параметр modelIndexService, поэтому полю приравнивается то же значение. Значит, в нём будет хранится значение null.

Вопросов нет

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

Issue 3

private async Task ShowVersionDialog(CivitModel model)
{
  ....
  var viewModel = dialogFactory.Get<SelectModelVersionViewModel>();
  viewModel.Dialog = dialog;
  viewModel.Title = model.Name;
  viewModel.Description = htmlDescription;
  viewModel.CivitModel = model;
  viewModel.SelectedVersionViewModel = viewModel.Versions.Any() ?
                                       viewModel.Versions[0] : null;
  ....
  var selectedVersion = viewModel?.SelectedVersionViewModel?.ModelVersion;
  var selectedFile = viewModel?.SelectedFile?.CivitFile;
  ....
}

Предупреждение PVS-Studio: V3095 The 'viewModel' object was used before it was verified against null. Check lines: 265, 335. CheckpointBrowserCardViewModel.cs 265

В этом примере анализатор PVS-Studio выдал срабатывание на использование переменной viewModel. Ниже автор использует её с оператором ? , чтобы проверить значения на null, защищая программу от падения. Но при первых использованиях переменной он не проверял её значения. Из-за этого может произойти падение программы с NullReferenceException.

partial void OnSelectedFileChanged(CivitFileViewModel? value)
{
  ....
  var fileSizeBytes = value?.CivitFile.SizeKb * 1024;
  ....
  IsImportEnabled = value?.CivitFile != null 
                  && canImport && !ShowEmptyPathWarning;
}

Предупреждение PVS-Studio: V3095 The 'CivitFile' object was used before it was verified against null. Check lines: 139, 157. SelectModelVersionViewModel.cs 139

Автор проекта часто так ошибался, вот ещё подобный случай. По сравнению с примером выше, переменная была использована небезопасно один раз. Но и это может привести к серьёзным последствиям.

Здесь автор первый раз вызывает свойство переменной, не проверяя её значение. Но ниже проверка на null реализована. Следовательно, при использовании переменной value?.CivitFile.SizeKb будет NRE.

public void ApplyStep(ModuleApplyStepEventArgs e)
{
  ....
  if (SelectedModel?.RelativePath.EndsWith("gguf", 
                   StringComparison.OrdinalIgnoreCase) is true)
  {
    ....
    UnetName = SelectedModel?.RelativePath ?? 
                   throw new ValidationException("Model not selected")

  }
}

Предупреждение PVS-Studio: V3095 The 'RelativePath' object was used before it was verified against null. Check lines: 102, 109. WanModelCardViewModel.cs 102

Сценарий тот же: автор использовал переменную 'RelativePath', не проверив её на null. При втором использовании такая проверка есть. Значит, разработчик понимал, что значение может быть null, но не защитил свой код.

Issue 4

partial void OnSelectedOutputTypeChanged
(SharedOutputType? oldValue, SharedOutputType? newValue)
{
  if (oldValue == newValue || oldValue == null || newValue == null)
    return;

  var path = newValue 
             == SharedOutputType.All
             ? SelectedCategory?.Path
             : Path.Combine(SelectedCategory.Path, newValue.ToString());
  GetOutputs(path);
}

Предупреждение PVS-Studio: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'SelectedCategory' object OutputsPageViewModel.cs 242

Анализатор выдал срабатывание, которое может привести к NullReferenceException. Снова был не использован оператор ? для переменной SelectedCategory.Path. Первый раз автор проверил эту переменную, а в следующий строке этого не сделал.

В вышеописанных случаях анализатор не может до конца знать, баг это или фича. Да, можно сказать, что автор был точно уверен, что ему будут приходить только заполненные данные, без null. Анализатор подсвечивает потенциальные ошибки, поэтому верифицировать срабатывание нужно уже разработчику. Только он может точно объяснить, что хотел от конкретного блока кода и определить, ожидаемое это поведение или нет.

Оператор ? играет ключевую роль для null-безопасности программы. Анализатор PVS-Studio выявил множество срабатываний в коде, связанных с его отсутствием. Однако по результатам такого разового анализа сложно с уверенностью сказать, было ли такое поведение программы осознанным решением автора или же это потенциально опасное упущение. Именно поэтому статический анализ должен быть не разовой акцией, а неотъемлемой частью всего жизненного цикла ПО: от разработки до поддержки.

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

В проекте Stability Matrix есть и другие интересные срабатывания, разберём и их.

Ошибки copy-paste

Сейчас предлагаю вам попробовать себя в роли анализатора. Найдите ошибку во фрагменте кода ниже.

Issue 5

public override void LoadStateFromJsonObject(JsonObject state)
{
  SelectedVae = model.SelectedVaeName is null
            ? HybridModelFile.Default
            : ClientManager.VaeModels.FirstOrDefault(x => x.RelativePath ==   
  model.SelectedVaeName);

  SelectedRefiner = model.SelectedRefinerName is null
                ? HybridModelFile.None
                : ClientManager.Models.FirstOrDefault(x => x.RelativePath == 
  model.SelectedRefinerName);

  SelectedClip1 = model.SelectedClip1Name is null
              ? HybridModelFile.None
              : ClientManager.ClipModels.FirstOrDefault(x => x.RelativePath == 
  model.SelectedClip1Name);

  SelectedClip2 = model.SelectedClip2Name is null
              ? HybridModelFile.None
              : ClientManager.ClipModels.FirstOrDefault(x => x.RelativePath == 
  model.SelectedClip2Name);

  SelectedClip3 = model.SelectedClip3Name is null
              ? HybridModelFile.None
              : ClientManager.ClipModels.FirstOrDefault(x => x.RelativePath == 
  model.SelectedClip3Name);

  SelectedClip4 = model.SelectedClip3Name is null
              ? HybridModelFile.None
              : ClientManager.ClipModels.FirstOrDefault(x => x.RelativePath == 
  model.SelectedClip4Name);
}

Типичная ошибка копипаста. Автор скопировал верхний блок, проверив переменную model.SelectedClip3Name, хотя, следуя логике кода, нужно было проверить model.SelectedClip4Name.

Скорее всего, ошибка нашлась легко: вы изначально знали, что она где-то притаилась в этом фрагменте. А представьте разработчика, который смотрит на весь код, где уже написано множество строк, классов, методов, переменных. Такую ошибку сложно найти в проекте. Нужно долго идти отладчиком, просматривать каждую переменную. Это требует много времени и сил. Чтобы упростить и ускорить процесс, лучше использовать анализатор кода, который легко справился с поиском этой ошибки.

Issue 6

[TestMethod]
public void TestDeserialize_UnknownEnum_ShouldUseEnumMemberValue()
{
  const string json = "\"Value 2\"";

  var result = JsonSerializer.Deserialize<UnknownEnum>(json);

  Assert.AreEqual(UnknownEnum.Value2, result);
}

[TestMethod]
public void TestSerialize_UnknownEnum_ShouldUseEnumMemberValue()
{
  const string json = "\"Value 2\"";

  var result = JsonSerializer.Deserialize<UnknownEnum>(json);

  Assert.AreEqual(UnknownEnum.Value2, result);
}

Предупреждение PVS-Studio: V3013 It is odd that the body of 'TestDeserialize_UnknownEnum_ShouldUseEnumMemberValue' function is fully equivalent to the body of 'TestSerialize_UnknownEnum_ShouldUseEnumMemberValue' function. DefaultUnknownEnumConverterTests.cs 51

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

Тесты — это такой же код, а значит, в них могут быть ошибки. Плохой тест хуже, чем его отсутствие, потому что он создаёт ложное чувство уверенности. Часто разработчики пренебрегают проверкой тестов, что может приводить к потери ценности и качеству тестирования, замедлению разработки, плохому качеству кода, росту затрат на поддержку кодовой базы. Из-за этого придётся тратить время и финансы, чтобы отладить и привести в порядок все тесты, и только потом продолжать разработку продукта. Чтобы этого избежать, нужно чаще проверять и тестировать написанные тесты. Подробнее этот вопрос разбирали в этой статье.

Init

Issue 7

public class SingletonAttribute : Attribute
{
  [DynamicallyAccessedMembers(
    DynamicallyAccessedMemberTypes.PublicConstructors)]
  public Type? InterfaceType { get; init; }

  [DynamicallyAccessedMembers(
    DynamicallyAccessedMemberTypes.PublicConstructors)]
  public Type? ImplType { get; init; }

  public SingletonAttribute() { }

  public SingletonAttribute(Type interfaceType)
  {
    InterfaceType = interfaceType;
  }

  public SingletonAttribute(Type interfaceType, Type implType)
  {
    InterfaceType = implType;
    ImplType = implType;
  }
}

Предупреждение PVS-Studio: V3117 Constructor parameter 'interfaceType' is not used. SingletonAttribute.cs 24

Начиная с C# 9.0 (подробнее про нововведения в C# 9.0 писали в статье), для свойств доступен модификатор init, который определяет сеттер, доступный только во время инициализации объекта. Значение такому свойству можно присвоить только в инициализаторе объекта, в его конструкторе или непосредственно при объявлении. После инициализации свойство становится неизменяемым (read-only).

Ключевое отличие от свойств с обычным getter'ом ({ get; }) в том, что init-свойства можно задавать в инициализаторе через new MyClass { Property = value }, что невозможно для read-only свойств.

Посмотрим на конструктор класса SingletonAttribute. В нём инициализируются два свойства этого класса, и оба имеют модификатор init. Свойство InterfaceType по логике должно иметь значение переменной interfaceType, но получило значение от implType. При объявлении свойство InterfaceType не получает значения. Скорее всего, функционал не дописан, но эта потенциальная ошибка может привести к незапланированному поведению программы в будущем.

Заключение

Отсутствие "вопросов" в коде может привести к плохим последствиям. Анализатор PVS-Studio помог найти участки кода, из-за которых разработчики могут столкнуться с проблемами. Надеюсь, что эта статья поможет им исправить потенциальные ошибки, и тогда качество кода станет лучше.

Это перспективный проект, который может быть полезен для использования ИИ генераторов. Но над кодом нужно немного поработать.

Чтобы проверить свой проект на наличие ошибок, вы можете воспользоваться нашим анализатором PVS-Studio.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Sviridov. No questions? The cost of a missing '?' in your project.