PVS-Studio 6.00 New Year release: checking Roslyn

The long-awaited event has arrived. We have released the release version of the PVS-Studio 6.00 static code analyzer, which supports checking C # projects. Now the code is written in the following languages: C, C ++, C ++ / CLI, C ++ / CX, C #. For the release of the sixth version of the analyzer, we timed the verification of the open Roslyn project. It was thanks to Roslyn that the C # support appeared in the PVS-Studio analyzer, and we are very grateful to Microsoft for the implementation and development of this project.

PVS-Studio 6.00

PVS-Studio is a static code analyzer focused on ease of use and search for errors at the stage of writing code.

We are constantly adding diagnostic rules to search for new types of errors in C / C ++ programs. For example, we recently added a search for class members that are not initialized in the constructor, which was a very difficult task . However, the improvement of diagnostics is not a reason for changing the older version. And we waited for something truly new to be added to the analyzer. And such a moment has come. We present the world the sixth version of the analyzer that supports the C # language.

The trial version of PVS-Studio 6.00 is available at:

http://www.viva64.com/en/pvs-studio-download/

In the sixth version of the analyzer, we refused to support older versions of Visual Studio. Now VS2005 and VS2008 are not supported. If your team still uses these versions of Visual Studio, then we suggest that you continue to use the previous version 5.31 or its upgrades, if any.

The demo version has the following limitation. You can perform 50 code jumps. After that, the analyzer will offer the person to send information about themselves. If he agrees, then he will be given another 50 transitions.

Limiting the demo may seem harsh. However, he has a justification, and we came to him after many experiments.

A small number of “clicks” will help to start communication in the mail faster. If someone wants to look at other messages, then we are ready to give him a registration key, say, for 1 week. It is enough for him to write us a letter. As a rule, in the process of correspondence, we help people quickly navigate, how they can quickly and easily benefit from using the analyzer.

Roslyn

Programmers are not susceptible to advertising. They cannot be carried away with the words “mission”, “impeccably” and “focus on innovation”. The programmer does not read advertisements and knows how to disable banners using Adblock Plus.

The only thing they can be carried away with is to show how this or that tool can be useful to them. We have chosen just such a path and show how the static analysis tool can benefit by checking open projects.

PVS-Studio can find errors in such well-known projects as CoreCLR, LibreOffice, Linux Kernel, Qt, Unreal Engine 4, Chromium and so on. At the moment, our team has checked 230 open projects and found 9355 in themmistakes. Yes, yes 9355 is the number of errors, not the number of diagnostic messages. The most interesting checks can be found in the relevant articles .

Now the turn has come to check C # projects. Not surprisingly, one of the first proven projects was Roslyn. In the end, thanks to this particular project, it became possible to support the analysis of C # code in PVS-Studio.

I want to thank Microsoft for creating this open source project. It will have a great impact on the development of C # infrastructure. Yes, he is already making an impact! For example, such well-known projects as ReSharper or CodeRush are transferred to the Rolsyn base .

Now a little about the Roslyn project itself.

The .NET Compiler Platform, better known by its code name Roslyn, is a set of open source compilers and APIs for analyzing source code for C # and Visual Basic .NET. The platform is developed by Microsoft.

This platform includes self-contained compilers for C # and VB.NET. They are available not only as traditional command-line applications, but also as native APIs that can be accessed from .NET code. Roslyn provides access to modules for parsing (lexical) code analysis, semantic analysis, dynamic compilation into CIL bytecode, and assembly generation. The APIs provided by Roslyn can be divided into three types: functional APIs (feature APIs), workspace APIs (workspace APIs), and compiler APIs (compiler APIs). Functional APIs facilitate refactoring and debugging. Workspace APIs allow plug-in developers to implement certain mechanisms specific to an IDE like Visual Studio — for example, finding matches between variables and values ​​or formatting code.

Additional links:

1. Github Roslyn .
2. Wikipedia .NET Compiler Platform ("Roslyn")
3. .NET Compiler Platform ("Roslyn") Overview .
4. MSDN Forum Microsoft "Roslyn" CTP .
5. MSDN Taking a tour of Roslyn .
6. Learn Roslyn Now .
7. Miguel de Icaza. Mono and Roslyn .

Found bugs

The PVS-Studio analyzer found few errors in Roslyn. This is natural for such a famous project being developed by Microsoft with its debugged quality control systems. Finding at least something in the Roslyn code is already a big win, and we are proud that we can do it.

Many of the errors below relate to tests or error handlers. It `s naturally. Errors in frequently used code fragments are fixed thanks to testing and user feedback. But for us, the fact itself is important that PVS-Studio can find errors. This means that many errors that were fixed with a lot of labor could be fixed immediately with regular use of PVS-Studio.

Once again, in other words. The value of the analyzer is not in one-time launches, but in its regular use. Static code analysis can be considered as an extended version of compiler warnings. It is foolish to include compiler warnings once a year. You need to look at warnings immediately after they occur. This saves development time by looking for stupid bugs using debugging. With a static analyzer, everything is the same.

Let's see what interesting was discovered using PVS-Studio:

Error N1 in tests. Copy-paste.

publicvoidIndexerMemberRace(){
  ....
  for (int i = 0; i < 20; i++)
  {
    ....
    if (i % 2 == 0)
    {
      thread1.Start();
      thread2.Start();
    }
    else
    {
      thread1.Start();
      thread2.Start();
    }
    ....
  }
  ....
}

PVS-Studio Warning: V3004 The 'then' statement is equivalent to the 'else' statement. GetSemanticInfoTests.cs 2269

Here is one example of a test error that can live in a project for years, since it doesn't bother anyone. It’s just that the test doesn’t check everything that was planned. At the beginning, stream 1 always starts, and then stream 2. Most likely, it was planned to write a test like this:

if (i % 2 == 0)
{
  thread1.Start();
  thread2.Start();
}
else
{
  // Поменяли порядок запуска потоков
  thread2.Start();
  thread1.Start();
}

Error N2 in the tests. Typo.

publicDiagnosticAsyncToken(
  AsynchronousOperationListener listener,
  string name,
  object tag,
  string filePath,
  int lineNumber)
  : base(listener){
  Name = Name;
  Tag = tag;
  FilePath = filePath;
  LineNumber = lineNumber;
  StackTrace = PortableShim.StackTrace.GetString();
}

PVS-Studio Warning: V3005 The 'Name' variable is assigned to itself. AsynchronousOperationListener.DiagnosticAsyncToken.cs 32

In my opinion, the error here is very difficult to find. Failed naming of variables. Class property names differ from function arguments only in the first capital letter. As a result, it is easy to make a typo, which is what happened: Name = Name.

Error N3, N4. Copy-paste.

private Task<SyntaxToken>
GetNewTokenWithRemovedOrToggledPragmaAsync(....)
{
  var result = isStartToken
    ? GetNewTokenWithPragmaUnsuppress(
        token, indexOfTriviaToRemoveOrToggle, _diagnostic, Fixer,
        isStartToken, toggle)
    : GetNewTokenWithPragmaUnsuppress(
        token, indexOfTriviaToRemoveOrToggle, _diagnostic, Fixer,
        isStartToken, toggle);
  return Task.FromResult(result);
}

Warning PVS-Studio: V3012 The '?:' Operator, regardless of its conditional expression, always returns one and the same value. AbstractSuppressionCodeFixProvider.RemoveSuppressionCodeAction_Pragma.cs 177

Regardless of the value of 'isStartToken', the GetNewTokenWithPragmaUnsuppress () function is called with the same set of actual arguments. Most likely, the function call was duplicated using Copy-Paste, and then they forgot to change something in the copied code.

Here is another similar case:

privatevoidDisplayDiagnostics(....){
  ....
  _console.Out.WriteLine(
    string.Format((notShown == 1) ?
      ScriptingResources.PlusAdditionalError :
      ScriptingResources.PlusAdditionalError,
      notShown));
  ....
}

Warning PVS-Studio: V3012 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: ScriptingResources.PlusAdditionalError. CommandLineRunner.cs 428

Errors N5, N6. Inattention.

I still have little statistics on typical errors that C # programmers make. But in addition to standard typos, the following error pattern clearly begins to lead: often after casting a link using the 'as' operator, they check not the resulting link, but the original one. Then use an unverified link. Synthetic code:

var A = B as T;
if (B) A.Foo();

And here is how this error looks in practice:

public override boolEquals(object obj){
  var d = obj as DiagnosticDescription;
  if (obj == null)
    returnfalse;
  if (!_code.Equals(d._code))
    returnfalse;
  ....
}

PVS-Studio Warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'obj', 'd'. DiagnosticDescription.cs 201

The following example is more verbose, but in fact, everything is the same:

protected override boolAreEqual(object other){
  var otherResourceString = other as LocalizableResourceString;
  return
    other != null &&
    _nameOfLocalizableResource == 
      otherResourceString._nameOfLocalizableResource &&
    _resourceManager == otherResourceString._resourceManager &&
    _resourceSource == otherResourceString._resourceSource &&
    ....
}

PVS-Studio Warning: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'other', 'otherResourceString'. LocalizableResourceString.cs 121

Error N7. Double detection.

Sometimes 2 or even 3 analyzer warnings immediately indicate the presence of errors in the code. Now we will just consider such a case.

privateboolHasMatchingEndTag(
  XmlElementStartTagSyntax parentStartTag){
  if (parentStartTag == null)
  {
    returnfalse;
  }
  var parentElement = parentStartTag.Parent as XmlElementSyntax;
  if (parentStartTag == null)
  {
    returnfalse;
  }
  var endTag = parentElement.EndTag;
  ....
}

PVS-Studio warnings:

• V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'parentStartTag', 'parentElement'. XmlTagCompletionCommandHandler.cs 123
• V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless XmlTagCompletionCommandHandler.cs 117

At the beginning of the function, it is checked that the argument 'parentStartTag' is not a null reference. If it is zero, then the function exits.

Then they wanted to verify that the link actually points to a class like 'XmlElementSyntax'. But at that moment a typo crept into the code. Instead of checking 'parentElement', 'parentStartTag' is re-checked.

The analyzer notices two anomalies at once. The first anomaly: it makes no sense to re-check the value of 'parentStartTag', because if this is a null reference, then we already left the function. Second anomaly: the analyzer suspects that it checks the wrong variable after the 'as' operator.

The correct version of the code:

if (parentStartTag == null)
{
  returnfalse;
}
var parentElement = parentStartTag.Parent as XmlElementSyntax;
if (parentElement == null)
{
  returnfalse;
}

Errors N8, N9. Copy-paste.

I apologize for the long piece of code, but did not want to replace it with a synthetic example.

internal staticboolReportConflictWithParameter(....){
  ....
  if (newSymbolKind == SymbolKind.Parameter ||
      newSymbolKind == SymbolKind.Local)
  {
    diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam,
                    newLocation, name);
    returntrue;
  }
  if (newSymbolKind == SymbolKind.TypeParameter)
  {
    returnfalse;
  }
  if (newSymbolKind == SymbolKind.Parameter ||
      newSymbolKind == SymbolKind.Local)
  {
    diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam,
                    newLocation, name);
    returntrue;
  }
  ....
}

PVS-Studio Warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless InMethodBinder.cs 264

The first and third 'if' statements match in the code snippet in the article. Apparently the block was copied, and then they forgot to change something in it. Although it is possible, the repeated 'if' is just superfluous and should be removed.

There is another similar piece of code, but I will not torment the reader with a long example. Just give a diagnostic warning:

V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless WithLambdaParametersBinder.cs 131

Error N10. Invalid condition.

publicenum TypeCode
{
  ....
  Object = 1,
  ....
  DateTime = 16,
  ....
}
static object GetHostObjectValue(Type lmrType, object rawValue)
{
  var typeCode = Metadata.Type.GetTypeCode(lmrType);
  return (lmrType.IsPointer || lmrType.IsEnum ||
          typeCode != TypeCode.DateTime ||
          typeCode != TypeCode.Object)
            ? rawValue : null;
}

PVS-Studio Warning: V3022 Expression 'lmrType.IsPointer || lmrType.IsEnum || typeCode! = TypeCode.DateTime || typeCode! = TypeCode.Object 'is always true. DkmClrValue.cs 136

The expression is quite complex, so I will highlight the main point:

(typeCode != 1 || typeCode != 16)

This expression is always true, regardless of the value of the 'typeCode' variable.

Error N11. Excessive condition.

publicenum EventCommand
{
  Disable = -3,
  Enable = -2,
  SendManifest = -1,
  Update = 0
}
protected override void OnEventCommand(
  EventCommandEventArgs command)
{
  base.OnEventCommand(command);
  if (command.Command == EventCommand.SendManifest ||
      command.Command != EventCommand.Disable ||
       FunctionDefinitionRequested(command))
  ....
}

Warning PVS-Studio: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. RoslynEventSource.cs 79

Once again, the point is:

if (A == -1 || A != -3)

This expression is erroneous or redundant. It can be reduced to:

if (A != -3)

Error N12 when logging.

staticCompilerServerLogger(){
  ....
  loggingFileName = Path.Combine(loggingFileName,
    string.Format("server.{1}.{2}.log",
                  loggingFileName,
                  GetCurrentProcessId(),
                  Environment.TickCount));
  ....
}

Warning PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 2. Present: 3. CompilerServerLogger.cs 49 The

variable 'loggingFileName' is not used in any way when calling the Format () function. This is suspicious.

Error N13 in the error handler.

privateconststring WriteFileExceptionMessage =
  @"{1}
  To reload the Roslyn compiler package, close Visual Studio and
  any MSBuild processes, then restart Visual Studio.";
privatevoidWriteMSBuildFiles(....){
  ....
  catch (Exception e)
  {
    VsShellUtilities.ShowMessageBox(
      this,
      string.Format(WriteFileExceptionMessage, e.Message),
      null,
      OLEMSGICON.OLEMSGICON_WARNING,
      OLEMSGBUTTON.OLEMSGBUTTON_OK,
      OLEMSGDEFBUTTON.OLEMSGDEFBUTTON_FIRST);
  }
}

Warning PVS-Studio: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 2. Present: 1. CompilerPackage.cs 105

Apparently an exception will be thrown when trying to show the Message Box. The fact is that the Format () function will try to print the second additional argument. But he is not.

It seems to me that a constant line specifying formatting should start with:

@"{0} 

Error N14, N15 in the error handler.

I bet that the DumpAttributes () function has not been used yet. It contains 2 errors at once, each of which should lead to the generation of an exception:

privatevoidDumpAttributes(Symbol s){
  int i = 0;
  foreach (var sa in s.GetAttributes())
  {
    int j = 0;
    foreach (var pa in sa.CommonConstructorArguments)
    {
      Console.WriteLine("{0} {1} {2}", pa.ToString());
      j += 1;
    }
    j = 0;
    foreach (var na in sa.CommonNamedArguments)
    {
      Console.WriteLine("{0} {1} {2} = {3}",
        na.Key, na.Value.ToString());
      j += 1;
    }
    i += 1;
  }
}

PVS-Studio warnings:

• V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Expected: 3. Present: 1. LoadingAttributes.cs 551
• V3025 Incorrect format. A different number of format items is expected while calling 'WriteLine' function. Expected: 4. Present: 2. LoadingAttributes.cs 558

In both cases, when the WriteLine () function is called, it receives less actual arguments than it needs. As a result, FormatException should be thrown.

Error N16. Dangerous expression.

I am sure that now you look at the following code fragment and do not want to understand it. This shows well how tireless code analyzers are useful.

privatestaticboolSymbolsAreCompatibleCore(....){
  ....
  var type = methodSymbol.ContainingType;
  var newType = newMethodSymbol.ContainingType;
  if ((type != null && type.IsEnumType() &&
       type.EnumUnderlyingType != null &&
       type.EnumUnderlyingType.SpecialType ==
         newType.SpecialType) ||
      (newType != null && newType.IsEnumType() &&
       newType.EnumUnderlyingType != null &&
       newType.EnumUnderlyingType.SpecialType ==
         type.SpecialType))
  {
    returntrue;
  }
  ....
}

PVS-Studio Warning: V3027 The variable 'newType' was utilized in the logical expression before it was verified against null in the same logical expression. AbstractSpeculationAnalyzer.cs 383

To clarify the danger of the code, I will make a simple synthetic example based on it:

if ((A != null && A.x == B.y) || (B != null && B.q == A.w))

As you can see, it is understood that A and B can be null references. The expression consists of two parts. In the first part, link A is checked, but link B is not checked. In the second part, link B is checked, but link A is not checked.

Maybe the code works thanks to luck, but it looks very suspicious and dangerous.

Errors N17, N18. Repeated assignments.

publicstaticstringStringize(this Diagnostic e){
  var retVal = string.Empty;
  if (e.Location.IsInSource)
  {
    retVal = e.Location.SourceSpan.ToString() + ": ";
  }
  elseif (e.Location.IsInMetadata)
  {
    return"metadata: ";
  }
  else
  {
    return"no location: ";
  }
  retVal = e.Severity.ToString() + " " + e.Id + ": " +
           e.GetMessage(CultureInfo.CurrentCulture);
  return retVal;
}

PVS-Studio Warning: V3008 The 'retVal' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 324, 313. DiagnosticExtensions.cs 324

Note that in one of the branches of the 'if' statement, the value 'retVal' is assigned. However, at the end of the function, the value of the variable 'retVal' is overwritten. I'm not sure, but maybe the last assignment should be like this:

retVal = retVal  + e.Severity.ToString() + " " + e.Id + ": " +
         e.GetMessage(CultureInfo.CurrentCulture);

Consider another similar case:

publicintGetMethodsInDocument(
  ISymUnmanagedDocument document,
  int bufferLength, 
  out int count,
  ....){
  ....
  if (bufferLength > 0)
  {
    ....
    count = actualCount;
  }
  else
  {
    count = extentsByMethod.Length;
  }
  count = 0;
  return HResult.S_OK;
}

PVS-Studio Warning: V3008 The 'count' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 317, 314. SymReader.cs 317

The function returns the value by reference to 'count'. In different parts of the function, different values ​​are written to 'count'. It is suspicious that at the end of the function, 0. is always always written to 'count'. Very strange.

Error N19 in tests. Typo.

internal voidVerifySemantics(....){
  ....
  if (additionalOldSources != null)
  {
    oldTrees = oldTrees.Concat(
      additionalOldSources.Select(s => ParseText(s)));
  }
  if (additionalOldSources != null)
  {
    newTrees = newTrees.Concat(
      additionalNewSources.Select(s => ParseText(s)));
  }
  ....
}

PVS-Studio Warning: V3029 The conditional expressions of the 'if' operators located alongside each other are identical. Check lines: 223, 228. EditAndContinueTestHelpers.cs 223

In the second condition, it was necessary to check not 'additionalOldSources', but 'additionalNewSources'. If the link 'additionalNewSources' suddenly turns out to be null, an exception will be thrown when trying to call the Select () function.

Error N20. From the controversial.

Naturally, in the article I did not analyze all the warnings that the PVS-Studio analyzer issued. There are many obviously false positives, but even more situations where I simply do not have knowledge about the Roslyn project to judge whether the code contains an error or not. Consider one of these cases.

publicstatic SyntaxTrivia Whitespace(string text){
  return Syntax.InternalSyntax.SyntaxFactory.Whitespace(
           text, elastic: false);
}
publicstatic SyntaxTrivia ElasticWhitespace(string text){
  return Syntax.InternalSyntax.SyntaxFactory.Whitespace(
           text, elastic: false);
}

V3013 It is odd that the body of 'Whitespace' function is fully equivalent to the body of 'ElasticWhitespace' function (118, line 129). SyntaxFactory.cs 118

Two functions have identical bodies. This is suspicious from the point of view of the analyzer. These functions are suspicious to me. But I do not know the project, and perhaps the code is written correctly. Therefore, I will only make an assumption. Perhaps inside the ElasticWhitespace () function, you should use the actual 'elastic' parameter equal to 'true'.

Error Nxx.

I hope readers understand that I can’t deal with each such case in detail, as was shown above. I check a lot of projects and each of them is not familiar to me. Therefore, I describe in articles only the most obvious mistakes. In this article, I focused on 20 such cases. However, I think PVS-Studio was able to detect much more defects. Therefore, I suggest that Roslyn developers do not rely solely on this article, but check the project on their own. The analyzer will not be enough for this demo version, but we are ready to provide a license key for a while.

Comparison with ReSharper

I wrote quite a few articles about checking C # projects and spoke at only one conference. But I already realized that each time a question will be asked: “Do you have a comparison with ReSharper?”.

I do not like this question for two reasons. Firstly, these are still different types of tools. PVS-Studio is a classic code analyzer focused on finding bugs. ReSharper is a Productivity Tool designed to facilitate programming and able to provide a large number of recommendations.

PVS-Studio and ReSharper have completely different approaches on many issues. See, for example, PVS-Studio has documentation with a detailed description of each diagnostics, examples and tips for fixing the code. In case of ReSharper there is a statementabout "1400 code inspections in C #, VB.NET, XAML, XML, ASP.NET, ASP.NET MVC, Razor, JavaScript, TypeScript, HTML, CSS, ResX." The number 1400 looks solid, but actually does not mean anything. Perhaps somewhere there is a description of all these code inspection, but I could not find it right away. How can I compare a tool if I can’t even read about exactly what errors ReSharper detects in C # programs.

Secondly, any comparison we make will be completely criticized. We have already gone through this and have committed to continue to make any comparisons. For example, we carefully approached PVS-Studio with Cppcheck and Visual Studio SCA. A lot of time and effort was spent. The results were presented in brief and detailed.option. After which only the lazy did not write that we did everything wrong, or that our comparison is unfair due to specially selected projects for verification.

We don’t see the point of wasting time comparing. No matter how carefully and honestly we approach him, one can always say that the comparison was biased.

Nevertheless, there should be an answer whether we are better at something than ReSharper. And I have such an answer.

Are you already using ReSharper? Fine! Now install PVS-Studio and find errors in your project!

Conclusion

I suggest not delaying the download of PVS-Studio and testing your project. See the mistakes? But you have quite working code. Most likely, the errors found are located in rarely used parts of the code. In frequently used sections of code, such errors, although slowly and painfully, were fixed. Imagine now how much PVS-Studio analyzer could save energy with regular use. Of course, the analyzer is not capable of finding all the errors. But than wasting time looking for typos and blunders, it is better to spend it with benefit. And give the silly mistakes to be torn to pieces by PVS-Studio.

PS You do not make silly mistakes? Oh well. It only seems so . They do everything. Look at least here .

If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. New Year PVS-Studio 6.00 Release: Scanning Roslyn .