An important event has taken place in the PVS-Studio analyzer's life: support of C#-code analysis was added in the latest version. As one of its developers, I couldn't but try it on some project. Reading about scanning small and little-known projects is not much interesting of course, so it had to be something popular, and I picked MonoDevelop.
MonoDevelop is an open source integrated development environment for Linux, OS X, and Windows. Its primary focus is development of projects that use Mono and .NET frameworks. MonoDevelop integrates features similar to those of NetBeans and Microsoft Visual Studio, such as automatic code completion, source control, a graphical user interface (GUI) and Web designer. MonoDevelop integrates a Gtk# GUI designer called Stetic. It supports Boo, C, C++, C#, CIL, D, F#, Java, Oxygene, Vala, and Visual Basic.NET.
In late 2003, a few developers from the Mono community began migrating SharpDevelop, a successful .NET open source IDE from System.Windows.Forms on Windows to Gtk# on Linux. Being an early fork of SharpDevelop, MonoDevelop architecturally differs from recent SharpDevelop releases.
Over time, the MonoDevelop project was absorbed into the rest of the Mono project and is actively maintained by Xamarin and the Mono community. Since Mono 1.0 Beta 2, MonoDevelop is bundled with Mono releases.
It provides such features as syntax highlighting, code folding, autocomplete, class browser, plugin support, integrated debugger, GUI designer, and unit testing.
The project source code can be downloaded from the repository at GitHub, and building guidelines can be found at the project official website.
As I already mentioned, the project was analyzed with the latest version of the PVS-Studio static code analyzer, which supports analysis of C# code. It is the first release of the C#-version of our analyzer, and it currently supports over 40 diagnostic rules. It's not that well evolved compared to the C++-version of course, but it's good enough to help you find some pretty interesting bugs (and we'll discuss some of them in this article). The C#-analyzer is not a separate product; it comes as part of the PVS-Studio pack. It's just that our tool has learned how to analyze code written in one more programming language.
The recent version of the analyzer can be downloaded here.
A total of 8457 files in 95 projects were analyzed.
The analyzer output 118 warnings of the first severity level, 128 warnings of the second level, and 475 warnings of the third level.
One may argue it's not very much for so many files. Well, remember that the current version supports fewer diagnostics than the C++-version. Besides, the analyzer is not very effective when used sporadically. We said it many times already, but I need to repeat it once again: to fully benefit from the use of static analysis tools, one must use them regularly, not occasionally. That way, it will help you save time on finding and debugging errors and, therefore, make development cheaper and easier.
Further in the article, I will discuss only some of the most interesting bugs found in MonoDevelop because covering them all would make it just too big. The article is divided into subsections, each of which deals with a certain type of bugs illustrated by code samples from the project. So you can move on to the errors you find most interesting.
In this subsection, we'll discuss errors of the 'A || A' pattern. Such bugs usually result from typos or bad "copy-paste" and programmers' carelessness. They are pretty difficult to catch in lengthy code, especially when variables' names are long and differ in just one character. This bug pattern usually deals with using a wrong variable as one of the operands, but sometimes it's just redundant code. Read on to find out more.
protected override SourceCodeLocation
GetSourceCodeLocation (string fixtureTypeNamespace,
string fixtureTypeName,
string methodName)
{
if (string.IsNullOrEmpty (fixtureTypeName) ||
string.IsNullOrEmpty (fixtureTypeName))
return null;
....
}PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'string.IsNullOrEmpty (fixtureTypeName)' to the left and to the right of the '||' operator. MonoDevelop.NUnit NUnitProjectTestSuite.cs 84
The error is easy to see: one string variable is checked twice for 'null' or 'String.Empty'. A bit further in the code (I didn't quote the whole body to keep the sample short, so just take my word for it), a variable called 'fixtureTypeNamespace' is checked in a similar manner, so we can conclude that either the second check shouldn't be there at all or the method in it should take the variable 'methodName' as an argument.
This is another example of a bug of this type:
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)
....
}
....
}PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'doc.Editor != null' to the left and to the right of the '&&' operator. MonoDevelop.AspNet RazorCSharpParser.cs 180
Again, two identical checks in one expression. In theory, after the 'sender' variable is cast using the 'as' operator, the value 'null' may be assigned to the 'doc' variable. As a result, a 'NullReferenceException' will be raised when trying to execute the check 'doc.Editor != null'. This is what the fixed version of that code may look like:
if (doc != null && doc.Editor != null)One more example of the same kind:
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;
}PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'mc_a.Location.File' to the left and to the right of the '!=' operator. ICSharpCode.NRefactory.CSharp membercache.cs 1319
Errors of this type may not be easily spotted, but the analyzer is not a human and never lets such bugs slip through. As seen from the code, property 'File' of the 'mc_a' object is compared with itself while it obviously should be compared with the corresponding property of object 'mc_b' instead.
The fixed code:
if (mc_a.Location.File != mc_b.Location.File)And here is an example of 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;PVS-Studio diagnostic message: V3001 There are identical sub-expressions 'resultIter != null' to the left and to the right of the '&&' operator. MonoDevelop.Ide GtkTreeModelResult.cs 125
The variable 'resultIter' is of a nullable type; therefore, the checks 'resultIter != null' and 'resultIter.HasValue' have the same meaning and we could leave one of them out.
The same code fragment was detected one more time. This is the message for it:
V3001 There are identical sub-expressions 'resultIter != null' to the left and to the right of the '&&' operator. MonoDevelop.Ide GtkTreeModelResult.cs 135
Now take a look at the following code fragment:
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);
}PVS-Studio diagnostic messages:
A typo again. Well, even two. Properties of one and the same object ('member1') are compared with themselves. Since these properties are primitive and don't contain any additional logic, those checks don't make much sense either. Besides, the code layout itself suggests that it is the properties of the objects 'member1' and 'member2' that should be compared. This is what the fixed code should look like:
if (member1.DeclaredAccessibility != member2.DeclaredAccessibility
|| member1.IsStatic != member2.IsStatic)This bug pattern is not as common as the previous one, but it's as interesting. Errors of this type usually occur when the value of one of the arguments to a method is to be assigned to a class member and the names differ in only the first letter's case. It opens a way for mistakes. There are also simpler cases of assigning variables to themselves, when the compiler keeps silent in case these variables are properties. It's okay to have such assignments when a property's getter or setter handles complex logic, but they do look strange when there is no such logic. Here are a few examples to illustrate what I said.
public ViMacro (char macroCharacter) {
MacroCharacter = MacroCharacter;
}
public char MacroCharacter {get; set;}PVS-Studio diagnostic message: V3005 The 'MacroCharacter' variable is assigned to itself. Mono.TextEditor ViMacro.cs 57
Just as I told you, because the names of the property and constructor's argument differ only in the first letter's case, the programmer mistakenly assigned the property's value to the property itself instead of overwriting it with the value passed as the argument. The property's definition, too, suggests that it doesn't contain any additional logic.
public ViMark (char markCharacter) {
MarkCharacter = MarkCharacter;
}
public char MarkCharacter {get; set;}PVS-Studio diagnostic message: V3005 The 'MarkCharacter' variable is assigned to itself. Mono.TextEditor ViMark.cs 45
The error here is exactly the same. Again, the programmer was confused by similarly looking names, and it resulted in an unexpected behavior of the constructor.
public WhitespaceNode(string whiteSpaceText,
TextLocation startLocation)
{
this.WhiteSpaceText = WhiteSpaceText;
this.startLocation = startLocation;
}
public string WhiteSpaceText { get; set; }PVS-Studio diagnostic message: V3005 The 'this.WhiteSpaceText' variable is assigned to itself. ICSharpCode.NRefactory.CSharp WhitespaceNode.cs 65
This bug is similar to the previous two, but it's a bit more interesting this time because the programmer didn't make any typos in the assignments. When touch-typing, mistakes like this are easy to overlook, especially when you use autocomplete. It could have been avoided, though, by regularly scanning new code with a static analyzer. For example, PVS-Studio offers a feature to automatically scan freshly written code once you've got it compiled (see incremental analysis mode).
void OptionsChanged (object sender, EventArgs e)
{
gutterMargin.IsVisible = Options.ShowLineNumberMargin;
iconMargin.IsVisible = iconMargin.IsVisible;
....
}
public bool IsVisible { get; set; }PVS-Studio diagnostic message: V3005 The 'iconMargin.IsVisible' variable is assigned to itself. MonoDevelop.HexEditor HexEditor.cs 241
This is the second type of the bug pattern discussed in this subsection. Again, a property is assigned its own value, but there are no local variables with similarly looking names around. The property doesn't contain any additional logic either. The fixed version of this sample should probably look something like this, but I can't be sure:
iconMargin.IsVisible = gutterMargin.IsVisible;The title sounds interesting, doesn't it? Well, it's the most accurate term for certain types of errors such as those detected by diagnostic rules V3004 and V3012. This bug pattern has to do with performing the same actions, no matter whether the condition affecting the flow of execution is true or false (diagnostic V3004 is for the 'if' statement, and V3012 is for the ternary operator). There were no V3004 messages for this project unfortunately, but the analyzer did find a couple of V3012 warnings. Here they are.
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 ();
}PVS-Studio diagnostic message: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: WindowCommands.NextDocument. MonoDevelop.Ide WindowCommands.cs 254
The ternary operator will always return one and the same item of the enumeration ('WindowCommands.NextDocument'). My guess is that it is the item 'WindowCommands.PrevDocument' that must be returned when 'next' is 'false'.
Again, I suspect that the autocomplete option is to blame for mistakes like that. When typing fast, you may not notice how the tool designed to help you write good code starts "helping" make mistakes. It's all just guesswork though, and any speculations on this subject are beyond the scope of this article.
There was one more interesting example of that kind:
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);
}
....
}PVS-Studio diagnostic message: V3012 The '?:' operator, regardless of its conditional expression, always returns one and the same value: result.Test.FullName. GuiUnit_NET_4_5 NUnit2XmlOutputWriter.cs 207
As you can see, no matter if the 'suite.TestType == "Assembly"' expression is true or false, the ternary operator will always return 'FullName'.
And this issue is specific to C#. What's more, judging by the projects already analyzed, it really is a bug pattern rather than occasional mistakes. As we all know, when casting a variable using the 'as' operator fails, we get the value 'null' (unlike explicit cast using the '(type_name)arg syntax', when an 'InvalidCastException' is raised). After such assignment, a check is done to make sure that the cast has been successful. However, programmers tend to make a mistake and check the source variable instead of the resulting one. A few examples of this mistake are 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);
}PVS-Studio diagnostic message: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'o', 'sr'. MonoDevelop.Core SolutionItemReference.cs 81
In this code, variable 'o' of type 'object' is cast to type 'SolutionItemReference'. If the cast fails, the 'sr' variable will be assigned the value 'null'. As a result, the 'o == null' check will pass (if 'o' is not 'null' of course), and the 'path == sr.path' check will fail and trigger a 'NullReferenceException'. All of this could have been avoided by checking the right variable:
if (sr == null)
return false;One more example:
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;
}
}
}PVS-Studio diagnostic message: V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'sender', 'selection'. MonoDevelop.Ide TasksOptionsPanel.cs 123
It's just like in the previous example. After casting 'sender' to 'TreeSelection', a wrong variable is checked for 'null' and we risk getting a 'NullReferenceException'.
There were two more bugs of this pattern:
There are cases when one condition is checked multiple times, and the variables in these conditions don't change in any way between the checks. Such bugs may have much more serious implications than it might seem. The following real-life examples will show you what kind of implications exactly.
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;
....
}PVS-Studio diagnostic message: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless ICSharpCode.NRefactory.CSharp.Refactoring ParameterCanBeDeclaredWithBaseTypeIssue.cs 356
You can see it clear that the 'localResolveResult == null' condition is checked twice instead of checking 'resolveResult == null'. With this fragment singled out from the rest of the code, you can see the bug very well. But would it be as easy to spot it when looking through the whole code, which also includes the method's logic (I omitted it to keep the sample short)? Anyway, the code keeps running instead of leaving the method when 'resolveResult' equals 'null', and it means that all the subsequent logic that uses 'resolveResult' will get disrupted.
One more example:
bool TryRemoveTransparentIdentifier(....)
{
....
string nae1Name = ExtractExpressionName(ref nae1);
if (nae1Name == null)
return false;
....
string nae2Name = ExtractExpressionName(ref nae2);
if (nae1Name == null)
return false;
....
}PVS-Studio diagnostic message: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless ICSharpCode.NRefactory.CSharp CombineQueryExpressions.cs 114
Again, checking a wrong variable will prevent loop termination and return of a correct value, thus breaking the method's subsequent logic.
The following example deals with the same mistake but is a bit more interesting:
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;
}Have you found it? Relax, I'm just kidding. It would be a bad shot for a human, anyway. But the analyzer doesn't have any difficulties with it and can easily spot the error.
PVS-Studio diagnostic message: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Xwt.WPF DataConverter.cs 217
To figure out what the problem is about, we need to take a closer 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' refer to the same value, although the comment clearly states that 'Book' should refer to the value 380.
What's more interesting, the method will still work well even if 'value' equals 380! In that case, none of the conditions will be executed and the return value will be the one returned when 'value == FontWeight.Book'. "It's a feature, not a bug" (c)
And the last one to wind up this subsection:
public override object GetData (TransferDataType type)
{
if (type == TransferDataType.Text)
return clipboard.WaitForText ();
if (type == TransferDataType.Text)
return clipboard.WaitForImage ();
....
}PVS-Studio diagnostic message: V3021 There are two 'if' statements with identical conditional expressions. The first 'if' statement contains method return. This means that the second 'if' statement is senseless Xwt.Gtk ClipboardBackend.cs 86
There is a typo here that can be easily spotted. The 'type == TransferDataType.Image' condition should have been checked instead of the 'type == TransferDataType.Text' condition.
Sometimes you may come across code where one variable is checked for being equal/not equal to some values within one expression. Such checks are redundant, to say the least, and sometimes they contain bugs which deal with checking a wrong variable for the second time. Some bugs of this type were found in MonoDevelop, too.
IEnumerable<ICompletionData>
CreateConstructorCompletionData(IType hintType)
{
....
if (!(hintType.Kind == TypeKind.Interface &&
hintType.Kind != TypeKind.Array))
....
}PVS-Studio diagnostic message: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. ICSharpCode.NRefactory.CSharp CSharpCompletionEngine.cs 2397
As the code's surroundings suggest, the programmer simply wrote a more complicated implementation of an expression check than necessary. It's not clear why one would need such complication since the whole condition can be reduced to the following:
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);
}
}PVS-Studio diagnostic message: V3023 Consider inspecting this expression. The expression is excessive or contains a misprint. MonoDevelop.Ide AddinsUpdateHandler.cs 97
As seen from this fragment, the programmer didn't mean to compare other variables, but there is still a redundant check. The 'Button' property doesn't have any additional logic, so there will be no "traps" when reading it. So, again, it can be safely reduced to the following:
if (args.Button == Xwt.PointerButton.Left)It's not a rare thing when bugs occur in format strings. They usually fall under one of the following types:
The MonoDevelop project contains errors of the first type only. Here is 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));
....
}PVS-Studio diagnostic message: V3025 Incorrect format. A different number of format items is expected while calling 'Format' function. Expected: 3. Present: 4. MonoDevelop.Core ConditionParser.cs 254
This bug almost certainly results from bad 'copy-paste' since the second call on method 'IsAtToken' is similar to the first one, except that it deals with a closing parenthesis. However, it doesn't use the 'prefix' argument in any way. It's not a critical issue, but it's no use leaving it there, either.
Other messages for this bug pattern:
We often need to check variables for 'null', especially when they are used as method arguments or returned by methods or result from casting variables using the 'as' operator. Before using such variables, we need to make sure they don't refer to 'null' since a 'NullReferenceException' will be raised if we attempt to call one of the object members, for example.
However, programmers sometimes use this check after dereferencing by mistake. This project has a few of these, too.
void Replace (RedBlackTreeNode oldNode, RedBlackTreeNode newNode)
{
....
if (oldNode.parent.left == oldNode ||
oldNode == null && oldNode.parent.left == null)
....
}PVS-Studio diagnostic message: 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
In this code, one of the object fields, 'oldNode.parent.left', is compared with the 'oldNode' object itself and then both the object and the field are checked for 'null'. However, if 'oldNode' does refer to 'null', the very first check will trigger a 'NullReferenceException'. The right solution is to check the object for 'null' in the first place.
Personally I find these analysis results satisfying since there were some pretty interesting bugs among them. Far not all of the bugs found were discussed here, and many were discussed just briefly since it was clear almost from the beginning that I would collect plenty of material for an article.
Some may argue it's not much impressive for a project of this size, but keep in mind that many bugs get caught only at the testing stage while a static analyzer could help catch and fix them as early as at the coding stage, thus both making the coding and debugging processes easier and reducing the total cost of the final product.
You may like to read about analysis results for a few other open-source C#-projects. Just keep in mind that some of those were checked while the analyzer was still in development and it would probably show better results now.