Операция «K». Ищем баги в коде IntelliJ IDEA
В этой статье мы проверим проект IntelliJ IDEA Community Edition на наличие ошибок и отправим наши правки разработчикам. Крупный проект, Open Source база и использование статического анализатора при разработке. Сложная задача для PVS-Studio.
О проекте
Каждый Java разработчик знаком с IDEA. У многих разработчиков эта интегрированная среда разработки была первой. Другие, возможно, предпочли эту IDE, попользовавшись другими. Тем не менее нельзя недооценить вклад IntelliJ для Java разработчиков.
IntelliJ IDEA разработана на языке программирования Java. Первая версия выпущена в 2001 году. В 2009 году JetBrains открыли исходный код под лицензией Apache, разрешающей использование и модификацию Community версии продукта. Кодовая база проекта проверяется инструментом Qodana — собственной платформой статического анализа кода.
Open Source контрибьюторы IntelliJ IDEA имеют доступ к открытой базе задач (YouTrack). Такая связка упрощает анализ кодовой базы и поможет нам определить источники и историю найденных нами проблем в коде проекта.
Так как команда разработки большая, а в работе они используют собственный статический анализатор, это серьёзный вызов для PVS-Studio! Запасайтесь печеньками, и мы погружаемся в мир срабатываний анализатора.
Проверяем проект
За пределами реальности
Предлагаю вам найти ошибку в следующем коде:
private static void doTest(@Nullable JBCefClient client) {
....
try {
browser = new JCEFHtmlPanel(client, "about:blank");
} catch (RuntimeException ex) {
fail("Exception occurred: " + ex.getMessage());
ex.printStackTrace(); // <=
}
....
}
В представленном фрагменте как будто проблем нет, но, чтобы их найти, нам нужно знать, что делает метод fail:
public static void fail(String message) {
if (message == null) {
throw new AssertionError();
}
throw new AssertionError(message);
}
У нас недостижимый код на горизонте! Посмотрите на функцию ex.printStackTrace () в первом фрагменте — мы пытаемся вывести трассировку стека в консоль уже после выброса исключения, из-за которого управление код никогда не получит. В подтверждение этого получаем сообщение:
V6019 Unreachable code detected. It is possible that an error is present. IDEA283718Test.java (55)
Учитывая, что код находится в unit-тестах, вывод stack trace был бы там очень кстати при отладке падения этого теста.
Проанализировав историю коммитов, могу сделать вывод, что ошибка была с момента первого написания в 2021 году.
Предлагаю поднять вывод stack trace до выброса исключений:
private static void doTest(@Nullable JBCefClient client) {
....
try {
browser = new JCEFHtmlPanel(client, "about:blank");
} catch (RuntimeException ex) {
ex.printStackTrace();
fail("Exception occurred: " + ex.getMessage());
}
....
}
Чудеса копирования
Часто ли вы копируете код вместо того, чтобы написать заново? Но невнимательность подкрадывается незаметно и оставляет свои следы. Следы в виде ошибок. Именно из-за такой практики программирования допускают вот такие опечатки:
private boolean areHomogenous(MouseWheelEvent e1,
MouseWheelEvent e2) {
if (e1 == null || e2 == null) return false;
double distance = ....;
return e1.getComponent() == e2.getComponent() &&
e1.getID() == e2.getID() &&
e1.getModifiersEx() == e2.getModifiersEx() &&
e1.isPopupTrigger() == e1.isPopupTrigger() && // <=
e1.getScrollType() == e2.getScrollType() &&
distance < TOLERANCE;
}
В этом методе программист использует неверную переменную и сравнивает значение isPopupTrigger с самим собой. Ничего удивительного, ведь это самый обычный пример ошибки Copy-Paste, которые анализатор, кстати, отлично ловит. Про это у нас есть статьи :)
И вот наше злополучное сообщение:
V6001 There are identical sub-expressions 'e1.isPopupTrigger ()' to the left and to the right of the '==' operator. JBCefOsrComponent.java (459)
Вот вам ещё одна ошибка Copy-Paste:
protected int locateUnicodeEscapeSequence(int start, int i) {
....
if (StringUtil.isOctalDigit(c)) {
if (i + 2 < myBufferEnd &&
StringUtil.isOctalDigit(myBuffer.charAt(i + 1)) &&
StringUtil.isOctalDigit(myBuffer.charAt(i + 1))) {
return i + 3;
}
}
....
}
Похожая ситуация: при разработке были допущены ошибки при копировании. Видимо, во второй проверке должно быть i + 2, но кто знает, как оно в действительности должно работать?
Адресованное нам сообщение анализатора:
V6001 There are identical sub-expressions 'StringUtil.isOctalDigit (myBuffer.charAt (i + 1))' to the left and to the right of the '&&' operator. JavaStringLiteralLexer.java (64), JavaStringLiteralLexer.java (64)
Делай или не делай
Давайте взглянем на следующий фрагмент кода:
public String buildBeanClass() {
if (recordsAvailable) {
out.append("(");
fields.stream().map(param -> param.getType()
.getCanonicalText(true) + " " + param.getName());
StringUtil.join(fields, param -> {
return ....;
}, ", ", out);
out.append("){}");
} else ....
}
Найдёте проблемную строку в этом коде сами? В качестве подсказки я представляю сообщение, которое нам выдал анализатор:
V6010 The return value of function 'map' is required to be utilized. ParameterObjectBuilder.java (103)
Объясняем. Мы создаём stream из коллекции fields и:
Не применяем терминирующей операции, используем только промежуточную операцию map (без терминирующей операции результат не будет получен);
Не используем результаты операции.
Буквально мы можем удалить эту строчку, и никто про неё не вспомнит.
Волшебство целочисленного деления
Представляю вам деление:
public class RectanglePainter2DTest extends AbstractPainter2DTest {
private static final int RECT_SIZE = 10;
private static final double ARC_SIZE = RECT_SIZE / 3;
...
}
Но деление-то не простое, а целочисленное, а значит мы вернём целое значение без знаков после запятой. Но переменная-то double. И во всех местах, где она используется, она представлена в виде double переменной.
Посмотрим сообщение анализатора:
V6094 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. RectanglePainter2DTest.java (24)
Могу ли я уверять, что срабатывание точно положительное? Думаю, что нет. Но мы такое ловим, и анализатор правильно поругался на потерю значений после запятой. Учитывая, что код используется в unit-тесте, — это, с одной стороны, не страшно. С другой — неточный набор данных, нот кул. В любом случае предложенное исправление будет таким:
private static final double ARC_SIZE = RECT_SIZE / 3f;
Рефакторинг, который привёл к избытку
Вы когда-нибудь думали, что иногда мы тратим время на что-то неоправданное? Так вот, именно здесь этим мы и занимаемся:
protected List<....> getEncodeReplacements(CharSequence input) {
for (int i = 0; i < input.length(); ++i) {
if (input.charAt(i) == '\n') {
final String replacement;
if (i + 1 >= input.length() ||
YAMLGrammarCharUtil.isSpaceLike(input.charAt(i + 1)) ||
input.charAt(i + 1) == '\n' ||
currentLineIsIndented)
{
replacement = "\n" + indentString;
}
else
{
replacement = "\n" + indentString;
}
....
continue;
}
....
}
}
Посмотрим сообщение:
V6004 The 'then' statement is equivalent to the 'else' statement. YAMLScalarTextImpl.java (116), YAMLScalarTextImpl.java (119)
Вне зависимости от условия выполнится одно и то же тело метода. Что больше всего пугает — из-за этой проверки код становится очень нечитабельным.
Просмотрев историю коммитов, могу сказать, что когда-то блок ветвления выглядел вот так:
if (i + 1 >= input.length() ||
YAMLGrammarCharUtil.isSpaceLike(input.charAt(i + 1)) ||
input.charAt(i + 1) == '\n' ||
currentLineIsIndented) {
replacement = "\n" + indentString;
}
else {
replacement = "\n\n" + indentString;
}
В какой-то момент один символ убрали со словами Yaml: 'YamlScalarTextEvaluator' extracted to implement old 'getTextValue' behavior. А мы спорить не будем, исправим код, уберём всю проверку, оставим только нужное:
for (int i = 0; i < input.length(); ++i) {
if (input.charAt(i) == '\n') {
final String replacement = "\n" + indentString;
....
continue;
}
....
}
Проделки одной константы
Привожу вам сокращённый фрагмент кода:
@Override
protected void customizeComponent(JList list, E value, boolean isSelected) {
....
boolean nextStepButtonSelected = false; // <=
....
myMnemonicLabel.setForeground(isSelected &&
isSelectable &&
!nextStepButtonSelected
? getSelectionForeground()
: foreground);
....
myShortcutLabel.setForeground(
isSelected &&
isSelectable &&
!nextStepButtonSelected
? UIManager.getColor("MenuItem.acceleratorSelectionForeground")
: UIManager.getColor("MenuItem.acceleratorForeground"));
....
boolean selected = isSelected && isSelectable && !nextStepButtonSelected;
setForegroundSelected(myValueLabel, selected);
....
}
Как вы думаете, что здесь не так? Переменная** **nextStepButtonSelected всегда false.
«Но на ней завязана куча поведения», — скажете вы. «Неважно», — отвечу вам.
V6007 Expression '! nextStepButtonSelected' is always true. PopupListElementRenderer.java (330)
V6007 Expression '! nextStepButtonSelected' is always true. PopupListElementRenderer.java (384)
V6007 Expression '! nextStepButtonSelected' is always true. PopupListElementRenderer.java (373)
Сразу три сообщения — нехорошо. Проанализировав код, могу сказать, что разработчики хотели отказаться от поведения, но убирать саму переменную не стали. У нас получилась константа false, которая привела к плохой читабельности и возможным проблемам при изменении кода в будущем.
Заключение
На этом, я думаю, нам стоит остановиться. Мы сделали pull request для разработчиков IDEA, и я поставленные задачи выполнил. Действительно, я рад помочь разработчикам своей любимой IDE.
Отличный результат проверки действующего проекта, который, к тому же, уже использует стат. анализ.
А вы хотите попробовать статический анализ? Вы всегда можете внедрить его в свой проект с помощью PVS-Studio, скачав по ссылке.
