Picture 8

PVS-Studio is a well-known static code analyzer that allows you to find many tricky errors hidden in the source. Recently, a beta test of the new version was completed, in which it became possible to analyze C # projects for Linux and macOS. In addition, the analyzer can now be integrated with the JetBrains cross-platform IDE - Rider. This article will allow you to get acquainted with these features on the example of checking the open source project RavenDB.

Introduction


Some time ago, my colleague Sergey Vasiliev wrote note about opening a beta test of a new version of the PVS-Studio static analyzer that we are developing. Now the beta test is completed and the new version can be downloaded by clicking on the link . In the same article, we will examine how to analyze C # projects under Linux/macOS using the console interface and Rider . After that, we will conduct a traditional analysis of some interesting analyzer operations.

RavenDB


Picture 5


For verification, an open RavenDB project was selected, a repository which includes almost 5 thousand source code files. It is a fairly popular NoSQL database. Details can be found on the website . It is easy to guess that I was attracted by its size, because in such a large and, without a doubt, serious project, there will probably be something interesting.

Command Line Interface


To begin, consider how to analyze through the console. This section, in my opinion, will be especially interesting for those who want to integrate the analyzer into the CI system. The team that starts the analysis has a number of interesting options, but in the general case, 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.sln 

The -t flag (short for target) is used to specify a solution or project file for verification. The line presented above starts the analysis and, as a result of the work, forms a file containing the errors found. It's simple, isn't it?

Rider


Working with the analyzer in Rider is about the same as in Visual Studio. The simple and intuitive interface of the plugin allows you to check the project in just a couple of clicks. This is not an exaggeration - for RavenDB analysis I just had to click on the Tools top menu, point to “PVS-Studio” and click “Check Current Solution/Project.”

Picture 2

The analysis results will be displayed in the lower part of the window on the PVS-Studio tab (but what else? :))

Picture 3

As with the plug-in for Visual Studio, double-clicking on a warning will display the location with which it is associated. Everything is convenient and clear.

More importantly, the PVS-Studio tool not only indicates errors, but also has an infrastructure that makes it easy to implement the static analysis methodology even in a large old project.

The general idea is as follows. The user started the analyzer and received many warnings. Since a project that has been developed for many years, is alive, develops and makes money, then most likely there will not be many warnings in the report indicating critical defects. In other words, critical bugs have already been fixed in one way or another in more expensive ways or thanks to feedback from customers.Accordingly, everything that the analyzer now finds can be considered a technical debt, which is impractical to try to eliminate immediately. It’s reasonable to ignore these warnings for now, but to write new code by doing regular analysis.

You can tell PVS-Studio to consider these warnings as irrelevant (postpone technical debt for later), and not show them again. The analyzer creates a special file where it stores information about so far uninteresting errors. And now PVS-Studio will issue warnings only on new or changed code. Moreover, all this is implemented smartly. If, for example, an empty line is added to the beginning of a file, then the analyzer understands that, in fact, nothing has changed, and will remain silent. This markup file can be embedded in the version control system. The file is large, but it’s not scary, because often it makes no sense to lay it.

Now all programmers will see warnings related only to the new or changed code. Thus, the analyzer can begin to use what is called the next day. And it will be possible to return to technical debt later, and gradually correct errors and configure the analyzer.

To suppress warnings on existing code in Rider, simply go to Tools- > PVS-Studio in the top menu and click “Suppress All Messages.”

Picture 1

In the window that appears, warning that all current operations will fall into the suppress list, click on “Ok”. As a result, a suppress file will be created in the project folder, which will be taken into account by the analyzer during further work.

It should be noted that Rider already has an analyzer that successfully highlights some errors. Thus, a series of warnings from PVS-Studio points to code that looks suspicious from the point of view of the editor. Nevertheless, PVS-Studio quite often finds errors that could get away from the sensitive eye of the analyzer from JetBrains. That is why the most effective solution is to let them work in a team.

Picture 10

And for dessert


Now, as promised, we will consider what interesting warnings the analyzer displayed according to the results of the verification. The project contains a huge number of source code files, so it was not surprising to find many suspicious moments in it. There is nothing to be done - everyone makes mistakes, but it is important to make every effort to detect and correct them in a timely manner. Static analysis can greatly simplify this task.

As a result of the check, about a thousand warnings were displayed:

Picture 11

You can read more about warning levels by clicking on the link .

Of course, not all triggers indicate super-scary mistakes. If this were so, it would be unlikely that at least something worked in the project :). But it’s important to understand that since the analyzer “swears”, the code looks strange and should be carefully studied.

In general, the project found many interesting positives. Nevertheless, I would not want the article to be very huge, so we will consider only a few of them.

Just an extra check?


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 response can be considered in different ways. On the one hand, an extra check is, of course, not very good, but in itself it is not a mistake. On the other hand, it’s worth considering: does this code really work the way the programmer intended?

Perhaps the developer really did not know that ToString would never return null .If this is not so, then we can make an assumption about what he wanted to achieve.

Perhaps break should be called when it is not possible to get the parent for the directory in question. If so, then checking for null makes sense. However, you need to consider not the result of ToString , 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 with the GetParent method throws an exception when calling ToString .

Typical null


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 weird because of what happens when the item is really null . Indeed, if InvalidLowLevelTransaction is also null , the condition is true and an attempt to get item.Id will throw an exception. And if InvalidLowLevelTransaction cannot be null , then the condition " item == InvalidLowLevelTransaction " is simply superfluous. That's because it is checked only when item == null . Well, if suddenly item cannot be null , then in general the whole condition loses its meaning and only adds unnecessary nesting.

I think that perhaps the wrong logical operator is selected here. If replaced by the condition "||" to "& amp; & amp;", then the code immediately begins to look logical. Well, no problems can arise in this case.

The occurrences of such a plan are typical representatives of the very dangerous in the theory of errors detected by the analyzer. In fairness, it should be noted that the analyzer built into Rider also marks this fragment as potentially dangerous.

Again, an extra check?


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, it is checked in the external condition that the number of elements 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. One way or another, this code looks very suspicious and should be carefully studied and revised if necessary.

Possible NRE


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 PVS-Studio and Rider consider suspicious. True, the wording is somewhat different: the analyzer from JetBrains just highlights the node.Indexer.DeclaringType with the comment "Possible NullReferenceException".

Both reviewers claim that an exception can indeed be thrown in this fragment. I must say that the warning from PVS-Studio not only says that a mistake is possible here, but also explains why he had such an opinion. It’s a trifle, but nice.

In fact, it’s not a fact that there really is a mistake. It is possible that if node.Object == null , then node.Indexer is precisely defined. In this case, a situation is permissible when node.Object and node.Indexer are both not equal null . Only in this case can the given operation of both analyzers be considered false. Therefore, it is worthwhile to carefully analyze all the possible options.

And if you dig deeper?


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

A warning is issued because the CreateRequest method never returns null .In fact, just look at its code to see this:

public GetDocumentsCommand CreateRequest() { _session.IncrementRequestCount(); if (Logger.IsInfoEnabled) Logger.Info(....); return new GetDocumentsCommand(....); } 

In general, this check is not such a problem, although it may be that the method used to return null in certain circumstances, but now it throws an exception. Who knows, it’s possible that instead of checking for null there should now be a try-catch.

You may have a very reasonable question: where is the exception thrown here? If they are not there, then we still just have an extra check, and there can be no mistake here.

But alas, when the method is executed, an exception can indeed be thrown, moreover, twice. First in the IncrementRequestCount method:

public void IncrementRequestCount() { if (++NumberOfRequests > MaxNumberOfRequestsPerSession) throw new InvalidOperationException(....); } 

Then, in the GetDocumentsCommand constructor:

public GetDocumentsCommand(string startWith,....) { _startWith=startWith ?? throw new ArgumentNullException(nameof(startWith)); .... } 

Traditional copy-paste



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 doubt very much that at least someone would see weirdness here by looking at the code. The function consists of 14 similar conditions and all variables end with Updated. Even when a small part is given here, the error is not immediately visible.

The human brain literally refuses to look for anything in such a code. At the same time, PVS-Studio easily discovered that it is assigned, most likely, it’s completely wrong, it should:

if (ClientConfigurationUpdated) json[nameof(ClientConfigurationUpdated)]=ClientConfigurationUpdated; if (ConflictSolverConfigUpdated) json[nameof(ConflictSolverConfigUpdated)]=ClientConfigurationUpdated; 

It is logical that the bottom line to the right of the assignment operator should be ConflictSolverConfigUpdated . I am sure that without static analysis, this oddity would be found only if something serious enough had broken because of it. The programmer will be able to notice that a problem is hidden in this function, unless it knows in advance.

Oh, that "??"


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, the possibility remains that this is not a mistake at all, that this is how it was intended. And yet this place looks very suspicious. After all, it is logical to assume that the idea of ​​the function is not to return 0 when _onBeforeStoreDocumentsByEntity == null.

I think that there really is an error related to the priorities of the operators. In this case, you must put the brackets:

_documentsByEntity.Count + (_onBeforeStoreDocumentsByEntity?.Count ?? 0) 

Well, if all of a sudden everything is really planned, then perhaps you should point it out clearly - then the analyzer and programmers who read the code will have no questions:

(_documentsByEntity.Count + _onBeforeStoreDocumentsByEntity?.Count) ?? 0 

But this is a matter of taste, of course.

Passing parameters


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 passed to the method in suspicious order. In fact, take a look at the ad:

private static bool ValidateLicense( License oldLicense, RSAParameters rsaParameters, License newLicense ) 

It is very pleasant that PVS-Studio is able to find even such errors. This is a great example of the benefits of static analysis over dynamic analysis.

Notwithstanding the foregoing, I initially suggested that it may not matter in which order these arguments are passed. Of course, in this case the names would not have been chosen correctly, but what can you do. However, the internal structure of ValidateLicense suggests that these parameters still have different meanings. You can see the code for this function by clicking on the link .

Never continue


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 consider this method in its entirety by going to 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 return true only if function != null . In the code discussed earlier, the null pointer is passed to the place of this parameter. So the call to continue is really unreachable.

This moment can be both a harmless omission and a problem related to some mistake in the condition. One way or another, you should pay attention to it.

Trust but verify


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

Extremely strange fragment. Usually, superfluous checks are at least somehow separated - here the correspondence of a variable to a null pointer is checked directly in the next lines. It seems that the code most likely does not do what the programmer wanted.

Nullable, which is never null


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 as such.

Turning 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 . Yes, and Rider did not consider that check unnecessary. Has the Unicorn Failed?

Picture 15


But what if you 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]; } 

Hmm, if the required == true parameter, then the ThrowRequiredMember method will be called. I wonder what is he doing? :) Well, so that certainly there is no doubt left:

private static void ThrowRequiredMember(string name) { throw new ArgumentException( $"Query string {name} is mandatory, but wasn't specified." ); } 

So, we summarize. The developer calls the GetBoolValueQueryString method. He probably believes that potentially the method will not get the necessary value. Well, as a result, it will return null . Internally, GetStringQueryString is called. If problems arise, it will either return null or throw an exception. The second occurs if the required parameter is set to true . Moreover, this is its default value. At the same time, when GetBoolValueQueryString is called, it, if you look at the code above, is just not transmitted.

Let's look again at the code of the SuspendObserver method, the snippet of which the analyzer swore:

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, by design, the execution flow should not be interrupted here if GetBoolValueQueryString could not get the value. In fact, after a block with checking for null , various actions are performed and the value is returned. I suppose, by design, these actions are performed independently from the failure of the GetBoolValueQueryString method. What will actually happen? An execution thread will be interrupted by an exception.

To fix this point, when calling GetBoolValueQueryString , pass false as the second parameter required . Then everything will really work as expected.

As I said earlier, sometimes it seems that the analyzer is mistaken (what a sin to conceal, sometimes it happens). Also quite often, the warning looks insignificant. It would seem that there is an extra check here, and okay.Well, or even better - remove it and no problems - the warning will disappear!

Even in cases where the operation seems strange and incomprehensible, do not rush to mark it as false. We must try to understand why the analyzer considers the place to be problematic and make a decision after that.

Oddities


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 is not used, but is immediately overwritten. Why is he needed here? They wanted to pass it through ref?

I was curious to see how this method is used in existing code. I hoped that since it is private, there should not be too many of them. Thanks to Rider, I easily found where the call is made. This 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 inside the method, this parameter is not used in any way. Em. Why all this?

Picture 6


Wrong logical operator


private OrderByField ExtractOrderByFromMethod(....) { .... if (me.Arguments.Count < 2 && me.Arguments.Count > 3) throw new InvalidQueryException(....); .... } 

Analyzer Warning : V3022 Expression 'me.Arguments.Count & lt; 2 & amp; & amp; me.Arguments.Count > 3 'is always false. Probably the '||' operator should be used here. QueryMetadata.cs (861) Raven.Server

The full method can be seen . br>
This time we have an obvious error, which consists in using the wrong logical operator. In its current form, checking the number of arguments simply does not work, because there is no value that is both less than 2 and more than 3. The true intentions of the developer 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.Count Чтобы проверка работала корректно, нужно просто заменить "&&" на "||". 

Strange try method


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

The full method can be seen 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 everything was planned like that, then this is... strange, but it does not matter, in principle. But what if not?

The ReadMethodArguments function, the code of which can be viewed
here , throws exceptions in some cases. This happens when a method cannot correctly perform its task.

It seems that the code calling the Method, function is not designed to throw exceptions. Most likely, it is assumed that when it is not possible to correctly obtain the value of the out variable, the Method function will return false . However, with the current implementation, an exception will be thrown instead.

Be that as it may, pay attention to this fragment.

null!=null?


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

A variable is assigned a null pointer, and then after a few lines it is checked for its inequality null . In this case, the code performing the check nextEdgeBlock.Index!=M_curEdgeBlock.Index + 1 becomes meaningless. In addition, an exception will never be thrown.

It is logical to assume that something is not working as it should, because the fragment looks very strange. Either the check is not needed here at all, or it is somehow implemented incorrectly.

You can consider triggering and, on the other hand, go from the opposite. Let's try to imagine a situation in which this response is false. I think this is possible only if the value of the variable can be changed by 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 a call to Dequeue can somehow affect the value of m_curEdgeBlock . Therefore, the trigger is most likely not false.

First or null


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

The operation, at first glance, may seem incomprehensible. Indeed, what does FirstOrDefault have to do with it? To make it clear why the analyzer is cursing, you 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 contain elements that match the specified condition. In this case, FirstOrDefault , and therefore GetTable will return null , since TableSchema is a reference type. That is why PVS-Studio says that in this code an attempt to dereference a null pointer may occur.

It may still be worth checking out such a case so that the execution does not interrupt with a NullReferenceException . If the option where Tables.FirstOrDefault returns null is impossible, then there is no point in using FirstOrDefault instead of First .

Always true


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 '& amp; & amp;' operator should be used here. DeleteCertificateFromClusterCommand.cs (21) Raven.Server

Another example of a situation where the wrong logical operator is most likely selected. In this case, the condition is always true, because the variable is definitely not equal to at least one of the values ​​with which it is compared.

I believe that "||" should be replaced with "& amp; & amp;". Then the written will make sense. If the logical operator is nevertheless chosen correctly, then most likely one of the conditions should be a comparison of other variables. One way or another, this fragment looks very suspicious and must be analyzed.

Conclusion


First of all, I want to thank everyone who got to this place. The article came out quite voluminous, but I hope you were interested in working with me to deal with the new version of PVS-Studio analyzer and study the errors found.

It is important to remember that the developer should set as his main goal not to reduce the number of operations. Not for the sake of reaching an empty error log you need to use PVS-Studio. Fighting positives is like fighting the symptoms of a disease from which the source code suffers.

When looking at analyzer messages, you should always try to understand why this or that warning is issued. Only by realizing the logic by which the analyzer issued a warning can we conclude whether it indicates an error or not. It is in this case that you will not fight with the symptom, but with the disease. And that is how your code will become cleaner and healthier. And of course, there will be much less problems with such an excellent source. Although I’d ​​better wish you not to have them at all :)

Picture 16

ITKarma picture


If you want to share this article with an English-speaking audience, then please use the link to the translation: Nikita Lipilin. How to find errors in a C # project working under Linux and macOS .

Source