Geekly Articles each Day
⬆️ ⬇️

We are looking for errors in MonoDevelop



An important event took place in the life of the PVS-Studio analyzer - the latest version added the ability to check the code written in C #. Being one of the developers of this analyzer, I just could not pass by without checking out any project. It is clear that very few people will be interested in reading about checking small and unknown projects, so you had to choose something known, and the choice fell on MonoDevelop.



Little about the project



MonoDevelop is a free development environment for creating C #, Java, Boo, Nemerle, Visual Basic .NET, Vala, CIL, C and C ++ applications. Support is also planned for Oxygene from Embarcadero Technologies.







Initially, it was the SharpDevelop port on Mono / GTK +, but since that time the project has gone far from its initial state.

')

MonoDevelop is part of the Mono project. Built into the distribution of Unity3D as a means of writing scripts, but the outdated version (4.0.1).



Among the features of this development environment, syntax highlighting, code folding, code code completion, class browser, plugin support, built-in debugger, visual form designer, unit testing are highlighted.



The source code of the project is available in the corresponding repository on GitHub , and the assembly instructions are described on the official project website.



What was checked?



As mentioned above, the project was analyzed using the latest version of the PVS-Studio static code analyzer, to which the ability to analyze the C # code was added. This is the first release of the C # analyzer, and at the moment it has implemented more than 40 diagnostic rules. It is clear that this version is still far from being developed as much as the C ++ analyzer, but using this tool you can already find quite interesting errors (some of which will be given in this article). The C # analyzer is not a separate product, it is part of the same PVS-Studio, which now simply knows how to analyze code written in another programming language.



Download the latest version of the analyzer at this link .



A few words about the analysis result



As a result of the analysis, 8457 files were included as part of 95 projects.



The analyzer issued 118 warnings of the first, 128 warnings of the second and 475 warnings of the third level.



Some may say that this is not so much for so many files. But here it is worth taking into account the fact that at the moment a smaller number of diagnostics is implemented than in the C ++ analyzer. Secondly, the analyzer is ineffective with one-time checks. Although this has been repeated many times , it is worth mentioning once again - to get the full benefits of using static analysis tools, they should be used regularly, rather than once. This will save time on finding and eliminating errors, and as a result - will make project development cheaper and easier.



Analysis results



The article will discuss some of the most interesting errors found, since a review of all errors found would increase the volume of this article to indecent sizes. The article is divided into subsections containing a description of certain types of errors with examples of code from the project. So you can go directly to viewing the most interesting mistakes for you.



Same expressions to the left and right of the operator



This subsection provides descriptions of errors like 'A || A '. Often, such errors are the result of typos or unsuccessful 'copy-paste' and carelessness of the programmer. Often, such errors can be difficult to find in large amounts of code, especially if the variable names are long enough and differ only in one character. As a rule, the use of another variable is implied, but sometimes such checks are simply redundant code. More about all this below. 

protected override SourceCodeLocation GetSourceCodeLocation (string fixtureTypeNamespace, string fixtureTypeName, string methodName) { if (string.IsNullOrEmpty (fixtureTypeName) || string.IsNullOrEmpty (fixtureTypeName)) return null; .... }


Analyzer Warning: V3001 There are identical sub-expressions 'string.IsNullOrEmpty (fixtureTypeName)' operator. MonoDevelop.NUnit NUnitProjectTestSuite.cs 84



The error can be seen with the naked eye - in the condition the same string variable is checked twice for equality 'null' or for equivalence 'String.Empty'. Below the code (not the whole body is shown here, so as not to complicate perception, so take a word) a similar check is performed for the variable 'fixtureTypeNamespace', so it can be assumed that the second check of this condition should take the variable methodName as the method argument 'or absent altogether.



Another example of a similar error: 

 bool TryAddDocument (string fileName, out OpenRazorDocument currentDocument) { .... var guiDoc = IdeApp.Workbench.GetDocument (fileName); if (guiDoc != null && guiDoc.Editor != null) .... guiDoc.Closed += (sender, args) => { var doc = sender as MonoDevelop.Ide.Gui.Document; if (doc.Editor != null && doc.Editor != null) .... } .... }


Analyzer Warning: V3001 There are identical sub-expressions 'doc.Editor! = Null' operator. MonoDevelop.AspNet RazorCSharpParser.cs 180



Again 2 identical checks within the same expression. Theoretically, after casting the 'sender' variable using the 'as' operator, the value 'null' can be written to the 'doc' variable. As a result, an exception of the 'NullReferenceException' type will be generated when the 'doc.Editor! = Null' check is performed. The corrected version of the code could look like this: 

 if (doc != null && doc.Editor != null)


Another piece of code with an error: 

 static MemberCore GetLaterDefinedMember (MemberSpec a, MemberSpec b) { var mc_a = a.MemberDefinition as MemberCore; var mc_b = b.MemberDefinition as MemberCore; if (mc_a == null) return mc_b; if (mc_b == null) return mc_a; if (a.DeclaringType.MemberDefinition != b.DeclaringType.MemberDefinition) return mc_b; if (mc_a.Location.File != mc_a.Location.File) return mc_b; return mc_b.Location.Row > mc_a.Location.Row ? mc_b : mc_a; }


Analyzer Warning: V3001 There’s the mc_a.Location.File to the left! ICSharpCode.NRefactory.CSharp membercache.cs 1319



Such an error may not be evident, but the analyzer is not a person, and does not miss such things. The code shows that the property 'File' of the object 'mc_a' is compared with itself, but it is obvious that it should be compared with the corresponding property of the object 'mc_b'.



Correct code: 

 if (mc_a.Location.File != mc_b.Location.File)


Redundant code: 

 public override AppResult Property (string propertyName, object value) { if (resultIter != null && resultIter.HasValue) { var objectToCompare = TModel.GetValue (resultIter.Value, Column); return MatchProperty (propertyName, objectToCompare, value); } return MatchProperty (propertyName, ParentWidget, value); } TreeIter? resultIter;


Analyzer Warning: V3001 There are identical sub-expressions 'resultIter! = Null' operator. MonoDevelop.Ide GtkTreeModelResult.cs 125



The variable 'resultIter' is nullable-type, therefore, checks of the form 'resultIter! = Null' and 'resultIter.HasValue' are identical and one could restrict one of them.



Exactly the same code met once more. Appropriate analyzer warning:



V3001 There are identical sub-expressions 'resultIter! = Null' operator. MonoDevelop.Ide GtkTreeModelResult.cs 135



Consider the following code snippet: 

 Accessibility DeclaredAccessibility { get; } bool IsStatic { get; } private bool MembersMatch(ISymbol member1, ISymbol member2) { if (member1.Kind != member2.Kind) { return false; } if (member1.DeclaredAccessibility != member1.DeclaredAccessibility || member1.IsStatic != member1.IsStatic) { return false; } if (member1.ExplicitInterfaceImplementations().Any() || member2.ExplicitInterfaceImplementations().Any()) { return false; } return SignatureComparer .HaveSameSignatureAndConstraintsAndReturnTypeAndAccessors( member1, member2, this.IsCaseSensitive); }


Analyzer Warnings:

Another typo. And not one, but two at once. Again, the properties of the same object ('member1') are compared between themselves. Since the properties are primitive and there is no additional logic in them, such checks also lose their meaning. And from the code it is clear that the properties of the objects 'member1' and 'member2' should be compared. The correct code is: 

 if (member1.DeclaredAccessibility != member2.DeclaredAccessibility || member1.IsStatic != member2.IsStatic)




Assigning a variable to itself



Not a common type of error, as the previous one, but no less interesting. Often erroneous situations are when some member of a class in a method must assign the value of one of the arguments passed, and these names often differ only in the case of the first character. It is easy to make a mistake. There are also simple cases of assigning a variable to itself, and if these are properties, the compiler will not issue any warnings. Such actions are understandable, if complex logic is hung on the getter / setter of properties, but if there is none, the assignment looks at least strange. But let's not be unfounded, better take a look at examples of such errors. 

 public ViMacro (char macroCharacter) { MacroCharacter = MacroCharacter; } public char MacroCharacter {get; set;}


Analyzer Warning: V3005 The 'MacroCharacter' variable is assigned to itself. Mono.TextEditor ViMacro.cs 57



What was mentioned above - because the names of the property and the constructor argument differ only in the case of the first character, the value of the property is written into itself instead of writing the value passed as an argument. Looking at the definition of the property, you can make sure that it does not contain any additional logic. 

 public ViMark (char markCharacter) { MarkCharacter = MarkCharacter; } public char MarkCharacter {get; set;}


Analyzer Warning: V3005 The 'MarkCharacter' variable is assigned to itself. Mono.TextEditor ViMark.cs 45



The error is exactly the same as the previous one. Again, the first character in the variable name is confused, which is why the constructor does not work as expected. 

 public WhitespaceNode(string whiteSpaceText, TextLocation startLocation) { this.WhiteSpaceText = WhiteSpaceText; this.startLocation = startLocation; } public string WhiteSpaceText { get; set; }


Analyzer Warning: V3005 The 'this. WhiteSpaceText' variable is assigned to itself. ICSharpCode.NRefactory.CSharp WhitespaceNode.cs 65



The error is again similar to the previous one, but this time the code is more interesting because in one of the two assignments the programmer was not sealed. It is easy to miss such an error during speed typing, especially if you use automatic code substitution. However, this could have been avoided by regularly checking the new code with a static analyzer. For example, in PVS-Studio you can automatically check the new code after compilation (see incremental analysis mode ). 

 void OptionsChanged (object sender, EventArgs e) { gutterMargin.IsVisible = Options.ShowLineNumberMargin; iconMargin.IsVisible = iconMargin.IsVisible; .... } public bool IsVisible { get; set; }


Analyzer Warning: V3005 The 'iconMargin.IsVisible' variable is assigned to itself. MonoDevelop.HexEditor HexEditor.cs 241



This is the second type of error that I described at the beginning of a subsection. The value of the property is assigned to itself, but there are no local variables with the name similar to this property. In this case, the property, again, is not tied to any additional logic. Perhaps the correct code would look like this, although it is impossible to say for sure here: 

 iconMargin.IsVisible = gutterMargin.IsVisible;




Illusion of choice



An interesting subtitle, isn't it? However, he, perhaps, most accurately describes some types of errors, such as those that are detected using diagnostic messages V3004 or V3012 . The essence of this type of error is that regardless of whether the condition being checked (V3004 for the 'if' operator and V3012 for the ternary) is true or false, the same actions will always be performed, or the same result will be returned. Unfortunately, the errors diagnosed by the warning V3004 were not found in the project, but on the other hand there was a couple of warnings V3012, which will be discussed below. 

 public enum WindowCommands { NextDocument, PrevDocument, OpenDocumentList, OpenWindowList, SplitWindowVertically, SplitWindowHorizontally, UnsplitWindow, SwitchSplitWindow, SwitchNextDocument, SwitchPreviousDocument } protected static void Switch (bool next) { if (!IdeApp.Preferences.EnableDocumentSwitchDialog) { IdeApp.CommandService.DispatchCommand ( next ? WindowCommands.NextDocument : WindowCommands.NextDocument); return; } var toplevel = Window.ListToplevels () .FirstOrDefault (w => w.HasToplevelFocus) ?? IdeApp.Workbench.RootWindow; var sw = new DocumentSwitcher (toplevel, next); sw.Present (); }


Analyzer warning: V3012 The '?:' Operator, regardless of its conditional expression, always returns the same: WindowCommands.NextDocument. MonoDevelop.Ide WindowCommands.cs 254



The ternary operator will always return the same enumeration element ('WindowCommands.NextDocument'). I suppose that in case the 'next' variable is 'false', the element 'WindowCommands.PrevDocument' should have been returned.



Again, I suspect that such errors are made due to the use of code auto-substitution tools. And when you work quickly, you may not notice at all that the tool that should help in writing the code has helped in writing the error. However, these are only assumptions and arguments on this topic are beyond the scope of this article.



I met another similar example: 

 private void StartTestElement(ITestResult result) { ITest test = result.Test; TestSuite suite = test as TestSuite; if (suite != null) { xmlWriter.WriteStartElement("test-suite"); xmlWriter.WriteAttributeString("type", suite.TestType); xmlWriter.WriteAttributeString("name", suite.TestType == "Assembly" ? result.Test.FullName : result.Test.FullName); } .... }


Analyzer warning: V3012 The '?:' Operator, regardless of its conditional expression, always returns the same value: result.Test.FullName. GuiUnit_NET_4_5 NUnit2XmlOutputWriter.cs 207



As can be seen from the code fragment, the expression 'suite.TestType == "Assembly"' or false will be true, the result of the ternary operator will be the value of the 'FullName' property.



Checking the wrong variable for equality 'null' after casting by operator 'as'



And this is a situation specific to C #. And, judging by the proven projects, this is a pattern of errors, and not isolated cases. As we all know, in case the cast failed using the 'as' operator, the result is the value 'null' (unlike the explicit cast using the syntax '(type_name) arg', when an exception of the type 'InvalidCastException' is generated ). Often, after such a cast, a check is performed to ensure that it is successful. However, they often make a mistake, checking inadvertently not the result of a cast, but a reducible variable. Several such cases will be discussed below. 

 public override bool Equals (object o) { SolutionItemReference sr = o as SolutionItemReference; if (o == null) return false; return (path == sr.path) && (id == sr.id); }


Analyzer warning: V3019 Possibly incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'sr'. MonoDevelop.Core SolutionItemReference.cs 81



This code coerces the variable 'o' of type 'object' to the type of 'SolutionItemReference'. If such a cast fails, the value 'null' will be written to the variable 'sr'. As a result, the test 'o == null' will pass successfully (of course, if 'o' is not 'null'), and when checking 'path == sr.path', an exception of the type 'NullReferenceException' will be generated. All of this could have been avoided by checking the correct variable in the appropriate place: 

  if (sr == null) return false;


Another example of this code: 

 void OnTokenSelectionChanged (object sender, EventArgs args) { TreeSelection selection = sender as TreeSelection; if (sender != null) { TreeIter iter; TreeModel model = (TreeModel)tokensStore; if (selection.GetSelected (out model, out iter)) { entryToken.Text = (string)tokensStore.GetValue (iter, 0); comboPriority.Active = (int)tokensStore.GetValue (iter, 1); } else { entryToken.Text = String.Empty; comboPriority.Active = (int)TaskPriority.Normal; } } }


Analyzer warning: V3019 Possibly incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'sender', 'selection'. MonoDevelop.Ide TasksOptionsPanel.cs 123



The situation is exactly the same as the previous one. After casting 'sender' to 'TreeSelection' to 'null', the wrong variable is checked, which makes it possible to get a 'NullReferenceException'.



Similar code examples with the same error patterns were met 2 more times:



Repetitive checks for similar conditions.



There are cases when the same condition is checked twice, while the variables used in these expressions do not change between them. This mistake can lead to much more serious consequences than it seems at first glance. Which ones are better to look at with real examples. 

 public override void VisitIndexerExpression( IndexerExpression indexerExpression) { .... var localResolveResult = context.Resolve(indexerExpression.Target) as LocalResolveResult; if (localResolveResult == null) return; var resolveResult = context.Resolve(indexerExpression); if (localResolveResult == null) return; .... }


Analyzer Warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. ICSharpCode.NRefactory.CSharp.Refactoring ParameterCanBeDeclaredWithBaseTypeIssue.cs 356



From this code snippet, it is clearly seen that instead of the 'resolveResult == null' check, the 'localResolveResult == null' check is performed twice. This is clearly seen from the cut out code snippet. Whether it would also be easy to find this error by looking at the code that includes, in addition to this fragment, the main logic to the method (it is not given here, so as not to stretch the example) - a big question. In any case, instead of exiting the method if the 'resolveResult' is equal to 'null', we successfully continue the work, which means that all subsequent logic using the 'resolveResult' flies to Tartar.



Here is another example of such a mistake: 

 bool TryRemoveTransparentIdentifier(....) { .... string nae1Name = ExtractExpressionName(ref nae1); if (nae1Name == null) return false; .... string nae2Name = ExtractExpressionName(ref nae2); if (nae1Name == null) return false; .... }


Analyzer Warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. ICSharpCode.NRefactory.CSharp CombineQueryExpressions.cs 114



Again, due to the fact that the variable was mixed up for verification, the loop will not exit and the correct value will be returned, and the subsequent logic of the method will be violated.



But a more interesting example, however, containing the same error: 

 public static SW.FontWeight ToWpfFontWeight (this FontWeight value) { if (value == FontWeight.Thin) return SW.FontWeights.Thin; if (value == FontWeight.Ultralight) return SW.FontWeights.UltraLight; if (value == FontWeight.Light) return SW.FontWeights.Light; if (value == FontWeight.Semilight) return SW.FontWeights.Light; if (value == FontWeight.Book) return SW.FontWeights.Normal; if (value == FontWeight.Medium) return SW.FontWeights.Medium; if (value == FontWeight.Semibold) return SW.FontWeights.SemiBold; if (value == FontWeight.Bold) return SW.FontWeights.Bold; if (value == FontWeight.Ultrabold) return SW.FontWeights.UltraBold; if (value == FontWeight.Heavy) return SW.FontWeights.Black; if (value == FontWeight.Ultraheavy) return SW.FontWeights.UltraBlack; return SW.FontWeights.Normal; }


Well, found? Just kidding, you can only poke a finger into the sky. But for the analyzer there is no problem, and he calmly coped with the task.



Analyzer Warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. If this statement is a senseless Xwt.WPF DataConverter.cs 217



In order to better understand what the problem is, you need to look at the FontWeight enumeration. 

 public enum FontWeight { /// The thin weight (100) Thin = 100, /// The ultra light weight (200) Ultralight = 200, /// The light weight (300) Light = 300, /// The semi light weight (350) Semilight = 350, /// The book weight (380) Book = 350, .... }


The constants 'Semilight' and 'Book' have the same values, although it is clearly seen from the comments that the 'Book' constant should be 380.



What is more interesting - if the value of 'value' is equal to 380 - this method will still work correctly! In this case, none of the listed conditions will be fulfilled, and therefore - just the value that would be returned if the 'value == FontWeight.Book' condition is met. "Not a bug, but a feature"



Well, at the end of this section: 

 public override object GetData (TransferDataType type) { if (type == TransferDataType.Text) return clipboard.WaitForText (); if (type == TransferDataType.Text) return clipboard.WaitForImage (); .... }


Analyzer Warning: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the statement is a senseless Xwt.Gtk ClipboardBackend.cs 86



In this code snippet it is easy to notice a typo. Instead of repeating the 'type == TransferDataType.Text' check, it was necessary to perform the 'type == TransferDataType.Image' check.



Check for conflicting conditions



There is a code when within the same expression the same variable is checked for equality / inequality to some value. Such checks are at least redundant, and perhaps even contain an error related to the fact that the value of the wrong variable is checked a second time. Such errors were also found in the project. 

 IEnumerable<ICompletionData> CreateConstructorCompletionData(IType hintType) { .... if (!(hintType.Kind == TypeKind.Interface && hintType.Kind != TypeKind.Array)) .... }


Analyzer Warning: Consider inspecting this expression V3023 . The expression is misprint. ICSharpCode.NRefactory.CSharp CSharpCompletionEngine.cs 2397



Judging by the environment of the code, it is simply overcomplicated with checking the expression. It is not clear what this complication is for, since all this condition could be simplified to the following code: 

 if (hintType.Kind != TypeKind.Interface)


A similar case: 

 void OnUpdateClicked (object s, StatusBarIconClickedEventArgs args) { if (args.Button != Xwt.PointerButton.Right && args.Button == Xwt.PointerButton.Left) { HideAlert (); AddinManagerWindow.Run (IdeApp.Workbench.RootWindow); } }


Analyzer Warning: Consider inspecting this expression V3023 . The expression is misprint. MonoDevelop.Ide AddinsUpdateHandler.cs 97



This code fragment shows that it was not intended to use other variables for comparison, but, nevertheless, there is a redundant comparison. There is no additional logic on the 'Button' property, therefore, there are no “pitfalls” when reading it. Again, the code is easily simplified: 

 if (args.Button == Xwt.PointerButton.Left)




Incorrectly formatted strings



Often there is a code that contains errors in the format strings. As a rule, such errors are of two types:

In this project, only errors of the first type were encountered. An example of one of them: 

 ConditionExpression ParseReferenceExpression (string prefix) { StringBuilder sb = new StringBuilder (); string ref_type = prefix [0] == '$' ? "a property" : "an item list"; int token_pos = tokenizer.Token.Position; IsAtToken (TokenType.LeftParen, String.Format ( "Expected {0} at position {1} in condition \"{2}\". Missing opening parantheses after the '{3}'.", ref_type, token_pos, conditionStr, prefix)); .... IsAtToken (TokenType.RightParen, String.Format ( "Expected {0} at position {1} in condition \"{2}\". Missing closing parantheses'.", ref_type, token_pos, conditionStr, prefix)); .... }


Analyzer warning: V3025 Incorrect format. A different number of formatted items is expected while calling 'Format' function. Expected: 3. Present: 4. MonoDevelop.Core ConditionParser.cs 254



Most likely, this error was the result of an unsuccessful 'copy-paste', since the second call to the 'IsAtToken' method is similar to the first, the only difference is that it concerns the closing bracket. However, the 'prefix' argument is not used in any way. Not critical, but there is no sense from it either.



Similar warnings:

Potential dereference of the null reference



Often it is necessary to check the variables for equality of 'null', especially if this variable is the argument of the method, the result of its work, was obtained using the 'as' operator. , 'null', , , , , 'NullReferenceException'.



, - . . 

 void Replace (RedBlackTreeNode oldNode, RedBlackTreeNode newNode) { .... if (oldNode.parent.left == oldNode || oldNode == null && oldNode.parent.left == null) .... }


: V3027 The variable 'oldNode' was utilized in the logical expression before it was verified against null in the same logical expression. MonoDevelop.HexEditor RedBlackTree.cs 167



, - 'oldNode.parent.left' 'oldNode', 'null'. 'oldNode' 'null', 'NullReferenceException'. 'null'.



Conclusion



, . . , , .



- , . , , , , .



C#-



, C#- . , , .





If you want to share this article with an English-speaking audience, then please use the link to the translation: Sergey Vasiliev. Looking for Bugs in MonoDevelop .



Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/274321/

More articles:



All Articles