Для разнообразия сегодня немного расскажем про процесс разработки и доработки диагностических правил для PVS-Studio Java. Посмотрим, почему старые срабатывания анализатора не слишком сильно плавают от релиза к релизу, а новые – не слишком сумасшедшие. А ещё немного заспойлерим "чего там у джавистов в планах" и покажем парочку красивых (и не очень) ошибок, найденных с помощью диагностик из следующего релиза.
Естественно, что каждое новое диагностическое правило начинается с идеи. А так как Java-анализатор – самое молодое направление развития PVS-Studio, то в основном эти идеи мы воруем у C/C++ и C# отделов. Но не всё так плохо: правила, придуманные самостоятельно (в том числе и пользователями – спасибо!), мы тоже добавляем, чтобы потом те же самые отделы их своровали у нас. Круговорот, как говорится.
Сама реализация правил в коде в большинстве случаев оказывается довольно конвейерным занятием. Создаёшь файл с несколькими синтетическими примерами, размечаешь руками, где должны быть ошибки, да с отладчиком наперевес бегаешь по синтаксическому дереву, пока не надоест и не покроешь все придуманные случаи. Иногда правила оказываются простыми до абсурда (например, V6063 состоит из буквально нескольких строк), а иногда приходится достаточно долго подумать над логикой.
Однако, на этом процесс только начинается. Как известно, синтетические примеры мы не особо любим, поскольку они крайне плохо отражают вид срабатываний анализатора в реальных проектах. Значительная часть этих примеров в наших юнит-тестах, кстати, набирается именно из реальных проектов – самостоятельно выдумать все возможные случаи практически нереально. А ещё юнит-тесты позволяют нам не потерять срабатывания на примерах из документации. Прецеденты были, да, только тсс.
Так вот, срабатывания в реальных проектах надо каким-то образом сначала находить. А ещё нужно как-то проверять, что:
В общем, здесь, подобно рыцарю на коне (немного хромающем, но мы над этим работаем), на передний план выходит SelfTester. Его главная и единственная задача – автоматически проверить пачку проектов и показать, какие срабатывания добавились, исчезли или изменились относительно "эталона" в системе контроля версий. Предоставить диффы по отчёту анализатора и показать соответствующий код в проектах, короче говоря. На данный момент SelfTester для Java проверяет 62 open-source проекта бородатых версий, среди которых числятся, например, DBeaver, Hibernate и Spring. Один полный прогон всех проектов занимает 2-2.5 часа, что, несомненно, больно, но ничего не поделаешь.
На скриншоте выше "зелёные" проекты – это те, в которых ничего не изменилось. Каждый дифф в "красных" проектах просматривается вручную и, если он корректен, подтверждается той самой кнопкой "Approve". Дистрибутив анализатора, кстати, соберётся только в случае, если SelfTester даёт чисто зелёный результат. Так мы, в общем-то, и поддерживаем консистентность результатов между разными версиями.
Кроме поддержания консистентности результатов, SelfTester позволяет нам избавляться от огромного количества ложных срабатываний ещё до релиза диагностики. Стандартный паттерн действий при разработке выглядит примерно так:
К счастью, полные прогоны SelfTester-а достаточно редки, и "2-2.5 часа" ждать приходится не очень часто. Периодически удача обходит стороной, и срабатывания оказываются на крупных проектах вроде Sakai и Apache Hive – самое время попить кофе, попить кофе и попить кофе. Ещё можно позаниматься документацией, но это на любителя.
"А зачем нужны юнит-тесты, раз есть такой волшебный инструмент?"
А затем, что тесты значительно быстрее. Несколько минут – и уже есть результат. А ещё они позволяют увидеть, какая конкретно часть правила отвалилась. И, кроме того, не всегда все допустимые срабатывания какого-либо правила отлавливаются в проектах SelfTester-а, но их работоспособность тоже необходимо проверять.
Изначально этот раздел статьи начинался со слов "Версии проектов в SelfTester-е достаточно старые, поэтому большая часть приведённых ошибок, скорее всего, уже исправлена". Однако, когда я решил в этом удостовериться, меня ждал сюрприз. Все ошибки до единой остались на своих местах. Абсолютно все. А это означает две вещи:
Для желающих покопать поглубже будут ссылки на конкретные версии, которые мы проверяем.
P.S. Сказанное выше не значит, что статический анализ отлавливает только безвредные ошибки в неиспользуемом коде. Мы проверяем релизные (и почти релизные) версии проектов – в которых наиболее актуальные баги разработчики и тестировщики (а иногда, к сожалению, и пользователи) находили руками, что долго, дорого и больно. Подробнее про это можно почитать в нашей статье "Ошибки, которые не находит статический анализ кода, потому что он не используется".
Диагностика "V6080 Consider checking for misprints. It's possible that an assigned variable should be checked in the next condition" уже была зарелизена в версии 7.08, но в наших статьях пока что не появлялась, так что самое время это исправить.
public class Menu
{
private Map<String, List<String>> menus = new HashMap<String, List<String>>();
public void putMenuItem(String menu, String item)
{
List<String> items = menus.get(menu);
if (item == null) // <=
{
items = new ArrayList<String>();
menus.put(menu, items);
}
items.add(item);
}
....
}Классический пример словаря "ключ-коллекция" и не менее классическая опечатка. Разработчик хотел создать коллекцию, соответствующую ключу, если её ещё не существует, но перепутал название переменной и получил не просто некорректную работу метода, но и NullPointerException в последней строке. Для Java 8 и позже для реализации подобных словарей стоит использовать метод computeIfAbsent:
public class Menu
{
private Map<String, List<String>> menus = new HashMap<String, List<String>>();
public void putMenuItem(String menu, String item)
{
List<String> items = menus.computeIfAbsent(menu, key -> new ArrayList<>());
items.add(item);
}
....
}Одной из диагностик, которые войдут в следующий релиз, является проверка корректной реализации паттерна "double-checked locking". Рекордсменом по срабатываниям из проектов SelfTester-а оказался Glassfish: всего PVS-Studio с помощью этого правила нашёл 10 проблемных мест в проекте. Предлагаю читателю немного развлечься и поискать два из них в отрывке кода ниже. За помощью можно обратиться к документации: "V6082 Unsafe double-checked locking". Ну или, если совсем не хочется, в конец статьи.
EjbComponentAnnotationScanner.java
public class EjbComponentAnnotationScanner
{
private Set<String> annotations = null;
public boolean isAnnotation(String value)
{
if (annotations == null)
{
synchronized (EjbComponentAnnotationScanner.class)
{
if (annotations == null)
{
init();
}
}
}
return annotations.contains(value);
}
private void init()
{
annotations = new HashSet();
annotations.add("Ljavax/ejb/Stateless;");
annotations.add("Ljavax/ejb/Stateful;");
annotations.add("Ljavax/ejb/MessageDriven;");
annotations.add("Ljavax/ejb/Singleton;");
}
....
}Доработка диагностик заключается не только в прямом изменении их кода с целью отловить больше подозрительных мест или убрать ложные срабатывания. Важную роль в процессе разработки анализатора играет и ручная разметка методов для data-flow – например, можно написать, что такой-то библиотечный метод всегда возвращает non-null. При написании новой диагностики мы совершенно случайно обнаружили, что у нас не размечен метод Map#clear(). Не считая явно глупого кода, который начала отлавливать диагностика "V6009 Collection is empty. The call of the 'clear' function is senseless", мы смогли найти отличную опечатку.
protected void after()
{
this.metricsById.clear();
this.metricsById.clear();
}На первый взгляд, повторная очистка словаря – не ошибка. И мы бы даже решили, что это случайно продублированная строка, если бы взгляд не опустился чуть ниже – буквально на следующий метод.
protected void after()
{
this.metricsById.clear();
this.metricsById.clear();
}
public Metric getByKey(String key)
{
Metric res = metricsByKey.get(key);
....
}Именно. У класса есть два поля с похожими именами metricsById и metricsByKey. Я уверен, что в методе after разработчик хотел очистить оба словаря, но либо его подвело автодополнение, либо он по инерции вписал то же имя. Таким образом, два словаря, которые хранят связанные данные, будут рассинхронизированы после вызова after.
Ещё одна новая диагностика, которая войдёт в следующий релиз, – "V6084 Suspicious return of an always empty collection". Достаточно легко забыть добавить элементы в коллекцию, а особенно – когда каждый элемент надо сначала проинициализировать. Из личного опыта, такие ошибки чаще всего приводят не к падению приложения, а к странному поведению или отсутствию какого-либо функционала.
public List getDaySelectItems()
{
List selectDays = new ArrayList();
Integer[] d = this.getDays();
for (int i = 0; i < d.length; i++)
{
SelectItem selectDay = new SelectItem(d[i], d[i].toString());
}
return selectDays;
}В том же классе, кстати, находятся очень похожие методы без такой же ошибки. Например:
public List getMonthSelectItems()
{
List selectMonths = new ArrayList();
Integer[] m = this.getMonths();
for (int i = 0; i < m.length; i++)
{
SelectItem selectMonth = new SelectItem(m[i], m[i].toString());
selectMonths.add(selectMonth);
}
return selectMonths;
}Не считая разных не очень интересных внутренних вещей, мы думаем насчёт добавления в Java-анализатор диагностик для Spring Framework. Он не только является основным хлебом для джавистов, но и содержит в себе достаточно много неочевидных моментов, на которых можно споткнуться. Мы пока не очень уверены, в каком именно виде эти диагностики в итоге появятся, когда это произойдёт и произойдёт ли вообще. Но зато мы уверены, что нам нужны идеи для них и open-source проекты, использующие Spring, для SelfTester-а. Так что, если есть что-то на примете – предлагайте! А чем больше этого добра мы наберём, тем больше приоритет будет на него смещаться.
Ну и в заключение ошибки в реализации double-checked locking из Glassfish:
Чем всё это плохо – опять же, можно посмотреть в документации.