Comparison of PVS-Studio C # and static analyzer built into Visual Studio based on the code of the CruiseControl.NET project

Recently, I conducted a comparison of C # analyzers PVS-Studio and SonarQube based on the code of the PascalABC.NET project. The study was quite interesting, so I decided to continue working in this direction. This time I compare the PVS-Studio C # analyzer with the static analyzer embedded in Visual Studio. In my opinion, this is a very worthy opponent. Despite the fact that the analyzer from the Visual Studio suite, in the first place, is not designed to find errors, but to improve the quality of the code, this does not mean that it cannot be used to find real errors, although this is difficult. Let's see what are the features of the analyzers will be revealed in the course of our research this time. Forward!

Introduction

At first - a small FAQ section to clarify a number of points.

Q: Why CruiseControl.NET? What is this project?

A: CruiseControl.NET is an automatic continuous integration server implemented using the .NET Framework. CruiseControl.NET source code is available on GitHub . The project has not been developing for some time and is not supported, although, in the recent past, it enjoyed a certain popularity. It does not hurt to use it in order to compare analyzers; rather, on the contrary, it will introduce some element of stability in the study. You can be sure that nobody improved the code using the latest versions of PVS-Studio or the analyzer built into Visual Studio. An additional advantage is the small size of CruiseControl.NET: the project contains about 256 thousand lines of code.
')
Q: Have you used Visual Studio 2017? I would like to get acquainted with the capabilities of the latest versions of analysis tools.

A: For the analysis of both tools, the Visual Studio 2017 Community was used.

Q: What about analyzer settings? Probably, everything was “specially tuned”, and therefore PVS-Studio turned out to be better?

A: For both analyzers, the default settings were used. For the purity of the experiment, installation and research were conducted on a “clean” machine with Windows 10.

Q: Oh well. But, surely, you have a little rigged the results or made the calculations is not quite correct? For example, for PVS-Studio, you could not take into account the confidence level of warnings “Low”, taking only “High” and “Medium”. Then PVS-Studio will take precedence over the analyzer built into Visual Studio, since the latter does not have a similar configuration.

A: When analyzing the results, absolutely all levels of warnings were taken into account and all available types of diagnostics were included.

Q: What about selecting files for analysis? Did you add something to the exceptions, for example, Unit tests?

A: For both analyzers, the entire solution was checked, with no exceptions. In this case, I note that CruiseControl.NET contains a project named "UnitTests". Quite a number of warnings were issued for this project, but all of them were not taken into account when searching for real errors, although they appear in the general indicator of the number of warnings issued.

Q: Real bugs? What is this term?

A: In our understanding, these are errors that are critical for execution, which are most likely to result in an exception being thrown, a program’s incorrect behavior, and incorrect results being issued. Errors that need to be corrected here and now. This is not a recommendation to improve the design and not minor flaws such as code duplication, which do not affect the final result. An example of a real error in the code of CruiseControl.NET:

public override string Generate(IIntegrationResult integrationResult) { .... IntegrationSummary lastIntegration = integrationResult.LastIntegration; // <= if (integrationResult == null || ....) // <= { .... } .... } 

Many analyzers will issue a warning for the above code snippet that the integrationResult variable is used without first checking for null equality. This is correct, but usually leads to a large number of false positives, among which it is very difficult to find the real error. Our approach is to conduct additional analysis, which increases the likelihood of finding a real error. In the above fragment, after using the variable, it is checked for null equality. Those. the programmer, in this case, assumes that the value of the variable may be null after being passed to the method, and does a check. This is the situation we will consider a mistake. If the method did not check the integrationResult for null equality, then, in our opinion, this would be a false positive:

 public override string Generate(IIntegrationResult integrationResult) { .... IntegrationSummary lastIntegration = integrationResult.LastIntegration; .... } 

The PVS-Studio analyzer will not issue a warning to this code fragment, while a number of analyzers will do this. And, as a result, such messages will be ignored or disabled by the programmer. Many warnings do not mean good.

Q: Let's say you did everything correctly. But why should I take it all on faith? How can I repeat your research?

A: Nothing is easier. The analyzer built into Visual Studio is free. Just install the free version of the Visual Studio 2017 Community. To analyze CruiseControl.NET using PVS-Studio, you will only need to download it and use it in demo mode. Yes, restrictions will prevent you from carrying out a full analysis, but you can write to us and we will send a temporary license key.

Research and Results

Visual studio

Checking the project code using the analyzer built into Visual Studio took only a couple of minutes. Immediately after this, the results are as follows (no filters are included):

The analyzer issued 10773 warnings. Yes, it will not be so easy to look for mistakes here. To begin with, I will exclude from this list the warnings on the project “UnitTests” using a filter:

Good. Almost half of the warnings were received for tests, which is not surprising. But more than 5 thousand of the remaining messages are not small either. These warnings fall into the following groups:

 Microsoft.Design: CA10XX (: 40, : 1637) Microsoft.Globalization: CA13XX (: 7, : 1406) Microsoft.Interoperability: CA14XX (: 2, : 10) Microsoft.Maintainability: CA15XX (: 3, : 74) Microsoft.Mobility: CA16XX (: 1, : 1) Microsoft.Naming: CA17XX (: 17, : 1071) Microsoft.Performance: CA18XX (: 15, : 489) Microsoft.Portability: CA19XX (: 1, : 4) Microsoft.Reliability: CA20XX (: 4, : 158) Microsoft.Globalization, Microsoft.Security: CA21XX (: 5, : 48) Microsoft.Usage: CA22XX (: 18, : 440) 

A number of compiler warnings were also issued. Apparently, there is no other choice but to study the description of each of the received diagnostics of the analyzer, and only then examine the warnings if necessary.

I will say right away that I did this work and I could not find almost anything that would help to find errors among the more than five thousand warnings issued by the analyzer. As a rule, it all comes down to recommendations for improving the design and optimization of the code. Due to the large number of diagnostic rules, I will not give a complete list with descriptions. If you wish, you can study this list thoroughly by checking the CruiseControl.NET project yourself using the analyzer built into Visual Studio. A detailed description of diagnostics is available in MSDN .

I studied a significant part of the warnings, but not all. Until the end, many groups of messages did not make sense to look through, since they were all of the same type and it was clearly not about errors. Not to be unfounded, I will give one example for each group.

Microsoft.Design

CA1002 Change 'List <NameValuePair>' in 'CruiseServerClient.ForceBuild (string, List <NameValuePair>)' to use Collection <T>, ReadOnlyCollection <T> or KeyedCollection <K, V> CruiseServerClient.cs 118

 public override void ForceBuild(...., List<NameValuePair> parameters) { .... } 

The recommendation is to use a universal collection (for example, Collection ), instead of List for the parameter parameters of a method.

Microsoft.Globalization

CA1300 Change 'AddProjects.RetrieveListOfProjects (BuildServer)' to call the MessageBox. CCTrayLib AddProjects.cs 86

 private void RetrieveListOfProjects(....) { .... MessageBox.Show(this, "Unable to connect to server " + server.DisplayName + ": " + ex.Message, "Error"); .... } 

A recommendation to use the overload of the MessageBox.Show () method, which takes a MessageBoxOptions argument. This is necessary to better support the multilingual interface and languages ​​that use right-to-left reading order.

Microsoft.Interoperability

CA1401 Change the accessibility of the P / Invoke 'NativeMethods.SetForegroundWindow (IntPtr)'. CCTrayLib NativeMethods.cs 12

 [DllImport("user32.dll")] [return: MarshalAs(UnmanagedType.Bool)] public static extern bool SetForegroundWindow(IntPtr handle); 

The recommendation is that you should not specify a public access level for methods with the DllImportAttribute attribute.

Microsoft.Maintainability

CA1500 'errorMessages', a variable named field response to the type. Change the name of one of these items. Remote Response.cs 152

 private List<ErrorMessage> errorMessages; .... public virtual string ConcatenateErrors() { List<string> errorMessages = new List<string>(); .... } 

A warning that a local variable has the same name as a class field.

Microsoft.Mobility

CA1601 Modify the call to 'Timer.Timer (double)' in method FileChangedWatcher.FileChangedWatcher (params string []) 'set. core FileChangedWatcher.cs 33

 public FileChangedWatcher(....) { .... timer = new Timer(500); .... } 

Warning that the timer is set to an interval of less than one second.

Microsoft.Naming

CA1702 In member 'Alienbrain.CreateGetProcess (string)', it should be expressed as a compound word, 'fileName'. core Alienbrain.cs 378

 public ProcessInfo CreateGetProcess(string filename) { .... } 

Warning about the need to use Camel Case to name compound variable names.

Microsoft.Performance

CA1800 'action', a variable, is a 'AdministerAction' multiple times the method 'AdministerPlugin.NamedActions.get ()'. Eliminating isint instruction. WebDashboard AdministerPlugin.cs 79

 public INamedAction[] NamedActions { get { .... if (action is AdministerAction) { (action as AdministerAction).Password = password; } .... } .... } 

Warning about the need to optimize repetitive type conversion.

Microsoft.Portability

FDWSound 'of P / Invoke' Audio.PlaySound (byte [], short, long) will be 8 bytes wide on 32-bit platforms. This indicates that this API does not correct 4 bytes wide on 32-bit platforms. Consult the MSDN Platform SDK. CCTrayLib Audio.cs 135

 [DllImport ("winmm.dll")] private static extern int PlaySound (byte[] pszSound, Int16 hMod, long fdwSound); 

Warning that the import method uses an unportable type for the fdwSound parameter. You must use IntPtr or UIntPtr .

Microsoft.Reliability

CA2000 In method 'About.famfamfamLink_LinkClicked (object, LinkLabelLinkClickedEventArgs)', call System.IDisposable.Dispose of the object 'urlLink' before it’s out. CCTrayLib About.cs 71

 private void famfamfamLink_LinkClicked(....) { Process urlLink = new Process(); urlLink.StartInfo = new ProcessStartInfo(....); urlLink.Start(); } 

Warning about the need to free the IDlposable urlLink object before it is out of scope. You can, for example, use using .

Microsoft.Globalization, Microsoft.Security

CA2101 to reduce security, Unicode, by setting DllImport.CharSet to CharSet.Unicode, or by explicitly marshaling. If you need to set it up, select Marshal As appropriate, and set BestFitMapping = false; for added security, also set ThrowOnUnmappableChar = true. core Impersonation.cs 100

 [DllImport("advapi32.dll", SetLastError = true)] private static extern bool LogonUser( string lpszUsername, string lpszDomain, string lpszPassword, int dwLogonType, int dwLogonProvider, ref IntPtr phToken); 

A warning that marshaling type is not specified for string arguments, for example, by specifying attributes as follows:

 [DllImport("advapi32.dll", SetLastError = true, CharSet = CharSet.Unicode)] 

Microsoft.Usage

CA2201 'CruiseServerClientFactory.GenerateClient (String, ClientStartUpSettings) creates an exception of the type' ApplicationException ', it’s not a rule. Could be thrown, use a different exception type. Remote CruiseServerClientFactory.cs 97

 public CruiseServerClientBase GenerateClient(....) { .... throw new ApplicationException("Unknown transport protocol"); .... } 

Warn about the release of exceptions are too general. In this case, it is recommended to throw an exception of a more specific type that is not reserved in the runtime environment.

Total

As a result of the work done, I came to the conclusion that the search for real errors, in this case, it makes sense to produce only for messages with code CA1062 from the Microsoft.Design group. These are warnings of a situation where the method parameter is a reference type, and before it is used, a check for null equality is not performed. After applying the filter for such messages, we get the following:

733 is still a lot. But, you see, this is already something. And if you examine the found code fragments, then they are really potentially unsafe. For example, in the ItemStatus.cs file:

 public void AddChild(ItemStatus child) { child.parent = this; childItems.Add(child); } 

The child reference to an instance of the ItemStatus class is not checked before use. Yes, it is dangerous. But, unfortunately, this cannot be called a mistake. Probably, checks may be contained in the calling code, although this is incorrect. Moreover, the method is declared as public . Of course, the author of the code should take measures and somehow respond to all these warnings, but I remind you that there are 733 of them. Most likely, the programmer will not do anything, since “everything works anyway”. This is precisely the danger of issuing a large number of messages to anything a little suspicious. For this reason, earlier I gave an example of a real error, one that the developer’s attention should be paid to. Triggers like this can be considered mostly false. And indeed it is.

After spending more time, I singled out among the 733 warnings issued - 5, which can be interpreted as errors. Here is an example of one of them (the BuildGraph.cs file):

 public override bool Equals(object obj) { if (obj.GetType() != this.GetType() ) return false; .... } 

The variable obj is not checked for null equality before use. Since this is an Equals overloaded method, we are dealing with an error. The Equals method is required to correctly handle null links. Perhaps in the CruiseControl.NET project such situations never arise, but the method code is still wrong and should be corrected.

A meticulous reader might argue that I could have missed some mistake without carefully examining every use of the methods. Maybe. But in practice, no one will study so carefully every warning, because the percentage of false positives is very large.

I note that despite the fact that I managed to find real errors in the code of CruiseControl.NET using the analyzer built in Visual Studio, the process itself took me about 8 hours (the whole working day) and required increased attention and concentration. Perhaps if the authors of the project used static analysis regularly, the overall picture would be more rosy.

PVS-Studio

Checking the project code using PVS-Studio on my machine took exactly one minute. Immediately after this, the results are of the form (no filters are included):

The analyzer issued 198 warnings. And 45 of them were received for the “UnitTests” project, and another 32 warnings have a low priority (among them it is difficult to find real errors). Total - 121 message for analysis, which I spent 30 minutes. As a result, 19 errors were identified:

Here is an example of one of them:

V3003 The use of if (A) {...} else if (A) {...} 'pattern was detected. There is a possibility of logical error presence. Check lines: 120, 125. CCTrayLib CCTrayProject.cs 120

 public override bool Equals(object obj) { .... if ((buildServer != null) && (objToCompare.buildServer != null)) { // If both instances have a build server then compare the build // server settings isSame = string.Equals(buildServer.Url, objToCompare.buildServer.Url); } else if ((buildServer != null) && (objToCompare.buildServer != null)) { // If neither instance has a build server then they are the same isSame = true; } .... } 

Both if blocks contain the same condition. A serious error was made that affects the logic of the program and leads to an unexpected result.

I note that among the errors that PVS-Studio detected, there are also those that I chose from those found by the analyzer embedded in Visual Studio.

I think here I have nothing more to add. PVS-Studio quickly and efficiently did its job in finding real errors. But for this he exists!

Conclusion

I will present the results in the form of a table:

Of course, an obvious advantage is observed on the side of PVS-Studio. But, I repeat, the analyzer built into Visual Studio is intended, first of all, to improve the design and optimization of the code, and not to search for errors. At the same time, PVS-Studio, on the contrary, is “honed” to search for errors with issuing the lowest possible level of false warnings. In addition, the developers of CruiseControl.NET, apparently, did not use any analyzers at all. I am sure that with regular use of the analyzer built into Visual Studio, the quality of their code would be much better, and the probability of a possible error is lower. What to say about PVS-Studio. Such tools allow you to achieve maximum effect with regular use, rather than “once a year”.

→ Download and try PVS-Studio

For purchasing 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.

Additional links

1. How and why static analyzers deal with false positives .
2. Checking the PascalABC.NET project with the help of SonarQube plugins: SonarC # and PVS-Studio .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Comparing PVS-Studio for C # and a built-in Visual Studio analyzer, using the CruiseControl.NET codebase