Top 10 errors in C # projects for 2016

To assess the quality of our analyzer, as well as to popularize the methodology of static analysis, we regularly check for open source projects for errors and write articles about it. The last year of 2016 was not an exception, which is remarkable also by the fact that it was the time of a kind of “growing up” C # analyzer. PVS-Studio has received a significant amount of new C # diagnostics, an improved mechanism for working with virtual values ​​(symbolic execution) and much more. According to the results of the work done by our team, I made a peculiar hit parade of the most interesting errors found in C # projects in 2016.



Tenth place: when in the minute is not always 60 seconds



I’ll start the charts with the error found during the verification of the Orchard CMS project. A description of the error can be found in the article . In general, all articles about the verification of projects can be found here .



V3118 Seconds component of TimeSpan is used, which does not represent the full time interval. Possibly 'TotalSeconds' value was intended instead. AssetUploader.cs 182

')

void IBackgroundTask.Sweep() { .... // Don't flood the database with progress updates; // Limit it to every 5 seconds. if ((_clock.UtcNow - lastUpdateUtc).Seconds >= 5) { .... } } 

Instead of TotalSeconds in this case, the developer mistakenly used Seconds . Thus, it will not receive the total number of seconds contained between the _clock.UtcNow dates and the lastUpdateUtc dates , as the programmer expected, but only the residual value of the range. For example, for a range value of 1 minute 4 seconds, the result is not 64, but 4 seconds. Incredibly, even experienced developers make similar mistakes.



Ninth place: expression is always true



The following error is given in the article about testing the GitExtensions project.



V3022 Expression 'string.IsNullOrEmpty (rev1) || string.IsNullOrEmpty (rev2) 'is always true. GitUI FormFormatPatch.cs 155

 string rev1 = ""; string rev2 = ""; var revisions = RevisionGrid.GetSelectedRevisions(); if (revisions.Count > 0) { rev1 = ....; rev2 = ....; .... } else if (string.IsNullOrEmpty(rev1) || string.IsNullOrEmpty(rev2)) // <= { MessageBox.Show(....); return; } 

Notice the else keyword. Probably he does not belong here at all. Inattention when refactoring or banal programmer fatigue, and here we get a radical change in the logic of the program, which leads to unpredictable behavior. Well, that static analyzer never gets tired.



Eighth place: possible typo



The article on checking the source code of FlashDevelop contains an interesting error related to a typo.



V3056 Consider reviewing the correctness of 'a1' item's usage. LzmaEncoder.cs 225

 public void SetPrices(....) { UInt32 a0 = _choice.GetPrice0(); UInt32 a1 = _choice.GetPrice1(); UInt32 b0 = a1 + _choice2.GetPrice0(); // <= UInt32 b1 = a1 + _choice2.GetPrice1(); .... } 

I agree with the analyzer, as well as with the author of the article. Instead of the variable a1 , the use of a0 is suggested in the marked line. In any case, it would not hurt to give the variables clearer names.



Seventh place: logical error



An article was also written based on the re-verification of the Umbraco project. An example of an interesting, in my opinion, error from this article.



V3022 Expression 'name! = "Min" || name! = "Max" 'is always true. Probably the '&&' operator should be used here. DynamicPublishedContentList.cs 415

 private object Aggregate(....) { .... if (name != "Min" || name != "Max") { throw new ArgumentException( "Can only use aggregate min or max methods on properties which are datetime"); } .... } 

An ArgumentException exception will be thrown for any value of the name variable. And all because of the erroneous use in the condition of the operator || instead of &&.



Sixth place: erroneous exit condition



The article on checking the Accord.Net project contains a description of several interesting errors. I chose two, one of which is again related to a typo.



V3015 It is likely that a variable is being compared inside the 'for' operator. Consider reviewing 'i' Accord.Audio SampleConverter.cs 611

 public static void Convert(float[][] from, short[][] to) { for (int i = 0; i < from.Length; i++) for (int j = 0; i < from[0].Length; j++) to[i][j] = (short)(from[i][j] * (32767f)); } 

The error is contained in the condition of the second for loop, the counter of which is the variable j . Using variable names of the form i , j for counters is a kind of classic of the genre. Unfortunately, these variables are very similar in spelling and developers often make typos in such code. I do not think that in this case it is worth recommending the use of more meaningful names. Nobody will do it anyway :). Therefore, I will give another recommendation: use static analyzers!

Fifth place: using a bit operator instead of a logical one



Another interesting and fairly common mistake from the article about checking Accord.Net project.



V3093 The '&' operator evaluates both operands. Perhaps a short-circuit '&& operator should be used instead. Accord.Math JaggedSingularValueDecompositionF.cs 461

 public JaggedSingularValueDecompositionF(....) { .... if ((k < nct) & (s[k] != 0.0)) .... } 

Obviously, even if the first condition is not fulfilled, erroneous use of the & operator instead of && will lead to testing the second condition, which, in turn, will result in going beyond the array.



Fourth place: one quote, two quote



In fourth place - an error from the article about checking the project Xamarin.Forms.



V3038 Replace function is equal to the second argument. ICSharpCode.Decompiler ReflectionDisassembler.cs 349

 void WriteSecurityDeclarationArgument(CustomAttributeNamedArgument na) { .... output.Write("string('{0}')", NRefactory.CSharp .TextWriterTokenWriter .ConvertString( (string)na.Argument.Value).Replace("'", "\'")); .... } 

In this case, quotes will be replaced by ... quotes. I do not think that this is exactly what the developer wanted.

And the analyzer is great!



Third place: ThreadStatic



The Mono project, about which the article was written, turned out to be rich in interesting errors. And one of them is a truly rare guest.



[ThreadStatic] V3089 Initializer The field will have a default value on different threads. System.Data.Linq-net_4_x Profiler.cs 16

 static class Profiler { [ThreadStatic] private static Stopwatch timer = new Stopwatch(); .... } 

Briefly: incorrect initialization of the field marked with the ThreadStatic attribute . The documentation for the diagnosis provides a detailed description of the situation, as well as advice on how to avoid such errors. A great example of a mistake that is not so easy to find and fix by ordinary means.



Second place: Copy-Paste, reference!



One of the reference, in my opinion, examples of errors such as Copy-Paste is contained in the article already mentioned earlier about the verification of the Mono project.



V3012 The '?:' Operator, regardless of its conditional expression, always returns one and the same value: Color.FromArgb (150, 179, 225). ProfessionalColorTable.cs 258



Here is a brief snippet of code in which the error was found:

 button_pressed_highlight = use_system_colors ? Color.FromArgb (150, 179, 225) : Color.FromArgb (150, 179, 225); 

You may ask: “What is it about?” Was it worth putting such an obvious mistake in second place of the charts? The fact is that the given code fragment was additionally formatted for clarity. Now prepare yourself and imagine that you are faced with the task of finding a similar error in the code without using tools. So, please, faint of heart, to remove, below is a screenshot, which contains the complete code fragment with an error:

It is obvious that any programmer can make such a mistake, regardless of his qualifications. A successful search for such errors demonstrates the power of static analysis.



First place: PVS-Studio



Yes, you did not think. “PVS-Studio” is really written here. And he is in the first place of our hit parade. But not only because it is a good static analyzer. And also because our team employs ordinary people who make ordinary human errors in the code. It was about this and the article was written at the time. It was an error, or rather two at once, which were found in the PVS-Studio code with the help of PVS-Studio.



V3022 Expression 'RowsCount> 100000' is always false. ProcessingEngine.cs 559



V3022 Expression 'RowsCount> 200000' is always false. ProcessingEngine.cs 561

 public void ProcessFiles(....) { .... int RowsCount = DynamicErrorListControl.Instance.Plog.NumberOfRows; if (RowsCount > 20000) DatatableUpdateInterval = 30000; //30s else if (RowsCount > 100000) DatatableUpdateInterval = 60000; //1min else if (RowsCount > 200000) DatatableUpdateInterval = 120000; //2min .... } 

The result of the work of this code fragment (provided that RowsCount> 20000 ) will always be a DatatableUpdateInterval value equal to 30000.



Fortunately, we have already done some work in this direction.

Due to the widespread use of incremental analysis in our team, the appearance of articles on finding errors in PVS-Studio with the help of PVS-Studio will be very unlikely in the future.



Conclusion



I want to note that now any of you can compile your hit parade of errors found by using the opportunity to use the free PVS-Studio static analyzer to test your own projects.



Download and try PVS-Studio: http://www.viva64.com/en/pvs-studio/



For questions about the acquisition of a commercial license, please contact us in the mail. You can also email us to get a temporary license for a comprehensive study of PVS-Studio if you want to remove the restrictions of the demo version.







If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Khrenov. Top 10 C # projects errors found in 2016