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?
Since then, both the core and the infrastructure have been developing. We added support for Visual Studio 2017 and Roslyn 2.0, and a deep integration with MSBuild. The article by my colleague Paul Eremeev «Support of Visual Studio 2017 and Roslyn 2.0 in PVS-Studio: sometimes it’s not that easy to use ready-made solutions as it may seem» describes our approach to integration with MSBuild and reasons for this decision.
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.
Note: you can read about basic analyzer mechanisms, such as data-flow and others in the article «Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities».
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).
Possible null dereference — Medium
V3080 Possible null dereference. Consider inspecting 'current'. CSharpSyntaxTreeFactoryService.PositionalSyntaxReference.cs 70
private SyntaxNode GetNode(SyntaxNode root)
{
var current = root;
....
while (current.FullSpan.Contains(....)) // <=
{
....
var nodeOrToken = current.ChildThatContainsPosition(....);
....
current = nodeOrToken.AsNode(); // <=
}
....
}
public SyntaxNode AsNode()
{
if (_token != null)
{
return null;
}
return _nodeOrParent;
}
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.
V3080 Possible null dereference. Consider inspecting 'directory'. CommonCommandLineParser.cs 911
private IEnumerable
ExpandFileNamePattern(string path, string baseDirectory, ....)
{
string directory = PathUtilities.GetDirectoryName(path);
....
var resolvedDirectoryPath = (directory.Length == 0) ? // <=
baseDirectory :
FileUtilities.ResolveRelativePath(directory, baseDirectory);
....
}
public static string GetDirectoryName(string path)
{
return GetDirectoryName(path, IsUnixLikePlatform);
}
internal static string GetDirectoryName(string path, bool isUnixLike)
{
if (path != null)
{
....
}
return null;
}
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.
V3080 Possible null dereference. Consider inspecting 'spanStartLocation'. TestWorkspace.cs 574
V3080 Possible null dereference. Consider inspecting 'spanEndLocationExclusive'. TestWorkspace.cs 574
private void MapMarkupSpans(....)
{
....
foreach (....)
{
....
foreach (....)
{
....
int? spanStartLocation = null;
int? spanEndLocationExclusive = null;
foreach (....)
{
if (....)
{
if (spanStartLocation == null &&
positionInMarkup <= markupSpanStart && ....)
{
....
spanStartLocation = ....;
}
if (spanEndLocationExclusive == null &&
positionInMarkup <= markupSpanEndExclusive && ....)
{
....
spanEndLocationExclusive = ....;
break;
}
....
}
....
}
tempMappedMarkupSpans[key].
Add(new TextSpan(
spanStartLocation.Value, // <=
spanEndLocationExclusive.Value - // <=
spanStartLocation.Value));
}
}
....
}
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.
Possible null dereference — High
V3080 Possible null dereference. Consider inspecting 'collectionType.Type'. AbstractConvertForToForEachCodeRefactoringProvider.cs 137
public override async Task
ComputeRefactoringsAsync(CodeRefactoringContext context)
{
....
var collectionType = semanticModel.GetTypeInfo(....);
if (collectionType.Type == null &&
collectionType.Type.TypeKind == TypeKind.Error)
{
return;
}
....
}
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:
if (collectionType.Type == null ||
collectionType.Type.TypeKind == TypeKind.Error)
....
By the way, things may unfold in another way: in the first part of the condition the operators == and != are messed up. Then the correct code would look like this:
if (collectionType.Type != null &&
collectionType.Type.TypeKind == TypeKind.Error)
....
This version of the code is less logical, but also corrects the error. The final solution rests for project’s authors to decide.
Another similar error.
V3080 Possible null dereference. Consider inspecting 'action'. TextViewWindow_InProc.cs 372
private Func GetLightBulbApplicationAction(....)
{
....
if (action == null)
{
throw new
InvalidOperationException(
$"Unable to find FixAll in {fixAllScope.ToString()}
code fix for suggested action '{action.DisplayText}'.");
}
....
}
The error is made when generating the message for the exception. It is followed by the attempt to access the action.DisplayText property via the action variable, which is known to be null.
Here comes the last V3080 error of the High level.
V3080 Possible null dereference. Consider inspecting 'type'. ObjectFormatterHelpers.cs 91
private static bool IsApplicableAttribute(
TypeInfo type,
TypeInfo targetType,
string targetTypeName)
{
return type != null && AreEquivalent(targetType, type)
|| targetTypeName != null && type.FullName == targetTypeName;
}
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):
return (type != null && AreEquivalent(targetType, type))
|| (targetTypeName != null && type.FullName == targetTypeName);
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:
return type != null &&
(AreEquivalent(targetType, type) ||
targetTypeName != null && type.FullName == targetTypeName);
I think, it’s enough for reviewing V3080 errors. Now it’s high time to see other interesting stuff that the PVS-Studio analyzer managed to find.
Typo
V3005 The 'SourceCodeKind' variable is assigned to itself. DynamicFileInfo.cs 17
internal sealed class DynamicFileInfo
{
....
public DynamicFileInfo(
string filePath,
SourceCodeKind sourceCodeKind,
TextLoader textLoader,
IDocumentServiceProvider documentServiceProvider)
{
FilePath = filePath;
SourceCodeKind = SourceCodeKind; // <=
TextLoader = textLoader;
DocumentServiceProvider = documentServiceProvider;
}
....
}
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:
public DynamicFileInfo(
string _filePath,
SourceCodeKind _sourceCodeKind,
TextLoader _textLoader,
IDocumentServiceProvider _documentServiceProvider)
{
FilePath = _filePath;
SourceCodeKind = _sourceCodeKind;
TextLoader = _textLoader;
DocumentServiceProvider = _documentServiceProvider;
}
Inadvertence
V3006 The object was created but it is not being used. The 'throw' keyword could be missing: throw new InvalidOperationException (FOO). ProjectBuildManager.cs 61
~ProjectBuildManager()
{
if (_batchBuildStarted)
{
new InvalidOperationException("ProjectBuilderManager.Stop()
not called.");
}
}
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:
~ProjectBuildManager()
{
if (_batchBuildStarted)
{
throw new InvalidOperationException("ProjectBuilderManager.Stop()
not called.");
}
}
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
internal bool TryExecuteCommand(....)
{
....
using (context.OperationContext.AddScope(....))
{
if (....)
{
return true;
}
}
....
return true;
}
The method TryExecuteCommand returns nothing but true. In doing so, in the calling code the returned value is involved in some checks.
public bool ExecuteCommand(....)
{
....
if (caretPos.HasValue && TryExecuteCommand(....))
{
....
}
....
}
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
Checked the wrong thing
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'value', 'valueToSerialize'. RoamingVisualStudioProfileOptionPersister.cs 277
public bool TryPersist(OptionKey optionKey, object value)
{
....
var valueToSerialize = value as NamingStylePreferences;
if (value != null)
{
value = valueToSerialize.CreateXElement().ToString();
}
....
}
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:
var valueToSerialize = value as NamingStylePreferences;
if (valueToSerialize != null)
{
value = valueToSerialize.CreateXElement().ToString();
}
Another similar bug:
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'columnState', 'columnState2'. StreamingFindUsagesPresenter.cs 181
private void SetDefinitionGroupingPriority(....)
{
....
foreach (var columnState in ....)
{
var columnState2 = columnState as ColumnState2;
if (columnState?.Name == // <=
StandardTableColumnDefinitions2.Definition)
{
newColumns.Add(new ColumnState2(
columnState2.Name, // <=
....));
}
....
}
....
}
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.
V3022 Expression 'navInfo == null' is always false. AbstractSyncClassViewCommandHandler.cs 101
public bool ExecuteCommand(....)
{
....
IVsNavInfo navInfo = null;
if (symbol != null)
{
navInfo = libraryService.NavInfoFactory.CreateForSymbol(....);
}
if (navInfo == null)
{
navInfo = libraryService.NavInfoFactory.CreateForProject(....);
}
if (navInfo == null) // <=
{
return true;
}
....
}
public IVsNavInfo CreateForSymbol(....)
{
....
return null;
}
public IVsNavInfo CreateForProject(....)
{
return new NavInfo(....);
}
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
private BoundExpression GetReceiverForConditionalBinding(
ExpressionSyntax binding,
DiagnosticBag diagnostics)
{
....
BoundExpression receiver = this.ConditionalReceiverExpression;
if (receiver?.Syntax != // <=
GetConditionalReceiverSyntax(conditionalAccessNode))
{
receiver = BindConditionalAccessReceiver(conditionalAccessNode,
diagnostics);
}
var receiverType = receiver.Type; // <=
if (receiverType?.IsNullableType() == true)
{
....
}
receiver = new BoundConditionalReceiver(receiver.Syntax, 0, // <=
receiverType ?? CreateErrorType(),
hasErrors: receiver.HasErrors) // <=
{ WasCompilerGenerated = true };
return receiver;
}
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:
private BoundExpression GetReceiverForConditionalBinding(
ExpressionSyntax binding,
DiagnosticBag diagnostics)
{
....
BoundExpression receiver = this.ConditionalReceiverExpression;
if (receiver?.Syntax !=
GetConditionalReceiverSyntax(conditionalAccessNode))
{
receiver = BindConditionalAccessReceiver(conditionalAccessNode,
diagnostics);
}
var receiverType = receiver?.Type;
if (receiverType?.IsNullableType() == true)
{
....
}
receiver = new BoundConditionalReceiver(receiver?.Syntax, 0,
receiverType ?? CreateErrorType(),
hasErrors: receiver?.HasErrors)
{ WasCompilerGenerated = true };
return receiver;
}
We also have to be sure that the constructor supports getting null values for its parameters or we need to perform additional refactoring.
Other similar errors:
- V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'containingType' object SyntaxGeneratorExtensions_Negate.cs 240
- V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions.cs 349
- V3042 Possible NullReferenceException. The '?.' and '.' operators are used for accessing members of the 'expression' object ExpressionSyntaxExtensions.cs 349
Error in the condition
V3057 The 'Substring' function could receive the '-1' value while non-negative value is expected. Inspect the second argument. CommonCommandLineParser.cs 109
internal static bool TryParseOption(....)
{
....
if (colon >= 0)
{
name = arg.Substring(1, colon - 1);
value = arg.Substring(colon + 1);
}
....
}
In case if the colon variable is 0, which is fine according to the condition in the code, the Substring method will throw an exception. This has to be fixed:
if (colon > 0)
Possible typo
V3065 Parameter 't2' is not utilized inside method’s body. CSharpCodeGenerationHelpers.cs 84
private static TypeDeclarationSyntax
ReplaceUnterminatedConstructs(....)
{
....
var updatedToken = lastToken.ReplaceTrivia(lastToken.TrailingTrivia,
(t1, t2) =>
{
if (t1.Kind() == SyntaxKind.MultiLineCommentTrivia)
{
var text = t1.ToString();
....
}
else if (t1.Kind() == SyntaxKind.SkippedTokensTrivia)
{
return ReplaceUnterminatedConstructs(t1);
}
return t1;
});
....
}
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.
Inadvertence
V3083 Unsafe invocation of event 'TagsChanged', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. PreviewUpdater.Tagger.cs 37
public void OnTextBufferChanged()
{
if (PreviewUpdater.SpanToShow != default)
{
if (TagsChanged != null)
{
var span = _textBuffer.CurrentSnapshot.GetFullSpan();
TagsChanged(this, new SnapshotSpanEventArgs(span)); // <=
}
}
}
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:
private void OnTrackingSpansChanged(bool leafChanged)
{
var handler = TagsChanged;
if (handler != null)
{
var snapshot = _buffer.CurrentSnapshot;
handler(this,
new SnapshotSpanEventArgs(snapshot.GetFullSpan()));
}
}
Usage of an additional handler variable prevents the problem. In the method OnTextBufferChanged, one has to make edits in order to safely handle the event.
Intersecting ranges
V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. ILBuilderEmit.cs 677
internal void EmitLongConstant(long value)
{
if (value >= int.MinValue && value <= int.MaxValue)
{
....
}
else if (value >= uint.MinValue && value <= uint.MaxValue)
{
....
}
else
{
....
}
}
For better understanding, let me rewrite this code, changing the names of the constants with their actual values:
internal void EmitLongConstant(long value)
{
if (value >= -2147483648 && value <= 2147483648)
{
....
}
else if (value >= 0 && value <= 4294967295)
{
....
}
else
{
....
}
}
Probably, there is no real error, but the condition looks strange. Its second part (else if) will be executed only for the range from 2147483648 + 1 to 4294967295.
Another couple of similar warnings:
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. LocalRewriter_Literal.cs 109
- V3092 Range intersections are possible within conditional expressions. Example: if (A > 0 && A < 5) {… } else if (A > 3 && A < 9) {… }. LocalRewriter_Literal.cs 66
More about checks for null (or lack of them)
A couple of V3095 errors on the check of a variable for null right after its usage. The first is ambiguous, let’s consider the code.
V3095 The 'displayName' object was used before it was verified against null. Check lines: 498, 503. FusionAssemblyIdentity.cs 498
internal static IAssemblyName ToAssemblyNameObject(string displayName)
{
if (displayName.IndexOf('\0') >= 0)
{
return null;
}
Debug.Assert(displayName != null);
....
}
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
internal void FlattenArgs(...., List scriptArgsOpt, ....)
{
....
while (args.Count > 0)
{
....
if (parsingScriptArgs)
{
scriptArgsOpt.Add(arg); // <=
continue;
}
if (scriptArgsOpt != null)
{
....
}
....
}
}
I think this code doesn’t need any explanations. Let me give you the fixed version:
internal void FlattenArgs(...., List scriptArgsOpt, ....)
{
....
while (args.Count > 0)
{
....
if (parsingScriptArgs)
{
scriptArgsOpt?.Add(arg);
continue;
}
if (scriptArgsOpt != null)
{
....
}
....
}
}
In Roslyn code, there were 15 more similar errors:
- V3095 The 'LocalFunctions' object was used before it was verified against null. Check lines: 289, 317. ControlFlowGraphBuilder.RegionBuilder.cs 289
- V3095 The 'resolution.OverloadResolutionResult' object was used before it was verified against null. Check lines: 579, 588. Binder_Invocation.cs 579
- V3095 The 'resolution.MethodGroup' object was used before it was verified against null. Check lines: 592, 621. Binder_Invocation.cs 592
- V3095 The 'touchedFilesLogger' object was used before it was verified against null. Check lines: 111, 126. CSharpCompiler.cs 111
- V3095 The 'newExceptionRegionsOpt' object was used before it was verified against null. Check lines: 736, 743. AbstractEditAndContinueAnalyzer.cs 736
- V3095 The 'symbol' object was used before it was verified against null. Check lines: 422, 427. AbstractGenerateConstructorService.Editor.cs 422
- V3095 The '_state.BaseTypeOrInterfaceOpt' object was used before it was verified against null. Check lines: 132, 140. AbstractGenerateTypeService.GenerateNamedType.cs 132
- V3095 The 'element' object was used before it was verified against null. Check lines: 232, 233. ProjectUtil.cs 232
- V3095 The 'languages' object was used before it was verified against null. Check lines: 22, 28. ExportCodeCleanupProvider.cs 22
- V3095 The 'memberType' object was used before it was verified against null. Check lines: 183, 184. SyntaxGeneratorExtensions_CreateGetHashCodeMethod.cs 183
- V3095 The 'validTypeDeclarations' object was used before it was verified against null. Check lines: 223, 228. SyntaxTreeExtensions.cs 223
- V3095 The 'text' object was used before it was verified against null. Check lines: 376, 385. MSBuildWorkspace.cs 376
- V3095 The 'nameOrMemberAccessExpression' object was used before it was verified against null. Check lines: 206, 223. CSharpGenerateTypeService.cs 206
- V3095 The 'simpleName' object was used before it was verified against null. Check lines: 83, 85. CSharpGenerateMethodService.cs 83
- V3095 The 'option' object was used before it was verified against null. Check lines: 23, 28. OptionKey.cs 23
Let’s consider V3105 errors. Here the conditional null operator is used when initializing the variable, but further the variable is used without checks for null.
Two warnings indicate the following error:
V3105 The 'documentId' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. CodeLensReferencesService.cs 138
V3105 The 'documentId' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. CodeLensReferencesService.cs 139
private static async Task
GetDescriptorOfEnclosingSymbolAsync(....)
{
....
var documentId = solution.GetDocument(location.SourceTree)?.Id;
return new ReferenceLocationDescriptor(
....
documentId.ProjectId.Id,
documentId.Id,
....);
}
The documentId variable can be initialized by null. As a result, creating an object ReferenceLocationDescriptor will result in throwing an exception. The code must be fixed:
return new ReferenceLocationDescriptor(
....
documentId?.ProjectId.Id,
documentId?.Id,
....);
Developers should also cover the possibility of variables, passed to a constructor, to be null.
Other similar errors in the code:
- V3105 The 'symbol' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. SymbolFinder_Hierarchy.cs 44
- V3105 The 'symbol' variable was used after it was assigned through null-conditional operator. NullReferenceException is possible. SymbolFinder_Hierarchy.cs 51
Priorities and parentheses
V3123 Perhaps the '?:' operator works in a different way than it was expected. Its priority is lower than the priority of other operators in its condition. Edit.cs 70
public bool Equals(Edit other)
{
return _kind == other._kind
&& (_oldNode == null) ? other._oldNode == null :
_oldNode.Equals(other._oldNode)
&& (_newNode == null) ? other._newNode == null :
_newNode.Equals(other._newNode);
}
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:
return _kind == other._kind
&& ((_oldNode == null) ? other._oldNode == null :
_oldNode.Equals(other._oldNode))
&& ((_newNode == null) ? other._newNode == null :
_newNode.Equals(other._newNode));
That concludes my description of the errors found.
Conclusion
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.
