| сохранено

H FindBugs помогает узнать Java лучше

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

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

В этом посте я расскажу о некоторых тонкостях Java, о которых я узнал в результате использования статического анализатора FindBugs. Возможно, какие-то вещи окажутся неожиданными и для вас. Важно, что все примеры не умозрительны, а основаны на реальном коде.

Тернарный оператор ?:


Казалось бы, нет ничего проще тернарного оператора, но у него есть свои подводные камни. Я считал, что нет принципиальной разницы между конструкциями
Type var = condition ? valTrue : valFalse;
и
Type var;
if(condition)
  var = valTrue;
else
  var = valFalse;

Оказалось, что тут есть тонкость. Так как тернарный оператор может быть частью сложного выражения, его результатом должен быть конкретный тип, определённый на этапе компиляции. Поэтому, скажем, при истинном условии в if-форме компилятор приводит valTrue сразу к типу Type, а в форме тернарного оператора сперва приводит к общему типу valTrue и valFalse (несмотря на то, что valFalse не вычисляется), а затем уже результат приводит к типу Type. Правила приведения оказываются не совсем тривиальными, если в выражении участвуют примитивные типы и обёртки над ними (Integer, Double и т. д.) Все правила подробно описаны в JLS 15.25. Посмотрим на некоторые примеры.

Number n = flag ? new Integer(1) : new Double(2.0);

Что будет в n, если flag установлен? Объект Double со значением 1.0. Компилятору смешны наши неуклюжие попытки создать объект. Так как второй и третий аргумент — обёртки над разными примитивными типами, компилятор разворачивает их и приводит к более точному типу (в данном случае double). А уже после выполнения тернарного оператора для присваивания снова выполняется боксинг. По сути код эквивалентен такому:
Number n;
if( flag )
    n = Double.valueOf((double) ( new Integer(1).intValue() ));
else
    n = Double.valueOf(new Double(2.0).doubleValue());

С точки зрения компилятора код не содержит проблем и прекрасно компилируется. Но FindBugs выдаёт предупреждение:
BX_UNBOXED_AND_COERCED_FOR_TERNARY_OPERATOR: Primitive value is unboxed and coerced for ternary operator in TestTernary.main(String[])

A wrapped primitive value is unboxed and converted to another primitive type as part of the evaluation of a conditional ternary operator (the b? e1: e2 operator). The semantics of Java mandate that if e1 and e2 are wrapped numeric values, the values are unboxed and converted/coerced to their common type (e.g, if e1 is of type Integer and e2 is of type Float, then e1 is unboxed, converted to a floating point value, and boxed. See JLS Section 15.25.
Разумеется, FindBugs предупреждает и о том, что Integer.valueOf(1) эффективнее, чем new Integer(1), но это уж все и так знают.

Или такой пример:
Integer n = flag ? 1 : null;

Автор хочет поместить null в n, если флаг не установлен. Думаете, сработает? Да. Но давайте усложним:
Integer n = flag1 ? 1 : flag2 ? 2 : null;

Казалось бы, особой разницы нет. Однако теперь, если оба флага сброшены, данная строчка генерирует NullPointerException. Варианты для правого тернарного оператора — int и null, поэтому результирующий тип Integer. Варианты для левого — int и Integer, поэтому по правилам Java результат — int. Для этого надо совершить unboxing, вызвав intValue, что и выдаёт исключение. Код эквивалентен такому:
Integer n;
if( flag1 )
    n = Integer.valueOf(1);
else {
    if( flag2 )
        n = Integer.valueOf(Integer.valueOf(2).intValue());
    else
        n = Integer.valueOf(((Integer)null).intValue());
}

Здесь FindBugs выдаёт два сообщения, которых достаточно, чтобы заподозрить ошибку:
BX_UNBOXING_IMMEDIATELY_REBOXED: Boxed value is unboxed and then immediately reboxed in TestTernary.main(String[])
NP_NULL_ON_SOME_PATH: Possible null pointer dereference of null in TestTernary.main(String[])
There is a branch of statement that, if executed, guarantees that a null value will be dereferenced, which would generate a NullPointerException when the code is executed.

Ну и последний пример на эту тему:
double[] vals = new double[] {1.0, 2.0, 3.0};
double getVal(int idx) {
    return (idx < 0 || idx >= vals.length) ? null : vals[idx];
}

Неудивительно, что этот код не работает: как функция, возвращающая примитивный тип, может вернуть null? Удивительно, что он без проблем компилируется. Ну почему компилируется — вы уже поняли.

DateFormat


Для форматирования даты и времени в Java рекомендуется пользоваться классами, реализующими интерфейс DateFormat. Например, это выглядит так:
public String getDate() {
    return new SimpleDateFormat("yyyy-MM-dd HH:mm:ss").format(new Date());
}

Зачастую класс многократно использует один и тот же формат. Многим придёт в голову идея оптимизации: зачем каждый раз создавать объект формата, когда можно пользоваться общим экземпляром?
private static final DateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

public String getDate() {
    return format.format(new Date());
}

Вот так красиво и здорово, только, к сожалению, не работает. Точнее работает, но изредка ломается. Дело в том, что в документации к DateFormat написано:
Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally.

И это действительно так, если посмотреть внутреннюю реализацию SimpleDateFormat. В процессе выполнения метода format() объект пишет в поля класса, поэтому одновременное использование SimpleDateFormat из двух потоков приведёт с некоторой вероятностью к неправильному результату. Вот что пишет FindBugs по этому поводу:
STCAL_INVOKE_ON_STATIC_DATE_FORMAT_INSTANCE: Call to method of static java.text.DateFormat in TestDate.getDate()
As the JavaDoc states, DateFormats are inherently unsafe for multithreaded use. The detector has found a call to an instance of DateFormat that has been obtained via a static field. This looks suspicous.

For more information on this see Sun Bug #6231579 and Sun Bug #6178997.

Подводные камни BigDecimal


Узнав, что класс BigDecimal позволяет хранить дробные числа произвольной точности, и увидев, что у него есть конструктор от double, некоторые решат, что всё ясно и можно делать так:
System.out.println(new BigDecimal(1.1));

Делать так действительно никто не запрещает, вот только результат может показаться неожиданным: 1.100000000000000088817841970012523233890533447265625. Так происходит, потому что примитивный double хранится в формате IEEE754, в котором невозможно представить 1.1 идеально точно (в двоичной системе счисления получается бесконечная периодическая дробь). Поэтому там хранится максимально близкое значение к 1.1. А конструктор BigDecimal(double) напротив работает точно: он идеально преобразует заданное число в IEEE754 к десятичному виду (конечная двоичная дробь всегда представима в виде конечной десятичной). Если же хочется представить в виде BigDecimal именно 1.1, то можно написать либо new BigDecimal("1.1"), либо BigDecimal.valueOf(1.1). Если число не выводить сразу, а поделать с ним какие-то операции, можно и не понять, откуда берётся ошибка. FindBugs выдаёт предупреждение DMI_BIGDECIMAL_CONSTRUCTED_FROM_DOUBLE, в котором даются те же советы.

А вот ещё одна штука:
BigDecimal d1 = new BigDecimal("1.1");
BigDecimal d2 = new BigDecimal("1.10");
System.out.println(d1.equals(d2));

Фактически d1 и d2 представляют собой одно и то же число, но equals выдаёт false, потому что он сравнивает не только значение чисел, но и текущий порядок (число знаков после запятой). Это написано в документации, но мало кто будет читать документацию к такому знакомому методу как equals. Такая проблема может всплыть далеко не сразу. Сам FindBugs об этом, к сожалению, не предупреждает, но есть популярное расширение к нему — fb-contrib, в котором данный баг учтён:
MDM_BIGDECIMAL_EQUALS

equals() being called to compare two java.math.BigDecimal numbers. This is normally a mistake, as two BigDecimal objects are only equal if they are equal in both value and scale, so that 2.0 is not equal to 2.00. To compare BigDecimal objects for mathematical equality, use compareTo() instead.

Переводы строк и printf


Нередко программисты, перешедшие на Java после Си, с радостью открывают для себя PrintStream.printf (а также PrintWriter.printf и т. д.). Мол, отлично, это я знаю, прямо как в Си, ничего нового учить не надо. На самом деле есть отличия. Одно из них кроется в переводах строк.

В языке Си есть разделение на текстовые и бинарные потоки. Вывод символа '\n' в текстовый поток любым способом автоматически будет преобразован в системно-зависимый перевод строки ("\r\n" на Windows). В Java такого разделения нет: надо передавать в выходной поток правильную последовательность символов. Это автоматически делают, например, методы семейства PrintStream.println. Но при использовании printf передача '\n' в строке формата — это просто '\n', а не системно-зависимый перевод строки. К примеру, напишем такой код:
System.out.printf("%s\n", "str#1");
System.out.println("str#2");

Перенаправив результат в файл, увидим:

Таким образом можно получить странную комбинацию переводов строки в одном потоке, что выглядит неаккуратно и может снести крышу какому-нибудь парсеру. Ошибку можно долго не замечать, особенно если вы преимущественно работаете на Unix-системах. Для того, чтобы вставить правильный перевод строки с помощью printf, используется специальный символ форматирования "%n". Вот что пишет FindBugs по этому поводу:
VA_FORMAT_STRING_USES_NEWLINE: Format string should use %n rather than \n in TestNewline.main(String[])

This format string include a newline character (\n). In format strings, it is generally preferable better to use %n, which will produce the platform-specific line separator.


Возможно, для некоторых читателей всё перечисленное было давно известно. Но я практически уверен, что и для них найдётся интересное предупреждение статического анализатора, которое откроет им новые особенности используемого языка программирования.
+65
~31200

комментарии (32)

+13
+14 –1
vsb ,  
Ничего себе. Был уверен, что джаву знаю достаточно хорошо, но таких выкрутасов с приведением типов, как в первых примерах, никогда не встречал. Укрепился во мнении, что автобоксинг зло, достаточно было сделать синтаксический сахар, но явный, чем за спиной программиста создавать объекты.

Про BigDecimal.equals тоже не знал, хотя как-то интуитивно использовал compareTo.

FindBugs — замечательный инструмент.
+3
+4 –1
jakobz ,  
Достаточно было сделать compile-time generics :)
0
lany ,  
Проблема в том, что я этих выкрутасов тоже не видел, пока FindBugs не заюзал, но при этом они были (один из них даже я сам и породил). Они жили в коде пару лет, и их никто не замечал :-)
0
deilux ,  
Вы используете compareTo для проверки совпадения значений? В общем случае, не с числами, он может вам неприятно удивить.
0
omickron ,  
Предлагаю Вам развить эту тему.
Как говорят, «сказал А, говори Б» :)
0
leventov ,  
Видимо, имеется ввиду, что compareTo может быть не согласован с equals, хотя в документации настоятельно рекомендуется делать его согласованным.
0
omickron ,  
Безусловно, ведь это же разные методы.
Но, может быть, deilux имел ввиду ещё что-нибудь…
0
deilux ,  
Я и имел ввиду эту банальность: compareTo определяет порядок следования двух элементов, а equals — их равенство. Для тех же строк, в которых встречаются хитрые символы алфавита, мы получим compareTo = 0, equals = false.
0
omickron ,  
Да, Вы совершенно правы.
+2
dim_s ,  
На счет double я всегда знал что этот тип сравнивать обычным способом нельзя, всегда нужно учитывать погрешность, не только в Java, но и во многих других языках.
0
+1 –1
lany ,  
Зависит от задачи и от ситуации. Может оказаться, например, что значение в переменную double попадает прямиком из пользовательского ввода (а не в результате какого-либо математического действия) и, скажем значение 1.0 надо по алгоритму обработать специально. Тогда сравнивать по == совершенно уместно. Либо сравнение с нулём перед возможным делением на это число. В непараметрической статистике для обработки спаренных значений, возможно, адекватнее будет сравнивать по ==, чем вводить какую-то дельту…
+1
KeFA ,  
Idea, кстати, про возможный NPE в тернарном операторе тоже предупреждает)
+4
lanseg ,   * (был изменён)
Напоминает задачки из java puzzlers. Ещё вот сравнение двух URL вспоминается, когда (new URL(«url1.org»)).equals(new URL(«url2.whatever»)) выдавало true, если у этих url совпадали ip.
+2
+3 –1
Athari ,  
Стало интересно, как C# ведёт себя в аналогичных ситуациях.

1.
Number n = flag ? new Integer(1) : new Double(2.0);

Ручного боксинга примитивных типов в зоопарк типов нет — проблем нет. (Вообще, странно это. Непонятно, зачем приводить типы.)

2.
Integer n = flag1 ? 1 : flag2 ? 2 : null;

Приведения int к null нет — проблем нет. (В шарпе нужно писать (int?)null, чтобы аналог скомпилировался.)

3.
private static final DateFormat format = new SimpleDateFormat("yyyy-MM-dd HH:mm:ss");

Какой ProblemFactory замечательный. :) В шарпе формат — это строка. Поглядел документацию — не вкурил, зачем было огород городить, тем более непотокобезопасный. Джависты, можете пояснить, какие это даёт преимущества? А то я в джаве нуб. :)

4.
System.out.println(new BigDecimal(1.1));

Здесь шарп в пролёте, потому что BigDecimal нет, есть только BigInteger. Попробовал с обычным decimal — шарп не позволил скомпилировать аналогичный код, потому что неявного приведения от double к decimal не существует. И даже если явно привести, то эта шибко умная зараза каким-то образом умудряется правильно округлить.

5.
System.out.printf("%s\n", "str#1");

В шарпе такая же хрень с переносами строк, только ни printf, ни %n нет. Пишите Environment.NewLine, чтоб ему пусто было (ну или WriteLine вместо Write).
0
dim_s ,   * (был изменён)
Ручной боксинг в жаве нужен также из-за отсутствия возможности использовать примитивные типы в generics, и поэтому, например, когда создается объект HashMap<Integer, String>, можно использовать обычные цифры в качестве ключей, не создавая объекты. Да и для удобства.
+2
lany ,  
На всякий случай, вдруг кто-нибудь ещё не в курсе: TIntObjectHashMap.
+1
lany ,  
Я так же сравнивал C++ и Java в очередном посте про PVS-Studio и получалось, что большинство C++ проблем для Java неактуальны. Потом понял, что в Java не меньше своих проблем, которые в плюсах не возникают. Наверняка и в Шарпе есть специфические проблемы :-)

Про DateFormat подозреваю, что так было сделано с целью улучшить производительность, если один формат используется многократно в рамках одного потока. Тогда строка формата парсится один раз. Конечно то, что сам форматтер не потокобезопасный, это какая-то ужасная кривость. Pattern (прекомпилированное регулярное выражение), например, не менее сложная сущность, но он потокобезопасен. Если затраты производительности вас не смущают, вы всегда можете написать утилитный метод типа
public static String formatDate(String format, Date date) {
  return new SimpleDateFormat(format).format(date);
}
0
+2 –2
Athari ,  
В целом да, в шарпе есть неочевидные вещи, про которые мало кто (а иногда практически никто) не знает. Чего стоит одно создание делегатов: сколько объектов создастся по лямбдам и method group'ам, при каких условиях — это мозговыносяще (нагромождение обратной совместимости и оптимизаций). Существующие засады ещё и выпиливают (вон, даже создание переменных в цикле for поменяли).

Что меня ужасно радует — в шарпе нет маразмов, которые преследуют тебя на каждом шагу. В джаве есть == у строк и прочие вещи, которые не имеют никакого оправдания кроме перфекционизма. BigDecimal.equals из статьи — из той же серии. В шарпе из таких глобальных обломов могу вспомнить разве что события, принимающие значение null.

Впрочем, в большинстве случаев ReSharper и IDEA помножают такие проблемы на ноль для обоих языков. Причём сразу по мере ввода кода.
0
SVlad ,  
Работа с датами в Java вообще не лучшим образом сделана. Видимо, тяжелое наследие Java 1.0. Гораздо лучше использовать стороннюю библиотеку типа JodaTime.
+1
dim_s ,  
В Java 8 обещают все это безобразие исправить.
0
dim_s ,   * (был изменён)
{del}
+1
ZyL ,  
Я как-то несколько лет назад работал в одной конторе, мы занимались переписыванием серверной части сайта symantec.com. И у нас в Эклипсе по их требованию стояли семантековские настройки (правда, мне кажется, это был PMD).

Так вот, там тернарные операторы помечались как ошибка. Т.е. совсем ошибка, код не компилировался, т.е. их нельзя было использовать вообще.

У меня нет мнения, насколько это правильно, просто вспомнил.
+1
ttools ,  
Отличный инструмент! Спасибо за то, что познакомили с ним!
0
+1 –1
sompylasar ,  
Маленький вопрос от не-джависта:
можно написать либо new BigDecimal("1.1"), либо BigDecimal.valueOf(1.1).

Здесь опечатка или valueOf(1.1) как-то специальным образом компилируется, что 1.1 оказывается не IEEE754?
0
leventov ,  
Вопрос не ясен. 1.1 не может быть выражено в стандарте IEEE 754 никак. Специально ничего не компилируется.
0
+1 –1
sompylasar ,  
Тогда в тексте должно быть BigDecimal.valueOf("1.1"), а не BigDecimal.valueOf(1.1), если я правильно Вас понял.
0
leventov ,  
А, ну да. Опечатка в посте.
+2
lany ,  
В посте нет ошибки. Данный метод реализован так:
public static BigDecimal valueOf(double val) {
    return new BigDecimal(Double.toString(val));
}
То есть double сперва преобразуется в строку, используя обычный алгоритм, который работает с точностью double, а не с идеальной точностью. А потом уже вызывается конструктор от строки. Поэтому valueOf сработает не так как конструктор от double.
0
sompylasar ,  
То есть ошибка в точности представления числа 1.1 в double при использовании valueOf(double) сохранится, поскольку double представляется в IEEE754?

Кстати, в посте еще одна опечатка:
А конструктор BigDecimal(double) напротив работает точно

— должно быть от string:
А конструктор BigDecimal(string), напротив, работает точно
+1
lany ,  
То есть ошибка в точности представления числа 1.1 в double при использовании valueOf(double) сохранится, поскольку double представляется в IEEE754?
Не понял, что вы имели в виду. Произойдёт следующее:
1. Во время компиляции компилятор заменит 1.1 на ближайшее допустимое число, представимое в IEEE754, а именно на 1.100000000000000088817841970012523233890533447265625.
2. Во время выполнения это число будет преобразовано в строку с использованием метода Double.toString. Этот метод преобразует в строку, имея в виду точность типа double. Когда он поймёт, что получается что-то типа 1.100000000000000, а более точно в типе double всё равно не выразишь, он просто обрежет нули и вернёт результат «1.1».
3. Отработает конструктор BigDecimal от строки «1.1», который создаст объект BigDecimal, представляющий собой число 1.1

А конструктор BigDecimal(double) напротив работает точно

Нет здесь никакой опечатки. Это истинное утверждение и именно то, что я хотел сказать.
0
sompylasar ,   * (был изменён)
Спасибо. Теперь понятно, этого алгоритма в тексте поста и не хватало, чтобы понять, что происходит на самом деле и почему конструктор BigDecimal(double), напротив, работает точно.
+1
TimReset ,  
Вчера подключил FindBugs у нас на проекте и обнаружил несколько одинаковых ошибок с тернарным оператором.
Есть два метода:
  static void m(int i) {}
  static void m(Integer i) {}

Есть код, который вызывает один из этих методов:
 m(s != null ? Integer.parseInt(s) : (Integer) null);

, где s — типа String.
И суть ошибки — вызывается метод с примитивом (int) и если s == null, то будет вызван m(((Integer)null).intValue()); что приведёт к NPE.
Если не кастить null к Integer, или вместо Integer.parseInt(s) использовать Integer.valueOf(s) то будет вызываться метод m(Integer). Но из за стечения обстоятельств , говнокода и кривых рук будет вызываться метод m(int) и в случае s == null будет NPE