There are many ways that can assist in improvement the program’s quality. In this article, we invite you to consider one of them – static code analysis.





Briefly about static code analysis

As you probably know, static analyzers allow you to check the code without running the program itself. Recently, popularity of static analysis in development has been gaining momentum, and on the top of that static analysis market is expanding every year. This is partly due to the fact that the age of linters, based only on regular expressions, is now past its prime. Nowadays static analysis is striking with its diversity and capabilities. All this hype around AI and machine learning couldn’t help but penetrate analyzers as well, and the Swiss have released the product that learns from open repositories. At the same time, we need to be aware of the fact that in the foreseeable future AI still won’t replace classic technologies, applied in static analyzers, but will enhance them.

One of the examples of modern static analyzers is the PVS-Studio tool. This tool enables you to detect errors and potential vulnerabilities in the source code of programs, written in C, C++, C#, and Java. Works in 64-bit systems on Windows, Linux, and macOS and can analyze code for 32-bit, 64-bit and embedded ARM platforms. Let’s take a quick look at technologies that PVS-Studio uses when analyzing the source code.

Let’s start with data flow analysis. It allows you to calculate possible variable values at different points in the program. With its help you can find such errors, as an array index out of bounds, memory leaks, null pointer dereference and others.

Manual and automated method annotation. Method annotation gives more information about used methods than it could be obtained by analyzing only methods’ signatures.

Pattern based analysis. When the analyzer checks the code, it can detect preliminarily specified patterns, typical for some errors. In the simplest version, this search is similar to finding bugs with regular expressions, but this option is a bit more complicated. To find bugs, the parsing tree is traversed and analyzed. From the article “Static analysis and regular expressions” you can find out why it’s not acceptable to use regular expressions for such tasks.

Symbolic execution. It allows you to detect flaws in code even when you don’t know what values of variables will be in the error line. A small example to make it more evident:

void Foo(int A, int B, int C) { if(A<B) { if(B<C) { if(A>C) { .... } } } }

Even not knowing about the values of A, B and C variables, the PVS-Studio analyzer is able to get that the condition (A > C) is always false, and report it to the developer. If you’d like to find out more about this and other principles underpinning the analyzer, you can check out the article “Technologies used in the PVS-Studio code analyzer for finding bugs and potential vulnerabilities“.

At this point, some reader might have had such thoughts. It’s all great, of course, but why do we need static analysis? I’ll give you a real-life example. I had a small pet-project – LED suits that glow and blink to the music (when you click “play” the program on the computer runs a timer that sends the RGB value to the LEDs). One day, when I made some edits in code, I turned on the suit and realized that it went crazy! The suit blinked erratically and glowed with colors that I didn’t expect at all. It seemed more like an epileptic nightmare than an LED fancy thing. It took me probably about an hour to find the error, I reread my code an unthinkable number of times, and the reason was in a banal typo in one digit… life happens.

By the way, the error I made could have been well found by static analysis.

private void saveip6_Click(object sender, RoutedEventArgs e) { saveIp(ip6.Text.ToString(), 6); .... } private void saveip7_Click(object sender, RoutedEventArgs e) { saveIp(ip6.Text.ToString(), 6); // It has to be 7 .... }

PVS-Studio warning: V3013 It is odd that the body of ‘saveip6_Click’ function is fully equivalent to the body of ‘saveip7_Click’ function (5254, line 5260). MainWindow.xaml.cs 5254

In this fragment, I copy-pasted the code that saves the ip address of costume controllers from textboxes. And, to tell the truth, the number 6 is out of my head. I don’t remember the exact handler where I wrote this failed copy-paste. And it actually doesn’t matter, the most important thing is to convey the essence.

However, I had a fairly small codebase and therefore a small amount of all sorts of errors and typos. Figures taken from Steve McConnell’s book “Code Complete” show that as the size of the project grows, so does the error density:

That is why static analysis tools are increasingly gaining popularity among large development companies.

Practice

Let’s move from theory to practice and see what errors can be caught by static code analysis. To do this, we’ll take a small real open project Extended WPF Toolkit and check it with PVS-Studio.

Extended WPF Toolkit is a collection of controls and components for WPF applications. The project includes about 600 files of source code in C#, which is about 112,000 lines of code. This free toolkit is open source and is available under the Microsoft Public License. Also developers offer to use Toolkit Plus Edition and Business Suite for paid. They have even more diverse components and controls, several themes under Metro and Windows 10 and more.

However, all these details are not very important to us. The main thing is that this is an ordinary model project, written in C#. Let’s look at some of the bugs that were found in it. I hope that these examples will be enough to get a general idea of the static code analysis technology. You can fully evaluate it if you download and run the analyzer on your projects. Check out also “How to quickly check out interesting warnings given by the PVS-Studio analyzer for C and C++ code?“.

PVS-Studio warning: V3006 The object was created but it is not being used. The ‘throw’ keyword could be missing: throw new InvalidOperationException(FOO). DockingManager.cs 1129

internal void InternalAddLogicalChild( object element ) { .... if(_logicalChildren.Select(ch => ch.GetValueOrDefault<object>()) .Contains( element ) ) new InvalidOperationException(); .... }

This analyzer warning indicates that the instance of the InvalidOperationException class was created but not used in the code. Seems like the programmer wanted to generate an exception when the condition is fulfilled, but forgot to write the throw operator that would throw the exception.

PVS-Studio warning: V3083 Unsafe invocation of event ‘PropertyChanged’, NullReferenceException is possible. Consider assigning event to a local variable before invoking it. CheckListsView.xaml.cs 124

public event PropertyChangedEventHandler PropertyChanged; protected void OnPropertyChanged( string propertyName ) { if( PropertyChanged != null ) { PropertyChanged( this, new PropertyChangedEventArgs( propertyName ) ); PropertyChanged( this, new PropertyChangedEventArgs( "ModelDisplay" ) ); } }

The analyzer warns that a potentially unsafe event handler call has been created. The problem with this code is that a single check for null in this case is not enough. In a multi threaded application between the check for null and the code in then branch with the if statement, the code in another thread might execute which will cancel subscription for this event. If it happens, there will be no subscribers which will result in NullReferenceException.

There are several ways to rewrite this code to enable safe execution of event call. I’ll give just one example. It’s for developers to decide whether they should use my version, choose another one, or leave the code as it is.

protected void OnPropertyChanged( string propertyName ) { PropertyChangedEventHandler eventHandler = PropertyChanged; if( eventHandler != null ) { eventHandler( this, new PropertyChangedEventArgs( propertyName ) ); eventHandler( this, new PropertyChangedEventArgs( "ModelDisplay" ) ); } }

In this example, we save the reference to the event handler in the eventHandler variable. It will contain the reference to the initial handler and the code will execute correctly even if the event gets unsubscribed and there are no subscribers.

I found more than 30 similar problems in the code. It will be a bit boring if we consider all alike warnings so I suggest that the authors try to find and fix them themselves.

PVS-Studio warning: V3117 Constructor parameter ‘ignore’ is not used. AnimationRate.cs 59

private AnimationRate( bool ignore ) { _duration = 0; _speed = double.NaN; _rateType = RateType.Speed; }

This warning indicates that the ignore parameter isn’t used in the code. According to its name, it’s a false positive and ‘ignore’ will be soon removed from this code. If it’s so, I suggest using the ‘Obsolete’ attribute, which is used right in such cases.

[Obsolete("remove the ignore parameter")] private AnimationRate( bool ignore ) { _duration = 0; _speed = double.NaN; _rateType = RateType.Speed; }

PVS-Studio warning: V3114 IDisposable object ‘reader’ is not disposed before method returns. CSharpFormat.cs 211

protected override string MatchEval( ....) //protected override { if( match.Groups[ 1 ].Success ) //comment { StringReader reader = new StringReader( match.ToString() ); .... } }

The analyzer points out that the reader object of the StringReader class implements the ‘IDisposable’ interface, but the Dispose() method for this object hasn’t been called in code. In fact, there is a twofold situation here. Indeed, the StringReader class implements this interface, but StringReader inherits it from the base class and it doesn’t own any resources, therefore calling Dispose() is not necessary in this case.

PVS-Studio warning: V3030 Recurring check. The ‘Layout.ActiveContent != null’ condition was already verified in line 2319. DockingManager.cs 2327

private void OnLayoutRootPropertyChanged( object sender, PropertyChangedEventArgs e ) { .... else if( e.PropertyName == "ActiveContent" ) { if( Layout.ActiveContent != null ) { //set focus on active element only after a layout pass is //completed //it's possible that it is not yet visible in the visual tree //if (_setFocusAsyncOperation == null) //{ // _setFocusAsyncOperation = Dispatcher.BeginInvoke( // new Action(() => // { if( Layout.ActiveContent != null ) FocusElementManager.SetFocusOnLastElement( Layout.ActiveContent); //_setFocusAsyncOperation = null; // } ), DispatcherPriority.Input ); //} } .... } }

The analyzer draws our attention to the fact that one and the same value is checked for null twice in a row. Perhaps, the check is redundant, but it is also possible that the second condition should look in another way. It seemed that this code was simply not finished.

PVS-Studio warning:

V3084 Anonymous function is used to unsubscribe from ‘HeaderDragDelta’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ChildWindow.cs 355

V3084 Anonymous function is used to unsubscribe from ‘HeaderIconDoubleClicked’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ChildWindow.cs 356

V3084 Anonymous function is used to unsubscribe from ‘CloseButtonClicked’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ChildWindow.cs 357

public override void OnApplyTemplate() { .... if( _windowControl != null ) { _windowControl.HeaderDragDelta -= ( o, e ) => this.OnHeaderDragDelta( e ); _windowControl.HeaderIconDoubleClicked -= ( o, e ) => this.OnHeaderIconDoubleClick( e ); _windowControl.CloseButtonClicked -= ( o, e ) => this.OnCloseButtonClicked( e ); } .... if( _windowControl != null ) { _windowControl.HeaderDragDelta += ( o, e ) => this.OnHeaderDragDelta( e ); _windowControl.HeaderIconDoubleClicked += ( o, e ) => this.OnHeaderIconDoubleClick( e ); _windowControl.CloseButtonClicked += ( o, e ) => this.OnCloseButtonClicked( e ); } .... }

In this code, _windowControl unsubscribes from the event and then subscribes back. The problem lies in the way in which events are manipulated through lambda expressions. The point is that each declaration of the anonymous function results in creating a separate delegate instance. To use anonymous functions correctly when subscribing to events and cancelling subscription, you need to save those lambda handlers in variables, and then use those. This can be implemented, for example, as follows:

_event = (o, e) => this.OnHeaderDragDelta (o, e);

Similar analyzer warnings:

V3084 Anonymous function is used to unsubscribe from ‘Loaded’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. ChildWindow.cs 644

V3084 Anonymous function is used to unsubscribe from ‘HeaderDragDelta’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. MessageBox.cs 327

V3084 Anonymous function is used to unsubscribe from ‘HeaderIconDoubleClicked’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. MessageBox.cs 328

V3084 Anonymous function is used to unsubscribe from ‘CloseButtonClicked’ event. No handlers will be unsubscribed, as a separate delegate instance is created for each anonymous function declaration. MessageBox.cs 329

PVS-Studio warning: V3013 It is odd that the body of ‘OnMaxScaleChanged’ function is fully equivalent to the body of ‘OnMinScaleChanged’ function (656, line 695). Zoombox.cs 656

private static void OnMinScaleChanged( DependencyObject o, DependencyPropertyChangedEventArgs e ) { Zoombox zoombox = ( Zoombox )o; zoombox.CoerceValue( Zoombox.MinScaleProperty ); zoombox.CoerceValue( Zoombox.ScaleProperty ); } private static void OnMaxScaleChanged( DependencyObject o, DependencyPropertyChangedEventArgs e ) { Zoombox zoombox = ( Zoombox )o; zoombox.CoerceValue( Zoombox.MinScaleProperty ); zoombox.CoerceValue( Zoombox.ScaleProperty ); }

In this code the analyzer has found two functions OnMinScaleChanged and OnMaxScaleChanged, implemented in a similar way. Also, the MaxScaleProperty property was created in the code. I suspect, in the second case the code should look as follows:

private static void OnMaxScaleChanged( DependencyObject o, DependencyPropertyChangedEventArgs e ) { .... zoombox.CoerceValue( Zoombox.MaxScaleProperty ); .... }

Similar analyzer warnings:

V3013 It is odd that the body of ‘OnCoerceLeft’ function is fully equivalent to the body of ‘OnCoerceTop’ function (299, line 355). WindowControl.cs 299

V3013 It is odd that the body of ‘OnMouseLeftButtonDown’ function is fully equivalent to the body of ‘OnMouseRightButtonDown’ function (156, line 162). LayoutDocumentControl.cs 156

PVS-Studio warning: V3031 An excessive check can be simplified. The ‘||’ operator is surrounded by opposite expressions ‘newValue != null’ and ‘newValue == null’. Selector.cs 181

public IList SelectedItems { .... private set { .... { .... { if(((newValue != null) && !newValue.Contains(item)) || (newValue == null)) { .... } } } .... }

This code is redundant and needs to be simplified, as reported by the analyzer. The thing is that there are (newValue != null) and (newValue == null) expressions on the left and on the right of the ‘||’ operator. At first, it seems that the logic of the program will suffer with simplification, because in the first sub-expression not only the presence of any value in the newValue variable is checked, but also item. On the other hand, if we write like this, not only will the program efficiency improve, but so will the code readability:

if (newValue == null || !newValue.Contains(item))

Similar errors found by the analyzer:

V3031 An excessive check can be simplified. The ‘||’ operator is surrounded by opposite expressions ‘oldValue != null’ and ‘oldValue == null’. Selector.cs 198

V3031 An excessive check can be simplified. The ‘||’ operator is surrounded by opposite expressions. ObjectContainerHelper.cs 85

PVS-Studio warning: V3051 An excessive type cast. The object is already of the ‘Magnifier’ type. MagnifierManager.cs 62

private void Element_MouseLeave( object sender, MouseEventArgs e ) { var magnifier = MagnifierManager.GetMagnifier( _element ) as Magnifier; .... } public static Magnifier GetMagnifier( UIElement element ) { return ( Magnifier )element.GetValue( CurrentProperty ); }

The analyzer reports that the developer has cast the object to its own type. This check is redundant. This is not an error and one can leave the var keyword in the magnifier declaration, but it will be more clear if one explicitly sets the variable type.

Usually, an error description is followed by a list of fragments with the similar erroneous code, but in this case, I won’t be able to write all warnings. There were more than 50 (!) similar analyzer warnings which in my opinion is too many. Not to mention the ones from the Low level, I didn’t look though them as searchingly as I did for other levels.

PVS-Studio warning: V3116 Consider inspecting the ‘for’ operator. It’s possible that the loop will be executed incorrectly or won’t be executed at all. CollectionControl.cs 642

internal void PersistChanges( IList sourceList ) { .... { .... { { var list = (IList)collection; list.Clear(); if( list.IsFixedSize ) { if( sourceList.Count > list.Count ) throw new IndexOutOfRangeException(....); for(int i = 0; i < sourceList.Count; ++i ) // <= list[ i ] = sourceList[ i ]; } .... } .... } .... }

The code inside the for loop will never execute for the following reasons. First, the program clears list, then compares the sourceList size with list (and generates the exception if the number of elements in sourceList is more than in the empty list). After that it tries to fill the list with values from sourceList via the loop.

PVS-Studio warning: V3020 An unconditional ‘break’ within a loop. LayoutRoot.cs 542

public void CollectGarbage() { bool exitFlag = true; .... do { exitFlag = true; .... foreach( .... ) { .... while( singleChild.ChildrenCount > 0 ) { .... } exitFlag = false; break; } } while( !exitFlag ); .... }

Regardless of the singleChild.ChildrenCount value, due to the break statement only one iteration of the foreach loop executes. Anyway, the code is very strange. It’s not clear if it’s a bug, maybe it was written intentionally…

Conclusion

With the example of the Extended WPF Toolkit project, we’ve witnessed the importance of static analysis in course of creating a program product. WPF Toolkit is a relatively small project. Nonetheless, in those 112,000 lines of code we’ve stumbled upon quite a few alike errors: similarly implemented methods, objects cast to their own types and others. All these flaws can be well detected using static code analysis, highly recommended to project authors. Especially since their code is open and posted on GitHub, as there is a free PVS-Studio analyzer option. You’re welcome to check it out in detail by the link.