Довольно частый (и логичный) вопрос к нашим статьям с проверкой открытых проектов: отправляются ли разработчикам баг-репорты? Так вот, ответ – да. Более того, мы на этом не останавливаемся и иногда отслеживаем прогресс. Сегодня хотелось бы рассказать об одном из случаев, где именно эта педантичность предотвратила фиктивное исправление бага.

Введение

Думаю, ни для кого не секрет, что сообщать разработчикам об ошибках очень важно. Ведь всем нравится, когда программы работают быстро, корректно и стабильно. Другое дело, что далеко не всех интересует, что происходит с их баг-репортами после отправки. А зря, ведь, уделив совсем немного своего внимания, можно значительно ускорить решение проблемы или даже помочь исправить больше, чем предполагалось изначально.

Так вот, о чём я. Пару месяцев назад мы опубликовали статью с проверкой проекта Chromium, в рамках которой я отправил разработчикам баг-репорт с ошибками из статьи. Но, как вы понимаете, если бы всё было хорошо, то и эта заметка не увидела бы свет. Что же пошло не так?

Важный дисклеймер:
ошибки свойственны всем, и у меня нет никаких претензий к разработчикам Chromium. Просто попался интересный случай, который можно привести в пример :)

Кроме того, хотелось бы выразить почтение разработчикам за то, настолько же оперативно они разбирают и исправляют присланные ошибки. Даже несмотря на бесконечный огромный список открытых issues, мой отчёт обработали практически в тот же день, да ещё и заложили исправление. Бывает и совсем по-другому.

Перед началом не помешает освежить в памяти ошибку, про которую дальше будет разговор (случай N8 из оригинальной статьи):

V501 There are identical sub-expressions 'file.MatchesExtension(L".xlsb")' to the left and to the right of the '||' operator. download_type_util.cc 60

ClientDownloadRequest::DownloadType GetDownloadType(const base::FilePath& file)
{
  ....
  if (file.MatchesExtension(FILE_PATH_LITERAL(".apk")))
    return ClientDownloadRequest::ANDROID_APK;
  ....
  else if (file.MatchesExtension(FILE_PATH_LITERAL(".pdf")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".doc")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".docb")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dot")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dotm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".dotx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xls")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) ||    // <=
           file.MatchesExtension(FILE_PATH_LITERAL(".xlt")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xltx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xltm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlsb")) ||    // <=
           file.MatchesExtension(FILE_PATH_LITERAL(".xla")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlam")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xll")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xlw")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppt")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pot")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pps")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pptx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".pptm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".potx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".potm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppam")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppsx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".ppsm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".sldx")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".xldm")) ||
           file.MatchesExtension(FILE_PATH_LITERAL(".rtf")))
    return ClientDownloadRequest::DOCUMENT;
  ....
}

Ближе к делу

Так вот, на одной из итераций проверки почты я заметил оповещение о том, что загрузили правки для моего отчёта. Хм… Всего через один день? Любопытство взяло верх, и я решил посмотреть, что же там происходит. И не зря…

Здесь случилось именно то, о чём меня предупреждали коллеги: в описании одной из ошибок я решил не полностью раскрыть описываемую проблему и оставил часть читателям для самостоятельной работы. Результат – красным цветом выделено исправление, внесённое разработчиками, а жёлтым цветом я выделил ещё одно дублированное значение, которое следовало найти самостоятельно.

Похоже, что разработчики немного поспешили с внесением изменений в код и не стали вчитываться в статью. Очень жаль, нужно будет учесть на будущее…

Конечно же, я сразу написал разработчикам, что следует уделить особое внимание внесённому изменению, и указал на дополнительное значение, которое следует проверить/убрать.

Что ж, теперь ошибка исправлена корректно, и можно даже немного подискутировать на тему, а можно ли было вообще избежать такой проблемы?

С одной стороны, у разработчиков Chromium и так достаточно работы, и за внимательное чтение чужих статей для поиска дополнительных проблем им, скорее всего, не доплачивают. А с другой – качество кода всё же страдает. В приведённом выше примере довольно сложно найти ошибку, даже зная, что она там точно есть. Эх, вот бы был способ как-нибудь отлавливать такие ошибки… Хотя подождите, он есть!

Не уверен насчёт классических код ревью (ведь этот код попал в репозиторий), но вот большинство статических анализаторов вполне справились бы с этим случаем. По крайней мере, должны были, ведь именно в этом и состоит смысл статического анализа кода – в быстром отслеживании ошибок в только что написанном или изменённом коде.

Кто-то может сказать, что нужно просто быть внимательным и хорошо структурировать код. Вариант хороший, но, к сожалению, в реальных проектах это удаётся далеко не всегда. Возможно, я не упомянул ещё какие-то варианты… Тогда буду рад выслушать ваше мнение по этому поводу в комментариях.

Кстати, у нас уже был похожий случай с проверкой проекта CovidSim. В нём разработчики также в спешке или по невнимательности попытались исправить код, но лучше не стало. Прочитать про этот и ещё один похожий случай можно в статьях моего коллеги "Как PVS-Studio защищает от поспешных правок кода" и "Как PVS-Studio защищает от поспешных правок кода, пример N2".

Ну и в конце хотелось бы узнать, а следите ли вы за дальнейшей судьбой своих баг-репортов?

Дополнительные ссылки

• Как PVS-Studio защищает от поспешных правок кода.
• Как внедрить статический анализатор кода в legacy проект и не демотивировать команду.
• Философия статического анализа кода: у нас 100 программистов, анализатор нашел мало ошибок, он бесполезен?

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Mikhail Gelvikh. A bug report's adventure.