PVS-Studio is a well-known static code analyzer that allows you to find a lot of tricky errors hidden in the source code. Beta testing of the new version has recently finished. It provides the possibility to analyze C# projects under Linux and macOS. The tool can also be integrated into the cross-platform IDE from JetBrains - Rider. This article will help you to get acquainted with these features using the example of checking the open source RavenDB project.
Some time ago, my colleague Sergey Vasiliev wrote a note that beta testing of a new version of the PVS-Studio static analyzer that we are developing had started. By this moment, the beta-testing has ended and you can download the new version by following the link. In this article, we'll consider the analysis of C# projects under Linux/macOS using the console interface and Rider. After that, we will traditionally review some interesting analyzer warnings.
I chose the RavenDB open source project for the check. Its repository contains almost 5 thousand source code files. It is a fairly popular NoSQL database. Details can be found on the website. No prizes for guessing why this project got my attention. It is its size that implies that in such a serious project there will definitely be something interesting.
First, let's look at how the analysis is performed via the console. This section, in my opinion, will be particularly interesting for those who want to integrate the analyzer into a CI system. The command running the analysis has a number of interesting options, but altogether everything is quite trivial. To analyze RavenDB, I go to the project folder and enter the following in the console:
pvs-studio-dotnet -t ./RavenDB.slnThe -t flag (short for target) is used to specify the solution or project file to check. The above line starts the analysis and generates a file containing the errors found. It's simple, isn't it?
Working with the analyzer in Rider is about the same as in Visual Studio. The plugin has a simple and intuitive interface that allows you to check the project in just a couple of clicks. This is not an exaggeration – to analyze RavenDB, all I had to do was click on the top Tools menu, point at "PVS-Studio" and click "Check Current Solution/Project".
The analysis results will be displayed in the lower part of the window on the PVS-Studio tab (well, which one else? :) )
The same as with the Visual Studio plugin, double-clicking on the warning will display the location it relates to. Everything is convenient and clear.
More importantly, the PVS-Studio tool doesn't just point out errors, but has an infrastructure that makes it easy to implement static analysis methodology even in a large old project.
The general idea is the following. Imagine, the user has started the analyzer and received many warnings. Since a project that has been developed for many years, is alive, still developing and bringing money, then most likely there won't be many warnings in the report indicating critical defects. In other words, critical bugs have already been fixed due to more expensive ways or with the help of feedback from customers. Thus, everything that the analyzer now finds can be considered technical debt, which is impractical to try to eliminate immediately. It is rational to ignore these warnings for now, but write new code while performing regular analysis.
You can tell PVS-Studio to consider all these warnings irrelevant so far (to postpone the technical debt for later), and not to show them any more. The analyzer creates a special file where it stores information about as-yet-uninteresting errors. From now on, PVS-Studio will issue warnings only for new or modified code. By the way, it's all implemented in a very smart way. If an empty line is added at the beginning of a file, the analyzer will size up the situation as if nothing has really changed and will remain quiet. You can put the markup file in the version control system. Even though the file is large, it's not a problem, as there's no need to upload it very often.
From this point, developers will see only warnings related to newly written or modified code. So you can start using the analyzer, as they say, from the next day. You can get back to technical debt later and gradually correct errors and tweak the analyzer.
To suppress warnings for existing code in Rider, just go to the top menu in Tools - >PVS-Studio and click "Suppress All Messages".
In the window that appears, which warns that all current warnings will get in the suppress list, click "Ok". A suppress file will be taken into account by the analyzer during further work. This file will be created in the project folder.
It should be noted that Rider already has an analyzer that successfully highlights some errors. Thus, a number of PVS-Studio warnings indicate code that looks suspicious from the editor's point of view. However, PVS-Studio quite often finds errors that could escape from the sharp look of the analyzer from JetBrains. This is why the most effective solution is to allow them to work as a team.
Now, as promised, let's see what interesting warnings the analyzer showed based on the check results. The project contains a huge number of source code files, so it wasn't surprising to find a lot of suspicious things in it. Nothing can be done here – everyone makes mistakes, but it is important to make every effort to detect and correct them on time. Static analysis makes this task much easier.
As a result of the check, about a thousand warnings were shown:
Read more about the different levels of warnings by following the link.
Of course, not all warnings indicate super-scary errors. If this were the case, it is unlikely that anything would work in the project:). What is important to realise is that if the analyzer complains about something, then the code looks weird and is worth a thorough investigation.
Overall, quite a lot of sapid warnings were detected in the project. However, we wouldn't like the article to be too huge, so we will consider only some of them.
public static void EnsurePathExists(string file)
{
var dirpath = Path.GetDirectoryName(file);
List<string> dirsToCreate = new List<string>();
while (Directory.Exists(dirpath) == false)
{
dirsToCreate.Add(dirpath);
dirpath = Directory.GetParent(dirpath).ToString();
if (dirpath == null) // <=
break;
}
dirsToCreate.ForEach(x => Directory.CreateDirectory(x));
}Analyzer warning: V3022 Expression 'dirpath == null' is always false. PosixHelper.cs(124) Voron
This warning can be considered in different ways. On the one hand, no doubts that an extra check isn't desirable, but it's not an error in itself. On the other hand, it is worth considering: does this code really work the way the programmer intended?
Perhaps the developer really didn't know that ToString would never return null. If this is not the case, then we can make an assumption about what the code author wanted to achieve.
Perhaps break should be called when it is not possible to get a parent for the considered directory. If this is the case, then checking for null makes sense. However, it's not the result of ToString that we need to consider, but the value returned by the GetParent method:
dirsToCreate.Add(dirpath);
var dir = Directory.GetParent(dirpath);
if (dir == null)
break;
dirpath = dir.ToString();Otherwise, returning null by the GetParent method leads to the exception when calling ToString.
public long ScanOldest()
{
....
for (int i = 0; i < copy.Length; i++)
{
var item = copy[i].Value;
if (item != null || item == InvalidLowLevelTransaction) // <=
{
if (val > item.Id) // <=
val = item.Id;
}
}
....
}Analyzer warning: V3125 The 'item' object was used after it was verified against null. Check lines: 249, 247. ActiveTransactions.cs(249), ActiveTransactions.cs(247) Voron
The code looks strange because of what happens when item is really null. Indeed, if InvalidLowLevelTransaction also turns out to be null, the condition will also be true and the attempt to get item.Id will result in the exception. If InvalidLowLevelTransaction can't be null, the condition "item == InvalidLowLevelTransaction" is simply redundant. This is because it is checked only when item == null. But if the item can't be null, then the whole condition becomes meaningless and only adds unnecessary nesting.
I think that the wrong logical operator may have been chosen here. If you replace "||" with "&& " in the condition, the code immediately starts to look logical. In addition, there can be no problems in this case.
Warnings of this type are typical representatives of potentially very dangerous errors detected by the analyzer. To be fair, the analyzer built into Rider also highlights this fragment as potentially dangerous.
public void WriteObjectEnd()
{
....
if (_continuationState.Count > 1)
{
var outerState =
_continuationState.Count > 0 ? _continuationState.Pop() : currentState
;
if (outerState.FirstWrite == -1)
outerState.FirstWrite = start;
_continuationState.Push(outerState);
}
....
}Analyzer warning: V3022 Expression '_continuationState.Count > 0' is always true. ManualBlittableJsonDocumentBuilder.cs(152) Sparrow
First, the external condition checks that the number of items in the collection is greater than 1, and then on the next line, the ternary operator checks that their number is greater than 0. It seems that one of the checks should look different. Anyway, this code looks very suspicious and should be carefully studied and rewritten if necessary.
protected override Expression VisitIndex(IndexExpression node)
{
if (node.Object != null)
{
Visit(node.Object);
}
else
{
Out(node.Indexer.DeclaringType.Name); // <=
}
if (node.Indexer != null) // <=
{
Out(".");
Out(node.Indexer.Name);
}
VisitExpressions('[', node.Arguments, ']');
return node;
}Analyzer warning: V3095 The 'node.Indexer' object was used before it was verified against null. Check lines: 1180, 1182. ExpressionStringBuilder.cs(1180), ExpressionStringBuilder.cs(1182) Raven.Client
In fact, this is another place that both PVS-Studio and Rider consider suspicious. However, the wording is slightly different: the analyzer from JetBrains just highlights the node.Indexer.DeclaringType with the comment "Possible NullReferenceException".
Both checkers state that this fragment might trigger an exception. I should note that not only does the warning from PVS-Studio say that there may be an error, but it also explains the reasons for it. Small thing, but still nice.
In fact, this doesn't mean that there is really an error. It's entirely possible that if node.Object == null, then node.Indexer is exactly set. However, a situation is possible when node.Object and node.Indexer both aren't null. This is the only case when this warning from analyzers can be considered false. Therefore, it is worth carefully analyzing all possible options.
private async Task LoadStartingWithInternal(....)
{
....
var command = operation.CreateRequest();
if (command != null) // <=
{
await RequestExecutor
.ExecuteAsync(command, Context, SessionInfo, token)
.ConfigureAwait(false)
;
if (stream != null)
Context.Write(stream, command.Result.Results.Parent);
else
operation.SetResult(command.Result);
}
....
}Analyzer warning: V3022 Expression 'command != null' is always true. AsyncDocumentSession.Load.cs(175) Raven.Client
The warning is issued because the CreateRequest method never returns null. In fact, just look at its code to make sure of this:
public GetDocumentsCommand CreateRequest()
{
_session.IncrementRequestCount();
if (Logger.IsInfoEnabled)
Logger.Info(....);
return new GetDocumentsCommand(....);
}Generally speaking, this check isn't such a problem. Although it may be that the method used to return null under certain conditions earlier, and now throws an exception if something happens. Who knows, it's possible that instead of that null check, there should now be a try-catch.
You may have a very reasonable question: where is the exception thrown here? If they aren't present, then we deal with an extra check, and there can be no error.
Alas, when executing the method, an exception can actually be thrown, and even twice. First in the IncrementRequestCount method:
public void IncrementRequestCount()
{
if (++NumberOfRequests > MaxNumberOfRequestsPerSession)
throw new InvalidOperationException(....);
}After - in the GetDocumentsCommand constructor:
public GetDocumentsCommand(string startWith, ....)
{
_startWith = startWith ?? throw new ArgumentNullException(nameof(startWith));
....
}public override void WriteTo(StringBuilder writer)
{
....
if (SqlConnectionStringsUpdated)
json[nameof(SqlConnectionStringsUpdated)] = SqlConnectionStringsUpdated;
if (ClientConfigurationUpdated)
json[nameof(ClientConfigurationUpdated)] = ClientConfigurationUpdated;
if (ConflictSolverConfigUpdated)
json[nameof(ConflictSolverConfigUpdated)] = ClientConfigurationUpdated;
if (PeriodicBackupsUpdated)
json[nameof(PeriodicBackupsUpdated)] = PeriodicBackupsUpdated;
if (ExternalReplicationsUpdated)
json[nameof(ExternalReplicationsUpdated)] = ExternalReplicationsUpdated;
....
}Analyzer warning: V3127 Two similar code fragments were found. Perhaps, this is a typo. SmugglerResult.cs(256), SmugglerResult.cs(253) Raven.Client
I highly doubt that anyone would have seen the oddity if they had looked at the code. The function consists of 14 similar conditions and all variable names end in Updated. Even when a small part of it is shown here, the error isn't immediately visible.
The human brain literally refuses to look for something in such code. At the same time, PVS-Studio easily detected that the assignment is most likely completely wrong:
if (ClientConfigurationUpdated)
json[nameof(ClientConfigurationUpdated)] = ClientConfigurationUpdated;
if (ConflictSolverConfigUpdated)
json[nameof(ConflictSolverConfigUpdated)] = ClientConfigurationUpdated;Logically, the lower line to the right of the assignment operator should have ConflictSolverConfigUpdated. I'm sure that without static analysis, this oddity would only be found if something serious enough broke because of it. The programmer will be able to notice that there is a problem hidden in this function, unless they know about it in advance.
public int Count =>
_documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count ?? 0;Analyzer warning: V3123 Perhaps the '??' operator works in a different way than it was expected. Its priority is lower than priority of other operators in its left part. InMemoryDocumentSessionOperations.cs(1952) Raven.Client
Of course, it's still possible that this is not an error and this was written intentionally. Still, this fragment looks very suspicious. After all, it is logical to assume that the point of the function is not to return 0 when _onBeforeStoreDocumentsByEntity == null.
I think there really is an error here related to operator priorities. In this case, you must add parenthesis:
_documentsByEntity.Count + (_onBeforeStoreDocumentsByEntity?.Count ?? 0)On the other hand, if the above fragment was written specifically in this way, then it maybe worth pointing it out explicitly. This way the analyzer and programmers reading this code won't have any questions:
(_documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count) ?? 0But this is a matter of taste, of course.
private static void UpdateEnvironmentVariableLicenseString(....)
{
....
if (ValidateLicense(newLicense, rsaParameters, oldLicense) == false)
return;
....
}Analyzer warning: V3066 Possible incorrect order of arguments passed to 'ValidateLicense' method: 'newLicense' and 'oldLicense'. LicenseHelper.cs(177) Raven.Server
Arguments are passed to the method in a strange order. Take a look at the declaration:
private static bool ValidateLicense(
License oldLicense,
RSAParameters rsaParameters,
License newLicense
)It is very nice that PVS-Studio is able to find even such errors. This is a great example of the advantages of static analysis over dynamic analysis.
Despite the above, I initially assumed that it might not matter in which order these arguments are passed. Of course, in this case, the names would not be quite correctly chosen, but what can we do? However, the internal structure of ValidateLicense suggests that these parameters still have different meanings. You can view the code of this function by following the link.
private List<CounterOperation> GetCounterOperationsFor(RavenEtlItem item)
{
....
for (var i = 0; i < counters.Count; i++)
{
counters.GetPropertyByIndex(i, ref prop);
if (
GetCounterValueAndCheckIfShouldSkip(
item.DocumentId,
null,
prop,
out long value,
out bool delete
)
) continue;
....
}
....
}Analyzer warning: V3022 Expression 'GetCounterValueAndCheckIfShouldSkip(item.DocumentId, null, prop, out long value, out bool delete)' is always false. RavenEtlDocumentTransformer.cs(362) Raven.Server
You can check out the entire method by following the link.
This warning indicates that the call to continue is not available in this loop. And if so, the fragment is really weird. But maybe it's just a false positive? Especially since Rider doesn't complain about this.
Let's look at the GetCounterValueAndCheckIfShouldSkip method:
private bool GetCounterValueAndCheckIfShouldSkip(
LazyStringValue docId,
string function,
BlittableJsonReaderObject.PropertyDetails prop,
out long value,
out bool delete
)
{
value = 0;
if (prop.Value is LazyStringValue)
{
delete = true;
}
else
{
delete = false;
value = CountersStorage.InternalGetCounterValue(
prop.Value as BlittableJsonReaderObject.RawBlob,
docId,
prop.Name
);
if (function != null)
{
using (var result = BehaviorsScript.Run(
Context,
Context,
function,
new object[] { docId, prop.Name }
))
{
if (result.BooleanValue != true)
return true;
}
}
}
return false;
}Obviously, this method can only return true if function != null. In the above code it is the null pointer that is passed in place of this parameter. This means that the continue call is really unreachable.
This point can be either a harmless omission or a problem related to an error in the condition. Anyway, this fragment should be paid attention.
public LicenseType Type
{
get
{
if (ErrorMessage != null)
return LicenseType.Invalid;
if (Attributes == null)
return LicenseType.None;
if (Attributes != null && // <=
Attributes.TryGetValue("type", out object type) &&
type is int
)
{
var typeAsInt = (int)type;
if (Enum.IsDefined(typeof(LicenseType), typeAsInt))
return (LicenseType)typeAsInt;
}
return LicenseType.Community;
}
}Analyzer warning: V3063 A part of conditional expression is always true if it is evaluated: Attributes != null. LicenseStatus.cs(28) Raven.Server
An extremely strange fragment. Usually, extra checks are somehow separated, whereas here the variable and null pointer matching is checked right in adjacent lines. It seems that the code probably doesn't do what the programmer wanted.
public Task SuspendObserver()
{
if (ServerStore.IsLeader())
{
var suspend = GetBoolValueQueryString("value");
if (suspend.HasValue) // <=
{
Server.ServerStore.Observer.Suspended = suspend.Value;
}
NoContentStatus();
return Task.CompletedTask;
}
RedirectToLeader();
return Task.CompletedTask;
}Analyzer warning: V3022 Expression 'suspend.HasValue' is always true. RachisAdminHandler.cs(116) Raven.Server
Another seemingly harmless "extra" check. Although it is not yet clear why the analyzer considers it such.
Let's turn to GetBoolValueQueryString:
protected bool? GetBoolValueQueryString(string name, bool required = true)
{
var boolAsString = GetStringQueryString(name, required);
if (boolAsString == null)
return null;
if (bool.TryParse(boolAsString, out bool result) == false)
ThrowInvalidBoolean(name, boolAsString);
return result;
}Indeed, sometimes this function returns null. Moreover, Rider didn't consider that check unnecessary. Did the unicorn really fail us?
What if we look at the GetStringQueryString method?
protected string GetStringQueryString(string name, bool required = true)
{
var val = HttpContext.Request.Query[name];
if (val.Count == 0 || string.IsNullOrWhiteSpace(val[0]))
{
if (required)
ThrowRequiredMember(name);
return null;
}
return val[0];
}Hm, if required == true, the ThrowRequiredMember method will be called. I wonder what it's doing? :) Well, let me cite this to dispel all doubts:
private static void ThrowRequiredMember(string name)
{
throw new ArgumentException(
$"Query string {name} is mandatory, but wasn't specified."
);
}So, let's sum up. The developer calls the GetBoolValueQueryString method. He probably believes that the method potentially won't get the required value. As a result, it returns null. Inside, GetStringQueryString is called. If problems occur, it will either return null or throw an exception. The second occurs if the required parameter is set to true. However, this is its default value. At the same time, when calling GetBoolValueQueryString, it is not passed, if you look at the code above.
Let's look again at the code of the SuspendObserver method, which triggered the analyzer:
public Task SuspendObserver()
{
if (ServerStore.IsLeader())
{
var suspend = GetBoolValueQueryString("value");
if (suspend.HasValue)
{
Server.ServerStore.Observer.Suspended = suspend.Value;
}
NoContentStatus();
return Task.CompletedTask;
}
RedirectToLeader();
return Task.CompletedTask;
}It seems that the execution thread shouldn't be interrupted here if GetBoolValueQueryString couldn't get the value. In fact, checking for null is followed by various actions and the returned value. I think that these actions are performed independently of GetBoolValueQueryString method progress. What will actually happen? The execution thread will be interrupted by an exception.
To correct this thing, when calling GetBoolValueQueryString, one has to pass the false value as the second parameter required. This way everything will really work as expected.
As I said earlier, sometimes it seems that the analyzer is wrong (truth be told, it happens). Also, quite often, the warning looks insignificant. It would seem that there is an extra check, but it's okay. You can even remove it and have no problems – the warning will disappear!
Even in cases where the warning seems strange and incomprehensible, don't hastily mark it as false. You should try to understand why the analyzer considers the place problematic and then make a decision.
private async Task<int> WriteDocumentsJsonAsync(...., int numberOfResults) // <=
{
using (
var writer = new AsyncBlittableJsonTextWriter(
context,
ResponseBodyStream(),
Database.DatabaseShutdown
)
)
{
writer.WriteStartObject();
writer.WritePropertyName(nameof(GetDocumentsResult.Results));
numberOfResults = await writer.WriteDocumentsAsync( // <=
context,
documentsToWrite,
metadataOnly
);
....
}
return numberOfResults;
}Analyzer warning: V3061 Parameter 'numberOfResults' is always rewritten in method body before being used. DocumentHandler.cs(273), DocumentHandler.cs(267) Raven.Server
The parameter passed to the function isn't used, but is immediately overwritten. Why is it needed here? Did the authors want to pass via ref?
I was curious to see how this method is used in existing code. I hoped that since it was private, there shouldn't be too many of them. Thanks to Rider, I easily found where the call is made. It was the only place:
private async Task GetDocumentsByIdAsync(....)
{
....
int numberOfResults = 0;
numberOfResults = await WriteDocumentsJsonAsync(
context,
metadataOnly,
documents,
includes,
includeCounters?.Results,
numberOfResults
);
....
}The variable is assigned 0, then it is passed to the method, the result of which is assigned to it. And this parameter isn't used inside the method in any way. Hm. Why is it all needed?
private OrderByField ExtractOrderByFromMethod(....)
{
....
if (me.Arguments.Count < 2 && me.Arguments.Count > 3)
throw new InvalidQueryException(....);
....
}Analyzer warning: V3022 Expression 'me.Arguments.Count < 2 && me.Arguments.Count > 3' is always false. Probably the '||' operator should be used here. QueryMetadata.cs(861) Raven.Server
You can view the full method here.
This time we deal with an obvious error - using of incorrect logical operator. In the current form, checking the number of arguments simply doesn't work, because there is no value that is both less than 2 and more than 3. The developer's true intentions are easily revealed by the first argument passed to the exception constructor:
"Invalid ORDER BY 'spatial.distance(from, to, roundFactor)' call,
expected 2-3 arguments, got " + me.Arguments.CountIn order for the check to work correctly, you just need to replace "&&" with "||".
private bool Operator(OperatorField fieldOption, out QueryExpression op)
{
....
switch (match)
{
....
case "(":
var isMethod = Method(field, out var method); // <=
op = method;
if (isMethod && Operator(OperatorField.Optional, out var methodOperator))
{
....
}
return isMethod;
....
}
}Analyzer warning: V3063 A part of conditional expression is always true if it is evaluated: isMethod. QueryParser.cs(1797) Raven.Server
You can view the full method here.
The var isMethod = Method(field, out var method) construction reminded me of standard methods like Int.TryParse. These methods attempt to get the result and write it to an out variable, and the operation success flag is the return value. Code that uses such functions usually checks the return value and then performs certain operations based on it.
In my opinion, the Method function is used here in this way. The result of Method is also the return value of the Operator method calling it.
According to the analyzer, the isMethod variable will always have the true value and its checking in the condition is pointless. This means that the Method function never returns false. Then what is the point of using such a construction?
First, let's make sure that the analyzer is not mistaken:
private bool Method(FieldExpression field, out MethodExpression op)
{
var args = ReadMethodArguments();
op = new MethodExpression(field.FieldValue, args);
return true;
}Indeed, the return value of this method is always true. And if that's what it was meant to be, this it is... strange, but on the whole not a big deal. But what if it's not so?
The ReadMethodArguments function throws exceptions in some cases. You can view its code here. This happens when the method can't perform its task correctly.
It seems that the code calling the Method function isn't meant to throw exceptions. Most likely, it is expected that when the value of the out variable fails to be obtained correctly, the Method function will return false. However, the current implementation results in an exception instead.
Whatever the case, authors should reconsider this fragment.
private Address GetNextEdge()
{
if (m_curEdgeBlock == null || m_curEdgeBlock.Count <= m_curEdgeIdx)
{
m_curEdgeBlock = null;
if (m_edgeBlocks.Count == 0)
{
throw new ApplicationException(
"Error not enough edge data. Giving up on heap dump."
);
}
var nextEdgeBlock = m_edgeBlocks.Dequeue();
if (
m_curEdgeBlock != null && // <=
nextEdgeBlock.Index != m_curEdgeBlock.Index + 1
)
{
throw new ApplicationException(
"Error expected Node Index " + (m_curEdgeBlock.Index + 1) +
" Got " + nextEdgeBlock.Index + " Giving up on heap dump."
);
}
m_curEdgeBlock = nextEdgeBlock;
m_curEdgeIdx = 0;
}
return m_curEdgeBlock.Values(m_curEdgeIdx++).Target;
}Analyzer warning: V3063 A part of conditional expression is always false if it is evaluated: m_curEdgeBlock != null. DotNetHeapDumpGraphReader.cs(803) Raven.Debug
The variable is assigned a null pointer, and then a few lines after it is checked for null. In doing so, the code checking nextEdgeBlock.Index != m_curEdgeBlock.Index + 1 becomes of no use. In addition, an exception will never be thrown.
It stands to reason, that something is not working as it should, because the fragment looks very weird. Either the check is not needed at all, or it is implemented incorrectly.
On the other hand, we can regard the warning by a reversal of logic. Let's try to imagine the case where this warning is false. I think this is only possible if the value of the variable can be changed when calling Deque. However, m_curEdgeBlock is a private field, and m_edgeBlocks is a standard queue that is initialized in the same class. Thus, it is highly doubtful that calling Dequeue can affect the value of m_curEdgeBlock in any way. Therefore, the warning is most likely not false.
public HashSet<string> FindSpecialColumns(string tableSchema, string tableName)
{
var mainSchema = GetTable(tableSchema, tableName);
var result = new HashSet<string>();
mainSchema.PrimaryKeyColumns.ForEach(x => result.Add(x)); // <=
foreach (var fkCandidate in Tables)
foreach (var tableReference in fkCandidate.References.Where(
x => x.Table == tableName && x.Schema == tableSchema
)
)
{
tableReference.Columns.ForEach(x => result.Add(x));
}
return result;
}Analyzer warning: V3146 Possible null dereference of 'mainSchema'. The 'Tables.FirstOrDefault' can return default null value. DatabaseSchema.cs(31) Raven.Server
At first glance, the warning might seem obscure. Indeed, what does FirstOrDefault have to do with it? To make it clear why the analyzer triggers, we need to look at the GetTable function:
public TableSchema GetTable(string schema, string tableName)
{
return Tables.FirstOrDefault(
x => x.Schema == schema && x.TableName == tableName
);
}Calling the FirstOrDefault method instead of First may be due to the fact that the collection may not have elements that match the specified condition. In this case, FirstOrDefault, and therefore GetTable, will return null, since TableSchema is a reference type. This is why PVS-Studio says that an attempt to dereference a null pointer may occur in this code.
It may still be worth checking such a case so that execution isn't interrupted with a NullReferenceException. If the scenario where Tables.FirstOrDefault returns null is not possible, then there is no point in using FirstOrDefault instead of First.
public override void VerifyCanExecuteCommand(
ServerStore store, TransactionOperationContext context, bool isClusterAdmin
)
{
using (context.OpenReadTransaction())
{
var read = store.Cluster.GetCertificateByThumbprint(context, Name);
if (read == null)
return;
var definition = JsonDeserializationServer.CertificateDefinition(read);
if (
definition.SecurityClearance != SecurityClearance.ClusterAdmin || // <=
definition.SecurityClearance != SecurityClearance.ClusterNode // <=
)
return;
}
AssertClusterAdmin(isClusterAdmin);
}Analyzer warning: V3022 Expression is always true. Probably the '&&' operator should be used here. DeleteCertificateFromClusterCommand.cs(21) Raven.Server
Another example of a situation where almost certainly the wrong logical operator was chosen. In this case, the condition is always true, because the variable isn't exactly equal to at least one of the values that it is compared with.
I suppose that "||" should be replaced with "&&". Then the above fragment will make sense. If the logical operator is chosen correctly, it is most likely that other variables should be compared in one of the conditions. Anyway, this fragment looks very fishily and it must be analyzed.
First of all, I'd like to thank everyone who got to this place. This article is quite long, but I hope you were interested in working with me on the new version of the PVS-Studio analyzer and studying the errors found.
It is important to remember that the main goal of a developer shouldn't be to reduce the number of warnings. You don't need to use PVS-Studio to achieve an empty error log. Dealing with the warnings is all the same as struggling with symptoms of a disease, which affects the source code.
When reviewing analyzer messages, you should always try to understand why a particular warning is issued. Only when you understand the logic behind the analyzer's warning you can draw conclusions whether it indicates an error or not. It is in this case that you will struggle not with the symptom, but with the disease. And this is how your code will become cleaner and healthier. Eventually, there will be less problems with such great source code. Although I'd rather wish you didn't have any at all :)