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

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

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

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

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

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

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

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

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

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

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

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

Иногда люди вызывают метод getClass() у объекта типа Class. В результате получается опять же объект типа Class с константным значением Class.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             = {0, 0, 0};
...
return "Dim:" + dim + " SizeX:" + grid.length + " SizeY:" + grid[0].length + " SizeZ:"...

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

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);

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

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();

Вероятно, имелось в виду currentChars = currentChars.trim();. Так как строки в Java неизменяемые, если результат не переприсвоить, ничего не произойдёт. Встречается ещё, например, 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);
}

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

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

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

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

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

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

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

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

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