Checking the source code of FlashDevelop with PVS-Studio

To check the quality of diagnostics of our static analyzer and its advertising, we regularly analyze open source projects. The developers of the FlashDevelop project themselves asked us to check their product, which we gladly did.

Introduction

FlashDevelop is a popular Flash development environment that supports Action Script versions 2 and 3, Haxe, JavaScript, HTML, PHP, C #, and has the functionality inherent in modern code editors, such as code completion, built-in support for svn, git, mercurial, templates , third-party plug-ins, syntax highlighting themes and more. It is noteworthy that FlashDevelop used Fireaxis Games in the development of XCOM: Enemy Unknown .

Test results

Considering that FlashDevelop is an open source product and is written in C #, we wanted to test it with our analyzer. For the analysis, the static analyzer PVS-Studio v6.05 was used. Since it is not possible to sort out all the problem areas found within this article, we consider the most interesting messages of the analyzer.

Unused return values ​​of methods

As you know, strings in C # are immutable objects, and the methods responsible for changing the string actually return a new object of type String, keeping the original string unchanged. However, as practice shows, developers forget about this feature. For example, the analyzer detected the following errors:
')
V3010 ASPrettyPrinter.cs 1263

public void emit(IToken tok) { .... lineData.Insert(0, mSourceData.Substring(prevLineEnd, ((CommonToken)t).StartIndex - prevLineEnd)); .... } 

V3010 MXMLPrettyPrinter.cs 383

 private void prettyPrint(....) { .... while (aToken.Line == currentLine) { lineData.Insert(0, aToken.Text); .... } .... } 

Probably, the developer had in mind this design:

 lineData = lineData.Insert(....); 

Another example of the V3010 diagnostic trigger:

V3010 The return value of the function 'NextDouble' is required to be utilized. ASFileParser.cs 196

 private static string getRandomStringRepl() { random.NextDouble(); return "StringRepl" + random.Next(0xFFFFFFF); } 

This code does not contain an error from the point of view of the functional, however, the call to random.NextDouble () does not carry any meaning and can be deleted.

Check for null after casting

The standard practice after a type cast operation is to check the resulting value for null in case the original type cannot be brought to the desired one. When performing such a routine operation, the developer may be inattentive and check the wrong variable. Our analyzer does not get tired and is closely watching such things:

V3019 Possibly incorrectly variable conversion 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; .... } 

Obviously, in this example, the null variable should be checked for the variable val , not item , and the code should look like this:

 string val = item as string; if (val == null) continue; 

Duplication of method bodies

When in a code there are methods with identical bodies, it always causes suspicions. At best, such code requires refactoring, and at worst, mechanical copy-paste distorts the logic of the program. Not to be unfounded, consider the following examples.

V3013 It is odd that the body of the SuspendMdiClientLayout function is fully equivalent to the body of the 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(); } 

As we can see, the bodies of the SuspendMdiClientLayout and PerformMdiClientLayout methods are absolutely identical. This was probably due to copying the code. The name of the SuspendMdiClientLayout method assumes that it is responsible for suspending the layout, however, the layout is redrawn instead: MdiClient.PerformLayout () . I assume that the correct implementation of this method should be like this:

 private void SuspendMdiClientLayout() { if (GetMdiClientController().MdiClient != null) GetMdiClientController().MdiClient.SuspendLayout(); //<= } 

Another example. The project is implemented type Lexer , intended for the lexical analysis of something. In this type, 28 methods of the same type are implemented with signatures of the type private static bool StateXX (FsmContext ctx) , where XX is in the range from 1 to 28. It is not surprising that when performing this amount of routine work, the developer’s eyes may be blurred, which resulted in in the error code, to which the PVS-Studio analyzer responds as follows:

V3013 It is odd that the body of the State11 function is fully equivalent to the body of the 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 that the two methods handle the same situation seems highly suspicious. It is not clear how to fix this problem, the logic of the work is known only to the author. I also doubt that this problem could easily be detected during the code review, because reading a large number of monotonous code is much more difficult than writing it. On the other hand, static analyzers are helpful here.

Unconditional exit from a cycle

Further analysis revealed an interesting point:

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; } .... } 

I would venture to suggest that the developer wanted to run through the elements of the _extensions collection, find the first existingExtension object with the appropriate extensionId, and exit the loop. But because of the savings on the parentheses, the loop certainly ends after the first iteration, which significantly affects the logic of the program.

Expression is always true / false

Another common source of error is conditional expressions. If the expression includes a large number of variables, boundary values, a fairly complex branch, the probability of an error increases. Consider, for example, the following conditional operator:

 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(); } } 

The PVS-Studio static analyzer reports the following error:

V3022 Expression 'setting == "GlobalClasspath"' is always false. PluginMain.cs 1194

Indeed, the condition else if (setting == "GlobalClasspath") will never be fulfilled, because the same condition is present in the very first if . But the fulfillment of some condition depends on the fulfillment of this condition. To simplify the readability of this method, I would rewrite it using the switch statement .

The following example is a never-satisfiable condition:

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 expression 'high == 0xBF' is always false. This is true because the value 0xBF falls into the range high> = 0xA1 && high <= 0xFE , checked in the first if .

Another example message from the V3022 diagnostic:

V3022 Expression '! Outline.FlagTestDrop' is always true. DockPanel.DockDragHandler.cs 769

 private void TestDrop() { Outline.FlagTestDrop = false; .... if (!Outline.FlagTestDrop) { .... } .... } 

In this snippet, we see that the Outline.FlagTestDrop field, which is assigned the value false and which does not change further in the code, is used in the conditional if statement . Perhaps, this method did not implement the functionality that changes the value of this field, because for some reason the developer implemented the if (! Outline.FlagTestDrop) check .

Using an instance before it was checked for null

In practice, there is always a need to check the variable for null, for example, after casting, selecting an item from the collection, etc. In such situations, we check that the resulting variable is not null, and then use it. However, as our practice shows, the developer can start using the received object immediately, and only then check that it is not null. Such errors are reported by the V3095 diagnostics:

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); } .... } 

In this example, the projectTree.SelectedNode field is of type GenericNode , which is the base type for DirectoryNode . Casting an object of a base type to a derived type may fail, and as a result, the node variable may contain a null reference. However, as we can see, after the type cast operation, the developer immediately accesses the node.InsideClasspath field, and only then checks the node variable for null. Such an implementation can lead to a NullReferenceException .

Overwrites the value of the argument passed.

The analyzer revealed such a potentially problematic place 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 the argument passed to this method b is not used, then overwritten, and still not further. It can be assumed that this method is not implemented as intended (the comment " // check it " also hints at this). Perhaps the signature of this method should look like this:

 public bool ReadByte(ref byte b) { .... } 

Invalid order of arguments passed to method

The following suspicious place found by the analyzer is not so easy to notice during the code review:

V3066 method can be 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 that is exactly what was intended. However, it is likely that when referring to this method, the fore and back arguments were interchanged. And the analyzer helps to check such places.

Insecure call to event handler

Diagnostics V3083 is designed to detect potentially unsafe calls to event handlers. In the analyzed project, this diagnosis revealed a large number of such places. Let us examine the situation of an unsafe handler call using a specific example:

V3083 Unsafe invocation of event 'OnKeyEscape', NullReferenceException is possible. Consider assigning an event to a local variable before invoking it. QuickFind.cs 849

 protected void OnPressEscapeKey() { if (OnKeyEscape != null) OnKeyEscape(); } 

At first glance, the code seems to be absolutely correct: if the OnKeyEscape field is not null, we call this event. However, this approach is not recommended. Suppose that the OnKeyEscape event has one subscriber, and suppose that after checking this field for null, this subscriber unsubscribes from this event (on another thread, for example). After the event has no subscribers left, the OnKeyEscape field will contain a blank reference, and an attempt to trigger an event will result in a NullReferenceException.

It is especially unpleasant that this is an extremely difficult to reproduce error. User may complain that pressing ESC resulted in an error. However, even pressing ESC a thousand times, it is unlikely that the programmer will be able to understand what is wrong.

You can secure the event call by declaring an additional intermediate variable:

 var handler = OnKeyEscape if (handler != null) handler(); 

In C # version 6, a check for null (?.) Operator appeared, which allows to simplify the code considerably:

 OnKeyEscape?.Invoke(); 

Possible typos

The heuristic capabilities of our analyzer allow us to detect very interesting suspicious places in the 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(); .... } 

It is likely that this code was written by copy-paste. And it seems to me that the variable a0 should be used instead of a1 to calculate the value of the variable b0 . In any case, the found suspicious place should serve as a reason for developers to look closely at this code. And in general, it is better to use more informative variable names.

Re-forwarding exceptions

In the code, several places were found in which the intercepted exception is forwarded further. This is implemented, for example, as follows:

 public void Copy(string fromPath, string toPath) { .... try { .... } catch (UserCancelException uex) { throw uex; } .... } 

When checking this method, the analyzer displays the following message:

V3052 The original exception object 'uex' was swallowed. Stack of original exception could be lost. FileActions.cs 598

Such throwing an exception causes the original call stack to be overwritten by a new one, starting with the current method. This makes it very difficult to find the place where the original exception occurred during debugging.

To preserve the original call stack when re-generating exceptions, you just need to use the throw statement:

 try { .... } catch (UserCancelException uex) { throw; } 

The likely occurrence of an InvalidCastException exception while traversing a collection

Further analysis of the code revealed a potentially unsafe place:

V3087 cas in variable variable cas cas cas cas cas cas 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 , while iterating elements of this collection are reduced to type TabVS2005 , which is a successor of type Tab . This cast is unsafe and can cause a System.InvalidCastException exception.

The same diagnostics found similar unsafe code:

 public int DocumentsCount { get { int count = 0; foreach (DockContent content in Documents) count++; return count; } } 

Here, the Documents collection contains IDockContent elements, and explicitly converting them to the DockContent type may be unsafe.

Redundant condition check

Well, and finally, let's look at examples of code that does not contain errors, but, nevertheless, overly complicated:

V3031 Anonymous 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 terms Pane! = OldPane and Pane == oldPane are mutually exclusive, and therefore this expression can be simplified:

 if (Pane != oldPane || oldDockState != oldPane.DockState) 

Similarly, a conditional expression in another method:

 void SetProject(....) { .... if (!internalOpening || (internalOpening && !PluginBase.Settings.RestoreFileSession)) { RestoreProjectSession(project); } .... } 

can also be simplified to:

 if (!internalOpening || !PluginBase.Settings.RestoreFileSession) 

Conclusion

The FlashDevelop project has been developing for more than 10 years and has a rather large code base. The use of static code analyzers on such projects brings interesting results and improves the quality of the software product. I think the project developers will be interested to look at the errors found. I suggest all developers in C, C ++ or C # to download the latest version of the PVS-Studio static code analyzer and check their projects.

If the trial version is not enough ( details ), then we suggest contacting us and getting a key for a more detailed study of the tool.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Pavel Kusnetsov. Checking the Source Code of FlashDevelop with PVS-Studio .