New Year's release of PVS-Studio 6.00: check Roslyn

It is a long-awaited event. We have released the release version of the PVS-Studio 6.00 static code analyzer, which supports the verification of C # projects. Now we check the code written in the following languages: C, C ++, C ++ / CLI, C ++ / CX, C #. By the release of the sixth version of the analyzer, we timed the test of the open Roslyn project. Thanks to Roslyn, 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 searching 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 quite a daunting task . However, the improvement of diagnostics is not a reason for changing the older version. And we waited to add something truly new to the analyzer. And this moment has come. We present to the world the sixth version of the analyzer that supports the C # language.

The trial version of PVS-Studio 6.00 is available at the link:
')
http://www.viva64.com/en/pvs-studio-download/

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

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

Limiting the demo version 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 . If someone wants to look at other messages, then we are ready to give him a registration key, say, for 1 week. He just needs to write us a letter. As a rule, in the process of correspondence, we help a person to quickly orient himself, as he can quickly and easily benefit from the use of the analyzer.

Roslyn

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

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

PVS-Studio is able to 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 checked 230 open projects and found 9355 errors in them. 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 checking C # projects. Not surprisingly, Roslyn was one of the first proven projects. In the end, thanks to this project, it was possible to support the analysis of C # code in PVS-Studio.

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

Now a little about the project Roslyn.

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

This platform includes self-sufficient 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 allows access to the modules for syntactic (lexical) code analysis, semantic analysis, dynamic compilation into CIL bytecode, and assembly generation. The Roslyn API provided can be divided into three types: functional API (feature API), workspace API (API) and compiler API (compiler API). Functional APIs make refactoring and debugging easier. The workspace APIs allow plug-in developers to implement certain mechanisms specific to the IDE like Visual Studio — for example, finding the correspondence between variables and values ​​or formatting code. The compiler APIs provide opportunities for even more advanced source code analysis, providing information on direct calls to the syntax tree analysis modules and control flow analysis at the stage of associating identifiers with values.

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 .

Bugs found

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

Many of the errors below refer to tests or error handlers. It `s naturally. Errors in frequently used code fragments are corrected due to testing and feedback from users. But the fact that PVS-Studio can find errors is important for us. This means that many errors that were corrected with great effort could be corrected immediately with regular use of PVS-Studio.

Once again, in other words. The value of the analyzer is not in one-time runs, but in its regular use. Static code analysis can be viewed as an advanced version of the compiler warnings. It is foolish to turn on compiler warnings once a year. Warnings should be watched immediately after they occur. This saves development time on finding stupid bugs with debugging. With the static analyzer everything is the same.

Let's see what interesting things were discovered with PVS-Studio:

Error N1 in tests. Copy-Paste.

public void IndexerMemberRace() { .... 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 an error in tests that can live in a project for years, as it does not bother anyone. Just the test does not check everything that was planned. In 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 tests. A typo.

 public DiagnosticAsyncToken( 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

At a glance, an error here is very difficult to find. Failed bad variable naming. The class property names differ from the function arguments only by the first capital letter. As a result, it's 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); } 

PVS-Studio warning : V3012 The '?:' Operator, regardless of the conditional expression. AbstractSuppressionCodeFixProvider.RemoveSuppressionCodeAction_Pragma.cs 177

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

Here is another similar case:

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

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

Errors N5, N6. Inattention.

I still have few statistics on typical errors that C # programmers allow. But besides the standard typos, the following error pattern clearly begins to lead: often after bringing the link using the 'as' operator, the link is not obtained, but the original one. Then use the unverified link. Synthetic code:

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

And this is how this error looks in practice:

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

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 reality everything is the same:

 protected override bool AreEqual(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. Dual detection.

Sometimes there are 2 or even 3 warnings of the analyzer that indicate an error in the code. Now we just consider this case.

 private bool HasMatchingEndTag( XmlElementStartTagSyntax parentStartTag) { if (parentStartTag == null) { return false; } var parentElement = parentStartTag.Parent as XmlElementSyntax; if (parentStartTag == null) { return false; } var endTag = parentElement.EndTag; .... } 

PVS-Studio warnings:

• V3019 Possibly incorrectly variable conversion 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 statement is a 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 we wanted to check that the link actually points to a class like 'XmlElementSyntax'. But at this moment there was a typo in the code. Instead of checking the 'parentElement', the parentStartTag is re-checked.

The analyzer notices two anomalies at once. First anomaly: it makes no sense to re-check the value of 'parentStartTag', since if this is a zero reference, then we have already left the function. The second anomaly: the analyzer suspects that the wrong variable is checked after the 'as' operator.

The correct code is:

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

Errors N8, N9. Copy-Paste.

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

 internal static bool ReportConflictWithParameter(....) { .... if (newSymbolKind == SymbolKind.Parameter || newSymbolKind == SymbolKind.Local) { diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam, newLocation, name); return true; } if (newSymbolKind == SymbolKind.TypeParameter) { return false; } if (newSymbolKind == SymbolKind.Parameter || newSymbolKind == SymbolKind.Local) { diagnostics.Add(ErrorCode.ERR_LocalSameNameAsTypeParam, newLocation, name); return true; } .... } 

PVS-Studio warning : V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. If this statement is a senseless InMethodBinder.cs 264

In the code snippet given in the article, the first and third if statements are the same. 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 one more similar code section, 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. If 'statement is senseless WithLambdaParametersBinder.cs 131

Error N10. Invalid condition.

 public enum 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 complicated, so I will highlight the main essence:

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

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

Error N11. Excess condition.

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

PVS-Studio warning : V3023 Consider inspecting this expression. The expression is misprint. RoslynEventSource.cs 79

I will once again highlight the essence:

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

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

 if (A != -3) 

Error N12 when logging.

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

PVS-Studio warning : V3025 Incorrect format. A different number of formatted 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.

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

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

Apparently, when you try to show the Message Box, an exception will be generated. The point is that the Format () function will try to print the second additional argument. And it is not.

It seems to me that the constant string specifying formatting should start with:

 @"{0} 

Error N14, N15 in the error handler.

I’m willing to bet that the DumpAttributes () function is not being used yet. It has 2 errors at once, each of which should lead to an exception being thrown:

 private void DumpAttributes(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 items is expected while calling the 'WriteLine' function. Expected: 3. Present: 1. LoadingAttributes.cs 551
• V3025 Incorrect format. A different number of items is expected while calling the 'WriteLine' function. Expected: 4. Present: 2. LoadingAttributes.cs 558

In both cases, the call to the WriteLine () function is passed to it with fewer actual arguments than required. As a result, FormatException exceptions should occur.

Error N16. Dangerous expression.

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

 private static bool SymbolsAreCompatibleCore(....) { .... 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)) { return true; } .... } 

PVS-Studio Warning: V3027 The variable "newType" was used for the same logical expression. AbstractSpeculationAnalyzer.cs 383

To clarify the danger of the code, I will create on its basis a simple synthetic example:

 if ((A != null && Ax == By) || (B != null && Bq == Aw)) 

As you can see, it is implied that A and B can be null links. The expression consists of two parts. In the first part, reference A is verified, but reference B is not verified. In the second part, reference B is verified, but reference A is not verified.

Perhaps the code works because of luck, but it looks very suspicious and dangerous.

Errors N17, N18. Repeat assignments.

 public static string Stringize(this Diagnostic e) { var retVal = string.Empty; if (e.Location.IsInSource) { retVal = e.Location.SourceSpan.ToString() + ": "; } else if (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

Notice that in one of the branches of the 'if' operator, the variable 'retVal' is assigned a value. 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:

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

Consider another similar case:

 public int GetMethodsInDocument( 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 'count'. Different values ​​are written to 'count' in different parts of the function. Suspiciously, at the end of the function in the 'count', suddenly it is always written 0. Very strange.

Error N19 in tests. A typo.

 internal void VerifySemantics(....) { .... 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' are agreed 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 zero, then an exception will be thrown when you try to call the Select () function.

Error N20. Of controversial.

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

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

V3013 It is odd that the body of the "Whitespace" function is fully equivalent (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 too. But I do not know the project, and, probably, the code is written absolutely correctly. Therefore, I will only make a guess. Perhaps inside the ElasticWhitespace () function, use the actual 'elastic' parameter equal to 'true'.

Error nxx.

I hope the readers understand that I cannot understand in detail with each such case, as was shown above. I check many projects and I don’t know each of them. Therefore, I describe in the articles only the most obvious mistakes. In this article, I stopped at 20 such cases. However, I think PVS-Studio was able to detect many more defects. Therefore, I suggest that Roslyn developers not only rely on this article, but check the project themselves. For this demo version of the analyzer will not be enough, but we are ready to provide a license key for the time being.

Comparison with ReSharper

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

I do not like this question for two reasons. First, it is still a different type of tools. PVS-Studio is a classic code analyzer focused on finding errors. ReSharper is a Productivity Tool designed to facilitate programming and able to give 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 diagnostic, examples, and tips for correcting the code. In the case of ReSharper there is a statement about “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 in fact does not say anything. Perhaps somewhere and there is a description of all these code inspection, but immediately I could not find it. How can I compare a tool if I can't even read about exactly what errors ReSharper finds in C # programs.

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

We do not see the point of spending time on comparison. No matter how carefully and honestly we approached him, one can always say that the comparison was biased.

Nevertheless, the answer must be whether we are better at something than ReSharper. And I have that answer.

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

Conclusion

I offer to immediately download PVS-Studio and test your project. See the mistakes? But you have quite a working code. Most likely the errors found are located in rarely used parts of the code. In frequently used parts of the code, such errors, though slow and painful, have been fixed. Imagine now how much PVS-Studio analyzer could save with regular use. Of course, the analyzer is able to find far from all errors. But than to spend time looking for typos and blunders, it is better to spend it with benefit. And give the stupid mistakes to the mercy of PVS-Studio.

PS You do not make stupid mistakes? Oh well. It only seems that way . Everybody does. 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 .