Geekly Articles each Day
📜 ⬆️ ⬇️

Checking the PascalABC.NET project with the help of SonarQube plugins: SonarC # and PVS-Studio

Picture 30

Last November, our blog published an article on the development and use of the PVS-Studio plugin for SonarQube. We received a lot of feedback from customers and simply interested users with requests to test the plug-in on a real project. Since the interest in this issue has not waned, it was decided to test on C # project PascalABC.NET. Also, let's not forget that SonarQube contains its own static C # code analyzer - SonarC #. To complete the picture we will conduct a study and SonarC #. The goal of this work is not to compare analyzers, but to show the main features of their interaction with the SonarQube service. A direct comparison of the analyzers would not be entirely correct, because PVS-Studio is a specialized tool for finding errors and potential vulnerabilities, while SonarQube is a service for assessing the quality of a code by a large number of parameters: code duplication, compliance with coding standards, code coverage unit tests, possible errors in the code, the density of comments in the code, technical debt, etc.

Introduction


I recommend beforehand to get acquainted with the materials of the article , where we talk about the SonarQube platform and the integration of the PVS-Studio analyzer with it.

Now a little about the project under study. PascalABC.NET is an implementation of the Pascal programming language of a new generation, containing its own development environment, as well as a Web environment for creating programs in PascalABC.NET, C #, Visual Basic.NET, F #, IronPython. The project is developed in C # and distributed under the free license LGPLv3. Project site . Source code can be downloaded from the repository on GitHub.

The PascalABC.NET solution includes 2628 files with the extension '.cs', which contain about 752 thousand lines of code (metrics are obtained using the SourceMonitor utility). Thus, the project is quite suitable size for our research purposes.
')

SonarC #


As mentioned earlier, the SonarQube service includes, among other things, a static analyzer of C # code. To, as in our case, add an open project to the site, as well as analyze it, a few simple steps are enough.

I used a GitHub account to register on the SonarQube website. Then I used the quick start guide . The whole configuration process, including linking the PascalABC.NET project to the account, obtaining a unique organization key and setting up on the local computer, took me about 15 minutes. Another 10 minutes was spent on the analysis of the project. After that, the result was uploaded to the SonarQube website, anyone can read it.

SonarQube issued 3636 warnings about possible errors in PascalABC.NET code:

Picture 1


Of these: 8 blockers (require immediate elimination), 64 critical, 1742 important and 1822 not critical. Informational messages were not issued. Let's try to get acquainted with the received warnings, find interesting errors and understand what percentage of false positives. To do this, we use convenient filtering tools in various dimensions provided by the SonarQube service. Let's start with blocking warnings.

Blocker


Picture 32


As you can see, blocking warnings are issued for two rules: endless recursion and clearing of IDisposable-resources. Here is an example of one of the blocker warnings:

Picture 5


In the get-section of the Instance property mistakenly returns Instance , instead of an instance , which causes infinite recursion.

All other warnings at the Blocker level are also errors.

Critical


Picture 33


At the Critical level, 64 warnings were issued for the rule on invalid casting. Consider one of these warnings:

Picture 8


Having studied the code and the list of implementations, I agree with the analyzer: at the moment there really is not a single type that implements both IBaseScope and IComparable at once , as a result of which the boxItem.Item is IComparable check will always be false . However, I would not talk about the error in this case, because, firstly, the very existence of such a check precludes the subsequent occurrence of an exception when attempting to cast (IComparable) boxItem.Item. Secondly, at any time, for example, a certain dll can be connected to the solution, in which a type that implements both IBaseScope and IComparable interfaces will be declared. Perhaps, the developer was counting on this, realizing the type conversion only after checking. The considered warning, in my opinion, should be attributed to the discharge of messages for the Minor section, and not critical for execution, and its presence at the Critical level should be considered a false positive.

The remaining 63 warnings are similar to those considered.

Major


At this level, a lot of warnings were issued - 1742, for fifteen types of diagnostics:

Picture 9


Let's go through the list of warnings in order to search for real errors and assess the capabilities of the analyzer.

General exceptions should never be thrown

The rule reports that a generic exception was thrown using throw . In the code of the PascalABC.NET project, 634 such constructions were found. The vast majority is as follows:

Picture 10


Also there are a lot of (more than 600) constructions similar to “stubs” in the code, intentionally left by the developers:

Picture 11


Of course, throwing generic exceptions is “bad form.” However, it seems to me that these are not errors at all. Moreover, it is unlikely that the authors of the code deliberately produce them in such quantity. Yes, the exception handling in the PascalABC.NET project is apparently not up to par. Nevertheless, the place for all these 634 warnings of the same type in the Minor section or even in the false positives of the analyzer.

By the way, this is a good example of the difference between SonarC # and the PVS-Studio analyzer. SonarC # indicates "smells" in the code and is absolutely right to issue these warnings. They allow you to judge the quality of the project. From the point of view of us, the developers of the PVS-Studio analyzer, these are false positives, since we are focused on finding errors and security defects.

Dead stores should be removed

It is also a very large group of 618 warnings about reassigning the value of a variable, if it is not used between assignments. Here the following pattern prevails:

Picture 12


The variable is initialized upon declaration, and then, without ever using the stored value, a new one is assigned. Of course, this should not be done. Here and issues of saving resources, and possible suspicions of another mistake or typo. But in fact - none of these structures is not an error. Again, it is not clear why all such warnings were placed in the section of errors of high importance? In my opinion all this is false positives.

There are several absolutely clear false-positive warnings of the form:

Picture 13


If in this case, follow the recommendations of the analyzer, the logic of the program will be violated.

Thus, I did not manage to find a single real error among the 618 warnings from the considered group.

Floating point numbers should not be tested for equality

151 warnings were issued for comparison constructions in which one or both of the compared operands are of the real type. Indeed, such comparisons often give an erroneous result, which is associated with the peculiarities of storing real variables in memory and can vary, for example, depending on the compiler settings. Such designs can work for a very long time without problems. In this case, it is necessary in each particular case to make a decision on the erroneousness of such a code. For example, if the compared values ​​are the result of mathematical calculations, then a direct comparison of these values ​​is usually erroneous. If you compare two real constants, then this is probably done meaningfully and there will be no error.

In the PascalABC.NET code, I mainly met the following pattern of comparison with a real variable:

Picture 4


Note that the comparison is made both of two real variables, and of a real variable with a variable of integer type. Of course, such code is not completely safe, since it is not known how the compared values ​​were obtained. Is it worth talking about a clear error? It is difficult to give a definite answer. But the code probably needs some work.

By the way, the PVS-Studio analyzer also warns about such suspicious comparisons, but these diagnostics refer to the Low confidence level and are not recommended for our study.

Also among the warnings issued by the analyzer there are obvious false positives, for example:

Picture 15


In this case, two byte variables are compared. The left and right variables are of 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 warning group is rightly located in the Major section. However, I would not consider all warnings found errors. The decision must be made by the author of the code in each specific case.

Multiline blocks should not be enclosed in curly braces

A group of 108 warnings, including potential formatting errors that affect the logic of program execution. Here I found quite suspicious constructions. Example:

Picture 16


This snippet may have skipped parentheses. In any case, the developer should format the code to better understand the logic of the program.

Another similar warning:

Picture 17


There is no error, but the code looks messy. Refactoring is required.

In principle, all warnings from this group were issued in the case, but they did not reveal any real errors.

Null pointers should not be dereferenced

75 warnings about the possible access to the zero link. In this block I found interesting errors:

Picture 18


Indeed, earlier in the code, the variable returned_scope is always checked for null equality before use, but in this case they forgot about it:
 public override void visit(....) { .... if (returned_scope != null && ....) { .... } else if (returned_scope != null) { .... } returned_scope.declaringUnit = entry_scope; // <= .... }

Another similar error:

Picture 19


In the first case, the variable pi is checked for null equality before use, but further, when the next call to pi.CompilationUnit, is forgotten.

The warning block contains a number of not so obvious errors, as well as false positives. I would rate the percentage of finding real errors here equal to 85%. Very good result.

Conditions should not unconditionally evaluate to "true" or to "false"

Block warnings about conditions that are feasible regardless of the logic of the program. Typical errors found:

Picture 20


Strange code that requires refinement by the author. Perhaps a serious mistake has been made.

In general, the group contains about 70% of these errors.

Excerpts should not be thrown from property getters

Do not throw exceptions in the get-section of the property, and if necessary, use methods instead of properties. This group contains 46 such warnings. The overwhelming majority of them are “stubs” left by the developers, intentionally or by forgetfulness:

Picture 21


There are not entirely correct constructions that require refactoring:

Picture 22


However, I do not consider these warnings to be errors. I think it would be more rational to attribute them to the Minor section.

Static fields should not be updated in constructors

Diagnostics about the danger of updating static fields in constructors: this can lead to inconsistent program behavior, since the field will be reinitialized for all instances of the class. In total, the analyzer issued 26 such warnings for the PascalABC.NET project. I did not find any real mistakes among them. Here are a couple of examples of code snippets found:

Picture 23


Every time in the static variable _instance a reference is written to a new instance of the class. Judging by the name of the variable - it was intended.

Picture 24


The parsers_loaded flag indicates that at least one instance of the class has already been created. Nothing criminal.

"= +" should not be used instead of "+ ="

An interesting diagnosis is that instead of the operator "- =", for example, they mistakenly used "= -". The analyzer issued 9 such warnings. Unfortunately, all of them are false positives. 6 warnings are issued for constructions that are variable declarations, where in principle it is impossible to use the operator "- =" or "+ =":

Picture 25


The remaining 3 warnings are related to the fact that the authors of the code, apparently, do not like to use spaces to format their code:

Picture 26


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 if blocks. Often this code is either already erroneous or contains the potential for an error. In our case, 4 out of 5 warnings contained a simple duplication of the condition, as well as the execution unit, which is, of course, suspicious, but not a gross error. One warning is more interesting:

Picture 27


Before part of the condition in the first if block was commented out, it was different from the condition in the next if block. Also note the block for doing this second else if : it is empty. There is only one operator: ";". Very strange and suspicious code.

Short circuit circuit should not be used

Diagnostics warns, for example, about the possible erroneous use of the & operator instead of && for expressions of type bool . A total of 5 suspicious constructions were found. All of them in one way or another require attention, although they may not contain mistakes. Here is an example of one of them:

Picture 28


In this case, it is impossible to assert that the use of the operator "|" erroneously, since in its right side a property with complex logic inside is checked. Perhaps the developer was trying to ensure that both conditions were always checked.

Exceptions should not be explicitly rethrown

Diagnosis about the loss of the stack exception. The analyzer issued 4 warnings of the same type:

Picture 29


Of course, this should not be done. Further debugging of the application will be difficult. But all these warnings are not so critical. In my opinion, their place in the Minor section.

Variables should not be self-assigned

3 warnings about assigning a variable to itself. Here is an example of one of the code snippets found:

Picture 3


Strange and obviously wrong code. The visitNode ad is:
 protected bool visitNode = true;

In total, this group of warnings contains two errors.

Identical expressions should not be used

Diagnostics searches for conditions in which there are identical subexpressions. 2 suspicious constructions were found. There is no obvious error in any of them, but perhaps the code should look and work differently. Example of one of the warnings:

Picture 7


Strange code. You may have forgotten to replace the second check.

"ToString ()" method should not return null

The last group of warnings in the Major section. The overload of the ToString () method is implemented incorrectly. 2 warnings are issued, and both are errors. An example of one of them:

Picture 14


It is incorrect to return null from the overloaded ToString () method . You must use string.Empty .

Minor


1822 warnings were issued here. Since this level is not critical, it is unlikely that here I will discover really interesting errors. Also, usually, quite a lot of false positives are recorded at this level. Therefore, I will not consider Minor level warnings in this study.

The results of the test analyzer SonarC #


Summing up, I can say that, in general, the analyzer found real errors at the Blocker, Critical and Major levels (I counted 268 erroneous or highly suspicious constructions with 1814 warnings), some of which are really interesting. However, the percentage of false positives is quite large, at over 85%. This greatly complicates the analysis of the results.

PVS-Studio plugin for SonarQube


The integration of the results of the work of the PVS-Studio analyzer in SonarQube is devoted to the section of the documentation on our website. It took me about 15 minutes to set up the integration from scratch. The same amount was taken by checking the project and downloading the results to the local SonarQube server.

PVS-Studio issued 1039 warnings during the verification of PascalABC.NET code. Of these: 156 warnings of the Critical level, 541 of the Major level, 342 of the Minor level.

Picture 31


We will not consider Minor level warnings, since among them the percentage of false positives is usually high.

Distribution of warnings by diagnostics at the Critical level:

Picture 34


Distribution of warnings by diagnostics at Major level:

Picture 35


After analyzing 697 warnings at the Critical and Major levels, I found out that 204 of them can be attributed to false positives. This represents about 29% of the total number of alerts at the first and second level of importance. Thus, the percentage of identifying real errors and suspicious constructions for the PascalABC.NET project is 71%. In terms of the number of lines of code (KLOC), this is 0.66 errors per KLOC. Let's take a look at the most interesting errors found. For convenience, I will cite errors ascending the number of the diagnostic rule.

Copy paste

V3001 There are identical sub-expressions "token.Kind == openBracketToken" 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 if block condition, the token.Kind == openBracketToken equality is checked twice. Among the fields declared in a class, you can find a field with the very similar name openBracesToken . It is probably this field that was omitted from the condition. In this case, the corrected version of the code could be:

 public void InitializeOpenedInsightWindow(....) { .... if (token.Kind == openParensToken || token.Kind == openBracketToken || token.Kind == openBracesToken) { bracketCount++; } .... }

Similar errors in the code:


Inattention

V3003 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility 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 above code snippet actually contains about thirty checks of the same type, two of which are completely identical. Perhaps there is no error, and the code is simply duplicated due to inattention. But one of the checks, according to the developer, could look different. In this case, we are dealing with a serious logical 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 contain identical subexpressions. In this case, it is difficult to conclude that the correct version of this fragment, but in the above form, the code will not work as expected.

Similar errors in the code:


I brought only the first 10 of these errors out of 20.

The 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 gets the same value twice during assignment. Probably, extra assignment added randomly. However, instead of one of the variables miGenerateRealization.Visible there could be some other variable that is not initialized now.

Found another similar error:

V3005 The 'visitNode' variable is assigned to itself. SyntaxVisitors SimplePrettyPrinterVisitor.cs 106

Reassignment

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

From the code it can be seen that there is absolutely no logical explanation for reassigning the codeCompileUnit variable to the same value.

Similar errors in the code:


The result of the method is always the same.

V3009 It's one of the same values ​​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 here we are dealing with inattention when refactoring. Earlier in the code were blocks of code that return true . However, now they are commented out, and the method, regardless of the result of their work, will return false .

Similar errors in the code:


Inattention

V3010 The '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 saved anywhere. Corrected version of the given fragment:

 static IEnumerable<ITreeNode<IClass>> FindDerivedClassesTree(....) { .... var result = new List<TreeNode<IClass>>(); .... return result.OrderBy(node => node.Content.FullyQualifiedName); }

And one more such mistake:

V3010 The return value of the function 'ToString' is required to be used. CodeCompletion SymTable.cs 2145

Problem with logic

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 if (e.Delta <0) . Based on how the code is formatted, as well as from the program logic, the conclusion follows: it is possible that the else keyword is missing. However, only the author can give an exact answer about the features of this design.

The classic mistake when working with the operator "as"

V3019 Possibly incorrectly variable conversion 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 a TypeScope type, an error on the equality of null is checked, not the field this.baseScope , but the argument baseScope . Corrected code:

 public TypeScope(...., SymScope baseScope) { .... this.baseScope = baseScope as TypeScope; .... if (this.baseScope == null) { .... } .... }

Similar errors in the code:


Inaccurate 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, but the program looks messy.

Similar constructions in code:


I brought only the first 10 similar warnings from more than 45.

Excess check or 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) { .... } .... } .... }

Usually such constructions are not an error, but there is a possibility that one of the checks should have contained another 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 a completely different impression is created. Probably using {} brackets would solve the problem.

Typo

V3038 of The 'enum_consts [i]' argument WAS PASSED to 'the Compare' method Several Times. It can be passed. 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 does not contain the declaration of a local variable enum_consts . Therefore, inside a for loop, the elements of the enum_consts list are repeatedly compared to themselves. By looking at the IsEqual method, you can make an assumption about the correct code version:

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

Problem with Logic v2.0

V3043 The code's operational logic does not correspond with its formatting. The statement is indented. 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 of punkt_sym = true will be performed regardless of the result of the test kav.Count == 0 . However, the code is formatted in such a way that it seems that this will be done only if kav.Count! = 0 .

Another similar error:

V3043 The code's operational logic does not correspond with its formatting. The statement is indented. It is possible that curly brackets are missing. ICSharpCode.SharpDevelop AbstractConsolePad.cs 159

Exception stack loss

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 COMException, the developer uses only the text of the message. Apparently, this is a sensible decision, since further an exception is thrown of type SaveAssemblyError , the constructor of which requires nothing but the text of the message:

 public class SaveAssemblyError : CompilationError { .... public SaveAssemblyError(string text) { _text = text; } .... }

Of course, such an implementation option is the author’s right. However, in my opinion, exception handling in this case does not look complete.

Similar errors in the code:


Error working with substrings

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 for the substring “reduction (” is meaningless, since the substring “reduction” will always be found earlier . Wrong

initialization order

V3070 Uninitialized variable 'event_add_method_prefix' is used

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

As a result of executing the above code snippet, the string event_add_method_nameformat will get the value "{0}" , instead of the expected "add_ {0}" . To correct the error, swap the field initialization lines:

 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

Zero link access: 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 you can see, the tc variable was previously initialized with the value GetTemplateClassReference () . However, now is the value null . As a result, the first iteration of the for loop will result in an access error on the null link. Perhaps, the error has not yet manifested itself, as there are no calls to the GetTemplateInstance () method in the code. But there is no guarantee that this will continue.

Similar errors in the code:


Access via null link: inattention

V3095 The 'VisualEnvironmentCompiler.RemoteCompiler' object was used against it. Check lines: 52, 54. CompilerController CompilerControllerPlugin.cs 52

 public CompilerController_VisualPascalABCPlugin(....) { .... VisualEnvironmentCompiler.RemoteCompiler.InternalDebug.RunOnMono = CompilerInformation.cbRunMono.Checked; .... if (VisualEnvironmentCompiler.RemoteCompiler != null) .... }

Check for possible equality of the null variable is done after its use. Corrected code:

 public CompilerController_VisualPascalABCPlugin(....) { .... if (VisualEnvironmentCompiler.RemoteCompiler != null) { VisualEnvironmentCompiler.RemoteCompiler. InternalDebug.RunOnMono = CompilerInformation.cbRunMono.Checked; .... } }

Similar errors in the code:


I cited only the first 10 of these errors from 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); }

Immediately two methods that implement infinite recursion. Both methods are similar and differ only in the type of the second argument. Nowhere in the code are not used. At least not yet used.

Similar errors in the code:


Careless implementation of the method Equals

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 snippet rather carelessly reacted to the safety issues of his work. At least one check for the null equality of the rhs variable after its initialization is missing . And in order not to do extra work at all, a preliminary check on the null variable obj is necessary :

 public override bool Equals(object obj) { if (obj == null || !(obj is ServiceReferenceMapFile)) return false; var rhs = obj as ServiceReferenceMapFile; return FileName == rhs.FileName; }

Not enough 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 code contains a check for the equality of the resources variable null , but this is not enough, since the else block does not contain such a check. Under certain circumstances, this will inevitably lead to access via a null link. Code needs to 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 cited only the first 10 similar errors from more than 80 (eighty!).

Incorrect 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 dockPanel field is first used in the SearchResultsPad constructor , and then initialized. At the same time, even if the CreateToolBarItems method or nested methods provides for the null equality of the first argument, the method will probably return null. This, in turn, will lead to further errors when using the variable defaultToolbarItems .

Statistics


I see the big picture as follows. SonarC # and PVS-Studio analyzers solve different tasks. SonarC # is designed to evaluate and control code quality. Therefore, it reports both errors and "smells" of the code. PVS-Studio is focused on finding errors or places in the code that may later lead to errors. Of course, the messages issued by these analyzers overlap partially, but are calculated for different needs:


I will give a summary table of the results of the verification of the project PascalABC.NET (Blocker, Critical and Major warning levels are taken):

Picture 6


Once again I want to note that analyzers cannot be directly compared by the number of errors found and the number of false positives. SonarC # tries to issue a warning to a code that, although bad, does not contain an error. This is exactly what makes it possible to evaluate the quality of the code. In its turn, the PVS-Studio analyzer in this case prefers to keep silent or issue a warning with a minimum level of confidence. At the same time, he tries to identify as many errors as possible and is trained to detect a greater number of defects that lead to program malfunctions.

Conclusion


So, as expected, when working with the PVS-Studio and SonarC # plug-ins for SonarQube, I did not have any problems. All functions and features of the tools are documented. After downloading the results to the SonarQube server, you get access to a variety of functionalities for assessing the quality of your software product. As for the actual search for errors by source code analyzers, both tools showed a decent result.

To download and analyze an online project on the SonarQube website, you need a minimum of effort and time.

Using the PVS-Studio plugin to integrate the results of its work into the SonarQube is also not difficult. The only restriction is that you will need the Enterprise version.analyzer. If you do not need integration with SonarQube, you can use PVS-Studio as an independent tool.

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

For questions regarding the acquisition of a commercial license of PVS-Studio, 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. Analysis of PascalABC.NET using SonarQube plugins: SonarC # and PVS-Studio

Read the article and have a question?
. : PVS-Studio, 2015 . , .

Source: https://habr.com/ru/post/325126/

More articles:


All Articles