Проверка проекта 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, нашли другие проблемы, о которых я не писал, и тоже стали их исправлять. Я сообщил о некоторых из этих багов в баг-трекер проекта. Я считаю это хорошим знаком: авторы осознали важность статического анализа.