Any bugs in your IDE? Checking AvalonStudio with PVS-Studio

Developers around the world use their favorite IDEs every day to create software. Today we are checking one of them and looking at the most interesting errors found.

Introduction

The modern developer uses many tools to succeed. These include IDEs (Integrated development environment). They contain a whole set of tools to make life easier. The modern development environment includes a text editor, compiler or interpreter, debugger, and so on.

In this article I am checking one of open-source IDEs. I am using the PVS-Studio static code analyzer to check the project. This analysis solution can search for bugs and potential vulnerabilities in C, C++, C#, and Java code.

Now let me introduce the project we are going to analyze.

AvalonStudio is a cross-platform IDE written in C#. This environment is primarily focused on development for Embedded C, C++, .NET Core, Avalonia, and TypeScript. AvalonStudio is based on Avalonia UI which we checked earlier.

It wasn't hard to check the source code of AvalonStudio, as it is available on GitHub. I have picked the most interesting errors the analyzer found :). Enjoy reading!

Checking

In this article, each issue is presented with an analyzer's warning, a code fragment, a description, and a dash of the author's opinion. If there are any points in the article that you disagree with, I would love to discuss it in the comments.

Issue 1

private async void Timer_Tick(object sender, EventArgs e)
{
  ....  
  if (AssociatedObject.IsPointerOver)
  {
    var mouseDevice = (popup.GetVisualRoot() as IInputRoot)?.MouseDevice;
    lastPoint = mouseDevice.GetPosition(AssociatedObject);
    popup.Open();
  }
}

V3105 The 'mouseDevice' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. TooltipBehavior.cs 173

The analyzer noticed something suspicious in the conversion through the as operator. The null-conditional operator checks the result for null. This means that the developer assumed that conversion via as could return null. The developer protected the code with operator '?.' from NullReferenceException only once, but further an exception can be thrown.

Issue 2

The next warning does not indicate an obvious error, but the code fragment looks suspicious nevertheless:

private static Signature BuildSignature(IMethodSymbol symbol)
{
  ....
  var returnTypeInfo = CheckForStaticExtension.GetReturnType(symbol);
  if(returnTypeInfo.HasValue)
  {
    if(returnTypeInfo.Value.inbuilt)
    {
      signature.BuiltInReturnType = returnTypeInfo.Value.name;
    }
    else
    {
      signature.ReturnType = returnTypeInfo.Value.name;
    }
  }
  signature.Parameters = symbol.Parameters.Select(parameter =>
  {
    var info = CheckForStaticExtension.GetReturnType(parameter);
    ....
    if(info.HasValue)
    {
      if(info.Value.inbuilt)
      {
        result.BuiltInType = info.Value.name;   // <=
      }
      else
      {
        result.BuiltInType = info.Value.name;   // <=
      }
    }
    ....
  }).ToList();
  ....
}

V3004 The 'then' statement is equivalent to the 'else' statement. InvocationContext.cs 400

PVS-Studio detected that then and else branches of the if statement are equivalent. At the same time, if we go a little higher up in the code, we notice that there is a similar fragment, but the branches are different. It is possible that one of the branches is unnecessary. By the way, it is possible that there must be a different property in the place of BuiltInType. For example, a similar Type property that does exist. The developer could have been using auto-completion and did not notice that the auto-completed code was wrong. By the way, VS2022 suggested me the wrong prompt, too. As the saying goes: "Trust but verify."

Issue 3

I formatted the following code because it was unstructured and hard to read, but I left the logic unchanged.

This is what the code looks like in the editor. If the code is formatted a bit better, the error becomes much more obvious.

private bool CursorIsValidDeclaration(ClangCursor c)
{
  var result = false;

  if (  (c.Kind == NClang.CursorKind.FunctionDeclaration)  // <=
      || c.Kind == NClang.CursorKind.CXXMethod 
      || c.Kind == NClang.CursorKind.Constructor 
      || c.Kind == NClang.CursorKind.Destructor 
      || c.Kind == NClang.CursorKind.FunctionDeclaration)  // <=
  {
    result = true;
  }
  return result;
}

V3001 There are identical sub-expressions 'c.Kind == NClang.CursorKind.FunctionDeclaration' to the left and to the right of the '||' operator. CPlusPlusLanguageService.cs 1275

Such errors are made due to inattentiveness. One of the comparisons may be unnecessary. Or it might be that, instead of one of the FunctionDeclaration items, there should be something else :).

Issue 4

public override void Render(DrawingContext context)
{
  ....
  foreach (var breakPoint in _manager?.OfType<Breakpoint>().Where(....))
  {
    ....
  }
  ....
}

V3153 Enumerating the result of null-conditional access operator can lead to NullReferenceException. BreakPointMargin.cs 46

The analyzer detected that the result of the '?.' operator is dereferenced immediately. This null check inside foreach makes no sense. NullReferenceException still catches up with the developer when the execution flow reaches a null reference :).

In general, this behavior is not obvious to many programmers. We even wrote an entire article on this topic: "Using the ?. operator in foreach: protection against NullReferenceException that doesn't work".

Issue 5

public async Task<(....)> LoadProject(....)
{
  ....
  return await Task.Run(() =>
  {
    ....
    if (   loadData.CscCommandLine != null                            // <=
        && loadData.CscCommandLine.Count > 0)
    {
      ....
      return (projectInfo, projectReferences, loadData.TargetPath);
    }
    else
    {
      ....
      return (projectInfo, projectReferences, loadData?.TargetPath);  // <=
    }
  });
}

V3095 The 'loadData' object was used before it was verified against null. Check lines: 233, 262. MSBuildHost.cs 233

This error seems pretty straightforward. If the loadData variable potentially equals null in the else branch, this variable can also be null in the same if condition. Except that there is no loadData check for null-conditional, which means an exception can be thrown. And yes, loadData does not change in else in any way. Most likely the developer missed the '?.' operator in the if statement. Although, it is possible that this operator in return is unnecessary and should be removed in order not to confuse the developers.

Issue 6

public override void Render(DrawingContext context)
{
  if (_lastFrame != null && !(   _lastFrame.Width == 0 
                              || _lastFrame.Width == 0))
  {
    ....
  }
  base.Render(context);
}

V3001 There are identical sub-expressions '_lastFrame.Width == 0' to the left and to the right of the '||' operator. RemoteWidget.cs 308

The analyzer found two identical sub-expressions to the left and right of the '||' operator. I think that fixing such a mistake is pretty easy: you need to change one of the Width properties to Height. This property exists and is used further in the method.

Issue 7

public GlobalRunSpec(....)
{
  ....
  if (specialEntry.Value != null)
  {
    ....
  }
  RunSpec spec = new(specialOps,
                     specialVariables ?? variables,
                     specialEntry.Value.VariableSetup.FallbackFormat);
  ....
}

V3125 The 'specialEntry.Value' object was used after it was verified against null. Check lines: 92, 86. GlobalRunSpec.cs 92

PVS-Studio has detected something suspicious. The specialEntry.Value is checked for null, and then is used without the appropriate verification. Inside the if statement, specialEntry.Value does not change its value.

Issue 8

private static StyledText InfoTextFromCursor(ClangCursor cursor)
{
  ....
  if (cursor.ResultType != null)
  {
    result.Append(cursor.ResultType.Spelling + " ",
                  IsBuiltInType(cursor.ResultType) ? theme.Keyword 
                                                   : theme.Type);
  }
  else if (cursor.CursorType != null)
  {
    switch (kind)
    {
      ....
    }
    result.Append(cursor.CursorType.Spelling + " ",
                  IsBuiltInType(cursor.ResultType) ? theme.Keyword   // <=
                                                   : theme.Type);
  }
  ....
}

V3022 Expression 'IsBuiltInType(cursor.ResultType)' is always false. CPlusPlusLanguageService.cs 1105

The analyzer expects IsBuiltInType(cursor.ResultType) to always return false. If the IsBuiltInType *method is called in the else branch, *cursor.ResultType *has the *null value. The IsBuiltInType method contains a check that makes sure that if the parameter is null, the method returns false.

private static bool IsBuiltInType(ClangType cursor)
{
  var result = false;
  if (cursor != null && ....)
  {
    return true;
  }
  return result;
}

It turns out that IsBuiltInType(cursor.ResultType) always returns false.

Hmm, why did this happen? There is a clear copy-paste problem here. The developer simply copied a similar code fragment from above — but forgot to change cursor.ResultType to cursor.CursorType.

Issue 9

private int BuildCompletionsForMarkupExtension(....)
{
  ....
  if (t.SupportCtorArgument == MetadataTypeCtorArgument.HintValues)
  {
    ....
  }
  else if (attribName.Contains("."))
  {
    if (t.SupportCtorArgument != MetadataTypeCtorArgument.Type)
    {
      ....
      if (   mType != null 
          && t.SupportCtorArgument ==
             MetadataTypeCtorArgument.HintValues)        // <=
      {
        var hints = FilterHintValues(....);
        completions.AddRange(hints.Select(.....));
      }
      ....
    }  
  }
}

V3022 Expression 'mType != null && t.SupportCtorArgument == MetadataTypeCtorArgument.HintValues' is always false. CompletionEngine.cs 601

The SupportCtorArgument property is compared to the enumeration value. This expression is in the else branch of the if statement. SupportCtorArgument is a normal auto-property. If the property was equal to MetadataTypeCtorArgument.HintValues, the code execution would not get to this else branch. Accordingly, the expression in the condition will always be false, and the code that is in the then branch will never be executed.

Issue 10

private void RegisterLanguageService(ISourceFile sourceFile)
{
  ....
  var languageServiceProvider = IoC.Get<IStudio>()
                                   .LanguageServiceProviders
                                   .FirstOrDefault(....)?.Value;

  LanguageService = languageServiceProvider.CreateLanguageService();  // <=

  if (LanguageService != null)
  {
    ....
  }
}

V3105 The 'languageServiceProvider' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. CodeEditorViewModel.cs 309

The analyzer indicates that a variable whose value was obtained by a null-conditional operator is being dereferenced.

And as we can see, the '?.' operator is really used to calculate the languageServiceProvider variable value. On the next line, this variable is dereferenced without being checked first. Such code can generate a NullReferenceException *exception. One way to fix the code is to use '?.' when calling *CreateLanguageService.

Issue 11

public void SetCursorPosition(int column, int row)
{
  ....
  if (LeftAndRightMarginEnabled)
  {
    if (CursorState.OriginMode && CursorState.CurrentColumn < LeftMargin)
      CursorState.CurrentColumn = LeftMargin;
    if (CursorState.CurrentColumn >= RightMargin)
      RightMargin = RightMargin;                   // <=
  }
  ....
}

V3005 The 'RightMargin' variable is assigned to itself. VirtualTerminalController.cs 1446

The variable RightMargin is set to itself. The code is certainly suspicious. It's difficult to guess what the developer intended to do here and what RightMargin should be set to.

Issue 12

The next triggering is intriguing and somewhat obscure.

public class ColorScheme
{
  private static List<ColorScheme> s_colorSchemes =
    new List<ColorScheme>();
  private static Dictionary<string, ColorScheme> s_colorSchemeIDs =
    new Dictionary<string, ColorScheme>();
  private static readonly ColorScheme DefaultColorScheme = 
    ColorScheme.SolarizedLight;
  ....
  public static readonly ColorScheme SolarizedLight = new ColorScheme
  {
    ....
  };
}

Try to spot an error among these lines. The task is simplified as much as possible, because behind "...." I have hidden almost 200 lines of code, which are not necessary to detect the error. Found anything?

Let's delete some more code and add the analyzer message.

public class ColorScheme
{
  private static readonly ColorScheme DefaultColorScheme = 
    ColorScheme.SolarizedLight;
  ....
  public static readonly ColorScheme SolarizedLight = new ColorScheme
  {
    ....
  };
}

V3070 Uninitialized variable 'SolarizedLight' is used when initializing the 'DefaultColorScheme' variable. ColorScheme.cs 32

I think the analyzer's message makes everything clear. The point is that the DefaultColorScheme field will be initialized with a different value than what the developer expected. At the time DefaultColorScheme is initialized, the SolarizedLight field will have a default value of null. It turns out that the DefaultColorScheme field will also be set to a null value. You can find more examples in the documentation of this diagnostic. The link is above, in the analyzer's message.

Conclusion

The PVS-Studio analyzer found several interesting errors in AvalonStudio. I hope that this article will help improve this project. It is worth mentioning that open-source IDEs are quite scarce these days. I hope there'll be more of them in the future — so that every developer can choose a development environment to their liking. They could even participate in the development of the tool they use.

You can also improve your project in an easy and free way. All you need to do is to check your code with PVS-Studio. And the trial key, which you can get on our website, will help you do it :).