Recheck SharpDevelop: What's New?

The PVS-Studio tool is constantly being improved. At the same time, the C # code analyzer is currently 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 re-check large open source projects, comparing the results. Today I will focus on re-checking the project SharpDevelop.

Introduction

The previous article about checking SharpDevelop was published by Andrey Karpov in November 2015. At that time, we were just testing a new C # analyzer, preparing for the release of the first release. However, even then, with the help of the beta version, Andrew was able to detect some interesting errors in the SharpDevelop project. After that, SharpDevelop was “placed on the shelf” and used together with other projects only for internal testing when developing new diagnostic rules. And now, finally, the time has come to check SharpDevelop again, but with a more “muscular” version of the PVS-Studio 6.12 analyzer.

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


We will not consider warnings at the Low level: there is a statistically large percentage of false positives. The analysis of warnings at the Meduim and High levels (582 warnings) revealed about 40% of erroneous or highly suspicious structures. This amounts to 233 warnings. In other words, the PVS-Studio analyzer found an average of 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 Andrey in his article was discovered. But most of the errors found are new. Further I will give the most interesting of them.

Test results

Reference Copy-Paste Error

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

PVS-Studio analyzer warning : V3102 Suspicious access to the element of the '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 gave an extra piece of code immediately after the if condition . It is easy to see where the string was placed from inside the loop. The variable name firstSequencePoint was replaced with a sequencePoint , but the access expression by index was forgotten. 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 the 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 statement is a senseless second 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.

Late equality test for null

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

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

The position variable is used without checking for null equality. The check is performed in another condition, in an else code block. The correct code could be:

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

Missing null equality test

PVS-Studio analyzer warning : V3125 The 'mainAssemblyList' object has been 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 equality. At the same time, another code fragment contains such a check. The correct code is:

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

Unexpected sort result

PVS-Studio analyzer warning : V3078 Original sorting order will be after the 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 the sorting of the SequencePoints collection only by the Column field. Apparently, this is not exactly 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, use ThenBy instead of calling OrderBy again:

 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<FileName>(); .... foreach (var item in ....) interestingFileNames.Add(item.FileName); .... workAmount = interestingFileNames.Count; workAmountInverse = 1.0 / workAmount; // <= } 

If the interestingFileNames collection is empty, a division by zero occurs. It is quite difficult to suggest a suitable way to correct the error. But, in any case, it is necessary to refine the algorithm for calculating the value of the variable workAmountInverse in a situation where the variable workAmount will be equal to zero.

Reassignment

PVS-Studio analyzer warning : V3008 The 'ignoreDialogIdSelectedInTextEditor' variable is assigned values ​​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 return false regardless of the result of the try block. To exclude the possibility of the presence of "pitfalls" let's review the declaration of the variables used. The declaration ignoreDialogIdSelectedInTextEditor has the form:

 bool ignoreDialogIdSelectedInTextEditor; 

IWorkbenchWindow and ActiveViewContent declaration :

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

As you can see, there are no obvious reasons for reassigning 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 search for a substring

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

In the systemStartOptions line, one of the substrings "/ debug" or "/ debugport" is sequentially searched. The problem is that the string "/ debug" itself is a substring for "/ debugport". Thus, finding the substring "/ debug" makes further search for the substring "/ debugport" meaningless. This is probably 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, a call to throw ex will “rub up” the stack of the original exception, since the intercepted exception will be re-generated. Corrected version:

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

Using an uninitialized field in class constructor

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. In this case, this field is initialized after use. Perhaps, in this case there is no error, since the body of the method CreateToolBarItems and further along the stack can take into account the possibility of the null equality of the variable contentPanel . But the code looks suspicious and requires verification by the author.

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

Conclusion

Again, PVS-Studio did not disappoint: during the re-check of the SharpDevelop project, new interesting errors were found. And this means that the analyzer does an excellent job with its work, allowing us to make the world around us a little more perfect.

I remind you that you can join in 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 about the acquisition of a commercial license, please contact us in the mail. You can also email us to get a temporary license for a comprehensive study of PVS-Studio if you want to remove the restrictions of the 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?