Our website uses cookies to enhance your browsing experience.
Your message has been sent. We will email you at
If you do not see the email in your inbox, please check if it is filtered to one of the following folders:
Webinar: Evaluation - 05.12
We continue analyzing various C# projects in order to show the abilities of the static code analyzer, PVS-Studio. In this article, we are providing analysis results of WPF examples by the Infragistics Company. Infragistics is a major software vendor, founded in 1989. It gained popularity mainly through the development of enterprise-ready user interface toolsets for developers, which are run on all platforms, including .NET.
Our tool - PVS-Studio 6.00 static code analyzer - had a number of C# general analysis diagnostics, that we implemented using our experience of developing the C++ analyzer. Starting with PVS-Studio 6.01, we are creating diagnostics specifically for the C# language. For a start we have chosen dependency properties that are used in WPF projects. Such a choice was made for a reason - DependencyProperties are quite difficult to create. The difficulty is that it is very easy to make a typo in the similar code that WPF tends to be made up of. We have developed a number of diagnostics [3044, 3045, 3046, 3047, 3048, 3049] specifically for the analysis of dependencies of this type.
As we know, one of peculiarities of DependencyProperty, is that any error during the registration of DependencyProperty can cause a program to crash during the runtime. The programmers have to fix these errors by running the program again and again; thus a programmer spends precious minutes and - in sum total - hours, looking for typos in the template code of DependencyProperty. On top of this, the WPF analysis showed that not all errors can be detected after the first run of the program.
The first test subject for our diagnostics, was the code of test examples from the Infragistics Company. The archive was downloaded on the 2nd of February; there are 11 projects that can be downloaded as one archive.
The analysis was done with the static code analyzer, PVS-Studio 6.01.
A large part of the projects are written on the basis of pre-used code, and this is where the analyzer detected the most errors.
Error N1
In the "IGExtensions.Common.WPF" project, in the file "LambertConformalConic.cs" we saw the following string of "DependencyProperty" registration:
public static readonly DependencyProperty CentralMeridianProperty
= DependencyProperty.Register("CentralMeridianProperty",
typeof(double), typeof(LambertConformalConic),
new PropertyMetadata(0.0,
new PropertyChangedCallback(UpdateConstants)));V3045 WPF: the names of the registered property 'CentralMeridianProperty', and the property 'CentralMeridian', do not correspond with each other. LambertConformalConic.cs 130
As you can see, during the registration of DependencyProperty, in its name "CentralMeridianProperty" was written instead of "CentralMeridian". This error of incorrect copying of the variable name occurs quite often, but it is especially dangerous because of the following fact:
To write/read in the dependency property from the C# code, the programmers create the following property:
public double CentralMeridian {
get { return (double)GetValue(CentralMeridianProperty); }
set { SetValue(CentralMeridianProperty, value); }
}When addressing from xaml markup, binding is written for the "CentralMeridian" property. WPF is smart enough to find the CentralMeridian property and read the original value from there, but the changes in the CentralMeridian values won't be processed.
Error N2
Continuing the topic of typos in the names of the registered dependency properties, let's have a look at the following error in the "TransverseMercator.cs" file of the "IGExtensions.Common.WPF"project.
public static readonly DependencyProperty CentralMeridianProperty
= DependencyProperty.Register("LongitudeOrigin", typeof(double),
typeof(TransverseMercator), new PropertyMetadata(0.0,
new PropertyChangedCallback(UpdateConstants)));
public double CentralMeridian { .... }V3045 WPF: the names of the registered property 'LongitudeOrigin', and of the property 'CentralMeridian', do not correspond with each other. TransverseMercator.cs 95
As the practice shows, several dependency properties are written by copying the same string, and editing it later. In other words, by using Copy-Paste. Quite often, we see that in the similar code a variable is omitted and gets a different name, the one that was the closest in the list. Taking into account that the list is somewhere in the Notepad [Notepad++, Sublime Text and such] in a different window, you can check only manually if the required objects were created. It's especially hard to detect such errors because the code is generally working, but in reality - only partially.
Error N3
The situation with the names of the registered properties is quite clear, but where else can a programmer make an error creating DependencyProperty? Another variant - is in the types of values that the properties should contain. Here is such an example - "IGExtensions.Common.WPF" project, "PropertyBrushColorEditor.cs" file.
public static readonly DependencyProperty BrushColorProperty =
DependencyProperty.Register(BrushColorPropertyName,
typeof(Brush), typeof(PropertyBrushColorEditor),
new PropertyMetadata(null, (sender, e) =>
{....})
);
public SolidColorBrush BrushColor
{
get { return (SolidColorBrush)GetValue(BrushColorProperty); }
set { SetValue(BrushColorProperty, value); }
}V3046 WPF: the type registered for DependencyProperty does not correspond with the type of the property used to access it.
It's good if you don't have questions, as to why it is not correct to specify the parent class "Brush" during the registration, and to specify the heir class "SolidColorBrush" addressing through the "BrushColor" property. If it's not so, let's have a look at a simplified case of such a "game" with the stored types.
Consider a simple case. Let's create a simple WPF project, and add to the class the following dependence property:
public static DependencyProperty MyIndexProperty =
DependencyProperty.Register("MyIndex", typeof(int),
typeof(MainWindow), new FrameworkPropertyMetadata(1));
int MyIndex
{
get { return (int)GetValue(MyIndexProperty); }
set { SetValue(MyIndexProperty, value); }
}In xaml markup we'll write the following:
....
Title="MainWindow" Height="350" Width="525"
DataContext="{Binding RelativeSource =
{RelativeSource Mode=Self}}">
<Grid>
<Grid.RowDefinitions>
<RowDefinition Height="Auto" />
<RowDefinition Height="Auto" />
<RowDefinition Height="Auto" />
</Grid.RowDefinitions>
<TextBlock Grid.Row="0" Text="{Binding Path=MyIndex}"/>
<Slider Grid.Row="1" Name="slider1"
Value="{Binding Path=MyIndex}" Maximum="100" />
<Button Grid.Row="2" Click="Button_Click">
Read value
</Button>
</Grid>And add to the window class the code for pressing the button:
private void Button_Click(object sender, RoutedEventArgs e)
{
this.Title = this.MyIndex.ToString();
}That's it. As you can see, everything works. We move the slider, the number changes. Click on the button, and the window title is immediately changed to the current value on the slider. By the way, and as you probably noticed, the TextBlock displays integer values.
And now let's change the "int" type to the common "object" type in the registered DependencyProperty.
public static DependencyProperty MyIndexProperty =
DependencyProperty.Register("MyIndex", typeof(object),
typeof(MainWindow), new FrameworkPropertyMetadata(1));Let's leave the rest unchanged, and rerun the program.
The program started and now when we move the slider, real values are displayed in the TextBlock. But it's not hard to guess, that if we press the button the program will crash, as it won't be able to convert a real value in MyIndexProperty into an integer one in the property MyIndex. It seems like a small thing, but it led to really bad consequences.
Error N4
We have provided some error examples which are common for the majority of projects (so sad, that they are rarely fixed), but there are some "local" errors - for example in IGEquityTrading project:
public static readonly DependencyProperty
AxisFinancialIndicatorYTemplateProperty =
DependencyProperty.Register("AxisFinancialIndicatorYTemplate",
typeof(DataTemplate),
typeof(DataChartEx),
new PropertyMetadata(default(DataTemplate)));
public DataTemplate AxisCategoryYTemplate{
get { return (DataTemplate)
GetValue(AxisFinancialIndicatorYTemplateProperty); }
set {
SetValue(AxisFinancialIndicatorYTemplateProperty, value); }
}V3045 WPF: the names of the property registered for DependencyProperty, and of the property used to access it, do not correspond with each other. DataChartEx.cs 469
Infragistics steps on the same rake by creating a property with the "AxisCategoryYTemplate" name, instead of the registered name "AxisFinancialIndicatorYTemplate".
Error N5
public static readonly DependencyProperty
FinancialIndicatorSeriesTemplateProperty =
DependencyProperty.Register("FinancialIndicatorTemplate",
typeof(DataTemplate),
typeof(DataChartEx),
new PropertyMetadata(default(DataTemplate)));
public DataTemplate FinancialIndicatorSeriesTemplate {
get { return (DataTemplate)
GetValue(FinancialIndicatorSeriesTemplateProperty); }
set {
SetValue(FinancialIndicatorSeriesTemplateProperty, value); }
}V3045 WPF: the names of the property registered for DependencyProperty, and of the property used to access it, do not correspond with each other. DataChartEx.cs 344
In the last case, the error most likely occurred after refactoring, when the variable was specified, and the word "Series" was inserted in the middle of the phrase "FinancialIndicatorTemplate". What's more interesting, it was changed everywhere, even in XAML markup and in "#region", but the name of the registered property remained unchanged.
At the same time, the registered name "FinancialIndicatorTemplate" isn't used anywhere. We already know what this can lead to.
We didn't see any other WPF errors in these builds from the Infragistics Company. As was already mentioned, the majority of WPF diagnostics are designed to find bugs before compiling and running the project. These projects with the examples have already been checked by programmers and QA specialists. Additionally, these projects were also viewed by the users who could judge the quality and operability of the tool, working with the test examples. I guess if they noticed an error, they notified the developers.
Of course, there are other errors in these builds besides the WPF ones. The analyzer issued several hundred warnings in total. Not all of the messages indicate a real error. Many warnings (for example, comparing double type with constant), are simply not relevant for this type of project. It's not a big problem, because the analyzer provides several mechanisms to suppress uninteresting messages.
In any case, there are a lot of warnings, and most of them show the anomalies in the code. These are real mistakes or code "smell". Therefore we recommend that developers do the analysis themselves, and examine all the analyzer warnings. In this article we'll have a look at the most interesting ones:
public bool IsValid
{
get {
var valid =
double.IsNaN(Latitude) || double.IsNaN(Latitude) ||
this.Weather.DateTime == Weather.DateTimeInitial;
return valid;
}
}V3001 There are identical sub-expressions 'double.IsNaN(Latitude)' to the left and to the right of the '||' operator. WeatherStation.cs 25
Programmers have a hard life. They must understand not only the programming, but also the areas in which the program should work. It turns out that they must understand the subject area, and know some specific words "Credit", "Debit", Latitude", "Longitude", for example, and so it just adds complexity, especially if the concepts are similar in spelling. It turns out that we mistakenly write checks of the same variable: double.IsNaN(Latitude) || double.IsNaN(Latitude).
Next error:
private static int clipSegment(....)
{
if (xmax > rc.Right && xmax > rc.Right)
{
return -1;
}
}V3001 There are identical sub-expressions 'xmax > rc.Right' to the left and to the right of the '&&' operator. Geometry. Geometry.CubicSpline.cs 529
It's quite a common thing - to check the limits of a variable, but it's quite easy to make an error writing symbols after, and in, the variable. To avoid such errors you should stick to the following pattern: The common variable is written from different sides in the expressions.
if (xmin < rc.Right && rc.Right < xmax)It's harder to make a mistake, and it becomes more readable.
P.S. The same trick, however, won't work in Entity Framework; the program will crash during the conversion of LINQ code to SQL. So here's the case :)
Infragistics developers put too much thought into these checks. Besides the error given above, the same error repeated in the following strings:
private static int clipSegment(....)
{
....
if (ymin < rc.Top && ymin < rc.Top) // <= here
....
if (ymax > rc.Bottom && ymax > rc.Bottom) // <= and here
....
}For the diagnostic V3001 it is still not enough, and it continues the expansion. Here is another example of its work:
private static bool IsInDesignModeStatic(this Application app)
{
....
if (_isInDesignMode != null && _isInDesignMode.HasValue)
return _isInDesignMode.Value;
....
}V3001 There are identical sub-expressions '_isInDesignMode != null' to the left and to the right of the '&&' operator. NavigationApp.cs 415
In this case we have redundant code, not an error. This was enough:
if (_isInDesignMode.HasValue)Another warning of V3001
void ParagraphSettingsPreviewAdapter_PropertyChanged(
object sender, PropertyChangedEventArgs e) {
....
if (LineSpacingType == Infrastructure.LineSpacingTypes.Exactly
|| LineSpacingType == Infrastructure.LineSpacingTypes.Exactly){
....
}V3001 There are identical sub-expressions 'LineSpacingType == Infrastructure.LineSpacingTypes.Exactly' to the left and to the right of the '||' operator. ParagraphSettingsPreviewAdapter.cs 268
It's not quite clear what the programmer meant here, but not what is really written.
Let's move on from V3001 to V3010.
There are a couple of function calls in the "IGEarthQuake.WPF" project.
public MapViewModel() {
....
WeakPropertyChangedListener.CreateIfNecessary(_service, this);
....
}V3010 The return value of function 'CreateIfNecessary' is required to be utilized. MapViewModel.cs 42
public TimeLineViewModel(){
....
WeakPropertyChangedListener.CreateIfNecessary(_service, this);
....
}V3010 The return value of function 'CreateIfNecessary' is required to be utilized. TimeLineViewModel.cs 50
The same rather simple function is called in both cases. Let's look at its implementation:
public static
WeakPropertyChangedListener CreateIfNecessary(object source,
IPropertyChangedListener listener){
INotifyPropertyChanged inpc = source as INotifyPropertyChanged;
return inpc != null ?
new WeakPropertyChangedListener(inpc, listener) : null;
}As you can see, this feature does not bring any global changes, and its result is also not used. So here is the question - why was it called at all? Looks very suspicious...
A similar example is in the "IGHospitalFloorPlan.WPF" project:
private void ParseAllShapefiles() {
....
this.ShapeFilesMaxBounds.Expand(new Thickness(10, 10, 10, 10));
....
}V3010 The return value of function 'Expand' is required to be utilized. HospitalView.xaml.cs 52
Its implementation is slightly trickier, but ultimately it just returns a new object that is never used.
We've reached the middle of the article. Have a look at this picture; relax, and then we'll continue.
One of the most common types of error, is a bad Copy-Paste:
public static EsriMapImageryView
GetImageryView(EsriMapImageryStyle imageryStyle){
....
if (imageryStyle ==
EsriMapImageryStyle.UsaPopulationChange2010Overlay)
return EsriMapImageryViews.UsaPopulationChange2010Overlay;
if (imageryStyle ==
EsriMapImageryStyle.UsaPopulationChange2010Overlay)
return EsriMapImageryViews.UsaPopulationChange2010Overlay;
....
}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 EsriMapImageryView.cs 97
In this case, the same code is under the same condition. At this stage, the error is a bad (redundant) Copy-Paste method. But after refactoring, it may happen that the programmer changes the body of the inferior if function, that is never executed, and an error occurs in the program logic.
Let's see other errors that occurred in the code of Infragistics company.
Warning V3022 was issued for the following string:
public static double GenerateTemperature(GeoLocation location){
....
else if (location.Latitude > 10 || location.Latitude < 25)
....
else if (location.Latitude > -40 || location.Latitude < 10)
....
}
public static WeatherCondition GenerateWeatherCondition(....){
....
else if (location.Latitude > 10 || location.Latitude < 25)
....
else if (location.Latitude > -40 || location.Latitude < 10)
....
}All errors are detected by this diagnostic:
V3022 Expression 'location.Latitude > -40 || location.Latitude < 10' is always true. Probably the '&&' operator should be used here.
What else can we say? Probably the same thing as in the description of one of the errors, found by V3001. It's useful to use this pattern when the same variable is written from both sides of the expression:
if (xmin < rc.Right && rc.Right < xmax)At this point we'll stop examining errors of the first level, and move on to the second and third level, because the same message number, depending on the situation, has a different priority.
The analyzer issues diagnostic warnings of the third level, when it's not quite sure of its correctness. Also third level is for those diagnostics which are not relevant for all projects.
In practice, the warnings of the third level are very rarely the signs of real bugs. Often these are false positives or messages that show some smell code, which is still working quite correctly. In any case, if there's time, these diagnostic messages should be explored, and code refactoring done.
Let's start with the code that has two identical functions:
// 0 reference
public static double Ramp(double a) {
return a - Math.Floor(a);
}
// 1 reference
public static double Frac(double a) {
return a - Math.Floor(a);
}V3013 It is odd that the body of 'Ramp' function is fully equivalent to the body of 'Frac' function (28, line 33). Math.cs 28
If Frac function has some meaning, then only in Pascal language; while Ramp has no analogues, or I just haven't found them. The counters of the fragments where this function is used speak for themselves (see the comments).
Let's have a look at a case when this error appeared on the second level.
public void StartCurrent()
{
StartTask("Current");
}
public void StopCurrent()
{
StartTask("Current");
}V3013 It is odd that the body of 'StartCurrent' function is fully equivalent to the body of 'StopCurrent' function (503, line 507). DataViewModel.cs 503
Apparently, in the second case, the function "StartTask" was confused with "StopTask; both of these functions are present in the code, and they act quite clearly according to their names.
Now let's look at a series of messages related to the following code:
{
IsUpdating = true;
....
IsUpdating = false;
}The similar code can be seen in 4 fragments (in every build).
Initially it seems that this variable is used for cross-thread communication. But as it turned out, in practice, this variable is nowhere to be found, except in strings which the diagnostic message was issued for.
Well, suppose you decide to use this variable for cross-thread synchronization. And then this nasty surprise waiting for us. Variable declaration looks as follows:
protected bool IsUpdating = false;As you can see, there is no "volatile" keyword, and as a result, the compiler optimizes it successfully, and it will work in completely the wrong way.
What else was found in the code? For example, some extra evaluations:
Example 1:
public static void Normalize(....)
{
var x = rect.X < boundingRect.X ? boundingRect.X : rect.X;
x = (rect.X + rect.Width) > boundingRect.Right ?
boundingRect.X : rect.X;
}V3008 The 'x' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 96, 95. RectEx.cs
Example 2:
private static GradientStopCollection fromInterpolation(....){
....
Color color=ColorTool.FromAHSV(ahsv[0],
ahsv[1],
ahsv[2],
ahsv[3]);
color = ColorTool.FromARGBInterpolation(min, p, max[i].Color);
....
}V3008 The 'color' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 165, 163. BrushTool.cs
Sometimes we come across very amusing code fragments:
private void UpdateAutoSavedState() {
AutoSaved = true;
AutoSaved = false;
}V3008 The 'AutoSaved' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 691, 690. ShellViewModel.cs 691
For those who are still in doubt, I provide property declaration:
private bool autoSaved;
public bool AutoSaved
{
get { return autoSaved; }
set { autoSaved = value; }
}And again there is no "volatile", or something of this kind, that would speak about the hidden meaning of this action.
Let's move on to another group of strings with error V3029:
public void OnPropertyChanged(PropertyChangedEventArgs ea) {
....
var index = this.SelectedBrushCollectionIndex;
....
if (index >= 0)
DebugManager.LogData(this.BrushCollectionList[index].ToText());
if (index >= 0)
this.SelectedBrushCollectionIndex = index;
....
}V3029 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 338, 339.
public static void EnableSeriesMouseDoubleClick(
this XamGeographicMap geoMap, bool isEnabled = true){
....
if (geoMap != null) geoMap.SeriesMouseLeftButtonDown +=
OnSeriesMouseLeftButtomDown;
if (geoMap != null) geoMap.SeriesMouseLeftButtonUp +=
OnSeriesMouseLeftButtonUp;
....
if (geoMap != null) geoMap.SeriesMouseLeftButtonDown -=
OnSeriesMouseLeftButtomDown;
if (geoMap != null) geoMap.SeriesMouseLeftButtonUp -=
OnSeriesMouseLeftButtonUp;
....
}V3029 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 92, 93. GeoMapAdapter.cs 92
V3029 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 100, 101. GeoMapAdapter.cs 100
public void SyncSeriesViewPropertyChanges() {
if (this.SeriesView != null)
this.SeriesView.PropertyUpdated += OnSeriesViewPropertyUpdated;
if (this.SeriesView != null)
this.SeriesView.PropertyChanged += OnSeriesViewPropertyChanged;
}V3029 The conditional expressions of the 'if' operators situated alongside each other, are identical. Check lines: 342, 343. GeoSeriesLayer.cs 342
As it is often said - "just in case"....
Although these are not errors, repeated checking clutters up your code, and makes it difficult to understand.
And here is redundant code, which most likely appeared during refactoring.
public Frame NavigationTarget
{
get { return (Frame)this.GetValue(NavigationTargetProperty); }
set {
var targetFrame = value as Frame;
if (targetFrame != null)
this.SetValue(NavigationTargetProperty, value);
}
}"value" already has the Frame type, casting is pointless. But in this case, it is necessary to consider the situation in a broader sense. Infragistics does the check against null when writing to a DependencyProperty. The developers provided a callback function "ValidateValueCallback" for checks of this kind. This function is set when you register a dependency property, and it checks the values that are written into DependencyProperty .
Once again our Rainbow Unicorn in shining armor detected a considerable number of problem areas (the article does not list all errors we found). The developers can now fix the code, and make it better than it was... Than it was when it was being written ... When it was being tested... Than when it was rewritten, run, and when it crashed again and again, or worked in ways other than it should ...
In my practice on my previous job, there were really tough times at the weekends and nights, several days before the deadline, when we had to do a lot of work in a very short period of time. The whole team knew what to do, but because of haste and tiredness it took more time to debug the code. I.e. we write code, run it, and it does not work as intended. We stop everything, put a breakpoint and run it again. Perform all actions repeatedly, set the breakpoint, and check string by string what is happening. Jumping back and forth along code, and reviewing values in variables. But in the end it turns out that we misplaced a variable or a character in the condition... That's how 15 minutes are spent looking for a simple typo during the Copy-Paste.
Project analysis is just the tip of the huge iceberg of problems which occur during the creation of code.
No one is immune to errors. Even writing the code that should be exemplary in the company, it is impossible to avoid errors.
My sincere advice to you-use PVS-Studio analyzer on a regular basis. It has all sorts of useful features. For example, there is a mode in which the changed files are rechecked - you don't have to run it - the analyzer checks itself what is necessary, and issues warnings where it is needed.
0
0
0
0