Clever Geek Handbook

Check Telerik UI for UWP to get acquainted with PVS-Studio





Picture 2






It has already become a tradition that programmers replenishing the PVS-Studio team begin their work by writing an article about analyzing an open source project. This time, such a proven project will be Telerik UI for UWP.



PVS-Studio code analyzer



PVS-Studio is a tool for detecting errors and potential vulnerabilities in the source code of programs written in C, C ++, C # and Java. Runs on Windows, Linux, and macOS.



The analyzer provides various usage scenarios:





Audited Project



Telerik UI for UWP is a set of user interface components for the Universal Windows Platform (UWP). The source code for the project can be found on Github . The set includes more than 20 components that allow you to visualize data in the form of graphs, create lists and tables, use a map to demonstrate data that is associated with a location.



Fragments that attracted attention when studying the analyzer report



PVS-Studio Warning : V3013 It is odd that the body of 'OnMinValuePropertyChanged' function is fully equivalent to the body of 'OnMaxValuePropertyChanged' function. RadGauge.cs 446 



private static void OnMinValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { double newVal = (double)args.NewValue; ValidateValue(newVal); RadGauge gauge = sender as RadGauge; if (gauge.panel != null) { gauge.panel.UpdateOnMinMaxValueChange(); } if(AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged)) { var peer = FrameworkElementAutomationPeer.FromElement(gauge) as RadGaugeAutomationPeer; if (peer != null) { peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); } } } private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { double newVal = (double)args.NewValue; ValidateValue(newVal); RadGauge gauge = sender as RadGauge; if (gauge.panel != null) { gauge.panel.UpdateOnMinMaxValueChange(); } if (AutomationPeer.ListenerExists(AutomationEvents.PropertyChanged)) { var peer = FrameworkElementAutomationPeer.FromElement(gauge) as RadGaugeAutomationPeer; if (peer != null) { peer.RaiseMinimumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); } } }


The analyzer detected two methods, OnMinValuePropertyChanged and OnMaxValuePropertyChanged , which perform the same actions. I have strong suspicions that an error has crept into this code. Note that both the OnMinValuePropertyChanged method and the OnMaxValuePropertyChanged method use RaiseMinimumPropertyChangedEvent . At the same time, in the RadGaugeAutomationPeer class you can find both a method for “Minimum” and for “Maximum”: 



 internal void RaiseMaximumPropertyChangedEvent(double oldValue, double newValue) { this.RaisePropertyChangedEvent( RangeValuePatternIdentifiers.MaximumProperty, oldValue, newValue); } internal void RaiseMinimumPropertyChangedEvent(double oldValue, double newValue) { this.RaisePropertyChangedEvent( RangeValuePatternIdentifiers.MinimumProperty, oldValue, newValue); }


The RaiseMaximumPropertyChangedEvent method is never used in the code, but RaiseMinimumPropertyChangedEvent is used twice. And, you know, the performance of the OnMaxValuePropertyChanged method raises questions ... I think it was supposed to write the following: 



 private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { .... peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); .... }


But even so, the code does not look very neat due to the large number of repeating elements. It is difficult to understand, and repeated lines dull the programmer’s attention, and it becomes more difficult to conduct code review in such conditions. But the static analysis tools do an excellent job of checking such code (but this does not mean that refactoring can be dispensed with, and especially without reducing the number of repeated lines).



Based on the above and the following code fragment, we can assume that the authors are not averse to copy-paste. However, like all of us ... :)



PVS-Studio Warning : V3001 There are identical sub-expressions 'element.RenderSize == emptySize' to the left and to the right of the '||' operator. TiltInteractionEffect.cs 181 



 private static bool IsPointInElementBounds(FrameworkElement element, Point position) { Size emptySize = new Size(0, 0); if (element.RenderSize == emptySize || element.RenderSize == emptySize) { return false; } return new Rect(....).Contains(position); }


The analyzer detected an erroneous code fragment in which to the right and to the left of the '||' operator the if statement uses the same subexpressions. The second subexpression clearly should have looked different. Perhaps in place of the second RenderSize should be DesiredSize . Or there should not be a second subexpression at all. In any case, this piece of code needs to be fixed.



PVS-Studio Warning : V3001 There are identical sub-expressions 'text [0] ==' - '' to the left and to the right of the '||' operator. RadNumericBox.cs 1057 



 private void ValidateText() { string text = this.textBox.Text; .... if (text.Length == 1 && (text[0] == '-' || text[0] == '-')) { if (this.isNegative) { this.isNegative = false; } else { this.SetText(string.Empty); } return; } .... }


Here, the developer writes the text entered in the text box field to a variable. Then the first character of the string is compared twice with the same '-' character, which is a suspicious decision. Obviously, the validation of the text in this function does not work as originally intended.



PVS-Studio Warning : V3001 There are identical sub-expressions 'newValue.HasValue' to the left and to the right of the '&&' operator. DateTimePicker.cs 576 



 private static void OnValueChanged(object sender, DependencyPropertyChangedEventArgs args) { DateTimePicker picker = sender as DateTimePicker; var newValue = (DateTime?)args.NewValue; if (newValue.HasValue && newValue != null) // <= .... }


The expression newValue.HasValue returns true if newValue contains any value, and the expression newValue! = Null does the same. The analyzer pays attention to this, and what to do is to remove one of the subexpressions or replace it with another (if something else had to be checked), it is up to the developers to decide.



PVS-Studio Warning : V3125 The 'CurrentAttachedMenu' object was used after it was verified against null. Check lines: 98, 96. PopupService.cs 98 



 internal static class PopupService { .... private static void Overlay_PointerPressed(....) { if (CurrentAttachedMenu == null || !CurrentAttachedMenu.hitTestService. HitTest(e.GetCurrentPoint(CurrentAttachedMenu).Position).Any()) { CurrentAttachedMenu.IsOpen = false; HideOverlay(); } } }


If the variable CurrentAttachedMenu turns out to be null , then evaluating the expression CurrentAttachedMenu.IsOpen will raise an exception. At first glance, it seems that this is a simple typo and did not mean a comparison with null , but the inverse operation - '! ='. But then an exception will occur in the condition of the if statement if the CurrentAttachedMenu variable is null .



Further in the code there were another 37 of the same warnings, some of which appear to indicate errors. But to describe them within the framework of one article is still too much, so I will leave them unattended.



PVS-Studio Warning : V3019 Possibly an incorrect variable is compared to null after type conversion using 'as' keyword. Check variables 'dragDropElement', 'uiDragDropElement'. DragDrop.cs 91 



 internal static void StartDrag(....) { var dragDropElement = sender as IDragDropElement; .... UIElement uiDragDropElement = dragDropElement as UIElement; .... if (dragDropElement == null || !dragDropElement.CanStartDrag(trigger, initializeContext)) { return; } .... }


It is very likely that the author mixed up the variables. The null inequality is checked not by the link obtained as a result of the cast, but by the original ( dragDropElement ). Most likely, the uiDragDropElement link was supposed to be checked . The conjecture is also confirmed by the fact that the programmer further used uiDragDropElement without checking for null .



PVS-Studio Warning : V3030 Recurring check. The '! ShowIndicatorWhenNoData' condition was already verified in line 139. RadDataBoundListBox.PullToRefresh.cs 141 



 internal void HandlePullToRefreshItemStateChanged(object item, ItemState state) { .... bool showIndicatorWhenNoData = this.ShowPullToRefreshWhenNoData; if (this.realizedItems.Count == 0 && !showIndicatorWhenNoData) { if (state == ItemState.Recycled && !showIndicatorWhenNoData) { this.StopPullToRefreshLoading(false); this.HidePullToRefreshIndicator(); } return; } .... }


The analyzer found a piece of code in which, under two conditions, the same showIndicatorWhenNoData variable is re-checked. Perhaps the check is simply redundant, but there is also the possibility that one of the duplicate subexpressions should be different at all.



PVS-Studio Warning : V3031 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions. SelectedItemCollection.cs 77 



 internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && ((!this.AllowMultipleSelect && this.Count == 0) || this.AllowMultipleSelect); } }


This piece of code is not formally erroneous. The analyzer hints at some code redundancy in the condition. However, it is worth remembering that extra code is sometimes the result of an error, for example, when instead of one variable, another is checked several times.



You can reduce this condition a bit and remove the extra code. For example, like this: 



 internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && (this.AllowMultipleSelect || this.Count == 0); } }


Other similar messages:





Consider another piece of code, to which the analyzer issued the following:



PVS-Studio warnings : 





 internal static class DragDrop { .... double leftMargin = 0d; double topMargin = 0d; if (frameworkElementSource != null) { leftMargin = frameworkElementSource.Margin.Left; // <= topMargin = frameworkElementSource.Margin.Top; // <= } if (dragDropElement == null || !dragDropElement.CanStartDrag(trigger, initializeContext)) { return; } var context = dragDropElement .DragStarting(trigger, initializeContext); if (context == null) { return; } var startDragPosition = e .GetCurrentPoint(context.DragSurface.RootElement).Position; var relativeStartDragPosition = e .GetCurrentPoint(uiDragDropElement).Position; var dragPositionMode = DragDrop .GetDragPositionMode(uiDragDropElement); AddOperation(new DragDropOperation( context, dragDropElement, dragPositionMode, e.Pointer, startDragPosition, relativeStartDragPosition)); }


The analyzer reports that the leftMargin and topMargin variables have been assigned values, but after that these variables are not used until the end of the method. There is probably no mistake here, but such code looks suspicious. This may be due to a typo or unsuccessful refactoring.



The same problem was found elsewhere: V3137 The 'currentColumnLength' variable is assigned but is not used by the end of the function. WrapLayout.cs 824



PVS-Studio Warning : V3061 Parameter 'index' is always rewritten in method body before being used. DataEngine.cs 1443 



 private static Tuple<Group, int> FindGroupAndItemIndex(.... int index, ....) { if (exhaustiveSearch) { .... } else { var aggregateRowGroup = rowRootGroup; var rowGroupNames = valueProvider.GetRowGroupNames(item); foreach (var groupName in rowGroupNames) { Group group; if (aggregateRowGroup.TryGetGroup(groupName, out group)) { aggregateRowGroup = group; } } index = aggregateRowGroup.IndexOf(item, // <= valueProvider.GetSortComparer()); return Tuple.Create(aggregateRowGroup, index); } }


The index parameter of the FindGroupAndItemIndex method is overwritten before being used. Most likely, this indicates a programmer error.



PVS-Studio Warning : V3083 Unsafe invocation of event 'Completed', NullReferenceException is possible. Consider assigning event to a local variable before invoking it. ActionBase.cs 32 



 internal abstract class ActionBase { .... protected virtual void OnCompleted() { this.IsCompleted = true; if (this.Completed != null) { this.Completed(this, EventArgs.Empty); } } }


The programmer allowed a potentially unsafe call to the event handler in this method, which can lead to an exception of type NullReferenceException . An exception will be thrown provided that between the null check and the call of the event handlers, this event does not remain.



There are another 49 similar problems in the code. It will not be interesting to watch all of them in this article, and the authors can easily find them on their own using PVS-Studio, so we will move on to other errors.



PVS-Studio Warning : V3145 Unsafe dereference of a WeakReference target, consider inspecting info.Target. The object could have been garbage collected between checking 'IsAlive' and accessing the 'Target' property. FadeAnimation.cs 84 



 public class RadFadeAnimation : RadAnimation { .... protected internal override void ApplyAnimationValues(PlayAnimationInfo info) { .... if (info.Target.Opacity != opacity) // <= { info.Target.Opacity = opacity; } .... } .... }


The analyzer warns of the danger of throwing an exception of type NullReferenceException when accessing the info.Target.Opacity property. In order to better understand the essence of the problem, you need to look at fragments of the PlayAnimationInfo class, in particular, the Target property. 



 public class PlayAnimationInfo { .... private WeakReference target; .... public PlayAnimationInfo(Storyboard storyboard, RadAnimation animation, UIElement target) { .... this.target = new WeakReference(target); .... } .... public UIElement Target { get { if (this.target.IsAlive) { return this.target.Target as UIElement; } return null; } } .... }


In general, the further you dig this code, the more potential problems you can find in it. Let's look at perhaps the most interesting one - the one to which the analyzer issued a warning. The fact is that even if execution follows the then-branch of the if statement , this does not guarantee that a non-zero reference will be returned. Regardless of conversations about the cast, here we consider everything permissible due to the initialization of the object in the constructor.



How is this possible? The fact is that if between the IsAlive check and the Target call a garbage collection is performed, under which the object referenced by WeakReference falls , this.target.Target will return null . That is, the IsAlive check does not guarantee that the next time the Target is accessed, the object is still available.



By the way, the situation is return null; catches another diagnostic rule: V3080 Possible null dereference. Consider inspecting 'info.Target'. FadeAnimation.cs 84



Similar problems in the code occurred several more times:





Let's move on to the next example.



Warning PVS-Studio : V3066 Possible incorrect order of arguments passed to 'NotifyCollectionChangedEventArgs' constructor: 'oldItem' and 'newItem'. CheckedItemsCollection.cs 470 



 public class CheckedItemsCollection<T> : IList<T>, INotifyCollectionChanged { .... private NotifyCollectionChangedEventArgs GenerateArgs(....) { switch (action) { case NotifyCollectionChangedAction.Add: .... case NotifyCollectionChangedAction.Remove: .... case NotifyCollectionChangedAction.Replace: return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex); // <= default: return new NotifyCollectionChangedEventArgs(action); } } }


To understand what this warning from the analyzer means, it is worth looking at the NotifyCollectionChangedEventArgs constructor parameters : 



  public NotifyCollectionChangedEventArgs( NotifyCollectionChangedAction action, object newItem, object oldItem, int index);


The analyzer warns that in the expression 

  return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);


swapped the variables oldItem and newItem . In the constructor definition, they are listed in a different order. Whether this was done consciously or not, one can only guess.



PVS-Studio Warning : V3102 Suspicious access to element of 'x' object by a constant index inside a loop. DataEngine.cs 1718 



 private class ObjectArrayComparer : IEqualityComparer<object[]> { public bool Equals(object[] x, object[] y) { .... for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[0], y[0])) // <= { return false; } } return true; } .... }


At each iteration in the loop, the programmer compares x [0] and y [0]. However, the loop does not make sense in this code, since only the first elements are compared. Most likely, this meant a comparison of the corresponding elements of arrays. Then the correct code will be like this: 



 for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[i], y[i])) { return false; } }


Warning PVS-Studio : V3123 Perhaps the '?:' Operator works in a different way than it was expected. Its priority is lower than priority of other operators in its condition. EditRowHostPanel.cs 35 



 protected override Size MeasureOverride(Size availableSize) { .... bool shouldUpdateRowHeight = editorLine == 0 || displayedElement == null ? false : displayedElement.ContainerType != typeof(DataGridGroupHeader); .... }


A warning is associated with the use of the '?:' Operator. It has a lower priority than ! =, ||, ==. Therefore, the expression may not be evaluated as planned by the programmer. Apparently, in this case this is a false positive and the code works correctly. But reading such code is very difficult and there is never any certainty that it is understood correctly. It feels like the developer wrote in such a way that no one understood anything :) The best way to do this is more readable - use brackets or the if statement .



PVS-Studio Warning : V3078 Original sorting order will be lost after repetitive call to 'OrderBy' method. Use 'ThenBy' method to preserve the original sorting. GridModel.Selection.cs 107 



 internal partial class GridModel { private void BuildCellSelectionRegions(....) { .... this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .OrderBy(c => c.RowItemInfo.LayoutInfo.Line)); } }


The error is related to calling OrderBy again for a collection of type IOrderedEnumerable . Here the collection is sorted first by columns, then by rows. Moreover, at the time of sorting by rows, the previous sorting by columns is not taken into account anywhere. In order not to lose the sorting by columns and sort the collection by several criteria at once, then use ThenBy : 



  this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));


PVS-Studio Warning : V3008 The 'currentColumnLength' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 791, 785. WrapLayout.cs 791 



 private void OnAvailableLengthChanged(double oldValue, double newValue) { .... if (....) { if (currentColumnLength > 0) { var paddingValue = Math.Max(0, newValue - currentColumnLength); this.paddingRenderInfo.Add(paddingValue); currentColumnLength = 0; // <= slotCount++; } this.ColumnSlotsRenderInfo.Update(i, newValue); this.paddingRenderInfo.Add(0); currentColumnLength = 0; // <= slotCount++; continue; } else { .... } .... }


It seemed suspicious to the analyzer that the currentColumnLength variable was assigned a value twice. The variable is not used between assignments. Regardless of the condition, this variable will ultimately be zero. This code is either incorrect or redundant.



PVS-Studio Warning : V3127 Two similar code fragments were found. Perhaps, this is a typo and 'emptyIconContainer' variable should be used instead of 'filledIconContainer' RadRatingItem.cs 240 



 public class RadRatingItem : RadContentControl { .... protected override void OnApplyTemplate() { .... this.filledIconContainer = this.GetTemplateChild( "FilledIconContainer") as Border; if (this.filledIconContainer == null) // <= { throw new MissingTemplatePartException( "FilledIconContainer", typeof(Border)); } this.emptyIconContainer = this.GetTemplateChild( "EmptyIconContainer") as Border; if (this.filledIconContainer == null) // <= { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); } this.Initialize(); } .... }


Because of a typo, two identical conditions appeared in the code. Judging by the generated exception, the second condition should look like this: 



 if (this.emptyIconContainer == null) { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); }


PVS-Studio Warning : V3020 An unconditional 'break' within a loop. NodePool.cs 189 



 public IEnumerable<KeyValuePair<int, List<T>>> GetUnfrozenDisplayedElements() { foreach (var item in this.generatedContainers) { foreach (var pair in item.Value) { if (!pair.IsFrozen) { yield return item; } break; } } }


The analyzer found that here the break statement does not belong to the if statement . Break will be executed regardless of the value of pair.IsFrozen and because of this in foreach , only one iteration will be performed.



That concludes my consideration of warnings. So that Telerik developers can conduct a more thorough code analysis and fix defects, we are ready to provide them with a temporary license. In addition, they can take advantage of the free use of PVS-Studio option provided to the authors of open source projects.



Conclusion



Although the developers of Telerik UI for UWP did a great job, it was not without typos, as it usually happens :). All these errors could easily be found by a static analyzer and corrected. The most important thing is to use the analyzer correctly and regularly .











If you want to share this article with an English-speaking audience, then please use the link to the translation: Ekaterina Nikiforova. Checking Telerik UI for UWP as a Way to Get Started with PVS-Studio .


More articles:


All Articles