In November 2016, we posted an article about the development and use of the PVS-Studio plugin for SonarQube. We received great feedback from our customers and interested users who requested testing the plugin on a real project. As the interest in this subject is not decreasing, we decided to test the plugin on a C# project PascalABC.NET. Also, it should be borne in mind, that SonarQube have their own static analyzer of C# code - SonarC#. To make the report more complete, we decided to test SonarC# as well. The objective of this work was not the comparison of the analyzers, but the demonstration of the main peculiarities of their interaction with the SonarQube service. Plain comparison of the analyzers would not be fair due to the fact that PVS-Studio is a specialized tool for bug detection and potential vulnerabilities, while SonarQube is a service for the assessment of the code quality by a large number of parameters: code duplication, compliance with the code standards, unit tests coverage, potential bugs in the code, density of comments in the code, technical debt and so on.
Before reading this article, I recommend having a look at other posts, where we give a description of the SonarQube platform and integration of PVS-Studio with it.
Now let us talk about the checked project. PascalABC.NET is the new generation Pascal programming language that combines simplicity of classic Pascal, a great number of modern extensions and broad capabilities of Microsoft .NET Framework. It has its own development environment and also a Web-environment for the creation of programs in PascalABC.NET, C#, Visual Basic.NET, F#, IronPython. The project is Written in C# and is distributed under a free software license LGPLv3. The site of the project. The source code can be downloaded from the repository on GitHub.
PascalABC.NET solution has 2628 files with the extension 'cs', which contain about 752 thousand lines of code (the metrics were obtained using the SourceMonitor utility). Thus, the project has a reasonable size for our research purposes.
As it was stated earlier, the SonarQube service has its own analyzer of C# code. To add an open project to the site and perform the analysis, it was enough to perform several simple actions.
To register on the SonarQube site, I used a GitHub account. Then I used the quick start instructions. The whole process of setting, including binding of the PascalABC.NET project to the account, getting the unique key of the organization and configuration on the local computer took me about 15 minutes. The analysis of the project took another 10 minutes. After that, the result was uploaded to the SonarQube site, where it can be accessed by anyone.
SonarQube issued 3636 warnings about possible errors in PascalABC.NET code:
Among them: 8 blocking (require immediate fix), 64 critical, 1742 important and 1822 not critical. There were no informational messages. Let us have a look at these warnings, find interesting errors and see the percentage of false positives in the analysis. To do it, we use convenient filtration means by various metrics, provided by SonarQube service. Let us start with blocker warnings.
As we see, the blocker warnings are issued for two rules: infinite recursion and disposing IDisposable-resources. Here is an example of a blocker-warning:
In the get-section of the Instance property, the program erroneously returns Instance instead of instance, which causes infinite recursion.
All the other warnings of the Blocker level are also errors.
There were 64 warnings of the Critical level for the rule of inappropriate type casting. Let us have a look at one of such warnings:
Having studied the code and the list of implementations, I agreed with the analyzer: at the moment there really isn't any type that implements both IBaseScope and IComparable interfaces, so the result of the check of boxItem.Item is IComparable will always be false. However, I would not consider it an error in this case, because, firstly, the mere presence of such check eliminates the subsequent throw of an exception when attempting to cast the type (IComparable)boxItem.Item. Secondly, at any moment a programmer may add a reference to dll to the the solution, where we will have the declaration of a type that would implement both interfaces IBaseScope and IComparable. Perhaps, that was the plan of a developer when he was doing the type casting only after the check. The reviewed warning should be put to the Minor warning, not critical for the execution; its presence on the Critical level is most likely a false positive.
The remaining 63 warnings are similar to the one we had a look at.
There were quite a lot of warnings on this level - 1742 for 15 types of diagnostics:
Let's go through the list of warnings to find real bugs and evaluate the real capabilities of the analyzer.
General exceptions should never be thrown
The rule reports that a general type exception is thrown with the throw operator. There were 634 similar constructs found in the code of PascalABC.NET project. The majority is of the following kind:
Also there are a lot (more than 600) constructs that look like "stubs" in the code, that were intentionally written by the developers.
Of course, throwing a general exception is considered as "bad manners". However, it seems to me that this are not errors at all. Moreover, it is unlikely that the code's authors deliberately multiplied them so much. Yes, apparently, the exception handling in PascalABC.NET leaves much to be desired. Nevertheless, the best place for these 634 similar warnings is in the Minor section or among the false positives of the analyzer.
By the way, this is a good example, showing the difference between SonarC# and PVS-Studio. SonarC# indicates "smells" in the code and is quite right, issuing these warnings. They help to judge about the quality of the project. From our point of view, as the developers of PVS-Studio analyzer, these are false positives, because we are focused on finding bugs and security issues.
Dead stores should be removed
This is also quite an extensive group of 618 warnings concerning a repeated variable assigning, when it is not used in any way between those assignments. We see the following pattern prevailing:
The variable is initialized during the declaration, and then, without using the new value, it is assigned with another value. Of course, it's a bad ideas to do so. We may raise questions concerning saving the resources and suspicions about a different error or a typo. But in fact, neither one of these structures is an error. It is unclear again, why are these warnings placed in the section of major severity? To my mind, these are false positives.
There are several warnings that are certainly false-positives:
If we follow the recommendations of the analyzer in this case, then we may ruin the logic of the program.
Thus, I could not find any real bugs among those 618 warnings from the reviewed group.
Floating point numbers should not be tested for equality
151 warnings were issued for the comparison constructions, where one or both operands have a real type. Indeed, such comparisons often give an erroneous result that is related to peculiarities of storing the real variables in memory and may vary, for example, depending on the compiler settings. Such constructions can work for a very long time without any problems. Therefore, a programmer has to decide about the falsity in each particular code fragment. For example, if the values being compared are a result of mathematical calculations, then, the direct comparison of these values is usually wrong. If you are comparing two real constants, then probably it is done deliberately and there is no error.
In the code of PascalABC.NET I saw mostly the following pattern of comparing with a real variable:
Note that there are both comparisons of two real variables and a real variable with an integer type variable. Of course, this code is not completely safe, since it is unknown how the compared values were obtained. Can we speak of a real error here? I find it difficult to give a definite answer. However, the code probably needs to be reviewed.
By the way, PVS-Studio analyzer also warns about such suspicious comparisons, but these diagnostics refer to the Low reliability level and are not recommended for studying.
There are also obvious false positives among the issued warnings:
In this case two variables of byte type get compared: The variables left and right have the type byte_const_node:
public class byte_const_node : concrete_constant<byte>,
SemanticTree.IByteConstantNode
{
public byte_const_node(byte value, location loc)
: base(value, loc)
{
}
....
}
public abstract class concrete_constant<ConstantType> : constant_node
{
private ConstantType _constant_value;
public concrete_constant(ConstantType value, location loc) :
base(compiled_type_node.get_type_node(typeof(ConstantType)), loc)
{
_constant_value = value;
}
....
public ConstantType constant_value
{
get
{
return _constant_value;
}
....
}
....
}
....
}I think this group is reasonably placed to the Major section. However, I would not consider all the warnings to be errors. It is up to the author to decide in each particular case.
Multiline blocks should be enclosed in curly braces
This is a group of 108 warnings, including potential formatting errors that affect the logic of the program execution. I found quite suspicious constructs here. Example:
In this fragment, the brackets are possibly missing. In any case, a developer should format the code for better understanding of the program logic.
Another similar warning:
There is no error, but the code looks sloppy. Refactoring is needed here.
In general, all the warnings from this group are issued correctly, but they haven't detected real errors.
Null pointers should not be dereferenced
75 warnings about possible access by the null reference. In this block I found interesting mistakes:
Indeed, previously the variable returned_scope has always been verified against null before it was used, but in this case, it was forgotten.
public override void visit(....)
{
....
if (returned_scope != null && ....)
{
....
}
else if (returned_scope != null)
{
....
}
returned_scope.declaringUnit = entry_scope; // <=
....
}A similar bug:
In the first case, the variable pi is verified against null before using, but further on, when accessing pi.CompilationUnit it is forgotten.
This block of warnings has some number of not very obvious errors and false positives. I would rate the percentage of finding real bugs here equal to 85%. A very good result.
Conditions should not unconditionally evaluate to "true" or to "false"
This block of warnings is related to the conditions that are true regardless of the program logic. Here is a typical of the errors found:
This code is strange and needs reviewing. Perhaps, we have a serious mistake here.
All in all, the group has about 70% of such errors.
Exceptions should not be thrown from property getters
You should not throw exceptions to get section of the property, and if it is necessary to use methods instead of properties. This group contains 46 such warnings. The vast majority of them are "stubs", left by the developers intentionally or due to forgetfulness:
There are also quite correct constructions that require refactoring.
Nevertheless, I do not believe these warnings to be errors. I think it would be more rational to classify them as Minor bugs.
Static fields should not be updated in constructors
The diagnostic about updating static fields in constructors: this can lead to inconsistent behavior, because the field will be newly initialized for all instances of the class. In sum total, the analyzer issued 26 warnings of this kind for PascalABC.NET project. I have not found real errors among them. Here are a couple examples of the detected code fragments:
Every time a reference to a new instance of a class is written to the static variable _instance. Judging by the variable name, it was an intended action.
The flag parsers_loaded indicates that at least one class item was already created. Nothing criminal.
"=+" should not be used instead of "+="
Quite an interesting diagnostic that instead of the "-=" operator it was erroneously written "=-". The analyzer issued 9 warnings of this kind. Unfortunately, they are all false positives. 6 warnings are issued for constructions that are declarations of variables, where the operator "-=" is not possible at all.
The other 3 warnings are apparently caused by the fact that the authors of the code don't like using spaces for formatting the code:
Related "if/else if" statements should not have the same condition
5 warnings were issued for code fragments with the same condition in the if and else blocks. Often this code is either erroneous already, or it has the possibility of an error. In our case, 4 of 5 warnings had simple duplication of conditions, as well as of an execution block, which of course is suspicious, but not a coarse mistake. One warning is more interesting:
Before the first part of the if block was commented out, it was different from the following condition in the consecutive else if block. Also note the execution block of the second else if: it is empty. There is only one operator: ";". This is very strange and suspicious code.
Short-circuit logic should be used in boolean contexts
The diagnostic warns for example about a possible erroneous use of the & operator instead of && in the expressions of bool type. There were found 5 such suspicious constructions. They all, in one way or another, require attention, although they may not contain errors. Here is an example of one of them:
In this case, we cannot exactly state that using the "|" operator is a mistake, because in the right part of it is a property with complex logic inside which is checked. Perhaps the developer's aim was to make the two conditions to be always checked.
Exceptions should not be explicitly rethrown
The diagnostic about the loss of the exception stack. The analyzer issued 4 identical warnings:
Of course, we should not write like this. It would be hard to debug the application further on. However, all these warnings are not too critical. To my mind, they should go to the Minor section.
Variables should not be self-assigned
3 warning about setting the value of the variable to itself. Here is an example of one of the detected code fragments:
This code is strange and apparently erroneous. The declaration of visitNode is like this:
protected bool visitNode = true;In total, there are two errors in this group of warnings.
Identical expressions should not be used on both sides of a binary operator
The diagnostic searches for conditions, which have identical subexpressions. 2 suspicious constructs were detected. There is no evident error there, but perhaps the code should work and look differently. Here is an example of one of the warnings:
Strange code. Perhaps, a programmer forgot to replace the second check.
"ToString()" method should not return null
This is the last group of warnings in the Major section. The overloading of the ToString() method is implemented incorrectly. 2 warnings are issued and both of them are errors. Here is an example of one of them:
It's incorrect to return null from the overloaded ToString() method. string.Empty should be used instead.
There were 1822 warnings issued here. As this level is not a critical one, it is unlikely that I will find any really interesting bugs. Also, this level usually has a large percentage of false positives. That's why I won't be looking at the Minor level bugs in this article.
To sum up, I should say that in general, the analyzer found real bugs on the Blocker, Critical and Major (I found 268 erroneous or extremely suspicious constructs per 1814 warnings); some of them were of real interest. Nevertheless, the percentage of false positives is still very high and is more than 85%. This greatly complicates the analysis of the results.
A whole documentation section on our website is devoted to the integration of PVS-Studio analyzer report to SonarQube. It took me about 15 minutes to set up the integration "from scratch". Another 15 minutes were spent on the project check and loading the results to the local server of SonarQube.
PVS-Studio issued 1039 warnings for PascalABC.NET code. Among them were: 156 warnings of the Critical level, 541 - Major and 342-Minor.
We are not going to review the Minor level warnings, as the percentage of false positives is usually too high here.
The distribution of warnings on the Critical level:
The distribution of warnings on the Major level:
Having analyzed 697 warnings on the Critical and Major level, I found out that 204 warnings can be called false positives. This is 29% of the total number of warnings on the first and second severity level. Thus, the percentage of detecting real errors and suspicious constructs on the project PascalABC.NET is 71%. In terms of the number of code lines (KLOC), it is 0.66 errors per KLOC. Let us take a look at the most interesting of the detected errors. For convenience, I have sorted the errors by the number of the diagnostics in ascending order.
Copy-Paste
V3001 There are identical sub-expressions 'token.Kind == openBracketToken' to the left and to the right of the '||' operator. ICSharpCode.SharpDevelop NRefactoryInsightWindowHandler.cs 66
readonly int eofToken,
commaToken,
openParensToken,
closeParensToken,
openBracketToken,
closeBracketToken,
openBracesToken,
closeBracesToken,
statementEndToken;
public void InitializeOpenedInsightWindow(....)
{
....
if (token.Kind == openParensToken ||
token.Kind == openBracketToken ||
token.Kind == openBracketToken) { // <=
bracketCount++;
}
....
}In the condition of the if block, the equation token.Kind == openBracketToken is checked twice. You may find a field with a very similar name openBracesToken among the fields declared in the class. Perhaps this field was skipped in the condition. In this case, a correct variant will be like this:
public void InitializeOpenedInsightWindow(....)
{
....
if (token.Kind == openParensToken ||
token.Kind == openBracketToken ||
token.Kind == openBracesToken) {
bracketCount++;
}
....
}Similar errors in the code:
Inadvertence
V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 597, 631. ParserTools SyntaxTreeComparer.cs 597
public void CompareInternal(....)
{
....
if (left is ident)
CompareInternal(left as ident, right as ident);
....
else if (left is int64_const)
CompareInternal(left as int64_const, right as int64_const);
....
else if (left is int64_const)
CompareInternal(left as int64_const, right as int64_const);
....
}The code fragment really contains about 30 similar checks, two of which are completely identical. Perhaps, there is no mistake here, the code was just carelessly copied. But one of the checks, according to the initial plan of the developer, could look different. In this situation we deal with a serious logic error.
Similar errors:
Copy-Paste v2.0
V3004 The 'then' statement is equivalent to the 'else' statement. VisualPascalABCNET CodeCompletionWindow.cs 204
public void HandleMouseWheel(....)
{
....
if (System.Windows.Forms.SystemInformation.MouseWheelScrollLines
> 0) {
newValue = this.vScrollBar.Value -
(control.TextEditorProperties.MouseWheelScrollDown ? 1 : -1) *
multiplier;
} else {
newValue = this.vScrollBar.Value -
(control.TextEditorProperties.MouseWheelScrollDown ? 1 : -1) *
multiplier;
}
....
}Both branches of the if block have identical subexpressions. In this case, it is difficult to draw a conclusion about the correct version of the fragment, but in the current state the code will not work as expected.
Such errors in the code:
I have cited only 10 bugs of this kind out of 20.
A variable is assigned to itself
V3005 The 'miGenerateRealization.Visible' variable is assigned to itself. VisualPascalABCNET OptionsManager.cs 342
public void UpdateUserOptions()
{
....
tsViewIntellisensePanel.Visible = tssmIntellisence.Visible =
tsGotoDefinition.Visible = tsGotoRealization.Visible =
tsFindAllReferences.Visible = miGenerateRealization.Visible =
miGenerateRealization.Visible = cmGenerateRealization.Visible =
cmsCodeCompletion.Visible = cmFindAllReferences.Visible =
cmGotoDefinition.Visible = cmGotoRealization.Visible =
UserOptions.AllowCodeCompletion;
}The variable miGenerateRealization.Visible receives the same value twice during the assignment. Probably, the unnecessary assignment was added by accident. However, instead of one of the miGenerateRealization.Visible variables, there could be some other variable that is now not initialized.
Here is another similar error.
V3005 The 'visitNode' variable is assigned to itself. SyntaxVisitors SimplePrettyPrinterVisitor.cs 106
Repeated assignment
V3008 The 'codeCompileUnit' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 126, 124. VisualPascalABCNET CodeDomHostLoader.cs 126
CodeCompileUnit codeCompileUnit = null;
private DesignSurface Designer;
....
protected override CodeCompileUnit Parse()
{
....
CodeCompileUnit ccu = null;
DesignSurface ds = new DesignSurface();
....
ccu = cg.GetCodeCompileUnit(idh);
....
codeCompileUnit = ccu;
Designer = ds;
codeCompileUnit = ccu; // <=
....
}You can see in the code that there is no logical explanation of the repeated assignment of the same value to the codeCompileUnit variable.
Such errors in the code:
The result of the method is always the same
V3009 It's odd that this method always returns one and the same value of 'false'. NETGenerator NETGenerator.cs 5434
private bool BeginOnForNode(IStatementNode value)
{
//if (value is IForNode) return true;
IStatementsListNode stats = value as IStatementsListNode;
if (stats == null) return false;
if (stats.statements.Length == 0) return false;
//if (stats.statements[0] is IForNode) return true;
return false;
}Probably, this issue was caused by inattention during the refactoring. Previously there were code blocks that returned true. Now they are commented out, and the method, regardless of the result of its work, will return false.
Similar errors in the code:
Inattention
V3010 The return value of function 'OrderBy' is required to be utilized. ICSharpCode.SharpDevelop RefactoringService.cs 86
static IEnumerable<ITreeNode<IClass>> FindDerivedClassesTree(....)
{
....
var result = new List<TreeNode<IClass>>();
....
result.OrderBy(node => node.Content.FullyQualifiedName); // <=
return result;
}The result of sorting the result list is not stored anywhere. The corrected version of the fragment given above:
static IEnumerable<ITreeNode<IClass>> FindDerivedClassesTree(....)
{
....
var result = new List<TreeNode<IClass>>();
....
return result.OrderBy(node => node.Content.FullyQualifiedName);
}One more similar bug:
V3010 The return value of function 'ToString' is required to be utilized. CodeCompletion SymTable.cs 2145
A logic problem
V3018 Consider inspecting the application's logic. It's possible that 'else' keyword is missing. VisualPascalABCNET InsightWindow.cs 145
public void HandleMouseWheel(MouseEventArgs e)
{
....
if (e.Delta > 0) {
if (control.TextEditorProperties.MouseWheelScrollDown) {
CurrentData = (CurrentData + 1) % DataProvider.InsightDataCount;
} else {
CurrentData = (CurrentData + DataProvider.InsightDataCount - 1)
% DataProvider.InsightDataCount;
}
} if (e.Delta < 0) { // <=
if (control.TextEditorProperties.MouseWheelScrollDown) {
CurrentData = (CurrentData + DataProvider.InsightDataCount
- 1) % DataProvider.InsightDataCount;
} else {
CurrentData = (CurrentData + 1) %
DataProvider.InsightDataCount;
}
}
....
}Pay attention to the condition f (e.Delta < 0). Looking at the formatting of the code and the program logic, we may say that perhaps the keyword else is missing here. However, only the author can give an exact answer about the specifics of this construction.
A classic error when working with the "as" operator
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'baseScope', 'this.baseScope'. CodeCompletion SymTable.cs 3497
public TypeScope(...., SymScope baseScope)
{
....
this.baseScope = baseScope as TypeScope;
....
if (baseScope == null)
{
....
}
....
}After casting the baseScope argument to the TypeScope, by mistake this argument gets verified against null, not the field this.baseScope. Correct variant of the code:
public TypeScope(...., SymScope baseScope)
{
....
this.baseScope = baseScope as TypeScope;
....
if (this.baseScope == null)
{
....
}
....
}Similar errors in the code:
Sloppy code
V3022 Expression 't == null' is always true. VisualPascalABCNET Debugger.cs 141
public static Type GetTypeForStatic(string name)
{
Type t = stand_types[name] as Type;
if (t != null) return t;
if (t == null) // <=
foreach (string s in ns_ht.Keys)
{
....
}
t = PascalABCCompiler.NetHelper.NetHelper.FindType(name);
....
}There is no error here, but the program looks very uncareful.
Similar constructions in the code:
I've given only the first 10 warnings out of the list of 45.
Redundant check or an error?
V3030 Recurring check. The 'upperScopeWhereVarsAreCaptured != scope' condition was already verified in line 383. TreeConverter CapturedVariablesSubstitutionClassGenerator.cs 391
private void VisitCapturedVar(....)
{
....
if (upperScopeWhereVarsAreCaptured != scope)
{
....
if (upperScopeWhereVarsAreCaptured != scope)
{
....
}
....
}
....
}Typically, such constructions are not error, but there is a chance that one of the checks was to contain a different condition.
Similar errors in the code:
Strange formatting
V3033 It is possible that this 'else' branch must apply to the previous 'if' statement. TreeConverter syntax_tree_visitor.cs 14894
public override void visit(....)
{
....
if (_var_def_statement.inital_value != null)
if (is_event) AddError(....);
else
{
....
}
....
}According to the logic of the program, the else keyword refers to the if (is_event) condition block. However, the code is formatted in such a way that creates quite a different impression. Perhaps, another pair of {} brackets would solve this problem.
A typo
V3038 The 'enum_consts[i]' argument was passed to 'Compare' method several times. It is possible that other argument should be passed instead. CodeCompletion SymTable.cs 2206
private List<string> enum_consts = new List<string>();
public override bool IsEqual(SymScope ts)
{
EnumScope es = ts as EnumScope;
if (es == null) return false;
if (enum_consts.Count != es.enum_consts.Count) return false;
for (int i = 0; i < es.enum_consts.Count; i++)
if (string.Compare(enum_consts[i],
this.enum_consts[i], true) != 0) // <=
return false;
return true;
}Unfortunately, the IsEqual method doesn't have the declaration of the local variable enum_consts. That's why the elements of the enum_consts inside the for loop are compared with themselves. Judging by the way IsEqual method looks, we can make an assumption about the correct variant of the code:
public override bool IsEqual(SymScope ts)
{
....
for (int i = 0; i < es.enum_consts.Count; i++)
if (string.Compare(enum_consts[i],
es.enum_consts[i], true) != 0)
....
}The problem with the logic v2.0
V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. VBNETParser LanguageInformation.cs 1002
public override string FindExpression(....)
{
....
switch (ch)
{
....
case '(':
if (kav.Count == 0)
{
....
}
else sb.Insert(0, ch); punkt_sym = true;
break;
}
....
}The assignment punkt_sym = true will be executed regardless of the result of the check kav.Count == 0. However, the code formatted in such a way that we have an impression that this will be done only upon the condition kav.Count != 0.
Another similar error:
V3043 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. ICSharpCode.SharpDevelop AbstractConsolePad.cs 159
A loss of an exception stack
V3052 The original exception object 'e' was swallowed. Stack of original exception could be lost. NETGenerator NETGenerator.cs 925
public void ConvertFromTree(....)
{
....
try
{
....
}
catch (System.Runtime.InteropServices.COMException e)
{
throw new TreeConverter.SaveAssemblyError(e.Message);
}
....
}From the object of the thrown exception of the COMException type, the developer uses only the text of the message. Apparently, this is a deliberate action, because further on, an exception of the SaveAssemblyError type is thrown, whose constructor doesn't require anything besides the text of the message:
public class SaveAssemblyError : CompilationError
{
....
public SaveAssemblyError(string text)
{
_text = text;
}
....
}Of course, it is a right of the author to implement in such a way. However, the exception handling in this case doesn't look complete.
Similar errors in the code:
Substring handling error
V3053 An excessive expression. Examine the substrings 'reduction' and 'reduction('. TreeConverter OpenMP.cs 267
private void ProcessClauses(string Text, ....)
{
....
if (....)
{
....
}
else if (AllowReduction &&
(Text.StartsWith("reduction") ||
Text.StartsWith("reduction(")))
{
....
}
....
}In this case the search of the "reduction(" substring is meaningless, because earlier there will always be a "reduction" substring found.
Erroneous initialization order
V3070 Uninitialized variable 'event_add_method_prefix' is used when initializing the 'event_add_method_nameformat' variable. TreeConverter compiler_string_consts.cs 313
public static class compiler_string_consts
{
....
public static string event_add_method_nameformat =
event_add_method_prefix + "{0}";
....
public static string event_add_method_prefix = "add_";
....
}The string event_add_method_nameformat will get "{0}" value instead of the expected "add_{0}" in the result of the code fragment execution. To fix this, we should switch places of the field initialization strings:
public static class compiler_string_consts
{
....
public static string event_add_method_prefix = "add_";
....
public static string event_add_method_nameformat =
event_add_method_prefix + "{0}";
....
}Another similar error:
V3070 Uninitialized variable 'event_remove_method_prefix' is used when initializing the 'event_remove_method_nameformat' variable. TreeConverter compiler_string_consts.cs 314
Access by null reference: sloppy refactoring
V3080 Possible null dereference. Consider inspecting 'tc'. CodeCompletion CodeCompletionPCUReader.cs 736
private TypeScope GetTemplateInstance()
{
TypeScope tc = null;//GetTemplateClassReference();
int params_count = br.ReadInt32();
for (int i = 0; i < params_count; i++)
{
tc.AddGenericInstanciation(GetTypeReference()); // <=
}
return tc;
}As we see, previously, the variable tc is initialized with the value GetTemplateClassReference(). However, now it is null. As a result, on the first iteration of the for loop we will get the error of the access by the null reference. Perhaps, the error hasn't revealed itself yet, as the calls of the method GetTemplateInstance() are absent in the code. There is no guarantee, that in the future it will be the same.
Similar errors in the code:
Access by the null reference: inattentiveness
V3095 The 'VisualEnvironmentCompiler.RemoteCompiler' object was used before it was verified against null. Check lines: 52, 54. CompilerController CompilerControllerPlugin.cs 52
public CompilerController_VisualPascalABCPlugin(....)
{
....
VisualEnvironmentCompiler.RemoteCompiler.InternalDebug.RunOnMono =
CompilerInformation.cbRunMono.Checked;
....
if (VisualEnvironmentCompiler.RemoteCompiler != null)
....
}The verification of the variable against null is done after it was used. Correct variant of the code:
public CompilerController_VisualPascalABCPlugin(....)
{
....
if (VisualEnvironmentCompiler.RemoteCompiler != null)
{
VisualEnvironmentCompiler.RemoteCompiler.
InternalDebug.RunOnMono =
CompilerInformation.cbRunMono.Checked;
....
}
}Similar errors in the code:
I have cited here the first 10 similar errors out of more than 40.
Infinite recursion: x2
V3110 Possible infinite recursion inside 'SetRange' method. TreeConverter SymbolInfoArrayList.cs 439
V3110 Possible infinite recursion inside 'SetRange' method. TreeConverter SymbolInfoArrayList.cs 444
public void SetRange(int index,SymbolInfo[] tnarr)
{
SetRange(index,tnarr);
}
public void SetRange(int index,SymbolInfoArrayList tnarl)
{
SetRange(index,tnarl);
}Here are two methods at once that implement an infinite recursion. Both methods are similar and differ only by the type of the second argument. They aren't used anywhere in the code. At least, they aren't used yet.
Similar errors in the code:
Careless initialization of the Equals method
V3115 Passing 'null' to 'Equals' method should not result in 'NullReferenceException'. ICSharpCode.SharpDevelop ServiceReferenceMapFile.cs 31
public override bool Equals(object obj)
{
var rhs = obj as ServiceReferenceMapFile;
return FileName == rhs.FileName; // <=
}The author of this code fragment was rather careless about the security issues in his work. At least one check against null of the rhs variable is missing after its initialization. To avoid doing extra work, it's better to do a preliminary check of the obj variable against null:
public override bool Equals(object obj)
{
if (obj == null || !(obj is ServiceReferenceMapFile))
return false;
var rhs = obj as ServiceReferenceMapFile;
return FileName == rhs.FileName;
}Insufficient number of checks
V3125 The 'resources' object was used after it was verified against null. Check lines: 215, 211. VisualPascalABCNET DesignerResourceService.cs 215
public System.Resources.IResourceReader
GetResourceReader(System.Globalization.CultureInfo info)
{
....
if (resources != null && resources.ContainsKey(info.Name)) {
resourceStorage = resources[info.Name];
} else {
resourceStorage = new ResourceStorage();
resources[info.Name] = resourceStorage; // <=
}
....
}The variable resources is verified against null, but it's not enough, because the else block doesn't have such a check. In certain circumstances, this will inevitably lead to access by null reference. The code should be corrected:
public System.Resources.IResourceReader
GetResourceReader(System.Globalization.CultureInfo info)
{
....
if (resources != null) {
if (resources.ContainsKey(info.Name)) {
resourceStorage = resources[info.Name];
} else {
resourceStorage = new ResourceStorage();
resources[info.Name] = resourceStorage;
}
}
....
}Similar errors in the code:
I have provided only 10 similar errors out of more than 80 (eighty!).
Erroneous initialization order
V3128 The 'dockPanel' field is used before it is initialized in constructor. ICSharpCode.SharpDevelop SearchResultsPad.cs 49
....
DockPanel dockPanel;
....
public SearchResultsPad()
{
....
defaultToolbarItems = ToolBarService.
CreateToolBarItems(dockPanel, ....); // <=
foreach (object toolBarItem in defaultToolbarItems) {
toolBar.Items.Add(toolBarItem);
}
....
dockPanel = new DockPanel {
Children = { toolBar, contentPlaceholder }
};
....
}The field dockPanel is firstly used in the constructor SearchResultsPad, and then it is initialized. Even if in the method CreateToolBarItems or in the nested methods the first argument the equality to null is presupposed, the method will most probably return null. This will lead to more errors when using the variable defaultToolbarItems.
I see the overall picture as follows. The analyzers SonarC# and PVS-Studio solve different tasks. SonarC# is designed to assess and monitor the quality of the code. Therefore, it warns about the code "smells" and errors. PVS-Studio is focused on finding bugs or code fragments that may later lead to errors. Of course, there are some messages of these analyzers that are very similar, but they are designed for different needs:
Here is a summary table of PascalABC.NET analysis results (warnings of Blocker, Critical and Major levels):
I would like to note once again that the analyzers cannot be directly compared by the number of the found bugs and the false positives. SonarC# tries issuing warnings for the code that may be poorly written, but does not contain an error. This helps to evaluate the quality of the code. PVS-Studio, in its turn, prefers to keep silent or issue a warning with Low level of reliability. At the same time, it tries to detect as many errors as possible and is taught to find a large amount of defects, leading to program failures.
So, as it was expected, I didn't have any problems working with PVS-Studio and SonarC# for SonarQube. All functions and features of the tools are documented. Once you upload the results to the SonarQube server, you get the access to numerous functional abilities to assess the quality of your software product. As for the bug search by the source code analyzers, both tools showed decent results.
It'll take minimum effort and time to upload and analyze the project online on the SonarQube site.
Using PVS-Studio plugin for the integration of the results of its work in SonarQube isn't hard at all as well. The only limitation - you'll need an Enterprise version of the analyzer. If there is no need to integrate with SonarQube, you can use PVS-Studio as a separate tool.
Download and try PVS-Studio: https://pvs-studio.com/en/pvs-studio/try-free/
To purchase a commercial license, please contact us via the email. You can also write to us to get a temporary license key for a comprehensive investigation of PVS-Studio, if you want to avoid the limitations of the demo version.