n0mo May 10, 2017 at 17:25

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

Recently, I compared the C # analyzers of PVS-Studio and SonarQube based on the PascalABC.NET project code. The study turned out to be quite interesting, so I decided to continue working in this direction. This time I am comparing the C # analyzer PVS-Studio with the static analyzer built into Visual Studio. In my opinion, this is a very worthy opponent. Despite the fact that the analyzer from the Visual Studio suite is primarily designed not to search for errors, but to improve the quality of the code, this does not mean at all that it cannot be used to find real errors, although this is difficult. Let's see what features of the analyzers will be revealed in the course of our study this time. Forward!

Introduction

First, a small FAQ section to clarify a number of points.

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

A: CruiseControl.NET is an automatic continuous integration server implemented using the .NET Framework. CruiseControl.NET Source Code Available on GitHub. The project has not been developed and not supported for some time, although in the recent past it enjoyed a certain popularity. This will not hinder its application for the purpose of comparing analyzers; rather, on the contrary, it will introduce some element of stability into the study. You can be sure that no one has improved the code using the latest versions of PVS-Studio or the analyzer built into Visual Studio. An additional plus 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: Visual Studio 2017 Community was used to analyze both tools.

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

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

Q: Well, good. But, surely, did you rig the results a little or did the calculations not quite right? For example, for PVS-Studio, you might not take into account the reliability level of the Low warnings, taking only High and Medium. Then PVS-Studio will have an advantage over the analyzer built into Visual Studio, since the latter does not have a similar configuration.

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

Q: What about choosing 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. At the same time, I note that CruiseControl.NET contains a project called "UnitTests". Quite a lot 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 overall indicator of the number of warnings issued.

Q: Real mistakes? What is this term?

A: In our understanding, these are errors that are critical to execution, which with a high degree of probability will lead to throwing an exception, incorrect behavior of the program, and issuing incorrect results. Errors that need to be fixed here and now. These are not recommendations for improving the design and not minor flaws such as duplication of code that do not affect the final result. An example of a real error in CruiseControl.NET code:

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

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

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

PVS-Studio analyzer will not generate a warning for 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. A lot of warnings don't mean good.

Q: Let's say you really 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. You just need to install the free version of Visual Studio 2017 Community. To analyze CruiseControl.NET using PVS-Studio, you just need to download it and use it in demo mode. Yes, restrictions will prevent you from conducting 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 built-in analyzer in 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, looking for errors here will not be so simple. First, I will exclude warnings from the “UnitTests” project from this list using the filter:

Good. Almost half of the warnings were received for tests, which is not surprising. But more than 5 thousand remaining messages are also not enough. These warnings belong to 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 have also been issued. Apparently, there is no other choice but to study the description of each of the analyzer diagnostics received, and only then conduct a study of 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 more than five thousand warnings issued by the analyzer. As a rule, it all comes down to recommendations for improving design and code optimization. Due to the large number of diagnostic rules, I will not provide a complete list with descriptions. If you wish, you can thoroughly examine this list by independently checking the CruiseControl.NET project using the analyzer built into Visual Studio. A detailed description of the diagnostics is available on MSDN .

I studied a substantial portion of the warnings, but not all. Until the end, many groups of messages did not make sense to browse, since they were all of the same type and it was obviously not about errors. In order not to be unfounded, I will give one example for each group.

Microsoft.Design

CA1002 Change 'List'in' CruiseServerClient.ForceBuild (string, List) 'to use Collection, ReadOnlyCollection or KeyedCollection CruiseServerClient.cs 118

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

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

Microsoft.Globalization

CA1300 Change 'AddProjects.RetrieveListOfProjects (BuildServer)' to call the MessageBox.Show overload that specifies MessageBoxOptions, and make sure to set MessageBoxOptions.RightAlign and MessageBoxOptions.RtlReading if RightToLeft is set to controlTo. CCTrayLib AddProjects.cs 86

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

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

Microsoft.Interoperability

CA1401 Change the accessibility of P / Invoke 'NativeMethods.SetForegroundWindow (IntPtr)' so that it is no longer visible from outside its assembly. 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 the public access level for methods with the DllImportAttribute attribute .

Microsoft.Maintainability

CA1500 'errorMessages', a variable declared in 'Response.ConcatenateErrors ()', has the same name as an instance field on the type. Change the name of one of these items. Remote Response.cs 152

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

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 []) 'to set the timer interval to a value that's greater than or equal to one second. core FileChangedWatcher.cs 33

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

Warning that the timer has an interval of less than one second.

Microsoft.Naming

CA1702 In member 'Alienbrain.CreateGetProcess (string)', the discrete term 'filename' in parameter name 'filename' should be expressed as a compound word, 'fileName'. core Alienbrain.cs 378

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

A warning about using Camel Case to name compound variable names.

Microsoft.Performance

CA1800 'action', a variable, is cast to type 'AdministerAction' multiple times in method 'AdministerPlugin.NamedActions.get ()'. Cache the result of the 'as' operator or direct cast in order to eliminate the redundant isint instruction. WebDashboard AdministerPlugin.cs 79

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

A warning about the need to optimize multiple repeat casts.

Microsoft.Portability

CA1901 As it is declared in your code, parameter 'fdwSound' of P / Invoke 'Audio.PlaySound (byte [], short, long)' will be 8 bytes wide on 32-bit platforms. This is not correct, as the actual native declaration of this API indicates it should be 4 bytes wide on 32-bit platforms. Consult the MSDN Platform SDK documentation for help determining what data type should be used instead of 'long'. CCTrayLib Audio.cs 135

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

Warning that the imported method uses the non-portable 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 on object 'urlLink' before all references to it are out of scope. CCTrayLib About.cs 71

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

A warning about the need to free the urlLink IDisposable before it is out of scope. You can, for example, use using .

Microsoft.Globalization, Microsoft.Security

CA2101 To reduce security risk, marshal parameter 'lpszDomain' as Unicode, by setting DllImport.CharSet to CharSet.Unicode, or by explicitly marshaling the parameter as UnmanagedType.LPWStr. If you need to marshal this string as ANSI or system-dependent, specify MarshalAs explicitly, 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);

Warning that a marshaling type for string arguments is not specified, 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 type 'ApplicationException', an exception type that is not sufficiently specific and should never be raised by user code. If this exception instance might be thrown, use a different exception type. Remote CruiseServerClientFactory.cs 97

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

An exception throw warning is too general. In this case, it is recommended to throw an exception of a more specific type that is not reserved in the runtime.

Summary

As a result of the work done, I came to the conclusion that it makes sense to search for real errors, in this case, only for messages with code CA1062 from the Microsoft.Design group. These are warnings about the situation when the method parameter is a reference type, and a null check is not performed before using it . 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);
}

Child's reference to an instance of the ItemStatus class is not tested before use. Yes, this is dangerous. But, unfortunately, this cannot be called a mistake. Probably, checks can 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 let me remind you that there are 733 of them. Most likely, the programmer will not do anything, since “everything works this way”. This is precisely the danger of issuing a large number of messages to everything that is more or less suspicious. For this reason, I previously gave an example of a real error, one that is worth paying attention to the developer. Workings such as this can be considered for the most part false. And indeed it is.

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

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

The obj variable is not checked for null before use. Since this is an overloaded Equals method , we are dealing with an error. The Equals method must correctly process null references. Perhaps in the CruiseControl.NET project such situations never arise, but the method code is still erroneous and should be fixed.

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

I note that despite the fact that I managed to find real errors in CruiseControl.NET code using the analyzer built into Visual Studio, the process itself took me about 8 hours (all 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 a minute. Immediately after this, the results look like (no filters are included):

The analyzer generated 198 warnings. And 45 of them were received for the UnitTests project, and another 32 warnings have a low priority (it is difficult to find real errors among them). Total - 121 messages for analysis, which I spent 30 minutes on. 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 probability 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 mistake has been 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 selected from those found by the analyzer built into Visual-Studio.

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

Conclusion

I will present the results in a table:

Of course, an obvious advantage is observed on the side of PVS-Studio. But, again, the analyzer built into Visual Studio is intended, first of all, to improve the design and code optimization, and not to search for errors. While PVS-Studio, on the contrary, is “imprisoned” for searching for errors with the issuance of 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, and not "once a year".

→ Download and try PVS-Studio

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

Sitelinks

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

Have you read the article and have a question?

Often our articles are asked the same questions. We collected the answers here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please see the list.

Tags: