Geekly Articles each Day
📜 ⬆️ ⬇️

Checking WPF source code for examples from Infragistics


We continue to test various C # projects to demonstrate the capabilities of the PVS-Studio static code analyzer. In this article, we will review the results of testing WPF examples from Infragistics. Infragistics itself is a global software provider founded in 1989. The company has made a name for itself in the development of user interface components for third-party developers on all platforms, including .NET.

In PVS-Studio 6.00, we basically implemented C # general diagnostics based on the experience of the C ++ analyzer we created earlier. Starting from PVS-Studio 6.01, we started creating diagnostics specialized for C # language. For the start, the dependency properties (DependencyProperty) used in WPF projects were selected. On DependencyProperty the choice fell due to the complexity of their creation. The difficulty is that it is very easy to make a typo in the same type of code, which WPF projects. We have developed a number of diagnostics [ 3044 , 3045 , 3046 , 3047 , 3048 , 3049 ], specializing in the analysis and testing of similar properties.

As you know, one of the features of DependencyProperty is that in most cases, any error during their registration will cause the program to crash at the execution stage (at runtime). Willy-nilly, programmers fix similar errors by running the program again and again. Thereby, precious minutes are spent searching for a typo in the template code for creating the DependencyProperty, and a total of hours are spent. At the same time, as the practice of checking WPF projects has shown, not all errors in dependency properties are obvious after the first launch of the program.

The first test code for these diagnostics was the code of test examples from Infragistics . The archive was loaded 02.02.2016 from here , the number of projects reaches 11 pieces, but you can download them all in one archive .
')
Checking the source code was performed using the PVS-Studio 6.01 static analyzer.

WPF errors


Many projects are based on a common reusable code, in which most errors are found.

Error n1

In the project “IGExtensions.Common.WPF” in the file “LambertConformalConic.cs” the following registration line “DependencyProperty” was found:
public static readonly DependencyProperty CentralMeridianProperty = DependencyProperty.Register("CentralMeridianProperty", typeof(double), typeof(LambertConformalConic), new PropertyMetadata(0.0, new PropertyChangedCallback(UpdateConstants)));

V3045 WPF: The Central Meridian Project, and the Central Meridian, do not correspond to each other. LambertConformalConic.cs 130

As you can see, when registering a DependencyProperty in its name, it was indicated “CentralMeridianProperty” instead of “CentralMeridian”. The error is made quite often due to copying the name of a variable, but it carries the following danger.

Namely, for writing / reading in a dependency property, C # code creates the following interlayer property:
 public double CentralMeridian { get { return (double)GetValue(CentralMeridianProperty); } set { SetValue(CentralMeridianProperty, value); } }

When accessing from the xaml markup, the binding write to the "CentralMeridian" property. WPF is smart enough to find the CentralMeridian property and initially read the value from there, but the value changes in CentralMeridian will not naturally be picked up.

N2 error

Continuing the topic of typos in the names of the registered dependency properties, we consider 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: 'This is a list of the' LongitudeOrigin ', and of the property of the' CentralMeridian ', do not correspond with each other. TransverseMercator.cs 95

As practice shows, dependency properties are prescribed en masse, with the help of multiple duplication of one line and subsequent edits. In other words, using Copy-Paste . And often, somewhere in this block of the same type of code, one variable is passed and another name is written to it, which was next to it in the list. And given that the list itself is usually located somewhere in the [Notepad, Notepad ++, Sublime Text, etc.] notebook next to another window, you can check the creation of all the necessary objects only with your eyes. To identify such errors is very difficult, because The code is quite working, but the truth is only half.

Error n3

With errors in the names of the registered properties everything seems to be clear, but where else can you make a mistake when creating a DependencyProperty? Well, for example, in the types of values ​​that should be contained in them, as in the file “PropertyBrushColorEditor.cs” of the same project “IGExtensions.Common.WPF”.
 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: DependencyProperty doesn’t correspond to the type used for access it.

If you have no questions, why it is wrong to specify the parent class "Brush" when registering, and if you use the "BrushColor" property to specify a descendant of the "SolidColorBrush", then this is good. Otherwise, we describe a simplified case of such a “game” with stored types.

For example, imagine a simple case. Create a simple WPF project and add the following dependency property to the window class:
 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 the xaml markup we 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">   </Button> </Grid>

And add the code for the button press event to the window class:
 private void Button_Click(object sender, RoutedEventArgs e) { this.Title = this.MyIndex.ToString(); }

Everything. How can you make sure everything works? Move the slider, the number changes. Click on the button, and the title of the window immediately changed its text to the current value in the slider. By the way, as you can see, integer values ​​are displayed in TextBlock.

Now let's replace the type with “int” with the generic “object” type when registering a DependencyProperty:
 public static DependencyProperty MyIndexProperty = DependencyProperty.Register("MyIndex", typeof(object), typeof(MainWindow), new FrameworkPropertyMetadata(1));

Leave the rest unchanged and run the program again.

The program has started and now, when we move the slider, real values ​​are displayed in TextBlock. As it is easy to guess, if we try to press a button, the program will simply fall, because it cannot convert a real value into MyIndexProperty into an integer in the property MyIndex. It would seem a trifle, but it led to sad consequences.

Error n4

In addition to the above errors, which relate to most projects (which is especially sad, because they have never been noticed or corrected in any project), there are local errors for one project IGEquityTrading.WPF:
 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: DataChartEx.cs 469

And again Infragistics comes on the same rake, instead of the registered name "AxisFinancialIndicatorYTemplate" create a property with the name "AxisCategoryYTemplate".

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: DataChartEx.cs 344

In the latter case, the error most likely arose after refactoring, when they decided to concretize the variable, and inserted the word “Series” in the middle of the phrase “FinancialIndicatorTemplate”. The most interesting thing is that they changed it everywhere, even in the XAML markup and in "#region", but they have forgotten the name of the registered property.
At the same time, the registered name “FinancialIndicatorTemplate” is not used anywhere, and we already know what this mistake leads to.

Non-C # Errors


There were no more WPF errors in these assemblies from Infragistics. As already mentioned, most WPF diagnostics are designed specifically for pre-finding errors before compiling and running the project. And these projects with examples of the use of components have already been tested by programmers and QA specialists. Additionally, the projects were viewed by users who rated the quality and usability of the product using test examples. I think if they noticed a mistake, then most likely they notified the developers.

Naturally, besides WPF errors, there are others in these assemblies. The analyzer issued a total of several hundred warnings. Not all of the messages indicate a real error. Many warnings (for example, about comparing double type with constant) are simply not relevant for this type of project. This is not a problem, since the analyzer provides several mechanisms for suppressing uninteresting messages.

In any case, there are many warnings, and most of them indicate anomalies in the code. These are the real errors or the code "with a smell". Therefore, we recommend the authors of the project to independently carry out a project check and examine the warnings. Here we will go through only the most interesting of them:
 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)' operator. WeatherStation.cs 25

Programmers live hard. They should understand not only the programming itself, but also the area where the program should work. So it turns out that they have to understand the subject area and know words such as “Vira” (Up), “Lane” (Down), “Latitude” (Latitude), “Longitude” (Longitude), etc. This only adds complexity, especially if the concepts are similar in writing. It turns out that we erroneously write the check of the same variable: double.IsNaN (Latitude) || double.IsNaN (Latitude).

The following 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. Geometry. Geometry.CubicSpline.cs 529

Checking the extent to which a variable lies is a common thing, but it is rather easy to make mistakes in signs, and then in a variable that needs to be written. In fact, to eliminate such errors, you just need to adhere to the following pattern: the general variable in the condition is written from different sides in expressions.
 if (xmin < rc.Right && rc.Right < xmax)

Making mistakes is harder and easier to read.

PS True, such a focus does not work in the Entity Framework, when the LINQ code is converted into an SQL query, the program will crash. These are the things here :)

In fact, in this Infragistics project, I’ve been too smart with such checks. In addition to the above code, a similar error was repeated in the following lines:
 private static int clipSegment(....) { .... if (ymin < rc.Top && ymin < rc.Top) //<=  .... if (ymax > rc.Bottom && ymax > rc.Bottom) //<=   .... }

Diagnosis of the V3001 is not enough of these errors, and it continues to expand on the project :). Here is another example of its operation:
 private static bool IsInDesignModeStatic(this Application app) { .... if (_isInDesignMode != null && _isInDesignMode.HasValue) return _isInDesignMode.Value; .... }

V3001 There are identical sub-expressions '_isInDesignMode! = Null' operator. NavigationApp.cs 415

In this case, we are dealing not with an error, but with a redundant code. It was enough to write:
 if (_isInDesignMode.HasValue)

Another V3001 trigger sends us to the “IgWord.Infrastructure” project:
 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 operator. ParagraphSettingsPreviewAdapter.cs 268

What exactly the developer wanted to do here is not very clear, but clearly not what is written.

From V3001, in turn, go to V3010.

In the IGEarthQuake.WPF project, there are a couple of function calls:
 public MapViewModel() { .... WeakPropertyChangedListener.CreateIfNecessary(_service, this); .... }

V3010 CreateIfNecessary 'is required to be utilized. MapViewModel.cs 42
 public TimeLineViewModel(){ .... WeakPropertyChangedListener.CreateIfNecessary(_service, this); .... }

V3010 CreateIfNecessary 'is required to be utilized. TimeLineViewModel.cs 50

Actually, in both cases one function is called, and it is quite simple. 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; }

Indeed, as you can see, the function does not produce any global changes, and its result is also not used. So the question arises - why was she called at all? Suspicious of all this ...

A similar example is waiting for us in the project “IGHospitalFloorPlan.WPF”:
 private void ParseAllShapefiles() { .... this.ShapeFilesMaxBounds.Expand(new Thickness(10, 10, 10, 10)); .... }

V3010 The return value of the function 'Expand' is required to be utilized. HospitalView.xaml.cs 52

Its implementation is a bit more cunning, but in the end it simply returns a new object that is not used anywhere.

We are in the middle of the article. Look at the picture. Relax, and then we will continue.



One of the most common types of errors is unsuccessful Copy-Paste code:
 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 statement is a senseless statement. EsriMapImageryView.cs 97

In this case, the same code is under the same condition. At this stage, the error is an unsuccessful (redundant) Copy-Paste code. But after refactoring, a situation may arise that the programmer will change the body of the lower-level if statement, which is never executed, which will lead to errors in the program logic.

And let's see what other errors are found in the code of the company Infragistics.

For each line below, warning V3022 was issued:
 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 reduced to the following message:

V3022 Expression 'location.Latitude> -40 || location.Latitude <10 'is always true. Probably the '&&' operator should be used here.

What can I say? Probably the same as above, when describing one of the errors V3001. It is useful to use a pattern when the same variable is written from different sides in the expression:
 if (xmin < rc.Right && rc.Right < xmax)

This concludes the description of the first level errors and proceeds to the second and third, because the same message number, depending on the situation, has a different priority.

At the third level, diagnostic messages are collected that the analyzer issues when it is weakly sure that it is right. Also on level 3 are diagnostics that are not relevant for all projects.

In practice, level 3 warnings rarely reveal true errors. Often, these are still false positives or messages that may indicate a correct code, but having a “smell”. In any case, if there is time, these diagnostics should be studied and refactored the code.

Let's start with a code with 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 the Frac function has at least some sense, because a similar function was in Pascal, but the Ramp function has no equivalent, or I did not find them. Actually, the counters of the number of places where functions are used speak for themselves (see comments).

In order not to go far from the error V3013, consider the case when this error appeared at the second level.
 public void StartCurrent() { StartTask("Current"); } public void StopCurrent() { StartTask("Current"); }

V3013 It is odd that the body of the StartCurrent function is fully equivalent to the body of the StopCurrent function (503, line 507). DataViewModel.cs 503

Apparently, in the second case, the “StartTask” function was confused with “StopTask”. These two functions are in the code, and they act quite unequivocally and according to their names.

Next, we consider a series of messages related to the code below:
 { IsUpdating = true; .... IsUpdating = false; }

Such code is found already in 4 places (in each of the assemblies)
Initially, it seems that this variable is used for inter-thread communication. But as it turned out, in practice, this variable is not found anywhere except for the lines for which the diagnostics worked.

Well, suppose we decide to use this variable for inter-stream synchronization. And here we are just waiting for an unpleasant surprise. Variable declaration is as follows:
 protected bool IsUpdating = false;

As you can see, there is no “volatile” keyword, and as a result, the compiler successfully optimizes it and will not work at all as we would like.

What else is interesting in the code? For example, extra calculations:

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 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 twice twice successively. Perhaps this is a mistake. Check lines: 165, 163. BrushTool.cs

Sometimes there are generally funny code snippets:
 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 doubt this code I quote property declaration:
 private bool autoSaved; public bool AutoSaved { get { return autoSaved; } set { autoSaved = value; } }

And again, neither “volatile” nor something like that, which would indicate the hidden meaning of this action - no.

Let's move on to another group of lines 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' are located 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' are located alongside each other are identical. Check lines: 92, 93. GeoMapAdapter.cs 92

V3029 The conditional expressions of the 'if' are located 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' are located alongside each other are identical. Check lines: 342, 343. GeoSeriesLayer.cs 342

As they say - "just in case, and then suddenly" ...

Although it is not an error, repeated calibrations clutter up the code and complicate its understanding.

But the redundant code that appeared most likely 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 type Frame, the cast is meaningless. But in this case it is necessary to consider the situation in a more broad sense. Infragistics checks for null when writing to the DependencyProperty. For such checks by dependency properties developers, a callback function “ValidateValueCallback” was provided. This function checks the values ​​that are written to the DependencyProperty, and it is set when the dependency property is registered.

Conclusion


Once again, our rainbow unicorn in shining armor revealed a considerable number of problem areas (the article does not list all the errors that we discovered). Yes, now developers can fix the code and make it better than it was ... What it was, when it was written ... What it was, when it was tested ... What it was, when it was rewritten, launched, and it fell again and again or worked wrong as needed ...

, , , . , , , . Those. , , , . , . , , . - . , - … , Copy-Paste- 15 …

, , , , .

. , , .

— PVS-Studio . . , , , , , .


, : Vitaliy Alferov. Analyzing source code of WPF examples by the Infragistics Company .

Read the article and have a question?
Often our articles are asked the same questions. We collected answers to them here: Answers to questions from readers of articles about PVS-Studio, version 2015 . Please review the list.

Source: https://habr.com/ru/post/276989/

More articles:


All Articles