No questions? The cost of a missing '?' in your project

• A few words about the project
  • Preparing for project analysis
  • Errors in constructor
  • No questions
  • Copy-paste errors
  • Init
• Conclusion

Usually, "no questions" at the end of meetings or presentations means that everything went ok. However, in the programming world, it can mean the exact opposite and even be alarming. In development, the absence of "questions" often indicates hidden problems rather than clarity. Let's examine how their absence can impact the project quality.

A few words about the project

Stability Matrix is an open-source project that delivers a user-friendly GUI to manage various AI tools for generating images, animations, and 3D content. It simplifies model management, parameter selection, and image generation, serving as a control panel for the Stable Diffusion model. The project is developed in C# and uses the .NET platform to ensure cross-platform compatibility.

Preparing for project analysis

Let's take a look at a scenario where the PVS-Studio plugin is used in Visual Studio Code on Linux. Since the Stability Matrix project is cross-platform, it runs on Linux. To analyze a C# (MSBuild) project on Linux, install the pvs-studio-dotnet console utility. You can read more about it in the documentation.

After installing pvs-studio-dotnet, install the PVS-Studio plugin in VS Code. In the Extensions section, enter PVS-Studio in the search field and click Install.

After installing the extension, the PVS-Studio tab will appear as one of the tabs on the VS Code bottom panel. To run the analysis, click Start analysis. Well done! We run the analysis of the entire solution.

After the first analysis run, the [workspace folder]/.PVS-Studio/MSBuildAnalyzerConfig.jsonc file will be automatically generated in the project folder. It can be edited to configure the analysis. The configurations are the same as those available in the analyzer console version.

When the analysis finishes, the analyzer will display the results.

Now, it's time to look at warnings that PVS-Studio detected in the project.

Errors in constructor

Issue 1

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

The PVS-Studio warning: V3064 Division inside method by the 2nd argument '0' which has zero value. HuggingFacePageViewModel.cs 226

The analyzer warns that there may be a potential division by zero in the method. Let's look closer at the ProgressReport method.

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;
  ....
}

Two parameters with zero values are passed to the ProgressReport structure constructor. Therefore, the current and total parameters are equal to 0. Developers checked these values, but didn't check whether they were equal to zero. Therefore, the values won't meet the conditions. Then, when evaluating the Progress property, the parameters are divided by each other. Dividing by 0 will result in NaN, since the result of the division is converted to 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>();
}

The PVS-Studio warning: V3005 The 'this.modelIndexService' variable is assigned to itself. InferenceFluxTextToImageViewModel.cs 63

The field value is assigned to itself, which is marked as the readonly property and can be changed only in this code fragment. However, because it's assigned to itself, the field will always benull.

Judging by the code, this was not intentional—it's an oversight. The class constructor doesn't declare the modelIndexService parameter, so the field is assigned the same value. So, it'll store null.

No questions

In this part, we'll show the warnings related to the ? operator. Developers often underestimate its importance and can make mistakes. And the authors of this project were no exception and were caught in this trap. The analyzer detected many issues related to this operator; several of them we described below and grouped together because they're very similar to each other.

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;
  ....
}

The PVS-Studio warning: V3095 The 'viewModel' object was used before it was verified against null. Check lines: 265, 335. CheckpointBrowserCardViewModel.cs 265

In this example, PVS-Studio analyzer issues a warning on the use of the viewModel variable. Below, developers used it with the ? operator to check values for null, protecting the program from crashing. But when they first used the variable, they didn't check its value, which may cause the program crash with a NullReferenceException.

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

The PVS-Studio warning: V3095 The 'CivitFile' object was used before it was verified against null. Check lines: 139, 157. SelectModelVersionViewModel.cs 139

Project developers make the same kind of mistake more than once in their code, and here's the similar case. Compared to the example above, the variable was used unsafely only once, but this could still lead to serious consequences.

Here, project developers called the variable property for the first time without checking its value. However, the check for null was implemented below. Therefore, we caught an NRE for the value?.CivitFile.SizeKb variable.

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

  }
}

The PVS-Studio warning: V3095 The 'RelativePath' object was used before it was verified against null. Check lines: 102, 109. WanModelCardViewModel.cs 102

The scenario repeats itself: developers used the 'RelativePath' variable without checking for null. On the second use, a null check is in place. This shows that developers may know the value could be null, but didn't properly safeguard the code.

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);
}

The PVS-Studio warning: V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'SelectedCategory' object OutputsPageViewModel.cs 242

The analyzer detected a potential NullReferenceException. Once again, the ? operator isn't used for the SelectedCategory.Path variable. Although developers checked this variable the first time, they didn't check it on the next line.

In cases like these, the analyzer can't say for certain whether it's a bug or a feature. Yes, we can say that developers may have been confident that the data would always be complete and non-null. The analyzer role is to highlight potential risks, leaving it to the developers to confirm whether the warning is relevant. Only they can accurately explain what they wanted a specific code block to do and determine whether this behavior is expected or not.

The ? operator plays a key role in ensuring program safety related to null errors. PVS-Studio analyzer detected numerous scenarios where there are no ?. However, from a single analysis run, it's difficult to say for sure whether this program behavior was deliberate or a potentially dangerous oversight. That's why static analysis shouldn't be a one-time event, but an integral part of the entire software lifecycle, from development to maintenance.

Only by integrating analysis into the daily workflow can achieve maximum warning relevance. The developer immediately sees the problem in the fresh code, understands its context, and can instantly decide to, for example, add the ? operator, change the logic, or ignore the warning for good reason. This approach not only allows defects to be fixed promptly, but also prevents them, resulting in cleaner, higher-quality code.

Well, let's move on: there are other curious warnings in the Stability Matrix project, so explore them.

Copy-paste errors

Now we suggest a little game: it's your turn to be the analyzer. Take a look at the code snippet below and try to spot the error.

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);
}

There's a classic copy-paste error where developers copied the upper block, checking the model.SelectedClip3Name variable, although, following the code logic, they should have checked model.SelectedClip4Name.

We think you spotted this typo very fast because you already knew it was hidden somewhere in this fragment. Imagine another case: a developer scrolling down an entire codebase where there are plenty of lines, classes, methods, and variables. It's difficult to find such an error in the whole project, and developers have to spend a long time debugging by checking every variable. This takes a lot of time and effort. To streamline the process, it's best to use the code analyzer, which can easily find this error.

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);
}

The PVS-Studio warning: 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 analyzer warns that test methods have the same bodies. The only difference between them is the name. The second method name should be rewritten, as it currently doesn't test the functionality specified in the name.

Tests are just code, which means they can also contain errors. A bad test is worse than no test at all because it creates a false sense of confidence. Developers often neglect test verification, which can lead to a loss of value and quality in testing, slower development, poor code quality, and higher maintenance costs. As a result, developers have to spend time and money debugging and fixing all the tests before they can continue developing the product. To avoid this, review and validate your tests regularly. This issue was discussed in more detail in this article.

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;
  }
}

The PVS-Studio warning: V3117 Constructor parameter 'interfaceType' is not used. SingletonAttribute.cs 24

Starting with C# 9.0 (more details about C# 9.0 here), the init modifier is available for properties, which defines a setter that is only accessible during object initialization. The property value can only be assigned in the object initializer, in its constructor, or directly at declaration. After initialization, the property becomes immutable (read-only).

The key difference from properties with a regular getter ({ get; }) is that init properties can be set in the initializer via new MyClass { Property = value }, which isn't possible for read-only properties.

Let's take a look at the constructor of the SingletonAttribute class. It initializes two properties of this class, and both have the init modifier. The InterfaceType property should logically have the value of the interfaceType variable, but it received the value from implType. When declared, the InterfaceType property doesn't receive a value. Most likely, the functionality hasn't been fully implemented, but this potential error could lead to unexpected program behavior in the future.

Conclusion

Today, we've seen that the absence of "questions" in the code can lead to unexpected issues. Stability Matrix is a promising project with great potential that can be used for AI generation tools, though the code could use a little refinement.

We hope that PVS-Studio analyzer, together with this article, will help developers fix potential issues and enhance the overall code quality.

To check the project for errors, you can use our PVS-Studio analyzer.