As part of the development process of SonarLint for Visual Studio we regularly check a couple of open source projects, such as Roslyn, to filter out false positives and to validate our rule implementations. In this post we’ll highlight a couple of issues found recently in Roslyn project.

Short-circuit logic should be used to prevent null pointer dereferences in conditionals (S1697)

This rule recognizes a few very specific patterns in your code. We don’t expect any false positives from it, so whenever it reports an issue, we know that it found a bug. Check it out for yourself; here is the link to the problem line.

When body is null, the second part of the condition will be evaluated and throw a NullReferenceException. You might think that the body of a method can’t be null, but even in syntactically correct code it is possible. For example method declarations in interfaces, abstract or partial methods, and expression bodied methods or properties all have null bodies. So why hasn’t this bug shown up yet? This code is only called in one place, on a method declaration with a body.

The ternary operator should not return the same value regardless of the condition (S2758)

We’re not sure if this issue is a bug or just the result of some refactoring, but it is certainly confusing. Why would you check isStartToken if you don’t care about its content?

 “IDisposables” should be disposed (S2930)

Lately we’ve spent some effort on removing false positives from this rule. For example, we’re not reporting on MemoryStream uses anymore, even though it is an IDisposable. SonarLint only reports on resources that should really be closed, which gives us high confidence in this rule. Three issues ([1], [2], [3]) are found on the Roslyn project, where a FileStream, a TcpClient, and a TcpListener are not being disposed.

Method overloads with default parameter values should not overlap (S3427)

Mixing method overloads and default parameter values can result in cases when the default parameter value can’t be used at all, or can only be used in conjunction with named arguments. These three cases ([1], [2], [3]) fall into the former category, the default parameter values can’t be used at all, so it is perfectly safe to remove them. In each case, whenever only the first two arguments are supplied, another constructor will be called. Additionally, in this special case, if you call the method like IsEquivalentTo(node: myNode), then the default parameter value is used, but if you use IsEquivalentTo(myNode), then another overload is being called. Confusing, isn’t it?

Flags enumerations should explicitly initialize all their members (S2345)

It is good practice to explicitly set a value for your [Flags] enums. It’s not strictly necessary, and your code might function correctly without it, but still, it’s better safe than sorry. If the enum has only three members, then the automatic 0, 1, 2 field initialization works correctly, but when you have more members, you most probably don’t want to use the default values. For example here FromReferencedAssembly == FromSourceModule | FromAddedModule. Is this the desired setup? If so, why not add it explicitly to avoid confusion?

“async” methods should not return “void” (S3168)

As you probably know, async void methods should only be used in a very limited number of scenarios. The reason for this is that you can’t await on async void method calls. Basically, these are fire and forget methods, such as event handlers. So what happens when a test method is marked async void? Well, it depends. It depends on your test execution framework. For example NUnit 2.6.3 handles them correctly, but the newer NUnit 3.0 dropped support. Roslyn uses xUnit 2.1.0 at the moment, which does support running async void test methods, so there is no real issue with them right now. But changing the return value to Task would probably be advisable. To sum up, double check your async void methods; they might or might not work as you expect. Here are two occurrences from Roslyn ([1], [2]).

Additionally, here are some other confusing pieces of code that are marked by SonarLint. Rule S2275 (Format strings should be passed the correct number of arguments) triggers on this call, where the formatting arguments 10 and 100 are not used, because there are no placeholders for them in the format string. Finally, here are three cases ([1], [2], [3]) where values are bitwise OR-ed (|) with 0 (Rule S2437).

We sincerely hope you already use SonarLint daily to catch issues early. If not, you can download SonarLint from the Visual Studio Extension Gallery or install it directly from Visual Studio (Tools/Extensions and Updates). SonarLint is free and already trusted by thousands of developers, so start using it today!