Проверка проекта CDK с помощью статического анализатора IntelliJ IDEA

Здесь я приведу некоторые ошибки, которые я нашёл. Я решил потестировать статический анализатор Java-кода IntelliJ IDEA и с его помощью проверил проект The Chemistry Development Kit. Думаю, что часть из них характерна для Java-программ в целом, поэтому могут быть интересны.

Когда я занимался биоинформатикой, мы активно её использовали. The Chemistry Development Kit — это Java-библиотека с открытыми исходниками для решения задач хемоинформатики и биоинформатики. Тем не менее, в проекте имеются юнит-тесты, а в pom.xml прописана интеграция с анализатором покрытия JaCoCo. Проект разрабатывается уже более 20 лет, у него десятки авторов, и качество кода там очень неровное. Тем интереснее проверить, какие же предупреждения остаются. Вдобавок там настроены плагины целых трёх статических анализаторов: FindBugs, PMD, Checkstyle.

Кроме того, практически все возможности статического анализа доступны в Community Edition — бесплатной версии IDE с открытым исходным кодом. Статический анализатор Java-кода, встроенный в IntelliJ IDEA, не уступает специализированным инструментам статического анализа, а в чём-то и превосходит их. В частности, бесплатная версия выдаёт все предупреждения, описанные в этой статье.

Можно проверить и весь проект или его часть в пакетном режиме с помощью меню Analyze | Inspect Code либо запустить отдельную инспекцию с помощью Analyze | Run Inspection by Name. По умолчанию статический анализ проводится постоянно в режиме редактирования кода, поэтому если вы пишете код в IntelliJ IDEA, то множество ошибок вы исправите буквально за секунды после того как их допустили, даже перед запуском тестов. Впрочем, таких инспекций немного. В этом случае становятся доступны некоторые инспекции, которые из-за трудоёмкости не работают в режиме редактирования.

Это полезно, когда вы постоянно работаете в IDE. Множество инспекций IntelliJ IDEA сообщают не о баге, а скорее о неаккуратности в коде или предлагают более идиоматичную, красивую или быструю альтернативу. В основном, интересна категория Java | Probable Bugs, хотя есть и другие категории, в которых стоит порыться, например, Numeric Issues. Однако в моём случае лучше начать с тех сообщений, которые предупреждают о реальных багах.

Я расскажу только о некоторых интересных предупреждениях.

1. Унарный плюс

Писать +1 вместо просто 1 иногда хочется для красоты. Унарных плюсов набралось аж 66 в проекте. Однако в некоторых случаях унарный плюс вылезает, если вместо += написали =+:

int totalCharge1 = 0;
while (atoms1.hasNext()) { totalCharge1 = +((IAtom) atoms1.next()).getFormalCharge();
}
Iterator<IAtom> atoms2 = products.atoms().iterator();
int totalCharge2 = 0;
while (atoms2.hasNext()) { totalCharge2 = +((IAtom) atoms2.next()).getFormalCharge();
}

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

2. Целочисленное деление с приведением к дробному

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

angle = 1 / 180 * Math.PI;

Похожая ошибка: К сожалению, угол получился вовсе не один градус, а ноль.

Integer c1 = features1.get(key);
Integer c2 = features2.get(key); c1 = c1 == null ? 0 : c1;
c2 = c2 == null ? 0 : c2;
sum += 1.0 - Math.abs(c1 - c2) / (c1 + c2); // integer division in floating-point context

Поэтому результат будет 0, если оба числа ненулевые, либо 1, если одно из них 0. Похоже, оба числа c1 и c2 неотрицательные, а, значит, модуль разности никогда не превысит сумму.

3. Вызов Class.getClass()

В результате получается опять же объект типа Class с константным значением Class.class. Иногда люди вызывают метод getClass() у объекта типа Class. Например, вот: Обычно это ошибка: getClass() вызывать не надо.

public <T extends ICDKObject> T ofClass(Class<T> intf, Object... objects) { try { if (!intf.isInterface()) throw new IllegalArgumentException("expected interface, got " + intf.getClass()); ...

Кстати, ошибки в процедуре обработки ошибок часто находятся статическим анализом в старых проектах: как правило, процедуры обработки ошибок тестируются хуже всего. Если случится исключение, сообщение о нём будет абсолютно бесполезно.

4. Вызов toString() на массиве

Обычно такое можно встретить в диагностических сообщениях. Это классика жанра: toString() для массивов не переопределён, и его результат довольно бесполезен.

int[] dim = ;
...
return "Dim:" + dim + " SizeX:" + grid.length + " SizeY:" + grid[0].length + " SizeZ:"...

Сразу же предлагается исправление — обернуть в Arrays.toString(dim). Глазами проблему заметить сложно, потому что здесь dim.toString() неявный, но конкатенация строк делегирует к нему.

5. Коллекция читается, но не заполняется

Вот простой пример: Такое тоже нередко встречается в кодовой базе, не проходящей постоянную проверку статическим анализатором.

final Set<IBond> bondsToHydrogens = new HashSet<IBond>();
// ... 220 строк логики, но bondsToHydrogens нигде не заполняется!
for (IBond bondToHydrogen : bondsToHydrogens) // в цикл не зайдём sgroup.removeBond(bondToHydrogen);

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

6. Наоборот: заполняем, но не читаем

Здесь пример с массивом: Обратные случаи тоже возможны.

int[] tmp = new int[trueBits.length - 1];
System.arraycopy(trueBits, 0, tmp, 0, i);
System.arraycopy(trueBits, i + 1, tmp, i, trueBits.length - i - 1);

Судя по логике кода, пропущена строка trueBits = tmp;. Инспекция знает, что третий аргумент метода arraycopy используется только для записи массива, а после этого массив никак не используется.

7. Сравнение Integer по ==

Такая проблема может совсем не бросаться в глаза: Это коварный баг, потому что малые значения объектов Integer кэшируются, и всё может неплохо работать, пока в один прекрасный день число не превысит 127.

for (int a = 0; a < cliqueSize; a++) { for (int b = 0; b < vecSize; b += 3) { if (cliqueList.get(a) == compGraphNodes.get(b + 2)) { cliqueMapping.add(compGraphNodes.get(b)); cliqueMapping.add(compGraphNodes.get(b + 1)); } }
}

Надо быть внимательным, чтобы увидеть, что в этих списках лежат объекты типа Integer. Ну, казалось бы, сравниваются какие-то объекты в каких-то списках, может, всё нормально.

8. Дубликат в Map

Видите ошибку?
В этой инспекции картинка стоит тысячи слов.

А так лучше?

9. Не используется результат метода

Результат некоторых методов глупо не использовать, о чём IDEA с готовностью сообщает:

currentChars.trim();

Так как строки в Java неизменяемые, если результат не переприсвоить, ничего не произойдёт. Вероятно, имелось в виду currentChars = currentChars.trim();. Встречается ещё, например, str.substring(2).

Помимо заранее подготовленного списка методов мы иногда пытаемся автоматически определять методы, результат которых использовать стоит. Кстати, это довольно сложная инспекция. И всё это делается на лету в процессе редактирования кода! Тут требуется межпроцедурный анализ, причём как по исходному тексту, так и по байткоду библиотек.

10. Недостижимые ветки switch

// if character is out of scope don't
if (c > 128) return 0;
switch (c) { case '\u002d': // hyphen case '\u2012': // figure dash case '\u2013': // en-dash case '\u2014': // em-dash case '\u2212': // minus return '-'; // 002d default: return c;
}

Кажется, исключать не стоило. Так как мы исключили символы с кодом больше 128, ветки \u2012-\u2212 недостижимы.

11. Недостижимое условие

Совершенно чудесная проблема в цепочке условий:

if (oxNum == 0) { if (hybrid.equals("sp3")) { ... } else if (hybrid.equals("sp2")) return 47;
} else if (oxNum == 1 && hybrid.equals("sp3")) return 47;
else if ((oxNum == 2 && hybrid.equals("sp3")) || (oxNum == 1 && hybrid.equals("sp2")) || (oxNum == 0 && hybrid.equals("sp"))) // вот это вот недостижимо return 48;
else if ((oxNum == 3 && hybrid.equals("sp3")) || (oxNum >= 2 && hybrid.equals("sp2")) || (oxNum >= 1 && hybrid.equals("sp"))) return 49;

Здесь мы имеем отдельную ветку oxNum == 0, а иначе проверяем oxNum == 0 && hybrid.equals("sp"), чего, конечно, быть не может. В сложной условной логике такое встречается нередко: мы проверяем условие, которое не может быть истинным, потому что его фрагмент уже проверили выше.

12. Пишем в массив нулевой длины

Иногда IntelliJ IDEA заметит, если вы пишете в массив за пределами его размера:

Point3d points[] = new Point3d[0]; // завели массив на 0 элементов
if (nwanted == 1) { points = new Point3d[1]; points[0] = new Point3d(aPoint); points[0].add(new Vector3d(length, 0.0, 0.0));
} else if (nwanted == 2) { // а тут пытаемся в него писать — исключение неминуемо points[0] = new Point3d(aPoint); points[0].add(new Vector3d(length, 0.0, 0.0)); points[1] = new Point3d(aPoint); points[1].add(new Vector3d(-length, 0.0, 0.0));
}

13. Проверка длины после обращения по индексу

Ещё одна частая проблема с порядком действий и опять во время обработки ошибок:

public void setParameters(Object[] params) throws CDKException { if (params.length > 1) { throw new CDKException("..."); } if (!(params[0] instanceof Integer)) { // раз прочитали нулевой элемент throw new CDKException("The parameter must be of type Integer"); } if (params.length == 0) return; // то длина точно не нуль maxIterations = (Integer) params[0];
}

Очевидно, порядок проверок нарушен. В случае пустого массива автор кода хотел выйти тихо, но из-за проверки выйдет, громко бахнув ArrayIndexOutOfBoundsException.

14. Проверка на null после обращения

И опять нарушен порядок действий, на этот раз с null'ом:

while (!line.startsWith("frame:") && input.ready() && line != null) { line = input.readLine(); logger.debug(lineNumber++ + ": ", line);
}

Случается, что проверка действительно избыточна, но здесь код выглядит так, будто null действительно может быть. IDEA пишет, что line != null всегда истинно.

15. Дизъюнкция вместо конъюнкции

Проект CDK — не исключение: Люди часто путают логические операторы И и ИЛИ.

if (rStereo != 4 || pStereo != 4 || rStereo != 3 || pStereo != 3) { ... }

Чему бы ни были равны rStereo и pStereo, понятно, что они не могут быть равны четвёрке и тройке одновременно, поэтому такое условие всегда истинно.

16. И снова дизъюнкция вместо конъюнкции

Похожая ошибка, но ловится другим сообщением:

if (getFirstMapping() != null || !getFirstMapping().isEmpty()) { ... }

Кстати, тут мы полагаемся на стабильность результатов метода getFirstMapping(). В правую часть мы можем попасть только если getFirstMapping() вернул null, но в этом случае нам гарантирован NullPointerException, о чём IDEA и предупреждает. Так как класс финальный, метод не может быть переопределён. Иногда мы используем эвристики, но здесь стабильность прямо проанализирована. null : firstSolution и определяет, что стабильность сводится к стабильности метода Map#isEmpty, который заранее проаннотирован как стабильный. IDEA проверяет его тело return firstSolution.isEmpty() ?

17. Иерархия интерфейсов и instanceof

Когда проверяете объект на принадлежность какому-либо интерфейсу, не забывайте, что интерфейсы могут наследовать друг друга:

if (object instanceof IAtomContainer) { root = convertor.cdkAtomContainerToCMLMolecule((IAtomContainer) object);
} else if (object instanceof ICrystal) { root = convertor.cdkCrystalToCMLMolecule((ICrystal) object);
} ...

Интерфейс ICrystal расширяет интерфейс IAtomContainer, поэтому вторая ветка заведомо недостижима: если сюда придёт кристалл, он попадёт в первую ветку.

18. Обход пустого списка

Вероятно, автор этого кода не очень знаком с языком Java:

List<Integer> posNumList = new ArrayList<Integer>(size); for (int i = 0; i < posNumList.size(); i++) { posNumList.add(i, 0);
}

Это используется для оптимизации, чтобы уменьшить число аллокаций, если вы знаете заранее, сколько туда сложить элементов. Параметр size в ArrayList указывает изначальный размер внутреннего массива. Поэтому следующий цикл с попыткой инициализировать элементы списка нулями совершенно бесполезен. При этом фактически элементы в списке не появляются, и метод size() возвращает 0.

19. Не забываем инициализировать поля

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

public class IMatrix { public double[][] realmatrix; public double[][] imagmatrix; public int rows; public int columns; public IMatrix(Matrix m) { rows = m.rows; columns = m.columns; int i, j; for (i = 0; i < rows; i++) for (j = 0; j < columns; j++) { realmatrix[i][j] = m.matrix[i][j]; // NullPointerException гарантирован imagmatrix[i][j] = 0d; } }
}

Поэтому IDEA смело выдаёт предупреждение, что обращение к элементу массива вызовет NullPointerException. Несмотря на то что поля публичные, здесь точно никто не мог вклиниться и инициализировать их перед конструктором.

20. По два раза не повторять

Вот пример: Повторные условия тоже случаются нередко.

if (commonAtomCount > vfMCSSize && commonAtomCount > vfMCSSize) { return true;
}

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

Любопытно, что когда авторы проекта исправили часть, они сами воспользовались анализатором IntelliJ IDEA, нашли другие проблемы, о которых я не писал, и тоже стали их исправлять. Я сообщил о некоторых из этих багов в баг-трекер проекта. Я считаю это хорошим знаком: авторы осознали важность статического анализа.

Теги

intellij idea Java баги Блог компании JetBrains проверка кода Программирование статический анализ Тестирование IT-систем