Есть много способов, которые помогут повысить качество программы. В этой статье предлагаю взглянуть на один из них – статический анализ кода.
Кратко о статическом анализе кода
Как вы, наверное, уже знаете, статические анализаторы позволяют проверять код без запуска самой программы. Последнее время популярность статического анализа при разработке набирает обороты, а рынок статического анализа с каждым годом становится больше. Отчасти это связано с тем, что прошла эра линтеров, которые были основаны только на регулярных выражениях.
Сейчас статический анализ поражает своим разнообразием и возможностями. Даже хайп с AI и машинным обучением не прошел мимо анализаторов, и швейцарцы выпустили продукт, обучающийся на открытых репозиториях. Однако надо понимать, что AI в обозримом будущем не заменит классические технологии, применяемые в статических анализаторах, а пополнит их.
Инструменты и технологии для статического анализа кода
Одним из современных статических анализаторов является инструмент PVS-Studio. Этот инструмент позволяет выявлять ошибки и потенциальные уязвимости в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в 64-битных системах на Windows, Linux и macOS и может анализировать код, предназначенный для 32-битных, 64-битных и встраиваемых ARM платформ. Кратко рассмотрим, какие технологии использует PVS-Studio при анализе исходного кода.
Анализ потока данных
Начнем с анализа потока данных. Он позволяет вычислить возможные значения переменных в разных точках программы. Благодаря Data-Flow Analysis можно находить такие ошибки, как выход за границу массива, утечки памяти, разыменование нулевого указателя и т.д.
Аннотирование методов
Ручное и автоматическое аннотирование методов. Оно предоставляет больше информации об используемых методах, чем может быть получено путём анализа только их сигнатуры.
Сопоставление с шаблоном
Сопоставление с шаблоном. Анализатор, проверяя код, может находить заданные заранее паттерны, типичные для какой-либо ошибки. В простейшем варианте этот поиск похож на поиск ошибок с помощью регулярных выражений, однако всё обстоит несколько сложнее. Для поиска ошибок используется обход и анализ дерева разбора. Почему для этих задач неприемлемо использовать регулярные выражения, можно узнать из статьи «Статический анализ и регулярные выражения».
Символьное выполнение
Символьное выполнение. Оно позволяет находить недочеты в коде, даже не зная, какое значение переменных будет в строке с ошибкой. Небольшой пример, чтобы было нагляднее:
void Foo(int A, int B, int C)
{
if(A<B)
{
if(B<C)
{
if(A>C)
{
....
}
}
}
}
Не зная ничего о значениях переменных A, B и C, анализатор PVS-Studio способен понять, что условие (A > C) всегда ложно, и сообщить об этом разработчику. Подробнее с этим и другими принципами, положенными в основу анализатора, можно познакомиться в статье «Технологии, используемые в анализаторе кода PVS-Studio для поиска ошибок и потенциальных уязвимостей».
Я знаю, о чем подумали некоторые читатели этой статьи. Это все, конечно, классно, но зачем нам статический анализ? Приведу пример из своей жизни. У меня был маленький пет-проект – светодиодные костюмы, которые светятся и моргают под музыку (при нажатии кнопки «плей» в программе на компьютере запускается таймер, который и отправляет на светодиоды значение RGB). Однажды, внеся очередные правки в код, я включила костюм и поняла, что он сходит с ума. Костюм хаотично моргал и светился теми цветами, которые я видеть вообще не ожидала, и больше напоминал кошмар эпилептика, чем светодиодную шмотку. На поиск ошибки у меня ушло, наверное, около часа, я перечитывала свой код немыслимое количество раз, а причина оказалась в банальной опечатке в одной цифре… мда.
К слову, ошибку, которую я допустила, успешно находит статический анализ.
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); // Должно быть 7
....
}
Предупреждение PVS-Studio: 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
Тут я методом копипасты писала код, сохраняющий ip-адрес контроллеров костюмов из текстбоксов. И, дабы быть честной, признаюсь, что цифра 6 тут из головы. Я не помню, в каком конкретно обработчике событий так неудачно накопипастила. Ну и не важно, самое главное – передать суть.
Однако у меня была довольно небольшая кодовая база и, следовательно, маленький объем всевозможных ошибок и опечаток. Цифры, взятые из книги Стива Макконнелла «Совершенный код», гласят, что с ростом размера проекта растёт и плотность ошибок.
Именно поэтому инструменты статического анализа все больше набирают популярность среди крупных компаний, занимающихся разработкой.
Заметка! Compiler Explorer использует анализатор PVS-Studio.
Статический анализ кода на практике
Давайте теперь перейдём от теории к практике и на примере посмотрим, какие ошибки можно выявлять с помощью статического анализа кода. Для этого возьмём небольшой реальный открытый проект Extended WPF Toolkit и проверим его с помощью PVS-Studio.
Extended WPF Toolkit – это коллекция элементов управления и компонентов для WPF приложений. Проект включает в себя около 600 файлов исходного кода на языке C# и около 112 тысяч строк. Этот бесплатный набор инструментов имеет открытый исходный код и предоставляется в рамках лицензии Microsoft Public License. Также разработчики уже на платной основе предлагают воспользоваться Toolkit Plus Edition и Business Suite, в которых еще больше разнообразных компонентов и элементов управления, несколько тем под Metro и Windows 10 и многое другое.
Впрочем, все эти подробности нам не очень важны. Главное, что это обыкновенный типовой проект, написанный на языке C#. Давайте рассмотрим некоторые из ошибок, который в нём были найдены. Надеюсь, приведённых примеров будет достаточно для получения общего представления о технологии анализа кода. Более полное впечатление вы сможете составить самостоятельно, скачав и запустив анализатор на своих проектах. См. также «Как быстро посмотреть интересные предупреждения, которые выдает анализатор PVS-Studio для C и C++ кода?».
Примеры найденных ошибок
Предупреждение PVS-Studio: 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();
....
}
Это предупреждение анализатора сообщает о том, что был создан экземпляр класса InvalidOperationException, который в коде не используется. Видимо, программист хотел, чтобы при выполнении условия генерировалось исключение, но забыл написать оператор throw, который бы это исключение вызывал.
Предупреждение PVS-Studio: 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" ) );
}
}
Анализатор предупреждает о том, что был создан потенциально небезопасный вызов обработчика события. Проблема этого кода заключается в том, что одной проверки на null в этом случае недостаточно. В многопоточном приложении между проверкой на null и кодом в then-ветви оператора if может выполниться код в другом потоке, выполняющий отписку от данного события. Если это произойдет, и подписчиков у события не останется, то такой случай приведет к возникновению исключения NullReferenceException.
Для того, чтобы гарантированно произошло безошибочное выполнение вызова события, есть несколько способов переписать этот код. Я приведу один пример, а разработчики сами решат – воспользоваться моим способом, найти другой, и стоит ли вообще с этим кодом что-то делать.
protected void OnPropertyChanged( string propertyName )
{
PropertyChangedEventHandler eventHandler = PropertyChanged;
if( eventHandler != null )
{
eventHandler( this, new PropertyChangedEventArgs( propertyName ) );
eventHandler( this, new PropertyChangedEventArgs( "ModelDisplay" ) );
}
}
В этом примере мы сохраняем ссылку на обработчик события в переменную eventHandler. Теперь, даже если произойдет отписка от события и подписчиков не останется, переменная eventHandler всё равно будет содержать ссылку на первоначальный обработчик и код выполнится корректно.
Подобные проблемы в коде встретились более 30 раз. Все подобные предупреждения в рамках этой статьи смотреть будет скучно, поэтому предлагаю автору проекта самостоятельно попробовать их найти и исправить.
Предупреждение PVS-Studio: V3117 Constructor parameter ‘ignore’ is not used. AnimationRate.cs 59
private AnimationRate( bool ignore )
{
_duration = 0;
_speed = double.NaN;
_rateType = RateType.Speed;
}
Это предупреждение говорит о том, что параметр ignore не используется в этом коде. Судя по названию параметра, это ложное срабатывание и вскоре ‘ignore’ уберут из этого кода. Если это так, то предлагаю использовать атрибут ‘Obsolete’, который используется как раз в таких случаях.
[Obsolete("убрать параметр ignore")]
private AnimationRate( bool ignore )
{
_duration = 0;
_speed = double.NaN;
_rateType = RateType.Speed;
}
Предупреждение PVS-Studio: 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() );
....
}
}
Анализатор указывает на то, что объект reader класса StringReader реализует интерфейс ‘IDisposable’, но метод Dispose() для этого объекта в коде не был вызван. Тут, на самом деле, возникла двоякая ситуация. Класс StringReader и правда реализует данный интерфейс, но он его унаследовал от базового класса и никакими ресурсами он не владеет, поэтому вызывать Dispose() в этом случае не обязательно.
Предупреждение PVS-Studio: 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 );
//}
}
....
}
}
Анализатор обращает внимание на то, что в двух условиях подряд проверяется одно и то же значение на null. Возможно, проверка избыточна, но так же есть и вероятность, что второе условие должно выглядеть иначе. Сложилось впечатление, что этот код просто не дописан.
Предупреждение PVS—Studio:
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 );
}
....
}
В этом коде _windowControl отписывается от события, а потом подписывается обратно. Проблема кроется в том, каким образом происходят манипуляции с событиями при помощи лямбда-выражений. Дело в том, что каждое объявление анонимной функции приводит к созданию отдельного экземпляра делегата. Корректный вариант использования анонимных функций при подписке с дальнейшей отпиской на события представляет собой создание и использование переменной, в которую сохранен обработчик-лямбда, например, так:
_event = (o, e) => this.OnHeaderDragDelta (o, e);
Подобные предупреждения анализатора:
- 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: 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 );
}
В этом коде анализатор нашел две функции OnMinScaleChanged и OnMaxScaleChanged, реализованные идентичным образом. При этом в коде было создано свойство зависимости с именем MaxScaleProperty. Есть подозрения, что во втором случае код должен выглядеть так:
private static void OnMaxScaleChanged( DependencyObject o,
DependencyPropertyChangedEventArgs e )
{
....
zoombox.CoerceValue( Zoombox.MaxScaleProperty );
....
}
Подобные сообщения анализатора:
- 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: 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))
{
....
}
}
}
....
}
Данный код является избыточным и его нужно упростить, о чем и сообщает анализатор. Все дело в том, что слева и справа от оператора ‘||’ стоят выражения (newValue != null) и (newValue == null). На первый взгляд кажется, что при упрощении пострадает логика программы, ведь в первом подвыражении проверяется не только наличие какого-либо значения у переменной newValue, но еще и item. Однако если написать так, то не только не пострадает работоспособность программы, но и улучшится читабельность кода:
if (newValue == null || !newValue.Contains(item))
Подобные ошибки, найденные анализатором:
- 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. cs 85
Предупреждение PVS-Studio: 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 );
}
Анализатор указывает на то, что программист привел объект к своему же типу. Данная проверка является избыточной. Это не является ошибкой и можно оставить ключевое слово var в объявлении magnifier, но нагляднее будет явно указать тип переменной.
Обычно после описания ошибки идет перечисление всех мест, где попадался подобный ошибочный код, но в этом случае выписать все предупреждения не получится. В коде попалось более 50 (!) подобных предупреждений анализатора (не считая того, что могло попасть на уровень Low, который я смотрела не так внимательно, как остальные), что на мой взгляд чересчур много.
Предупреждение PVS-Studio: 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 ];
}
....
}
....
}
....
}
Код в цикле for не выполнится ни разу по следующим причинам: сначала программа очищает list, потом сравнивает размер sourceList и list (генерируя исключение, если количество элементов в sourceList больше чем в пустом list), а далее пытается заполнить список list значениями из sourceList через цикл.
Предупреждение PVS-Studio: 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 );
....
}
Вне зависимости от значения singleChild.ChildrenCount, из-за оператора break выполняется ровно одна итерация цикла foreach. Да и вообще код очень странный, непонятно, стоит ли это расценивать как ошибку, вдруг это так и было задумано…
Заключение
На примере проекта Extended WPF Toolkit мы убедились в значимости статического анализа при создании программного продукта. WPF Toolkit представляет собой совсем небольшой проект, однако на 112 тысяч строк кода попалось довольно много однотипных ошибок, вроде идентично реализованных методов, приведения объектов к своему же типу и т.д. Все эти ошибки легко находятся при помощи статического анализатора кода PVS-Studio, которым можно воспользоваться, скачав пробную версию по ссылке. Если ввести промокод #infocomp в поле «Сообщение», то можно получить лицензию на один месяц вместо 7 дней.
Автор: Екатерина Никифорова
