# Checking the Source Code of FlashDevelop with PVS\-Studio 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\. ![0412_FlashDevelop/image1.png](https://import.viva64.com/docx/blog/0412_FlashDevelop/image1.png) ## Introduction [FlashDevelop](http://www.flashdevelop.org/) 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_](https://xcom.com/xcom-enemy-unknown/)\. ## Analysis results 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\. ### Method return values unused 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](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Insert' is required to be utilized\. ASPrettyPrinter\.cs 1263 ```cpp public void emit(IToken tok) { .... lineData.Insert(0, mSourceData.Substring(prevLineEnd, ((CommonToken)t).StartIndex - prevLineEnd)); .... } ``` [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'Insert' is required to be utilized\. MXMLPrettyPrinter\.cs 383 ```cpp private void prettyPrint(....) { .... while (aToken.Line == currentLine) { lineData.Insert(0, aToken.Text); .... } .... } ``` The programmer must have meant the following construct instead: ```cpp lineData = lineData.Insert(....); ``` Another V3010 warning: [V3010](https://pvs-studio.com/en/docs/warnings/v3010/) The return value of function 'NextDouble' is required to be utilized\. ASFileParser\.cs 196 ```cpp 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\. ### Testing for null after type conversion 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](https://pvs-studio.com/en/docs/warnings/v3019/) Possibly an incorrect variable is compared to null after type conversion using 'as' keyword\. Check variables 'item', 'val'\. WizardHelper\.cs 67 ```cpp 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: ```cpp string val = item as string; if (val == null) continue; ``` ### Duplicate method bodies 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](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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 ```cpp 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\. ### Unconditional loop termination The analyzer also found the following interesting fragment: [V3020](https://pvs-studio.com/en/docs/warnings/v3020/) An unconditional 'break' within a loop\. AirWizard\.cs 1760 ```cpp 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\. ### Always true/false expression 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: ```cpp 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](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression 'high \=\= 0xBF' is always false\. JapaneseContextAnalyser\.cs 293 ```cpp 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](https://pvs-studio.com/en/docs/warnings/v3022/) Expression '\!Outline\.FlagTestDrop' is always true\. DockPanel\.DockDragHandler\.cs 769 ```cpp 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\. ### Using an instance before testing it for null 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](https://pvs-studio.com/en/docs/warnings/v3095/) The 'node' object was used before it was verified against null\. Check lines: 364, 365\. ProjectContextMenu\.cs 364 ```cpp 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_\. ### Overwriting the value of a passed argument The analyzer found the following potential defect in the code: [V3061](https://pvs-studio.com/en/docs/warnings/v3061/) Parameter 'b' is always rewritten in method body before being used\. InBuffer\.cs 56 ```cpp 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: ```cpp public bool ReadByte(ref byte b) { .... } ``` ### Arguments passed to a method in the wrong order The next suspicious fragment found by the analyzer can't be easily spotted through code review: [V3066](https://pvs-studio.com/en/docs/warnings/v3066/) Possible incorrect order of arguments passed to '\_channelMixer\_OVERLAY' method: 'back' and 'fore'\. BBCodeStyle\.cs 302 ```cpp private static float _channelMixer_HARDLIGHT(float back, float fore) { return _channelMixer_OVERLAY(fore, back); } ``` The _\_channelMixer\_OVERLAY_ method has the following signature: ```cpp 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\. ### Unsafe call to an event handler The [V3083](https://pvs-studio.com/en/docs/warnings/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](https://pvs-studio.com/en/docs/warnings/v3083/) Unsafe invocation of event 'OnKeyEscape', NullReferenceException is possible\. Consider assigning event to a local variable before invoking it\. QuickFind\.cs 849 ```cpp 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: ```cpp var handler = OnKeyEscape if (handler != null) handler(); ``` C\# 6 provides a null\-conditional operator \(?\.\), which can help simplify the code greatly: ```cpp OnKeyEscape?.Invoke(); ``` ### Potential typos Our analyzer's heuristic capabilities help find rather interesting issues in code, for example: [V3056](https://pvs-studio.com/en/docs/warnings/v3056/) Consider reviewing the correctness of 'a1' item's usage\. LzmaEncoder\.cs 225 ```cpp 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\. ### Re\-throwing exceptions A few fragments were found where a caught exception is re\-thrown\. Here is one example: ```cpp public void Copy(string fromPath, string toPath) { .... try { .... } catch (UserCancelException uex) { throw uex; } .... } ``` The analyzer issues the following warning for this method: [V3052](https://pvs-studio.com/en/docs/warnings/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: ```cpp try { .... } catch (UserCancelException uex) { throw; } ``` ### Potential raising of InvalidCastException when iterating through a collection Among other defects, the analyzer found the following unsafe fragment: [V3087](https://pvs-studio.com/en/docs/warnings/v3087/) Type of variable enumerated in 'foreach' is not guaranteed to be castable to the type of collection's elements\. VS2005DockPaneStrip\.cs 1436 ```cpp private void WindowList_Click(object sender, EventArgs e) { .... List tabs = new List(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: ```cpp 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_\. ### Redundant conditions Finally, let's take a look at a few examples of correct yet unreasonably complicated code: [V3031](https://pvs-studio.com/en/docs/warnings/v3031/) An excessive check can be simplified\. The '\|\|' operator is surrounded by opposite expressions\. DockContentHandler\.cs 540 ```cpp 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: ```cpp if (Pane != oldPane || oldDockState != oldPane.DockState) ``` In a similar way, the conditional expression in the following method: ```cpp void SetProject(....) { .... if (!internalOpening || (internalOpening && !PluginBase.Settings.RestoreFileSession)) { RestoreProjectSession(project); } .... } ``` can be reduced to this code: ```cpp if (!internalOpening || !PluginBase.Settings.RestoreFileSession) ``` ## Conclusion 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](https://pvs-studio.com/en/pvs-studio/download/) of PVS\-Studio static code analyzer and try it on your projects\. If you find that the trial version isn't enough \([more](https://pvs-studio.com/en/blog/posts/0395/)\), please [contact](https://pvs-studio.com/en/about-feedback/) us to get a product key for closer study of the analyzer's capabilities\.