Checking the Roslyn Source Code

Once in a while we go back to the projects that we have previously checked using PVS-Studio, which results in their descriptions in various articles. Two reasons make these comebacks exciting for us. Firstly, the opportunity to assess the progress of our analyzer. Secondly, monitoring the feedback of the project's authors to our article and the report of errors, which we usually provide them with. Of course, errors can be corrected without our participation. However, it is always nice when our efforts help to make a project better. Roslyn was no exception. The previous article about this project check dates back to December 23, 2015. It's quite a long time, in the view of the progress that our analyzer has made since that time. Since the C# core of the PVS-Studio analyzer is based on Roslyn, it gives us additional interest in this project. As a result, we're as keen as mustard about the code quality of this project. Now let's test it once again and find out some new and interesting issues (but let's hope that nothing significant) that PVS-Studio will be able to find.
Many of our readers are likely to be well aware of Roslyn (or .NET Compiler Platform). In short, it is a set of open source compilers and an API for code analysis of C# and Visual Basic .NET languages from Microsoft. The source code of the project is available on GitHub.

I won't give a detailed description of this platform and I'll recommend checking out the article by my colleague Sergey Vasiliev "Introduction to Roslyn and its use in program development" to all interested readers. From this article, you can find out not only about the features of the Roslyn's architecture, but how exactly we use this platform.

As I mentioned earlier, it has been more than 3 years since my colleague Andrey Karpov wrote the last article about the Roslyn check "New Year PVS-Studio 6.00 Release: Scanning Roslyn". Since then the C# PVS-Studio analyzer had got many new features. Actually, Andrey's article was a test case, as at that time the C# analyzer was just added in PVS-Studio. Despite this, we managed to detect errors in the Roslyn project, which was certainly of high-quality. So what has changed in the analyzer for C# code by this moment that will allow us to perform a more in-depth analysis?

At the moment we are actively working on moving to Roslyn 3.0 in the same way as we initially supported Visual Studio 2017. It requires usage of our own toolset, included in the PVS-Studio distributive as a «stub», which is an empty MSBuild.exe file. Despite the fact that it looks like a «crutch» (MSBuild API is not very friendly for reusing in third-party projects due to low libraries portability), such approach has already helped us to relatively seamlessly overcome multiple Roslyn updates in terms of Visual Studio 2017. Until now it was helping (even with some challenges) to pass through the Visual Studio 2019 update and maintain full backward compatibility and performance for systems with older MSBuild versions.

The analyzer core has also undergone a number of improvements. One of the main features is a complete interprocedural analysis with consideration of input and output methods' values, evaluating (depending on these parameters) reachability of the execution branches and return points.

We are on our way to completing the task of monitoring parameters inside the methods (for example, potentially dangerous dereferences) along with saving their auto-annotations. For a diagnostic that uses data-flow mechanism, this will allow taking into account dangerous situations, occurring when passing a parameter in a method. Before this, when analyzing such dangerous places, a warning wasn't generated, as we couldn't know about all possible input values in such a method. Now we can detect danger, as in all the places of calling this method, these input parameters will be taken into account.

Interprocedural analysis in PVS-Studio C# is limited neither by input parameters, nor the depth. The only limitation is virtual methods in classes, open for inheritance, as well as getting into recursion (the analysis stops when it stumbles upon a repeated call of the already evaluated method). In doing so, the recursive method itself will be eventually evaluated assuming that the return value of its recursion is unknown.

Another great new feature in the C# analyzer has become taking into account possible dereference of a potentially null pointer. Before that, the analyzer complained of a possible null reference exception, being ensured that in all execution branches the variable value will be null. Of course, it was wrong in some cases, that's why the V3080diagnostic had been previously called potential null reference.

Now the analyzer is aware of the fact that the variable could be null in one of the execution branches (for example, under a certain if condition). If it notices access to such a variable without a check, it will issue the V3080 warning, but at a lower level of certainty, than if it sees null in all branches. Along with the improved interprocedural analysis, such a mechanism allows finding errors that are very difficult to detect. Here is an example — imagine a long chain of method calls, the last of which is unfamiliar to you. Under certain circumstances, it returns null in the catch block, but you haven't protected from this, as you simple haven't known. In this case, the analyzer only complains, when it exactly sees null assignment. In our view, it qualitatively distinguishes our approach from such feature of C# 8.0 as nullable type reference, which, in fact, confines to setting checks for null for each method. However, we suggest the alternative — to perform checks only in places where null can really occur, and our analyzer can now search for such cases.

So, let's not delay the main point for too long and go to blame-storming — analyzing the results of the Roslyn check. First, let's consider the errors, found due to the features, described above. In sum, there were quite a lot of warnings for the Roslyn code this time. I think it is related to the fact that the platform is very actively evolving (at this point the codebase is about 2 770 000 lines excluding empty), and we haven't analyzed this project for long. Nevertheless, there are not so many critical errors, whereas they are of the most interest for the article. As usual, I excluded tests from the check, there are quite a lot of them in Roslyn.

I will start with V3080 errors of the Medium level of certainty, in which the analyzer has detected a possible access by null reference, but not in all possible cases (code branches).

Let's consider the method GetNode. The analyzer suggests the access by null reference is possible in the condition of the while block. The variable is assigned a value in the body of the while block, which is a result of the AsNode method. In some cases, this value will be null. A good example of interprocedural analysis in action.

Now let's consider a similar case, in which the interprocedural analysis was carried out via two method calls.

The directory variable in the body of the ExpandFileNamePattern method gets the value from the method GetDirectoryName(string). That, in turn, returns the result of the overloaded method GetDirectoryName (string, bool) whose value can be null. Since the variable directory is used without a preliminary check for null in the body of the method ExpandFileNamePattern — we can proclaim the analyzer correct about issuing the warning. This is a potentially unsafe construction.

Another code fragment with the V3080 error, more precisely, with two errors, issued for a single line of code. The interprocedural analysis wasn't needed here.

The variables spanStartLocation and spanEndLocationExclusive are of the nullable int type and are initialized by null. Further along the code they can be assigned values, but only under certain conditions. In some cases, their value remains null. After that, these variables are accessed by reference without preliminary check for null, which the analyzer indicates.

The Roslyn code contains quite a lot of such errors, more than 100. Often the pattern of these errors is the same. There is some kind of a general method, which potentially returns null. The result of this method is used in many places, sometimes through dozens of intermediate method calls or additional checks. It is important to understand that these errors are not fatal, but they can potentially lead to access by null reference. While detecting of such errors is quite challenging. That's why in some cases one should consider code refactoring, in which case if null returns, the method will throw an exception. Otherwise, you can secure your code only with general checks which is quite tiring and sometimes unreliable. Anyway, it's clear that each specific case requires a solution based on project specifications.

Note. It so happens, that at a given point there are no such cases (input data), when the method returns null and there is no actual error. However, such code is still not reliable, because everything can change when introducing some code changes.

In order to drop the V3080 subject, let's look at obvious errors of High certainty level, when the access by null reference is the likeliest or even inevitable.

Due to the typo in the condition (&& is used instead of the operator ||), the code works differently than intended and the access to the collectionType.Type variable will be executed when it is null. The condition should be corrected as follows:

The method is quite small, so I cite it entirely. The condition in the return block is incorrect. In some cases, when accessing type.FullName, an exception may occur. I'll use parentheses to make it clear (they won't change the behavior):

According to the operations precedence, the code will work exactly like this. In case if the type variable is null, we'll fall in the else-check, where we'll use the type null reference, having checked the variable targetTypeName for null. Code might be fixed, for example, as follows:

Due to failing variables naming, a typo was made in the constructor of the DynamicFileInfo class. The SourceCodeKind field is assigned its own value instead of using the parameter sourceCodeKind. To minimize the likelihood of such errors, we recommend that you use the underscore prefix to the parameter names in such cases. Here is an example of a corrected version of the code:

Under a certain condition, the destructor must throw an exception, but it's not happening while the exception object is simply created. The throw keyword was missed. Here is the corrected version of the code:

The issue with destructors in C# and throwing exceptions from them is a topic for another discussion, which is beyond the scope of this article.

When the result is not important

Methods, which received the same value in all cases, triggered a certain number of V3009 warnings. In some cases it can be not critical or the return value is simply not checked in the calling code. I skipped such warnings. But a few code snippets seemed suspicious. Here is one of them:

V3009 It's odd that this method always returns one and the same value of 'true'. GoToDefinitionCommandHandler.cs 62

It's hard to say exactly to what extent such behaviour is dangerous. But if the result is not needed, maybe the type of the return value should be changed to void and one should make small edits in the calling method. This will make the code more readable and secure.

Similar analyzer warnings:

V3009 It's odd that this method always returns one and the same value of 'true'. CommentUncommentSelectionCommandHandler.cs 86

V3009 It's odd that this method always returns one and the same value of 'true'. RenameTrackingTaggerProvider.RenameTrackingCommitter.cs 99

V3009 It's odd that this method always returns one and the same value of 'true'. JsonRpcClient.cs 138

V3009 It's odd that this method always returns one and the same value of 'true'. AbstractFormatEngine.OperationApplier.cs 164

V3009 It's odd that this method always returns one and the same value of 'false'. TriviaDataFactory.CodeShapeAnalyzer.cs 254

V3009 It's odd that this method always returns one and the same value of 'true'. ObjectList.cs 173

V3009 It's odd that this method always returns one and the same value of 'true'. ObjectList.cs 249

The value variable is cast to the type NamingStylePreferences. The problem is in the check that follows this. Even if the value variable was non null, it doesn't guarantee that type casting was successful and valueToSerialize doesn't contain null. Possible throwing of the exception NullReferenceException. The code needs to be corrected as follows:

The columnState variable is cast to the type ColumnState2. However, the operation result, which is the variable columnState2, is not checked for null further. Instead, the columnState variable is checked using the conditional null operator. Why is this code dangerous? Just like in the previous example, casting with the as operator may fail and the variable will be null which will result in an exception. By the way, a typo may be to blame here. Take a look at the condition in the if block.

Perhaps, instead of columnState?.Name the author wanted to write columnState2?.Name. It is very likely, considering rather faulty variable names columnState and columnState2.

Redundant checks

Quite a large number of warnings (more than 100) were issued on noncritical, but potentially unsafe constructions related to redundant checks. For example, this is one of them.

May be there is no actual bug here. It's just a good reason to demonstrate «interprocedural analysis + dataflow analysis» working in a tow. The analyzer suggests the second check navInfo == null is redundant. Indeed, before it the value assigned to navInfo will be obtained from the method libraryService.NavInfoFactory.CreateForProject, which will construct and return a new object of the NavInfo class. No way it will return null. Here the question arises, why didn't the analyzer issue a warning for the first check navInfo == null? There are some reasons. Firstly, if the symbol variable is null, the navInfo value will remain a null reference as well. Secondly, even if navInfo gets the value from the method ibraryService.NavInfoFactory.CreateForSymbol, this value can also be null. Thus, the first check navInfo == null is really needed.

Insufficient checks

Now the reverse situation from the discussed above. Several V3042 warnings were triggered for the code, in which access by null reference is possible. Even one or two small checks could have fixed everything.

Let's consider another interesting code fragment, which has two such errors.

V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'receiver' object Binder_Expressions.cs 7770

V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'receiver' object Binder_Expressions.cs 7776

The receiver variable may be null. The author of the code knows about this, as he uses the conditional null operator in the condition of the if block to access receiver?.Syntax. Further the receiver variable is used without any checks to access receiver.Type, receiver.Syntax and receiver.HasErrors. These errors must be corrected:

The lambda expression accepts two parameters: t1 and t2. However, only t1 is used. It looks suspicious, taking into account the fact how easy it is to make a mistake when using variables with such names.

The TagsChanged event is invoked in an unsafe way. Between checking for null and invoking the event, someone may unsubscribe from it, then an exception will be thrown. Furthermore, other operations are performed in the body of the if block right before invoking the event. I called this error «Inadvertence», because this event is handled more carefully in other places, as follows:

It is assumed that the reference displayName can be null. For this, the check Debug.Assert was performed. It is not clear why it goes after using a string. It also has to be taken into account that for configurations different from Debug, the compiler will remove Debug.Assert at all. Does it mean that getting a null reference is possible only for Debug? If it is not so, why did the author make the check of string.IsNullOrEmpty(string), for example. It is the question to authors of the code.

The following error is more evident.

V3095 The 'scriptArgsOpt' object was used before it was verified against null. Check lines: 321, 325. CommonCommandLineParser.cs 321

The condition in the return block is evaluated not as the developer intended. It was assumed that the first condition will be _kind == other._kind, (this is why after this condition there is a line break), and after that the blocks of conditions with the operator "?" will be evaluated in sequence. In fact, the first condition is _kind == other._kind && (_oldNode == null). This is due to the fact that the operator && has a higher priority than operator "?". To fix this, a developer should take all expressions of the operator "?" in parentheses:

Despite the large number of errors, which I managed to find, in terms of the size of the Roslyn project code (2 770 000 lines), it is not too much. As Andrey wrote in a previous article, I am also ready to acknowledge the high quality of this project.

I'd like to note that such occasional code checks have nothing to do with the methodology of static analysis and are almost unhelpful. Static analysis should be applied regularly, and not on a case-by-case basis. This way, many errors will be corrected at the earliest stages, and hence the cost of fixing them will be ten times less. This idea is set forth in more detail in this little note, please, check it out.

You can check yourself some errors both in this project and in another. To do this, you just need to download and try our analyzer.