Анализ кода CUBA Platform с помощью PVS-Studio

[tagir_valeev]

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

  StringBuilder orderBy = new StringBuilder();
  ....
  if (orderBy.length() > 0) {
      orderBy.delete(orderBy.length() - 2, orderBy.length());
      orderBy.insert(0, " order by ");
  }

Сразу же видна разница в экспертизе :-) Я себе мгновенно представил, что в опущенном коде происходит: там в цикле что-то дописывается в StringBuilder, и в конец приписывается ", ". И так и оказалось! В этом StringBuilder никогда не может быть одного символа. Либо ноль (если в цикл ни разу не зашли), либо не меньше двух (потому что как минимум одна запятая с пробелом). Вы можете сколько угодно говорить, что код выглядит опасно, ненадёжно и т. д., но это чистый false-positive.

@Override
public boolean handle(ErrorEvent event, App app) {
  Throwable t = event.getThrowable();
 ..
  return true;

Хвастаетесь же, что у вас по десять исключений в каждой инспекции :-) Этот метод реализует метод интерфейса. В этом случае он действует в соответствии с документацией интерфейса: всегда возвращает true, сигнализируя о том, что исключение всегда обработано. В данном случае это не ошибка. Не стоит выдавать это предупреждение для реализаций интерфейсов.

public <T> T getSingleResultFromCache(QueryKey queryKey, List<View> views) {
  ....
  for (Object id : queryResult.getResult()) {
    return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....));
  }
  ....
}

Для циклов for-each это может быть спорный, но вполне используемый паттерн в джаве. Аналогичный код без цикла будет:

  Iterator<?> it = queryResult.getResult().iterator();
  if (it.hasNext()) {
    Object id = it.next();
    return (T) em.find(metaClass.getJavaClass(), id, views.toArray(....));
  }

Итог: две дополнительных строчки и дополнительная переменная в скоупе. И ради чего? Мы по умолчанию в данном случае предупреждение не выдаём (хотя опция есть выдавать), потому что тех, кто так пишет, можно понять. Во всех остальных типах циклов кроме for-each предупреждение выдаётся.

Анализатор обнаружил ситуацию, когда сравнение классов осуществляется по имени. Такое сравнение является некорректным, т.к., согласно спецификации, JVM классы имеют уникальное имя только внутри пакета. Это может стать причиной некорректного сравнения и выполнения не того кода, который планировался.

Это бла-бла совершенно не относится к данному случаю. Это совершенно нормальная ситуация: там сравнение с каким-то названием свойства. Именно с названием, которое может из какого-нибудь XML приходит или ещё откуда-то. Как в данной ситуации сравнить с конкретным классом, загруженным конкретным загрузчиком классов? Сериализовать класс-лоадер в XML? Ну-ну. В бизнес-логике это вполне частый случай, использовать простое имя класса в качестве имени какого-то свойства. Возможно, есть отдельный тест, который убеждается, что в системе нет двух классов с одинаковым именем, объявленных как MetaProperty. В общем, мусорное предупреждение.

---

Однако есть и весьма вкусные предупреждения. Статью плюсанул. Работайте дальше.

[dimkss]

  StringBuilder orderBy = new StringBuilder();
  ....
  if (orderBy.length() > 0) {
    ....
  }

Я со своей колокольни не назвал бы это ложным предупреждением.
Допустим, над проектом поработает не очень опытный разработчик, или просто ужасно спешащий, который закомментирует дописывание в StringBuilder символов.
Или спецификация измениться, и разработчик не удосужиться проскроллить текст функции вниз.
В общем ИМХО для чистоты я бы код таки поправил.

[tagir_valeev]

Вот когда закомментирует, тогда и надо выдавать предупреждение. А для чистоты надо на Stream API переписать, что авторы кода уже и сделали.

[dimkss]

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

[tagir_valeev]

Это вы, похоже, издеваетесь. Давайте в пустом проекте подчеркнём и напишем «в этом проекте в будущем возможны ошибки». Любой код можно испортить так, чтобы там была ошибка в будущем.

[tagir_valeev]

Интересно, кстати, это вы вручную преаннотировали org.apache.commons.lang3.StringUtils.isNotEmpty? Вряд ли у вас такой крутой межпроцедурный анализ :-)

[jreznot]

А можно и в IDEA это как-то проаннотировать для всех разработчиков в проекте? Догадываюсь, что можно контракты как-то сохранить в XML файлах проекта, но буду рад ссылкам на мануал.

[tagir_valeev]

У нас можно аннотировать контракты через действие Edit method contract, и они сохраняются в специальные XML-ки. Их можно как коммитить в VCS, так и деплоить в бинарный репозиторий и подкачивать вместе с библиотекой. Вот тут можно почитать про внешние аннотации, а вот тут конкретно про контракты.

К сожалению, такой контракт как у isNotEmpty целиком не опишешь внешней аннотацией, можно только null -> false написать, но полностью поведение метода не специфицируешь. Такие методы мы можем захардкодить при необходимости (внутренний язык контрактов богаче, чем опубликованный для всех). Есть предложение расширить общий язык контрактов, чтобы можно было писать (param1 != null && param1.length() > 0) -> true; () -> false, но никаких обещаний, будет ли это сделано, когда и в какой форме.

[jreznot]

Будем ждать контракты commons-lang3 в IntelliJ )