To assess the quality of our static analyzer's diagnostics and to advertise it, we regularly analyze various open-source projects. The developers of FlashDevelop project contacted us on their own initiative and asked us to check their product, which we have gladly done.
FlashDevelop is a popular development environment for development of Flash software. It supports such languages as Action Script 2 and 3, Haxe, JavaScript, HTML, PHP, and C#, and provides functions found in modern code editors, for example, autocomplete, integrated svn support, git, mercurial, templates, third-party plugins, syntax highlighting themes, and so on. It is noteworthy that Fireaxis Games used FlashDevelop when working on XCOM: Enemy Unknown.
Since FlashDevelop is an open-source product and is written in C#, we found it an interesting idea to check it with our analyzer. The analysis was done with PVS-Studio v6.05. The scope of an article doesn't allow us to discuss all the issues found, so we'll talk about only the most interesting ones.
As you know, strings in C# are immutable and methods used to change a string actually return a new object of type string, while the original string remains unchanged. As the experience shows, however, developers tend to forget about this detail. Here are some examples found by the analyzer:
V3010 The return value of function 'Insert' is required to be utilized. ASPrettyPrinter.cs 1263
public void emit(IToken tok)
{
....
lineData.Insert(0, mSourceData.Substring(prevLineEnd,
((CommonToken)t).StartIndex - prevLineEnd));
....
}V3010 The return value of function 'Insert' is required to be utilized. MXMLPrettyPrinter.cs 383
private void prettyPrint(....)
{
....
while (aToken.Line == currentLine)
{
lineData.Insert(0, aToken.Text);
....
}
....
}The programmer must have meant the following construct instead:
lineData = lineData.Insert(....);Another V3010 warning:
V3010 The return value of function 'NextDouble' is required to be utilized. ASFileParser.cs 196
private static string getRandomStringRepl()
{
random.NextDouble();
return "StringRepl" + random.Next(0xFFFFFFF);
}This code is flawless from the functionality viewpoint, but the call random.NextDouble() makes no sense and can be deleted.
It is a standard technique to test a value resulting from a type-conversion operation for null. Such a check is done just in case the original type cannot be cast to the desired one. Sometimes developers lose concentration when writing such a routine operation and check wrong variables. Our analyzer is tireless and always keeps track of such defects:
V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'item', 'val'. WizardHelper.cs 67
public static void SetControlValue(....)
{
....
string val = item as string;
if (item == null) continue;
....
}What should be tested for null in this example is obviously val, not item, and the code should look like this:
string val = item as string;
if (val == null) continue;Whenever you see methods with identical bodies, it makes you suspect something is wrong. At best, such code needs to be refactored; at worst, it's a result of mechanical copy-paste, which distorts the program's execution logic. Here are some examples as a proof.
V3013 It is odd that the body of 'SuspendMdiClientLayout' function is fully equivalent to the body of 'PerformMdiClientLayout' function (377, line 389). DockPanel.MdiClientController.cs 377
private void SuspendMdiClientLayout()
{
if (GetMdiClientController().MdiClient != null)
GetMdiClientController().MdiClient.PerformLayout(); // <=
}
private void PerformMdiClientLayout()
{
if (GetMdiClientController().MdiClient != null)
GetMdiClientController().MdiClient.PerformLayout();
}The bodies of the methods SuspendMdiClientLayout and PerformMdiClientLayout are completely identical, which probably results from copying a code lines. The SuspendMdiClientLayout method's name suggests that it is responsible for suspending the layout, while it actually redraws it: MdiClient.PerformLayout(). I think a correct version of this method should look like this:
private void SuspendMdiClientLayout()
{
if (GetMdiClientController().MdiClient != null)
GetMdiClientController().MdiClient.SuspendLayout(); // <=
}Here is another example. The project uses type Lexer, which is designed to perform lexical parsing of something. This type implements 28 similar looking methods with signatures following the private static bool StateXX (FsmContext ctx) pattern, where the value of XX belongs to the range from 1 to 28 inclusive. It's no wonder that a programmer may lose concentration when carrying out the routine task of writing a lengthy block of code like that, which in this case results in a bug triggering the following warning:
V3013 It is odd that the body of 'State11' function is fully equivalent to the body of 'State15' function (532, line 589). Lexer.cs 532
private static bool State11 (FsmContext ctx)
{
ctx.L.GetChar ();
switch (ctx.L.input_char) {
case 'e':
ctx.Return = true;
ctx.NextState = 1;
return true;
default:
return false;
}
}
private static bool State15 (FsmContext ctx)
{
ctx.L.GetChar ();
switch (ctx.L.input_char) {
case 'e':
ctx.Return = true;
ctx.NextState = 1;
return true;
default:
return false;
}
}The fact of two methods handling one situation is very strange. I'm not sure how to fix this issue, as the program's execution logic is known to its author alone; and I strongly doubt that this defect could be easily spotted through code review, as reading a large piece of monotonous code is way harder than writing it. On the other hand, static analyzers are very good at catching bugs like that.
The analyzer also found the following interesting fragment:
V3020 An unconditional 'break' within a loop. AirWizard.cs 1760
private void ExtensionBrowseButton_Click(....)
{
....
foreach (var existingExtension in _extensions)
{
if (existingExtension.ExtensionId
== extensionId) extension = existingExtension;
break;
}
....
}My guess is that the developer wanted to iterate through the elements of the _extensions collection to find the first existingExtension object with the corresponding extensionId and exit the loop. However, because they saved on parentheses, the loop is exited unconditionally immediately after the first iteration, which greatly affects the program's execution logic.
Conditional expressions are another common source of bugs. If an expression includes a lot of variables, boundary values, or notably complex branching, the risk of making a mistake is very high. Consider the following example:
private void SettingChanged(string setting)
{
if (setting == "ExcludedFileTypes"
|| setting == "ExcludedDirectories"
|| setting == "ShowProjectClasspaths"
|| setting == "ShowGlobalClasspaths"
|| setting == "GlobalClasspath")
{
Tree.RebuildTree();
}
else if (setting == "ExecutableFileTypes")
{
FileInspector.ExecutableFileTypes =
Settings.ExecutableFileTypes;
}
else if (setting == "GlobalClasspath") // <=
{
// clear compile cache for all projects
FlexCompilerShell.Cleanup();
}
}PVS-Studio static analyzer reports the following bug:
V3022 Expression 'setting == "GlobalClasspath"' is always false. PluginMain.cs 1194
Indeed, the else if (setting == "GlobalClasspath") condition will never execute because the same condition is found in the very first if statement, which is bad since there is some logic relying on the second condition. To make the method clearer, I would rewrite it using the switch statement.
Here's one more example of a condition that will never be executed:
V3022 Expression 'high == 0xBF' is always false. JapaneseContextAnalyser.cs 293
protected override int GetOrder(byte[] buf, int offset,
out int charLen)
{
byte high = buf[offset];
//find out current char's byte length
if (high == 0x8E || high >= 0xA1 && high <= 0xFE)
charLen = 2;
else if (high == 0xBF)
charLen = 3;
....
}The analyzer tells us that the 'high == 0xBF' expression is always false. It really is, as the value 0xBF belongs to the range high >= 0xA1 && high <= 0xFE, which is checked in the first if statement.
One more V3022 warning:
V3022 Expression '!Outline.FlagTestDrop' is always true. DockPanel.DockDragHandler.cs 769
private void TestDrop()
{
Outline.FlagTestDrop = false;
....
if (!Outline.FlagTestDrop)
{
....
}
....
}The Outline.FlagTestDrop field, which was assigned the value false and which does not change further in the code, is used in an if statement. Perhaps, this method lacks some functionality for changing that field's value. There must be some reason for using the if (!Outline.FlagTestDrop) check, after all.
When writing the code, you often need to verify some variables against null, for example, after casting it to another type, or when retrieving a collection element, and so on. In such situations, you want to make sure that the resulting variable is not equal to null, and only then do you use it. As experience shows, however, developers sometimes start using the variable immediately and only then verify it against null. Such errors are detected by the V3095 diagnostic:
V3095 The 'node' object was used before it was verified against null. Check lines: 364, 365. ProjectContextMenu.cs 364
private void AddFolderItems(MergableMenu menu, string path)
{
....
DirectoryNode node = projectTree.SelectedNode
as DirectoryNode;
if (node.InsideClasspath == node)
menu.Add(RemoveSourcePath, 2, true);
else if (node != null && ....)
{
menu.Add(AddSourcePath, 2, false);
}
....
}The projectTree.SelectedNode field is of type GenericNode, which is a base type for DirectoryNode. Casting a base-type object to a derived type might fail, which in this case will result in the node variable containing an empty reference. Nevertheless, the developer still uses the node.InsideClasspath field immediately after the type-conversion operation and only then tests the node variable for null. Handling variables in a way like that might lead to raising NullReferenceException.
The analyzer found the following potential defect in the code:
V3061 Parameter 'b' is always rewritten in method body before being used. InBuffer.cs 56
public bool ReadByte(byte b) // check it
{
if (m_Pos >= m_Limit)
if (!ReadBlock())
return false;
b = m_Buffer[m_Pos++]; // <=
return true;
}The value of argument b passed to the method is not used, although it is overwritten a bit later just to never be used anyway. Perhaps this method was meant to be implemented in a different way (this idea is also suggested by the comment "// check it"). This is what its signature should probably look like:
public bool ReadByte(ref byte b)
{
....
}The next suspicious fragment found by the analyzer can't be easily spotted through code review:
V3066 Possible incorrect order of arguments passed to '_channelMixer_OVERLAY' method: 'back' and 'fore'. BBCodeStyle.cs 302
private static float _channelMixer_HARDLIGHT(float back,
float fore)
{
return _channelMixer_OVERLAY(fore, back);
}The _channelMixer_OVERLAY method has the following signature:
static float _channelMixer_OVERLAY(float back, float fore)Perhaps it was really conceived that way. However, it looks like the arguments fore and back were swapped by mistake when being passed to the method. The analyzer is good at catching issues like that.
The V3083 diagnostic was designed to detect potentially unsafe calls to event handlers. In the project under analysis, this diagnostic found numbers of those. Let's take one example of such an unsafe call:
V3083 Unsafe invocation of event 'OnKeyEscape', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. QuickFind.cs 849
protected void OnPressEscapeKey()
{
if (OnKeyEscape != null) OnKeyEscape();
}The code appears to be fine at first sight: if the OnKeyEscape field is not equal to null, the event is called. However, using this approach is not recommended. Suppose the OnKeyEscape event has one subscriber, which unsubscribes from it (in a different thread, for example) after the field has been tested for null. Once there are no subscribers left, the OnKeyEscape field will be containing an empty reference so that attempting to call the event will cause NullReferenceException.
What's especially annoying about this error is that it's very hard to reproduce. A user might complain that it showed up after pressing ESC, but then you may press ESC a thousand times and never get it.
To make an event call safer, declare an auxiliary variable:
var handler = OnKeyEscape
if (handler != null) handler();C# 6 provides a null-conditional operator (?.), which can help simplify the code greatly:
OnKeyEscape?.Invoke();Our analyzer's heuristic capabilities help find rather interesting issues in code, for example:
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();
....
}This code must have been written using the copy-paste technique. I suspect that variable a0 should be used instead of a1 to compute the value of the b0 variable. Anyway, this defect should motivate the authors to examine this code. In any case, a better style is to use more meaningful variable names.
A few fragments were found where a caught exception is re-thrown. Here is one example:
public void Copy(string fromPath, string toPath)
{
....
try
{
....
}
catch (UserCancelException uex)
{
throw uex;
}
....
}The analyzer issues the following warning for this method:
V3052 The original exception object 'uex' was swallowed. Stack of original exception could be lost. FileActions.cs 598
Re-throwing exceptions in a way like that leads to overwriting the original call stack with a new one starting with the current method, which makes it hard to track down the method where the original exception came from, when debugging the code.
To keep the original call stack when re-throwing exceptions, just use the throw statement:
try
{
....
}
catch (UserCancelException uex)
{
throw;
}Among other defects, the analyzer found the following unsafe fragment:
V3087 Type of variable enumerated in 'foreach' is not guaranteed to be castable to the type of collection's elements. VS2005DockPaneStrip.cs 1436
private void WindowList_Click(object sender, EventArgs e)
{
....
List<Tab> tabs = new List<Tab>(Tabs);
foreach (TabVS2005 tab in tabs)
....
}The tabs collection contains elements of type Tab, which are cast to type TabVS2005 when iterating through them. This type is derived from type Tab. Such type conversion is unsafe and may cause System.InvalidCastException.
There was one more similar issue found by this diagnostic:
public int DocumentsCount
{
get
{
int count = 0;
foreach (DockContent content in Documents)
count++;
return count;
}
}The Documents collection contains elements of type IDockContent, and it may be unsafe to explicitly cast them to type DockContent.
Finally, let's take a look at a few examples of correct yet unreasonably complicated code:
V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. DockContentHandler.cs 540
internal void SetDockState(....)
{
....
if ((Pane != oldPane) || (Pane == oldPane
&& oldDockState != oldPane.DockState))
{
RefreshDockPane(Pane);
}
....
}The conditions Pane != oldPane and Pane == oldPane are mutually exclusive, so this expression can be simplified:
if (Pane != oldPane ||
oldDockState != oldPane.DockState)In a similar way, the conditional expression in the following method:
void SetProject(....)
{
....
if (!internalOpening || (internalOpening
&& !PluginBase.Settings.RestoreFileSession))
{
RestoreProjectSession(project);
}
....
}can be reduced to this code:
if (!internalOpening || !PluginBase.Settings.RestoreFileSession)FlashDevelop project has been developing over 10 years now and embraces a rather large code base. Running static code analyzers on projects like that may reveal interesting results and help developers improve their products' quality. I'm sure the authors of this project would like to study the analyzer's report. If you develop programs in C, C++, or C#, welcome to download the latest version of PVS-Studio static code analyzer and try it on your projects.
If you find that the trial version isn't enough (more), please contact us to get a product key for closer study of the analyzer's capabilities.