PVS-Studio - это инструмент для выявления ошибок в исходном коде программ, написанных на языках С, C++ и C#.
PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.
Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.
PVS-Studio выполняет статический анализ кода и генерирует отчёт, помогающий программисту находить и устранять ошибки. PVS-Studio выполняет широкий спектр проверок кода, но наиболее силён в поисках опечаток и последствий неудачного Copy-Paste. Показательные примеры таких ошибок: V501, V517, V522, V523, V3001.
Анализатор ориентирован на разработчиков, использующих среду Visual Studio, и может в фоновом режиме выполнять анализ измененных файлов после их компиляции. В идеале ошибки будут обнаружены и исправлены ещё до попадания в репозиторий. Однако ничто не мешает использовать анализатор для проверки всего решения целиком или для встраивания в системы непрерывной интеграции. Эти и иные способы использования анализатора описаны в документации.
Microsoft открыла исходники Xamarin.Forms. Мы не могли упустить шанс проверить их с помощью PVS-Studio
Запись от el_programmer размещена 25.05.2016 в 10:30
Показов 2508
Комментарии 0
Метки c#, csharp, microsoft, open source, programming, xamarin
|
Автор: Сергей Васильев Не так давно, как вы наверняка знаете, корпорация Microsoft купила компанию Xamarin. Даже несмотря на то, что в последнее время Microsoft начала постепенно открывать исходные коды своих продуктов, открытие кода Xamarin.Forms стало большим сюрпризом. Я не смог пройти мимо такого события, и решил проверить исходный код этого проекта с помощью статического анализатора кода. Анализируемый проект Xamarin.Forms - кроссплатформенный набор инструментов, позволяющий создавать пользовательские интерфейсы, общие для различных платформ: Windows, Windows Phone, iOS, Android. Пользовательские интерфейсы отрисовываются, используя нативные компоненты конечной платформы, что позволяет сохранять приложениям, созданным с помощью Xamarin.Forms, общий вид для каждой платформы. Для создания пользовательских интерфейсов с привязками данных и различными стилями вы можете использовать C# или XAML-разметку. Код самого фреймворка также написан на языке C# и доступен в репозитории на GitHub. Инструмент анализа Проект проверялся с помощью статического анализатора кода PVS-Studio, в разработке которого я принимаю активное участие. Мы постоянно работаем над его улучшением, в том числе дорабатывая существующие и добавляя новые диагностические правила. Поэтому с каждой проверкой нового проекта нам удаётся выявлять всё больше разновидностей ошибок. Каждое диагностическое правило содержит документацию, включающую в себя описание ошибки, а также примеры неверного и верного кода. Она поясняет, почему выбраны именно такие ограничения демонстрационной версии и что нужно сделать, чтобы попробовать весь функционал. Подозрительные фрагменты кода Начнём с "классических" ошибок, выявляемых диагностическим правилом V3001:
Как видно из кода, вычисляется значение какого-то выражения с использованием битовых операций. При этом в одном из подвыражений - RwWait | RwWait участвуют одинаковые константные поля. Это не имеет смысла. При этом видно, что набор констант, объявленный выше, имеет значения, равные степеням двойки, следовательно, подразумевалось их использование как флагов (что мы и видим на примере использования битовых операций). Думаю, практичнее было бы вынести их в перечисление, отмеченное атрибутом [Flags], что дало бы ряд преимуществ при работе с этим перечислением (см. документацию V3059). Что до текущего примера - скорее всего, подразумевалось использование константы RwWrite. Это можно отнести к одному из минусов IntelliSense - несмотря на то, что это средство очень помогает в написании кода, порой оно может "подсказать" не ту переменную, из-за чего по невнимательности можно допустить ошибку. Следующий пример кода, где была допущена схожая ошибка:
В выражении 2 раза встречается подвыражение Left == 0. Очевидно, что это ошибка. На месте последнего подвыражения должен находиться следующий код - Bottom == 0, так как это единственное свойство (следуя логике и исходя из набора свойств), не проверяемое в данном выражении. Следующая ошибка интересна тем, что находится в 2-ух файлах с одинаковыми именами и частично схожим кодом. Вот так и получается, что ошибки размножаются - ошиблись в одном месте, скопировали этот код в другое - оп! - вот вам ещё одно ошибочное место.
В данном фрагменте кода наблюдается странная логика в операторе if. Проверяется некоторое условие (widthConstraint <= 0) и, если оно не выполняется, вновь проверяется это же условие. Ошибка? Ошибка. А вот как исправить, сказать уже сложнее. Это задача уже ложится на плечи программиста, писавшего код. Как я говорил, нашлась точно такая же ошибка в файле с таким же названием. Вот соответствующее сообщение анализатора: V3003 The use of 'if (A) {...} else if (A) {...}' pattern was detected. There is a probability of logical error presence. Check lines: 26, 28. Xamarin.Forms.Platform.Android SwitchRenderer.cs 26 Благодаря механизму виртуальных значений удалось значительно улучшить ряд диагностических правил, в том числе и диагностику V3022, определяющую, что выражение всегда имеет значение true или false. Предлагаю взглянуть на несколько примеров, которые удалось найти с её помощью:
Несмотря на то, что я удалил ту часть метода, которая нас не интересует, даже сейчас ошибка может быть не слишком заметной. Чтобы исправить эту ситуацию, предлагаю ещё немного упростить код, переписав его с более короткими именами переменных:
Если вы ещё не догадались в чём проблема - объясняю. В случае, если программа дойдёт до вычисления значения тернарного оператора, уже точно известно, что выражение a == enVal имеет значение false, следовательно, в тернарном операторе оно будет иметь то же значение. Итог: результат работы тернарного оператора всегда один и тот же. Ошибочка. Сходу подобные дефекты обнаружить бывает тяжело, ведь даже вырезав лишний код из метода, ошибка ловко утаивается в оставшемся. Пришлось прибегать к дополнительным упрощениям, чтобы выявить этот "подводный камень". Впрочем, для анализатора такой проблемы нет, и он легко справился с поставленной задачей. Конечно, это не единственный подобный случай. Вот ещё один:
Опять же, для того, чтобы легче заметить подвох, вырежем весь лишний код. И вот оно - 2 раза проверяется условие forceInferChildren, при этом между операторами if данная переменная никак не используется. Если учесть, что это параметр метода, можно сделать заключение, что ни другие потоки, ни какие-либо методы не могут изменить его без прямого обращения. Следовательно - если выполняется первый оператор if, всегда будет выполняться и второй. Странная логика. Есть диагностика, схожая с V3022 - V3063. Это диагностическое правило определяет, что часть условного выражения всегда истинна или ложна. Благодаря ей удалось обнаружить несколько интересных фрагментов кода:
Нас интересует подвыражение exception == null. Очевидно, что оно всегда будет иметь значение true. К чему тогда эта проверка? Неясно. К слову, никаких комментариев, каким-то образом сигнализирующих о том, что значение может быть изменено в процессе отладки (вроде // new Exception();) здесь нет. Это не единственные подозрительные места, найденные диагностическими правилами V3022 и V3063. Но не будем на них зацикливаться, а посмотрим, что же ещё интересного удалось найти.
Из этого кода нас интересует метод Replace, вызываемый для некоторой строки. Видимо, программист хотел заменить все символы одинарной кавычки символом слеша и кавычки. Но дело в том, что во втором случае символ слеша экранируется, поэтому вызов данного метода заменяет одинарную кавычку ей же. Не верите? Equals("'", "\'") в помощь. Для кого-то это может быть неочевидно, но не для анализатора. Чтобы избежать экранирования, можно использовать перед строковым литералом символ @. Тогда корректный вызов метода Replace выглядел бы так:
Независимо от того, какие аргументы пришли на вход и что выполняется в этом методе, он всегда возвращает значение false.Согласитесь, выглядит это как-то странно. Кстати, этот код встретился ещё раз - метод скопировали полностью и перенесли в другое место. Сообщение анализатора: V3009 It's odd that this method always returns one and the same value of 'false'. Xamarin.Forms.Xaml.Xamlg Options.cs 1020 Встретились несколько фрагментов кода с повторным генерированием исключения, потенциально содержащих ошибки.
Казалось бы, логика проста. В случае возникновения исключения мы совершаем какие-то действия и повторно генерируем его. Но дьявол кроется в деталях. В данном случае при повторной генерации исключения стек оригинального исключения полностью "затирается". Чтобы этого избежать, не нужно заново генерировать это же исключение, достаточно сделать "переброс" существующего, вызвав оператор throw. Тогда код блока catch мог бы выглядеть так:
В обоих случаях теряется информация о предыдущем исключении. И если во втором случае ещё можно предположить, что эта информация не будет актуальной (хотя всё равно странно), то в первом - вряд ли, скорее всего, хотели перебросить исключение вверх, но вместо этого сгенерировали новое. Решение такое же, как и в предыдущем примере - вызов оператора throw без аргументов. На счёт следующего фрагмента - не возьмусь сказать наверняка, ошибка это или нет, но выглядит странно.
Анализатор насторожил тот факт, что обращение к свойству Title выполняется разными способами - Element.Titleи Element?.Title, причём сначала обращаются напрямую, а затем - с использованием null-условного оператора. Но тут всё не так однозначно. Как вы могли заметить, в начале метода выполняется проверка Element?.Detail == null, предполагающая, что если Element == null, то выход осуществится здесь и до дальнейших операций дело не дойдёт. В то же время выражение Element?.Title предполагает, что на момент его выполнения Element может иметь значение null. Если это так, то на предыдущем этапе, в момент обращения к свойству Title напрямую, будет сгенерировано исключение типа NullReferenceException, а следовательно – никакого толку от использования null-условного оператора нет. В любом случае, этот код выглядит очень странно и его нужно поправить. Выглядело странно, когда объект приводили к собственному же типу. Вот пример такого кода:
В данном случае это не ошибка, но выглядит код как минимум подозрительно, с учётом того, что объект Control уже имеет тип FormsPivot. К слову, это не единственное предупреждение подобного рода, встретились и другие:
Есть условия, которые можно было бы упростить. Пример одного из них:
Данное выражение можно упростить, убрав подвыражение _scroller != null. Оно будет вычисляться только в случае, когда ложно выражение, стоящее слева от оператора '||' - _scroller == null, следовательно - _scroller не равен null и можно не опасаться получить исключение NullReferenceException. Тогда упрощённый код будет выглядеть так:
Ложка дёгтя К сожалению, собрать решение полностью не удалось – порядка 6 проектов остались непроверенными, а те места, где классы из них так или иначе использовались, подвергались не такому тщательному анализу, как могли бы. Возможно, в них удалось бы найти ещё что-нибудь интересное, но, увы. К слову, узнать о проблемах с анализом можно по сообщению V051, находящемуся на 3 уровне. Наличие таких предупреждений, как правило, является сигналом того, что C# проект содержит какие-то ошибки компиляции, из-за чего анализатор не может получить полную информацию, необходимую для проведения сложного анализа. Тем не менее, он постарается выполнить проверки, для которых не нужна подробная информация о типах и объектах. При проверке проекта желательно убедиться, что у вас отсутствуют предупреждения V051. А если они всё же есть – постараться избавиться от них (проверить, что проект скомпилирован, убедиться, что все зависимости загружены). Заключение Проверка Xamarin.Forms оправдала себя - нашлись разные интересные места, как явно ошибочные, так и весьма подозрительные или странные. Надеюсь, разработчики не обойдут статью стороной и исправят выписанные здесь фрагменты кода. Все подозрительные места, которые удалось обнаружить, можно посмотреть, загрузив пробную версию анализатора. Ещё лучшим и более правильным решением будет внедрение PVS-Studio на постоянной основе, что позволит обнаруживать и исправлять ошибки сразу же после их появления. | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Метки c#, csharp, microsoft, open source, programming, xamarin
Размещено в Без категории
Надоела реклама? Зарегистрируйтесь и она исчезнет полностью.
Всего комментариев 0
Комментарии


