PVS-Studio, как и другие статические анализаторы кода, часто выдаёт ложные срабатывания. Но не стоит спешить считать странные срабатывания ложными. Это короткая история о том, как PVS-Studio вновь оказался внимательнее нескольких человек.
Нам в поддержку написал пользователь, утверждая, что анализатор выдаёт сразу четыре ложных срабатывания на одну строчку кода. Письмо, написанное в поддержку, изначально попало к Евгению Рыжкову, который, бегло прочитав его и не заметив аномального в фидбеке, сразу переслал его ведущему разработчику Святославу Размыслову. Евгений не всматривался в код, так что будет честно посчитать его только за половину программиста :).
Святослав прочитал письмо и засомневался, что анализатор мог так грубо ошибиться. Поэтому он пришёл ко мне на консультацию. У Святослава была надежда, что мой глаз намётан и я замечу что-то такое, что подскажет, почему анализатор выдал все эти странные сообщения. К сожалению, я только подтвердил, что сообщения действительно очень странные и их не должно быть. Однако что же послужило причиной их возникновения, я заметить не смог. Было решено открыть задачу в багтрекере и начать разбираться, что же не так.
И только когда Святослав начал делать синтетические примеры, чтобы подробно расписать проблему в багтрекере, на него снизошло озарение. Давайте теперь посмотрим, сможете ли вы быстро найти причину, почему анализатор выдаёт 4 сообщения.
Вот текст письма, публикуемый с разрешения автора. И поясняющая картинка, которая была прикреплена к письму.
V560 warnings here are all false. Running with most recent version of PVS-Studio for personal use. Basically, «IF» statement is correct. Outer one is done for speed — inner ones are still needed and non are always true or false.
Теперь, уважаемый читатель, ваше время проверить себя! Видите ошибку?
Ваше время проявить внимательность. А единорог пока немного подождёт.
После вводной части статьи, скорее всего многие нашли ошибку. Когда настроен найти ошибку, она находится. Намного сложнее заметить ляп после прочтения письма, где это называется «ложными срабатываниями» :).
Теперь пояснение для тех, кто поленился искать баг. Ещё раз рассмотрим условие:
if (!((ch >= 0x0FF10) && (ch <= 0x0FF19)) ||
((ch >= 0x0FF21) && (ch <= 0x0FF3A)) ||
((ch >= 0x0FF41) && (ch <= 0x0FF5A)))Автор кода планировал проверить, что символ не входит ни в один из трёх диапазонов.
Ошибка в том, что оператор логический NOT (!) применяется только к первому подвыражению.
Если выполнилось условие:
!((ch >= 0x0FF10) && (ch <= 0x0FF19))то вычисление выражения прерывается согласно short-circuit evaluation. Если условие не выполняется, то значение переменной ch лежит в диапазоне [0xFF10..0xFF19]. Соответственно, четыре дальнейших сравнения не имеют смысла. Все они будут ложными или истинными.
Ещё раз. Смотрите, если ch лежит в диапазоне [0xFF10..0xFF19] и продолжается вычисление выражения, то:
- ch >= 0x0FF21 — всегда false
- ch <= 0x0FF3A — всегда true
- ch >= 0x0FF41 — всегда false
- ch <= 0x0FF5A — всегда true
Об этом и предупреждает анализатор PVS-Studio.
Вот так, статический анализатор оказался внимательнее пользователя и двух с половиной программистов из нашей команды.
Чтобы исправить ситуацию, надо добавить дополнительные скобки:
if (!(((ch >= 0x0FF10) && (ch <= 0x0FF19)) ||
((ch >= 0x0FF21) && (ch <= 0x0FF3A)) ||
((ch >= 0x0FF41) && (ch <= 0x0FF5A))))Или переписать условие:
if (((ch < 0x0FF10) || (ch > 0x0FF19)) &&
((ch < 0x0FF21) || (ch > 0x0FF3A)) &&
((ch < 0x0FF41) || (ch > 0x0FF5A)))Впрочем, я не могу рекомендовать к использованию ни один из этих вариантов. Я бы для упрощения чтения кода написал так:
const bool isLetterOrDigit = (ch >= 0x0FF10 && ch <= 0x0FF19) // 0..9
|| (ch >= 0x0FF21 && ch <= 0x0FF3A) // A..Z
|| (ch >= 0x0FF41 && ch <= 0x0FF5A); // a..z
if (!isLetterOrDigit)Обратите внимание, что я убрал часть скобок. Как только что мы видели, большое количество скобок вовсе не помогло избежать ошибки. Скобки должны облегчать чтение кода, а не усложнять. Программисты хорошо помнят, что приоритет сравнений =<, => выше, чем у оператора &&. Поэтому здесь скобки не нужны. А вот если спросить, что приоритетней — && или ||, многие растеряются. Поэтому для задания последовательности вычислений &&, || скобки лучше поставить.
Почему лучше писать || в начале я описывал в статье "Главный вопрос программирования, рефакторинга и всего такого" (см. главу: Выравнивайте однотипный код «таблицей»).
Всем спасибо за внимание. Скачивайте и начинайте использовать PVS-Studio. Он поможет выявлять многие ошибки и потенциальные уязвимости на самых ранних этапах.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. How PVS-Studio Proved to Be More Attentive Than Three and a Half Programmers.
Zolg
Примерно поэтому придерживаюсь правила 'лишних скобок не бывает'
Archius11
Вы будто статью не дочитали.
Zolg
Лично мне так наглядней. Оператор — явно указанная область действия. Плюс подсветка в IDE и вот это вот все.
Ошибку в примере нашел сразу.
Your milage may vary.
AgentRX
Простота и читаемость кода лишней не бывает. А множество скобок лишь ухудшают восприятие логики.
Yuuri
Вы сейчас просто разом всех лисперов идеологически оскорбили!
EviLOne
Большое количество скобок увеличивает вероятность ошибки. Лучше разделять операции по строчкам, по идеи для компилятора это не имеет смысла.
Antervis
когда много строк относится к одной операции читать код тоже неприятно. Обычно лучше вынести сложную проверку в функцию, аля:
Самое главное в таком подходе то, что условие именуется. С удачно выбранным именем функции во-первых её назначение понятно без чтения реализации, во-вторых проще найти ошибку в условии, нарушающем предполагаемую из имени логику
questor
Красивый пример. Коротко, наглядно, понятно. Переписано хорошо. Можно статью джунам показывать в качестве примера.
MonkAlex
Поэтому мне нравится подсветка скобочек в IDE.
После этого самого "!" ставишь курсор и смотришь, подсветило в конце или нет.
Vlad_IT
Я вот таким чудом пользуюсь https://marketplace.visualstudio.com/items?itemName=TomasRestrepo.Viasfora там сразу без курсора видно, т.к. для каждой пары задается свой цвет.
MonkAlex
А можете вариант из статьи показать как будет выглядеть?
ПС: не всегда пишу в студии, так что конкретно студийное решение не всегда хорошо.
Vlad_IT
Студии под рукой нет, в VS Code такое же расширение у меня стоит.
MonkAlex
Красных скобок в итоге много, т.е. первая и последняя в принципе красные, в конкретно этом кейсе не сильно помогает.
AngReload
Здесь нужна по настоящему мощная подсветка:
berezuev
И вариант для тех, кто использует продукты Intellij: plugins.jetbrains.com/plugin/10080-rainbow-brackets
lany
В Идейке (правда только для Джавы, видимо) и подсветка скобок не нужна, варнинги будут сразу в редакторе при вводе текста. У нас это онлайн-анализатор ловит:
Любопытно, кстати, что PVS-Studio анализирует недостижимый код. Мы, например, не выдаём предупреждение, что
ch <= 0x0FF3Aвсегда истинно, потому что оно вообще недостижимо. Видимо, когда PVS-Studio находит всегда ложное условие, оно выдаёт на него варнинг и считает, что этого условия нету и продолжает анализ дальше?lany
А ещё нажав Ctrl+Shift+P два раза на выражении, можно увидеть статически выведенный диапазон значений:
Правда он не настолько умён, чтобы по контексту догадаться выводить в хексах. Но таким образом иногда можно отдебажить варнинг и понять, что собственно произошло.
Все эти фичи есть в бесплатной версии IDEA Community :-)
Andrey2008 Автор
PVS-Studio иногда анализирует недостижимый код, хотя многие диагностики туда не лезут. Вот именно V560 это в рамках одного выражения этого не учитывает. Так уж диагностика была исторически реализована. Хорошо это или плохо, не знаю. Всё относительно :). Менять поведение особенного смысла не вижу.
А вот, например, диагностика V522 ведёт себя иначе. Здесь будет предупреждение:
V522 CWE-476 Dereferencing of the null pointer 'a' might take place. consoleapplication2017.cpp 125
А здесь (в мёртвом коде) уже нет:
Здесь нет V522, зато ожидаемо есть V560 CWE-570 A part of conditional expression is always false: !a. test.cpp 125
lany
Менять не надо, если откровенно путающих ситуаций не возникает, согласен.
А если наоборот,
!a && a && *a == 2Andrey2008 Автор
V560 CWE-570 A part of conditional expression is always false: a. test.cpp 125
vsb
На самом деле скобочки там как раз почти все избыточны. Приоритет операторов там и так правильный. Не было бы лишних скобочек, было бы очевидно, в чём проблема. Любят насовать скобочек не думая вместо того, чтобы один раз выучить маленькую таблицу приоритетов.
thatsme
От этого "!", захотелось избавиться с первого взгляда на код. И только потом спросил себя, а не хотел-ли автор вынести это «НЕ» за общие скобки. Вобщем да, — лишних скобок не бывает.
Sabubu
А в PHP есть костыль: при использовании
!по-другому интерпретируются приоритеты операторов:if (!$x = 2)Интерпретируется как
!($x = 2), а не как(!$x) = 2, как следует из приоритетов.AllexIn
Ужас, блин! Тут надо ворнинг пихать, а не другую интрепретацию делать…
GreedyIvan
Не надо такое в ворнинг.
Это базовый шаблон для PHP
lex_r
В условиях лучше не использовать присваивания независимо от языка. Лучше писать так:
a-tk
А ещё в PHP есть операторы && и AND, || и OR, с разными приоритетами (буквенные — ниже)
Goodkat
!($x = 2) всегда будет false.
(!$x) = 2 — тут ошибка.
if (!$x = 2) — условие никогда не выполнится же, так как !2 даст false.
aamonster
Плохо стало уже при попытке прочитать код. Такое надо сразу переписывать, не дожидаясь диагностики анализатора (всё равно она тут "понятна" как ошибки при компиляции сложносочинённых c++ темплейтов), хотя бы в три строчки, как вы сделали при разборе.
Mingun
Хотя бы банально выкинуть все лишние скобки, которые только запутывают, но не несут никакого практического смысла.
aamonster
В данном случае ещё можно, но для большинства случаев я с вами не соглашусь: я в гробу видал идею запоминать приоритеты всех операторов для всех языков, а главное — не буду закладываться, что их назубок помнит тот, кому доведётся читать мой код. Лучше уж лишние скобки, а ещё лучше разбить на подвыражения, как ниже предложил Akon32.
Mingun
Речь именно что про данный случай. Ошибка то возникла как раз из-за того, что программист запутался в скобках. А было их всего две пары — для
if'а и отрицания — проблемы бы вообще не было.Ну, и никто не заставляет помнить приоритеты всех операторов, но, извините, приоритеты сложения/умножения и ИЛИ/И/НЕ должны отскакивать на зубок, на мой взгляд, и в дополнительных скобках не нуждаться.
aamonster
У меня в анамнезе Паскаль, в котором приоритет операции and выше, чем операции сравнения. Я, конечно, запомнил разницу, но скобки зачастую ставлю. И не поручусь, что кто-то по запарке не прочитает неправильно.
vanxant
Приоритеты & и | запоминаются очень легко. & это умножение, | сложение.
lany
Главное не распространить это на битовый сдвиг, который у многих ассоциируется с умножением и делением, а приоритет его ниже сложения.
Akon32
С кем вам приходится работать? Гнать надо этих "многих", извините.А не лучше ли написать вместо
multiprogramm
Вот да. Правда, второй вариант не всегда может прокатить, не каждый символ можно так просто добавить в исходник — и в таких случаях я бы рекомендовал завести константы с говорящими именами, типа BIG_LETTER_A или FIRST_BIG_LETTER_EN, а в них уже можно любые коды засовывать, всё равно далее исходник будет читаемым.
А вот третий вариант лично я бы рекомендовал почти всем и всегда. А то и в отдельный метод IsLetterOrDigit.
Ошибку, кстати, нашёл, нарисовав интервалы на прямой, но изначально ожидал от задачи подвох в стиле дефайна скобки в начале файла или чего-то такого.
beeruser
У вас ASCII символы, а не unicode.
Код '0' будет 0x30 а не как ожидалось 0xff10
unicode-table.com/ru/blocks/halfwidth-and-fullwidth-forms
Akon32
И правда. Тогда нужно
u'a', u'z'и т.п., или как там выглядят юникод литералы на С++. Писать 0xff10 — явно плохая практика.beeruser
И получится у вас ещё большая путаница. Эти символы лишь похожи на обычные, но на самом деле другие. Ссылку я не случайно привёл.
a !=a
Вот вы собрались кого-то там гнать с работы, но сами с двух попыток не смогли правильный символ задать.
Может сейчас всё иначе, но раньше использовать исходники в unicode/uft8 — хороший способ познать боль, получив на экране квадратики любимым шрифтом или проблемы с контролем версий.
Salabar
Еще вариант:
multiprogramm
Кстати, вы могли бы этот таск переклассифицировать из баги в фичреквест и не спешить закрывать. Ведь на самом деле ситуация показательная и с другой стороны: лог анализатора не дал трём с половиной программистам понять, что же происходит. Т.е. анализатор умён, спору нет, но мысль свою до людей он не донёс. Может быть, стоит на такие проблемы в логических выражениях выдавать другую ошибку? Какую-нибудь одну, красивую, которая подсветит сереньким цветом выражения, не влияющие на истинность условия, или как-то так? Ведь, насколько я понимаю, вы же всё равно строите деревья разборов этих условий, т.е. самая сложная часть уже есть и отлично работает.
Andrey2008 Автор
Мы по возможности стараемся решать эту задачу. Иногда один общий алгоритм находит ошибку, которая затем для разных случаев разбивается на пяток сообщений для понятности. Но все случаи не предусмотреть.
aamonster
Ну, тут imho невозможно выдать понятную диагностику, пока не будет понятного кода. Т.е., получается, что надо навязывать codestyle?
Mingun
Я думаю, будет достаточно добавить в сообщение ", потому что ...". В данном примере,
Подчеркнутые места сделать ссылками на соответствующие места в коде. Так как абстрактный вычислитель значений уже есть, то кажется, что это не очень сложно должно быть сделать
slonopotamus
Эмм… Это вот вы серьёзно про "многих" которые не знают у чего выше приоритет — && или ||? Они точно программисты? А бухгалтеры многие не знают что приоритетнее — умножение или сложение?
Andrey2008 Автор
Добавил опрос.
slonopotamus
Спасибо. Результаты… удивляют.
iig
Джва с половиной программиста уже запуталась в казалось бы несложном выражении.
Antervis
вот только знание приоритетов этих операций нужно лишь для явных нарушений правил «хорошего тона»
slonopotamus
Утверждаете ли вы что запись 2 + 2 * 2 нарушает правила "хорошего тона"? И кто устанавливает эти правила? Чем эта запись лучше/хуже a || b && c?
Antervis
как минимум тем, что не выучив приоритет арифметических операций даже школу не закончишь. Во-вторых, логических операций несколько больше, и, даже если вы способны вспомнить приоритет конъюнкции и дизъюнкции, сомневаюсь, что выражение аля x & y && z | q || w окажется для вас таким же тривиальным.
mSnus
Конечно, нарушает. Найти вам примеры языков, где 2+3*2 будет равно 10?
slonopotamus
Текст правила или статьи или чего там именно оно «конечно» нарушает в студию. Желательно со ссылкой на место откуда ваше правило взялось.
slonopotamus
Что докажет ваш пример? Я могу выдумать язык программирования где результатом этого выражения будет «КРАСНЫЙ КРУГ,» и что? Есть языки в которых это вообще не является валидным выражением, и, опять же, что с того?
mSnus
Вы же понимаете, что "хороший тон" — это некоторая обобщённая условность. Придерживать за собой дверь — хороший тон, но если это дверь с красным кругом на броне — вход в бомбоубежище, а за вами гонится толпа зомби, то дверь можно уже и не придерживать.
Также считается хорошим тоном не засовывать взведённый пистолет себе за пояс, чтобы не выстрелить себе в… ногу.
Возвращаясь от метафор к языкам — если вы можете парой простых приёмов сделать ваш код однозначнее или понятнее для других, то использовать эти приемы — хороший тон.
В исходном случае правило, что && это умножение, а || — сложение, может быть далеко не очевидно тем, кто привык к другим языкам. И подобная привычка легко может сработать как выстрел в ногу.
slonopotamus
Я понимаю что «хороший тон», «правила приличия», «мораль» и всё такое штука субъективная и различается от социума к социуму и от индивидуума к индивидууму. Но если вы посмотрите тредик, мои прямые вопросы о том, что собеседники вкладывают в понятие «хорошего тона» применительно к &&/||, успешно игнорируются и мне видимо предлагается догадываться с помощью телепатии.
Разверните мысль, какая именно привычка может сработать как выстрел в ногу? Привыкание к языкам в которых || приоритетнее &&? Вполне допускаю что вы правы и лучше бы не привыкать к таким языкам.
Am0ralist
Могу в ответ баянчик приложить:
NDA81
Расставленные скобки однозначно показывают какое выражение должно(хотел написать автор) быть записано:
(2 + 2) * 2 или 2 + (2 * 2).
ads83
Когда я учился, я знал про приоритет этих операций. Также как и чем отличается сектор диска от кластера и на каком прерывании «висит» клавиатура. Но сегодня мне нужно посмотреть документацию.
На это есть причины
Aiditz
Несмотря на знание приоритетов, читать выражения типа (a || b && c || d) крайне трудно.
ads83
Хотя выражение
(a || b && c || d)уже неприятно читать, через N коммитов оно может стать(a || b && c || d && !e),а позже мутировать через
(a || b && c || d && !e || (f && g))в
(a || b && c || d && !e || (f && g || !a)).Через два года тимлид тратит четыре дня чтобы отловить плавающий баг, доходит до этой строчки, в гневе пишет git history… и видит коммиты от троих людей, двое из которых давно уволились, а init commit писал этот тимлид, когда был молодым.
ads83
И уже не у кого спросить, зачем появились скобки в хвосте выражения и нельзя по коду понять что хотели достичь последние два коммита, а контакты в телеграмме давно протухли. По старым адресам жили другие люди (тимлид на пару с битой проверял лично).
Осталось последнее средство, к которому прибегают в крайнем случае. Несмотря на всю его мощь, мало кто рисковал использовать его. Но выбора не было. Тимлид закрылся в своем кабинете, сдул пыль с бумажной стопки и открыл ТЗ…
конец бесплатного фрагмента
AdAbsurdum
А почему бы не сделать выделение метода для условий и назвать его нормально?
IsDigit, IsUpperLetter, IsLowerLetter?
bodqhrohro
А почему статья на английском не на Хабре?
Error1024
А Delphi/Object Pascal просто не даст скомпилироваться такому коду, пока все скобки у логических операций не проставишь.
mayorovp
Конкретно эту ошибку система типов Паскаля не ловит.
UncleJey
Доброго дня! А вы только по c? Есть ли у вас angularJS анализатор? Или может подскажет кто?
ads83
Я далек от мира фронтенда вообще и JavaScript фреймворков в частности. Поэтому вполне серьезно спрашиваю: AngularJS действительно так развился, что для него нужен особенный анализатор, отличный от plain old JavaScript анализатора?
Просветите, пожалуйста
lany
Ангуляр никогда не трогал, но мой опыт подсказывает, что даже для какой-нибудь маленькой библиотеки можно написать статические правила, которые уберегут вас от ошибочных или неоптимальных использований. А уж Ангуляр-то совсем немаленькая штука.
В IntelliJ IDEA вижу одну инспекцию: Angular|Empty Event Handler. Что бы это ни значило.
Andrey2008 Автор
PVS-Studio: C, C++, C#, Java (на подходе).
Другие анализаторы: en.wikipedia.org/wiki/List_of_tools_for_static_code_analysis
MadHacker
Хороший список.
А для Pascal/Delphi не планируете сделать анализатор? Там в списке про такой язык упоминают в комментариях два универсальных анализатора. :)
Andrey2008 Автор
Нет. Быть может дальше (но не скоро): JavaScript или PHP.
UncleJey
а не анализировали doom например?
Andrey2008 Автор
Анализировали (ещё в 2011 году).
Примечание: Обновляемый список статей, в которых мы рассказываем об ошибках, найденных с помощью PVS-Studio в открытых проектах.
olekl
Я вот сначала подумал, что там word стал отрицательным (0x0FF00 в 16-битном представлении будет отрицательным) и поэтому «больше-меньше» местами поменялось…
chabapok
А зачем люди препендят 0 к hex числам? 0x0ff21 вместо 0xff21?
DelphiCowboy
Действительно странно! Тогда было бы логичнее писать 0x00ff21, чтобы не прочитать в спешке ff с лишним ноликом как 0f f…
hhba
Статья хоть и короткая, но как обычно интересная! Занятно то, что даже такой лось, как я, нашел ошибку в коде буквально с первых прочитанных символов. Так что автор кода меня слегка пугает… И, да, я бы предпочел лучшее раскрытие условий в тексте программы (например так, как автор статьи предлагает, хотя варианты в комментариях есть и получше), но при этом скобок я бы не стеснялся. Как выше написали — в гробу я видал правила приоритета в тех или иных языках. При правильном раскрытии условий скобок будет минимум, но ровно там, где нужно.