Под капотом PVS-Studio для Java: разработка диагностик05.08.2020 17:03

Для разнообразия сегодня немного расскажем про процесс разработки и доработки диагностических правил для PVS-Studio Java. Посмотрим, почему старые срабатывания анализатора не слишком сильно плавают от релиза к релизу, а новые — не слишком сумасшедшие. А ещё немного заспойлерим «чего там у джавистов в планах» и покажем парочку красивых (и не очень) ошибок, найденных с помощью диагностик из следующего релиза.

Процесс разработки диагностик и SelfTester

Естественно, что каждое новое диагностическое правило начинается с идеи. А так как Java-анализатор — самое молодое направление развития PVS-Studio, то в основном эти идеи мы воруем у C/C++ и C# отделов. Но не всё так плохо: правила, придуманные самостоятельно (в том числе и пользователями — спасибо!), мы тоже добавляем, чтобы потом те же самые отделы их своровали у нас. Круговорот, как говорится.

Сама реализация правил в коде в большинстве случаев оказывается довольно конвейерным занятием. Создаёшь файл с несколькими синтетическими примерами, размечаешь руками, где должны быть ошибки, да с отладчиком наперевес бегаешь по синтаксическому дереву, пока не надоест и не покроешь все придуманные случаи. Иногда правила оказываются простыми до абсурда (например, V6063 состоит из буквально нескольких строк), а иногда приходится достаточно долго подумать над логикой.

Однако, на этом процесс только начинается. Как известно, синтетические примеры мы не особо любим, поскольку они крайне плохо отражают вид срабатываний анализатора в реальных проектах. Значительная часть этих примеров в наших юнит-тестах, кстати, набирается именно из реальных проектов — самостоятельно выдумать все возможные случаи практически нереально. А ещё юнит-тесты позволяют нам не потерять срабатывания на примерах из документации. Прецеденты были, да, только тсс.

Так вот, срабатывания в реальных проектах надо каким-то образом сначала находить. А ещё нужно как-то проверять, что:

• Правило не упадёт на open-source безумии, где частенько встречаются «интересные» решения;
• Нет явно ложных срабатываний (или же их очень-очень мало, и мы с ними не можем ничего поделать по тем или иным соображениям);
• Новые вручную добавленные аннотации для data-flow (механизм анализа потока данных) ничего не сломали и принесли чего-нибудь интересного;
• После изменений в ядре анализатор не развалился на всё том же open-source безумии;
• Новое правило не увеличивает время анализа проектов на over 9000%;
• Мы не потеряли «хороших» срабатываний, которые были раньше;
• Всякое прочее в этом духе.

В общем, здесь, подобно рыцарю на коне (немного хромающем, но мы над этим работаем), на передний план выходит SelfTester. Его главная и единственная задача — автоматически проверить пачку проектов и показать, какие срабатывания добавились, исчезли или изменились относительно «эталона» в системе контроля версий. Предоставить диффы по отчёту анализатора и показать соответствующий код в проектах, короче говоря. На данный момент SelfTester для Java проверяет 62 open-source проекта бородатых версий, среди которых числятся, например, DBeaver, Hibernate и Spring. Один полный прогон всех проектов занимает 2–2.5 часа, что, несомненно, больно, но ничего не поделаешь.

На скриншоте выше «зелёные» проекты — это те, в которых ничего не изменилось. Каждый дифф в «красных» проектах просматривается вручную и, если он корректен, подтверждается той самой кнопкой «Approve». Дистрибутив анализатора, кстати, соберётся только в случае, если SelfTester даёт чисто зелёный результат. Так мы, в общем-то, и поддерживаем консистентность результатов между разными версиями.

Кроме поддержания консистентности результатов, SelfTester позволяет нам избавляться от огромного количества ложных срабатываний ещё до релиза диагностики. Стандартный паттерн действий при разработке выглядит примерно так:

• Наиболее общая реализация правила по синтетическим тестам, необязательно полная. Например, «найти все использования паттерна double-checked locking» при поиске некорректных реализаций этого паттерна;
• Полный прогон SelfTester-а, обычно на ночь;
• По диффам отбираются и реализуются несколько признаков ложных срабатываний, которые добавляются в юнит-тесты;
• Прогон SelfTester-а по проектам, в которых ещё остались срабатывания;
• Повтор пунктов 3–4, пока остаются ложные срабатывания;
• Финальный просмотр диффов, их утверждение, подготовка документации (той самой, что на сайте);
• Добавление диагностики, нового эталона и документации в master.

К счастью, полные прогоны SelfTester-а достаточно редки, и »2–2.5 часа» ждать приходится не очень часто. Периодически удача обходит стороной, и срабатывания оказываются на крупных проектах вроде Sakai и Apache Hive — самое время попить кофе, попить кофе и попить кофе. Ещё можно позаниматься документацией, но это на любителя.

«А зачем нужны юнит-тесты, раз есть такой волшебный инструмент?»

А затем, что тесты значительно быстрее. Несколько минут — и уже есть результат. А ещё они позволяют увидеть, какая конкретно часть правила отвалилась. И, кроме того, не всегда все допустимые срабатывания какого-либо правила отлавливаются в проектах SelfTester-а, но их работоспособность тоже необходимо проверять.

Новые проблемы в старых знакомых

Изначально этот раздел статьи начинался со слов «Версии проектов в SelfTester-е достаточно старые, поэтому большая часть приведённых ошибок, скорее всего, уже исправлена». Однако, когда я решил в этом удостовериться, меня ждал сюрприз. Все ошибки до единой остались на своих местах. Абсолютно все. А это означает две вещи:

• Эти ошибки не являются сверхкритичными для работы приложения. Многие из них, кстати, находятся в коде тестов, а некорректные тесты сложно назвать состоятельными.
• Эти ошибки находятся в нечасто используемых файлах больших проектов, в которые разработчики практически не заходят. Из-за этого некорректный код обречён лежать там очень долго: скорее всего, до тех пор, пока из-за него не возникнет какой-нибудь критический баг.

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

P.S. Сказанное выше не значит, что статический анализ отлавливает только безвредные ошибки в неиспользуемом коде. Мы проверяем релизные (и почти релизные) версии проектов — в которых наиболее актуальные баги разработчики и тестировщики (а иногда, к сожалению, и пользователи) находили руками, что долго, дорого и больно. Подробнее про это можно почитать в нашей статье «Ошибки, которые не находит статический анализ кода, потому что он не используется».

Apache Dubbo и пустое меню

GitHub

Диагностика «V6080 Consider checking for misprints. It’s possible that an assigned variable should be checked in the next condition» уже была зарелизена в версии 7.08, но в наших статьях пока что не появлялась, так что самое время это исправить.

Menu.java:40

public class Menu
{
  private Map> menus = new HashMap>();

  public void putMenuItem(String menu, String item)
  {
    List items = menus.get(menu);
    if (item == null)                      // <=
    {
      items = new ArrayList();
      menus.put(menu, items);
    }
    items.add(item);
  }
  ....
}

Классический пример словаря «ключ-коллекция» и не менее классическая опечатка. Разработчик хотел создать коллекцию, соответствующую ключу, если её ещё не существует, но перепутал название переменной и получил не просто некорректную работу метода, но и NullPointerException в последней строке. Для Java 8 и позже для реализации подобных словарей стоит использовать метод computeIfAbsent:

public class Menu
{
  private Map> menus = new HashMap>();

  public void putMenuItem(String menu, String item)
  {
    List items = menus.computeIfAbsent(menu, key -> new ArrayList<>());
    items.add(item);
  }
  ....
}

Glassfish и double-checked locking

GitHub

Одной из диагностик, которые войдут в следующий релиз, является проверка корректной реализации паттерна «double-checked locking». Рекордсменом по срабатываниям из проектов SelfTester-а оказался Glassfish: всего PVS-Studio с помощью этого правила нашёл 10 проблемных мест в проекте. Предлагаю читателю немного развлечься и поискать два из них в отрывке кода ниже. За помощью можно обратиться к документации: «V6082 Unsafe double-checked locking». Ну или, если совсем не хочется, в конец статьи.

EjbComponentAnnotationScanner.java

public class EjbComponentAnnotationScanner
{
  private Set 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;");
  }

  ....
}

SonarQube и data-flow

GitHub

Доработка диагностик заключается не только в прямом изменении их кода с целью отловить больше подозрительных мест или убрать ложные срабатывания. Важную роль в процессе разработки анализатора играет и ручная разметка методов для data-flow — например, можно написать, что такой-то библиотечный метод всегда возвращает non-null. При написании новой диагностики мы совершенно случайно обнаружили, что у нас не размечен метод Map#clear (). Не считая явно глупого кода, который начала отлавливать диагностика «V6009 Collection is empty. The call of the 'clear' function is senseless», мы смогли найти отличную опечатку.

MetricRepositoryRule.java:90

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.

Sakai и пустые коллекции

GitHub

Ещё одна новая диагностика, которая войдёт в следующий релиз, — «V6084 Suspicious return of an always empty collection». Достаточно легко забыть добавить элементы в коллекцию, а особенно — когда каждый элемент надо сначала проинициализировать. Из личного опыта, такие ошибки чаще всего приводят не к падению приложения, а к странному поведению или отсутствию какого-либо функционала.

DateModel.java:361

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:

• Поле не объявлено как 'volatile'.
• Объект сначала публикуется, а потом инициализируется.

Чем всё это плохо — опять же, можно посмотреть в документации.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Nikita Lazeba. Under the Hood of PVS-Studio for Java: How We Develop Diagnostics.

© Habrahabr.ru