Rechecking SharpDevelop: What's New?

    PVS-Studio tool is constantly being improved. At the same time, the C # code analyzer is developing most dynamically: in 2016, ninety new diagnostic rules were added to it. Well, the best indicator of the quality of the analyzer is the errors it detects. It is always interesting, and also quite useful, to carry out repeated checks of large open projects, comparing the results. Today I will focus on re-checking the SharpDevelop project.

    Introduction


    A previous article about SharpDevelop verification was published by Andrey Karpov in November 2015. At that time, we were only testing the new C # analyzer, preparing to release the first release. However, even then, using the beta version, Andrei managed to detect several interesting errors in the SharpDevelop project. After that, SharpDevelop was “put on a shelf” and was used together with other projects only for internal testing when developing new diagnostic rules. And now, finally, it's time to check SharpDevelop again, but with a more “muscular" version of PVS-Studio 6.12 analyzer .

    For verification, I downloaded the current version of the SharpDevelop source code from the GitHub portal. The project contains about a million lines of C # code. During operation, the analyzer issued 809 warnings. Of these, the first level - 74, the second - 508, the third - 227:


    We will not consider warnings at the Low level: statistically there is a large percentage of false positives. Analysis of warnings at Meduim and High levels (582 warnings) revealed the presence of about 40% of erroneous or extremely suspicious constructs. This amounts to 233 warnings. In other words, the PVS-Studio analyzer on average found 0.23 errors per 1000 lines of code. This indicates the high quality of the SharpDevelop project code. On many other projects, everything looks much more sad.

    As a result of the repeated check, a part of the errors described earlier by Andrei in his article was discovered. But most of the errors found are new. Next I will give the most interesting of them.

    Validation Results


    Copy-Paste Reference Error

    An error that can rightfully take pride of place in the " chamber of weights and measures ". A vivid illustration of the usefulness of using static code analysis and the danger of Copy-Paste.

    PVS-Studio analyzer warning : V3102 Suspicious access to element of 'method.SequencePoints' object by a constant index inside a loop. CodeCoverageMethodTreeNode.cs 52

    public override void ActivateItem()
    {
      if (method != null && method.SequencePoints.Count > 0) {
        CodeCoverageSequencePoint firstSequencePoint =  
          method.SequencePoints[0];
        ....
        for (int i = 1; i < method.SequencePoints.Count; ++i) {
          CodeCoverageSequencePoint sequencePoint = 
            method.SequencePoints[0];  // <=
          ....
        }
        ....
      }
      ....
    }

    At each iteration of the for loop , access to the zero element of the collection is used. I deliberately cited an additional piece of code immediately after the if condition . It is easy to see where the line placed inside the loop was copied from. The variable name firstSequencePoint was replaced by sequencePoint , but they forgot to change the access expression by index. The correct version of this design is:

    public override void ActivateItem()
    {
      if (method != null && method.SequencePoints.Count > 0) {
        CodeCoverageSequencePoint firstSequencePoint =  
          method.SequencePoints[0];
        ....
        for (int i = 1; i < method.SequencePoints.Count; ++i) {
          CodeCoverageSequencePoint sequencePoint = 
            method.SequencePoints[i];
          ....
        }
        ....
      }
      ....
    }

    “Find 10 differences,” or again Copy-Paste

    PVS-Studio analyzer warning : V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless NamespaceTreeNode.cs 87

    public int Compare(SharpTreeNode x, SharpTreeNode y)
    {
      ....
      if (typeNameComparison == 0) {
        if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
          return -1;
        if (x.Text.ToString().Length < y.Text.ToString().Length)  // <=
          return 1;
      }  
      ....
    }

    Both if blocks contain the same condition. In this case, it is difficult to judge how the correct version of the program should look. It is necessary to study the code fragment by its author.

    Untimely equality check null

    PVS-Studio analyzer warning : V3095 The 'position' object was used before it was verified against null. Check lines: 204, 206. Task.cs 204

    public void JumpToPosition()
    {
      if (hasLocation && !position.IsDeleted)  // <=
        ....
      else if (position != null)
        ....
    }

    The variable position is used without checking for equality null . Validation is done in another condition, in the else code block . The correct version of the code could be:

    public void JumpToPosition()
    {
      if (hasLocation && position != null && !position.IsDeleted)
        ....
      else if (position != null)
        ....
    }

    Missing equality test null

    PVS-Studio analyzer warning : V3125 The 'mainAssemblyList' object was used after it was verified against null. Check lines: 304, 291. ClassBrowserPad.cs 304

    void UpdateActiveWorkspace()
    {
      var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
      if ((mainAssemblyList != null) && (activeWorkspace != null)) {
        ....
      }
      ....
      mainAssemblyList.Assemblies.Clear();  // <=
      ....
    }

    The mainAssemblyList variable is used without first checking for null . However, another piece of code contains such a check. The correct version of the code:

    void UpdateActiveWorkspace()
    {
      var mainAssemblyList = SD.ClassBrowser.MainAssemblyList;
      if ((mainAssemblyList != null) && (activeWorkspace != null)) {
        ....
      }
      ....
      if (mainAssemblyList != null) {
        mainAssemblyList.Assemblies.Clear();
      }  
      ....
    }

    Unexpected sorting result.

    PVS-Studio analyzer warning : V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. CodeCoverageMethodElement.cs 124

    void Init()
    {
      ....
      this.SequencePoints.OrderBy(item => item.Line)
                         .OrderBy(item => item.Column);  // <=
    }

    The result of this code fragment will be sorting the SequencePoints collection only by the Column field . Apparently, this is not quite what the author of the code expected. The problem is that calling the OrderBy method again will sort the collection without considering the result of the previous sort. To remedy the situation must be used ThenBy instead recall the OrderBy :

    void Init()
    {
      ....
      this.SequencePoints.OrderBy(item => item.Line)
                         .ThenBy(item => item.Column);
    }

    Potential division by zero

    PVS-Studio analyzer warning : V3064 Potential division by zero. Consider inspecting denominator 'workAmount'. XamlSymbolSearch.cs 60

    public XamlSymbolSearch(IProject project, ISymbol entity)
    {
      ....
      interestingFileNames = new List();
      ....
      foreach (var item in ....)
        interestingFileNames.Add(item.FileName);
      ....
      workAmount = interestingFileNames.Count;
      workAmountInverse = 1.0 / workAmount;  // <=
    }

    If the interestingFileNames collection is empty, division by zero will occur. It’s hard enough to suggest a suitable error correction option. But, in any case, the refinement of the algorithm for calculating the value of the variable workAmountInverse is required in a situation where the variable workAmount is equal to zero.

    Reassignment

    Warning-Studio analyzer of PVS : V3008 of The 'ignoreDialogIdSelectedInTextEditor' variable is Assigned values of twice successively. Perhaps this is a mistake. Check lines: 204, 201. WixDialogDesigner.cs 204

    void OpenDesigner()
    {
      try {
        ignoreDialogIdSelectedInTextEditor = true;  // <=
        WorkbenchWindow.ActiveViewContent = this;
      } finally {
        ignoreDialogIdSelectedInTextEditor = false;  // <=
      }
    }

    The variable ignoreDialogIdSelectedInTextEditor will be false regardless of the result of the try block . To eliminate the likelihood of pitfalls, we’ll review the declaration of the variables used. The ignoreDialogIdSelectedInTextEditor declaration is:

    bool ignoreDialogIdSelectedInTextEditor;

    Declaration of IWorkbenchWindow and ActiveViewContent :

    public IWorkbenchWindow WorkbenchWindow {
      get { return workbenchWindow; }
    }
    IViewContent ActiveViewContent {
      get;
      set;
    }

    As you can see, there are no obvious reasons for re-assigning the variable ignoreDialogIdSelectedInTextEditor to a value. I would venture to suggest that the correct version of the above construction could differ from the original using the catch keyword instead of finally :

    void OpenDesigner()
    {
      try {
        ignoreDialogIdSelectedInTextEditor = true;
        WorkbenchWindow.ActiveViewContent = this;
      } catch {
        ignoreDialogIdSelectedInTextEditor = false;
      }
    }

    Erroneous substring search

    PVS-Studio analyzer warning : V3053 An excessive expression. Examine the substrings '/ debug' and '/ debugport'. NDebugger.cs 287

    public bool IsKernelDebuggerEnabled {
      get {
        ....
        if (systemStartOptions.Contains("/debug") ||
         systemStartOptions.Contains("/crashdebug") ||
         systemStartOptions.Contains("/debugport") ||  // <=
         systemStartOptions.Contains("/baudrate")) {
          return true;
        }
        ....
      }
    }

    The systemStartOptions line sequentially searches for one of the substrings "/ debug" or "/ debugport". The problem is that the string "/ debug" itself is a substring for "/ debugport". Thus, finding the substring "/ debug" makes a further search for the substring "/ debugport" pointless. Perhaps this is not a mistake, but the code can be simplified:

    public bool IsKernelDebuggerEnabled {
      get {
        ....
        if (systemStartOptions.Contains("/debug") ||
         systemStartOptions.Contains("/crashdebug") ||
         systemStartOptions.Contains("/baudrate")) {
          return true;
        }
        ....
      }
    }

    Exception handling error

    PVS-Studio analyzer warning : V3052 The original exception object 'ex' was swallowed. Stack of original exception could be lost. ReferenceFolderNodeCommands.cs 130

    DiscoveryClientProtocol DiscoverWebServices(....)
    {
      try {
        ....
      } catch (WebException ex) {
        if (....) {
          ....
        } else {
          throw ex;  // <=
        }
      }
      ....
    }

    In this case, calling throw ex will overwrite the stack of the original exception, since the caught exception will be thrown again. Corrected version:

    DiscoveryClientProtocol DiscoverWebServices(....)
    {
      try {
        ....
      } catch (WebException ex) {
        if (....) {
          ....
        } else {
          throw;
        }
      }
      ....
    }

    Using an uninitialized field in the constructor of a class

    PVS-Studio analyzer warning : V3128 The 'contentPanel' field is used before it is initialized in constructor. SearchResultsPad.cs 66

    Grid contentPanel;
    public SearchResultsPad()
    {
      ....
      defaultToolbarItems = ToolBarService
        .CreateToolBarItems(contentPanel, ....);  // <=
      ....
      contentPanel = new Grid {....};
      ....
    }

    The contentPanel field is passed as one of the parameters to the CreateToolBarItems method in the constructor of the SearchResultsPad class . At the same time, this field is initialized after use. Perhaps in this case there is no error, since the possibility of equality of null to the variable contentPanel can be taken into account in the body of the CreateToolBarItems method and further along the stack . But the code looks suspicious and requires verification by the author.

    I emphasize once again that in the article I described far from all the errors that the PVS-Studio analyzer found, but only those that seemed interesting to me. The authors of the project can contact us and get a temporary license key in order to perform a more thorough verification of the project.

    Conclusion


    And again PVS-Studio did not fail: during the re-examination of the SharpDevelop project, new interesting errors were found. And this means that the analyzer does its job well, allowing us to make the world around us a little more perfect.

    I remind you that you can join this process at any time, using the opportunity to use the PVS-Studio static analyzer for free to check your own projects.

    Download and try PVS-Studio: http://www.viva64.com/en/pvs-studio/

    For questions regarding the acquisition of a commercial license, please contact us by mail. You can also write to us to get a temporary license for a comprehensive study of PVS-Studio, if you want to remove restrictions demo version.



    If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Rechecking SharpDevelop: Any New Bugs?

    Have you read the article and have a question?
    Often our articles are asked the same questions. We collected the answers here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please see the list.

    Also popular now: