开始撰写PVS-Studio团队的开发人员的传统是开始撰写一篇文章，回顾分析仪在某些开源项目中发现的错误，这是一种传统。 用于UWP的Telerik UI是为今天的回顾而挑选的项目。
PVS-Studio代码分析器
PVS-Studio是用于检测用C，C ++，C＃和Java编写的程序的源代码中的错误和潜在漏洞的工具。 分析仪可在Windows，Linux和macOS上运行。
PVS-Studio可以通过多种方式运行：
- 作为开发人员的本地计算机上Visual Studio或IntelliJ IDEA的插件；
- 通过与SonarQube集成：连续代码质量检查平台；
- 作为集成到构建系统中的独立应用程序；
- 通过与特殊的编译监视实用程序结合运行；
- 通过与Azure DevOps，Jenkins，TeamCity，Travis CI和其他类似系统集成；
- 等
正在分析的项目
用于UWP的Telerik UI是用于通用Windows平台（UWP）的一组UI控件。 该项目的源代码
可在Github上获得 。 该集合包括20多个组件，允许用户以图表形式可视化数据，创建列表和表格，并使用地图在地理环境中显示内容。
分析器报告的有趣代码段
PVS-Studio诊断消息： V3013 “ OnMinValuePropertyChanged”功能的主体与“ OnMaxValuePropertyChanged”功能的主体完全等效是很奇怪的。 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); } } }
OnMinValuePropertyChanged和
OnMaxValuePropertyChanged这两种方法执行相同的操作。 我强烈怀疑这里有错误。 请注意，这两种方法都调用同一方法
RaiseMinimumPropertyChangedEvent ，而
RadGaugeAutomationPeer类为“ Minimum”和“ 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); }
RaiseMinimumPropertyChangedEvent方法使用两次，而
RaiseMaximumPropertyChangedEvent方法则完全不使用。 这使我怀疑
OnMaxValuePropertyChanged方法是否工作良好……我想它的样子是这样的：
private static void OnMaxValuePropertyChanged( DependencyObject sender, DependencyPropertyChangedEventArgs args) { .... peer.RaiseMaximumPropertyChangedEvent((double)args.OldValue, (double)args.NewValue); .... }
但是即使进行了此修复，由于存在大量重复元素，因此代码看起来也不整洁。 很难阅读，外观相似的线条使您的注意力减弱，这使代码审查变得困难。 相反，静态分析工具可以轻松地处理它（这并不意味着您不应该重构代码，尤其是消除重复的行）。
查看此片段和下一个片段，我怀疑项目作者不时沉迷于复制粘贴。 好吧，我们都做... :)
PVS-Studio诊断消息： V3001 '||'的左右
两侧有相同的子表达式'element.RenderSize == emptySize' 操作员。 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); }
“ ||”的两个操作数
if语句的条件表达式中的运算符由相同的子表达式表示。 显然，第二个子表达式应该不同。 也许第二个
RenderSize应该是
DesiredSize，或者第二
个子表达式根本不应该存在。 无论如何，此代码都需要修复。
PVS-Studio诊断消息： V3001在'||'的左侧和右侧有相同的子表达式'text [0] =='-'' 操作员。 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; } .... }
输入到文本框字段中的文本将被读入变量。 然后，将字符串的第一个字符与字符“-”（它看起来不正确）进行两次比较。 显然，此功能未按预期执行文本验证。
PVS-Studio诊断消息： V3001 '&&'运算符的左侧和右侧有相同的子表达式'newValue.HasValue'。 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)
如果
newValue有值，则两个条件表达式
newValue.HasValue和
newValue！= Null都返回
true 。 分析器指出了这一点，但是该错误是应该通过删除其中一个子表达式还是将其替换为另一个（如果要检查其他内容）来解决，则只有该代码的作者才能确定。
PVS-Studio诊断消息： V3125在验证是否为null之后使用了“ CurrentAttachedMenu”对象。 检查行：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(); } } }
如果
CurrentAttachedMenu变量恰好等于
null ，则评估
CurrentAttachedMenu.IsOpen表达式将导致引发异常。 看起来好像只是一个错字，开发人员实际上是在进行相反的操作'！='，而不是空检查，但是如果是这样，如果
CurrentAttachedMenu变量为，则
if语句的条件将引发异常。等于
null 。
还有
37种此类警告，其中一些显然是真正的错误。 但是对于一篇文章来说，警告太多了，因此我将跳过它们。
PVS-Studio诊断消息： V3019可能在使用'as'关键字进行类型转换后将不正确的变量与null进行比较。 检查变量“ 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; } .... }
程序员必须将一个变量与另一个变量混淆了。 空检查是在源引用
dragDropElement上完成的，而不是在
强制转换uiDragDropElement产生的引用上进行的，该引用实际上是要检查的。
uiDragDropElement在没有任何空检查的情况下被进一步使用的事实支持了这种假设。
PVS-Studio诊断消息： V3030定期检查。 在行139中已经验证了'！ShowIndicatorWhenNoData'条件。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; } .... }
两个条件检查同一变量
showIndicatorWhenNoData 。 第二个检查可能是多余的，但重复的子表达式之一也可能完全是其他东西。
PVS-Studio诊断消息： V3031可以简化过度检查。 “ ||” 运算符被相反的表达式包围。 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); } }
从技术上讲，此代码段是正确的； 分析仪仅指出情况中的某些冗余。 但是请记住，冗余代码通常是编程错误的标志，例如检查一个变量比检查其他变量多于必要次数。
通过删除如下所示的不必要代码，可以稍微简化条件：
internal class SelectedItemCollection : ObservableCollection<object> { .... private bool CanInsertItem(object item) { return this.suspendLevel == 0 && this.AllowSelect && (this.AllowMultipleSelect || this.Count == 0); } }
其他类似的警告：
- V3031过度检查可以简化。 “ ||” 运算符被相反的表达式包围。 SelectedItemCollection.cs 93
- V3031过度检查可以简化。 “ ||” 运算符被相反的表达式包围。 StackVirtualizationStrategy.cs 49
- V3031过度检查可以简化。 “ ||” 运算符被相反的表达式'state == null'和'state！= null'包围。 LocalFieldDescriptionsProviderBase.cs 24
让我们考虑分析器向其发布以下代码的另一段代码：
PVS-Studio诊断消息：- V3137分配了'leftMargin'变量，但在函数末尾未使用该变量。 DragDrop.cs 87
- V3137分配了“ topMargin”变量，但在函数末尾未使用该变量。 DragDrop.cs 88
internal static class DragDrop { .... double leftMargin = 0d; double topMargin = 0d; if (frameworkElementSource != null) { leftMargin = frameworkElementSource.Margin.Left;
变量
leftMargin和
topMargin区域分配了一些值，但此后从未使用过。 它不一定是bug，但是类似的代码仍然令人怀疑。 这可能是拼写错误或重构错误的标志。
此类型的另一个警告：V3137分配了“ currentColumnLength”变量，但未在函数末尾使用。 WrapLayout.cs 824
PVS-Studio诊断消息： V3061在使用前，始终在方法主体中重写参数“索引”。 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,
使用前，
FindGroupAndItemIndex方法的
index参数将被覆盖。 这很可能表明程序员有错误。
PVS-Studio诊断消息： V3083对事件“已完成”的不安全调用，可能会发生NullReferenceException。 请考虑在调用事件之前将事件分配给局部变量。 ActionBase.cs 32
internal abstract class ActionBase { .... protected virtual void OnCompleted() { this.IsCompleted = true; if (this.Completed != null) { this.Completed(this, EventArgs.Empty); } } }
以可能不安全的方式调用事件处理程序，但有引发
NullReferenceException的风险。 如果事件在null检查和事件处理程序的调用之间没有订阅者，则会发生这种情况。
报告指出了
49个此类问题。 在这里讨论它们并不有趣，毕竟，项目作者可以使用PVS-Studio轻松地找到它们，因此让我们跳到下一个示例。
PVS-Studio诊断消息： V3145不安全地取消引用WeakReference目标，请考虑检查info.Target。 在检查“ IsAlive”和访问“ Target”属性之间可能已被垃圾回收。 FadeAnimation.cs 84
public class RadFadeAnimation : RadAnimation { .... protected internal override void ApplyAnimationValues(PlayAnimationInfo info) { .... if (info.Target.Opacity != opacity)
解决
info.Target.Opacity属性时，可能会引发
NullReferenceException 。 为了更好地了解问题所在，我们需要看一下
PlayAnimationInfo类的某些块，尤其是
Target属性。
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; } } .... }
实际上，您深入研究此代码，就会发现更多潜在问题。 让我们看一下最有趣的一个-触发警告的那个。 问题在于，即使执行遵循
if语句的
else分支，即使我们没有考虑类型转换的影响（对象由构造函数初始化），也不能保证返回非null引用。 。
那怎么可能？ 您会看到，如果
WeakReference引用的对象是在
IsAlive检查和对
Target的调用之间进行垃圾回收的，则
this.target.Target将返回
null 。 也就是说，
IsAlive检查不能保证对象在您下次调用
Target时仍然可用。
顺便说一句，
返回null； 另一个诊断程序检测到此问题：V3080可能为空的取消引用。 考虑检查“ info.Target”。 FadeAnimation.cs 84
还有更多类似的缺陷：
- V3145不安全地取消引用WeakReference目标，请考虑检查目标。 在访问“目标”属性之前，该对象可能已被垃圾回收。 MoveXAnimation.cs 80
- V3145不安全地取消引用WeakReference目标，请考虑检查目标。 在访问“目标”属性之前，该对象可能已被垃圾回收。 MoveYAnimation.cs 80
- V3145不安全地取消引用WeakReference目标，请考虑检查info.Target。 在访问“目标”属性之前，该对象可能已被垃圾回收。 平面投影动画.cs 244
- V3145对WeakReference目标的不安全取消引用。 在检查“ IsAlive”和访问“ Target”属性之间可能已被垃圾回收。 WeakEventHandler.cs 109
让我们继续下一个示例。
PVS-Studio诊断消息： V3066传递给'NotifyCollectionChangedEventArgs'构造函数的参数的可能错误顺序：'oldItem'和'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);
要弄清楚此警告的含义，我们需要查看
NotifyCollectionChangedEventArgs构造函数的参数：
public NotifyCollectionChangedEventArgs( NotifyCollectionChangedAction action, object newItem, object oldItem, int index);
分析器告诉我们，变量
oldItem和
newItem在以下表达式中交换：
return new NotifyCollectionChangedEventArgs( action, oldItem, newItem, changeIndex);
但是，构造函数的实现以相反的顺序列出了这些变量。 您只能想知道是否故意这样做。
PVS-Studio诊断消息： V3102通过循环内的常量索引可疑访问“ x”对象的元素。 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]))
在每次循环迭代中比较元素
x [0]和
y [0] 。 但是由于仅比较了第一个元素，因此循环没有意义。 开发人员可能打算比较数组的各个元素。 在这种情况下，正确的版本应如下所示：
for (int i = 0; i < x.Length; i++) { if (!object.Equals(x[i], y[i])) { return false; } }
PVS-Studio诊断消息： V3123也许“？：”操作符的工作方式与预期的不同。 其优先级低于其条件下其他运营商的优先级。 EditRowHostPanel.cs 35
protected override Size MeasureOverride(Size availableSize) { .... bool shouldUpdateRowHeight = editorLine == 0 || displayedElement == null ? false : displayedElement.ContainerType != typeof(DataGridGroupHeader); .... }
此警告涉及使用'？：'运算符。 它的优先级低于
！=，||和
==的优先级，这意味着上述表达式的求值顺序可能与预期的不同。 这种特殊情况似乎是错误的肯定，因为代码实际上按预期工作。 但是这样的代码很难阅读，而且您永远无法确定自己是否正确理解。 看起来好像是故意写的，所以没人能弄清楚它。:)使其更容易阅读的最好方法是使用括号或
if语句。
PVS-Studio诊断消息： V3078重复调用'OrderBy'方法后，原始排序顺序将丢失。 使用“ ThenBy”方法保留原始排序。 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)); } }
此错误与对
IOrderedEnumerable类型的集合的
OrderBy方法的反复调用有关。 集合首先按列排序，然后按行排序。 问题在于，第一个按列排序的结果没有存储在任何地方，开始按行排序时，它将丢失。 如果要保留按列排序的结果并执行多条件排序，请使用
ThenBy方法：
this.MergeCellSelectionRegions(selectedItemsInView .OrderBy(c => c.Column.ItemInfo.LayoutInfo.Line) .ThenBy(c => c.RowItemInfo.LayoutInfo.Line));
PVS-Studio诊断消息： V3008为 “ currentColumnLength”变量连续两次分配值。 也许这是一个错误。 检查行：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;
分析器发现奇怪的是，
currentColumnLength变量被分配了两次值，而在这两个分配之间的任何地方都没有使用。 无论条件如何，该变量最终都会以
null结束。 该代码错误或冗余。
PVS-Studio诊断消息： V3127找到两个相似的代码片段。 也许这是一个错字，应该使用'emptyIconContainer'变量而不是'filledIconContainer'RadRatingItem.cs 240
public class RadRatingItem : RadContentControl { .... protected override void OnApplyTemplate() { .... this.filledIconContainer = this.GetTemplateChild( "FilledIconContainer") as Border; if (this.filledIconContainer == null)
上面两个相同的条件是由于输入错误而出现的。 此代码引发的异常表明第二个条件应如下所示：
if (this.emptyIconContainer == null) { throw new MissingTemplatePartException( "EmptyIconContainer", typeof(Border)); }
PVS-Studio诊断消息： V3020循环内无条件的“中断”。 节点池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; } } }
break语句不是
if语句的一部分。 无论对
pair.IsFrozen中存储了什么值，它都将执行，因此
foreach循环将仅迭代一次。
以上就是我对Telerik中发现的错误的回顾。 我们准备为开发人员提供免费的临时许可证，以便他们可以进行更彻底的分析并修复缺陷。 他们还可以使用开源开发人员可以使用的
免费PVS-Studio许可选项 。
结论
尽管用于UWP的Telerik UI的作者在开发他们的项目方面做得很出色，但他们仍然让很多错别字悄悄出现，就像我们通常会发生的那样:)。 使用静态分析器可以很容易地捕获并修复所有这些错误，但是要记住有关静态分析的关键是应该以
正确的方式并定期地使用它 。