Статичний аналізатор HuntBugs: перевіряємо IntelliJ IDEA

Як багато хто пам'ятає, деякий час я розвивав статичний аналізатор Java-байткода FindBugs. Проте проблем у FindBugs накопичилося стільки, що я вирішив, що простіше написати новий аналізатор байткода. Я не дуже творчо назвав його HuntBugs. Розробка ведеться на GitHub. Він поки в ранній стадії розробки, іноді глючить і покриває приблизно 35% діагностик з FindBugs, але при цьому додає свої цікаві штуки. Спробувати можна на вашому Maven-проекті з допомогою команди
mvn one.util:huntbugs-maven-plugin:huntbugs
(звіт пишеться в
target/huntbugs/report.html
). Альтернативно можна зібрати вручну з гіта і запустити додаток командного рядка
one.util.huntbugs.HuntBugs
, якому можна подавати на вхід JAR-файли або каталоги .class-файлами.
Як-небудь потім, коли проект трохи подорослішає, я розповім про нього детальніше. А в цій статті я покажу, чого цікавого знайшов HuntBugs IntelliJ IDEA Community Edition. Я скачав з офіційного сайту і поставив останню версію цієї IDE, а потім натравив HuntBugs на файл
lib/idea.jar
, в якому майже все і лежить. Я люблю тестувати статичний аналіз на IDEA, тому що це IDE, в якій є дуже непоганий статичний аналізатор і розробники їм явно користуються. Цікаво подивитися, що залишається після нього.
Формат цієї статті не особливо відрізняється від того, що робить PVS-Studio: помилки, шматки коду, пояснення. Зрозуміло, статтю ввійшло тільки найцікавіше.
Field is assigned to itself
Як правило помилок типу
this.field = this.field
вже ніхто не допускає, навіть не сама нова IDE зазвичай про такі попередить. Однак HuntBugs вміє дивитися трохи глибше. Ось фрагмент коду:
private int myLastOffsetInNewTree;
...

private int getNewOffset(ASTNode node){
int optimizedResult = haveNotCalculated;
...
if (myLastNode == prev) {
...
optimizedResult = myLastOffsetInNewTree;

myLastNode = node;
myLastOffsetInNewTree = optimizedResult;
...
}
}

Поле
myLastOffsetInNewTree
завантажено в локальну змінну
optimizedResult
, а потім чомусь знову збережено у полі, хоча за цей час воно змінитися не могло. Останнім приваивание дивне, або його треба прибрати, або малося на увазі щось інше.
An integer value is cast to float and passed to the rounding method
Іноді зустрічаються помилки з несвоєчасним перетворенням цілого типу до дробовому. Не завжди це можна відловити, але ось тут вийшло:
final int width = icon.getIconWidth();
final int height = icon.getIconHeight();
final int x = (int)Math.ceil((actionButton.getWidth() - width) / 2);
final int y = (int)Math.ceil((actionButton.getHeight() - height) / 2);

Тут двічі використовується округлення вгору (
Math.ceil
), але аргументом в обох випадках подається ціле число, так як в Java поділ цілого на ціле видає ціле (з округленням вниз). Ймовірно, передбачалося поділити на
2.0
або іншим чином перейти до дробовим числом перед поділом.Якщо ж поточний поведінка влаштовує, то
(int)Math.ceil
слід прибрати: ця частина коду марна.
Switch branch is unreachable due to expression range
Досить цікавий фрагмент коду, який, мабуть, хтось коли-то автоматично згенерував, а тепер вже ніхто не розуміє, чи це правильно і що там повинно бути:
int state = getState() & 0xF;

tokenType = fixWrongTokenTypes(tokenType, state);
if (...) {

// TODO: do not know when this happens!
switch (state) {
case __XmlLexer.DOCTYPE:
tokenType = XmlTokenType.XML_DECL_START;
break;
}
}

Константа
__XmlLexer.DOCTYPE
має значення 24, але вище виконується
state = getState() & 0xF
, тому значення
state
може бути тільки від 0 до 15 і гілка switch гарантовано не виконається. Можливо, коли в черговий раз міняли вихідний файл лексера, константи були перегенеріровани з іншими значеннями, а цей файл перегенерувати забули. Так чи інакше, код вельми підозрілий, про що свідчить і коментар.
Synchronization on getClass() rather than class literal
фрагмент класу MatcherImpl синхронізується на
getClass()
. Причому це робиться в публічному нефинальном класі, у якого реально є підклас Matcher. В результаті при виконанні цього коду підкласу синхронізація буде відбуватися не по
MatcherImpl.class
, а
Matcher.class
. Проблему поглиблює те, що в цьому ж класі є явна синхронізація
MatcherImpl.class
і обидві критичні секції (які можуть виявитися не взаємовиключними) оновлюють одне і те ж статичне поле
lastMatchData
. В результаті весь зміст синхронізації втрачається. Зазвичай
synchronized(getClass())
— це неправильно, використовуйте явний текст класу
synchronized(MatcherImpl.class)
.
Exception created and dropped rather than thrown
Досить часта в Java помилка: об'єкт виключення створений, але не впадає. Наприклад, здесь:
public void remove() {
new OperationNotSupportedException();
}

Про таких ситуаціях IDEA сама теж попереджає. Ще аналогічне місце.
Invariant loop condition
Ось ще автогенерированный файл. В принципі тут, ймовірно, все нормально, і можна нічого не правити, але в написаному вручну коді це б виглядало підозріло:
boolean r = ...;
while ® {
if (!value(b, l + 1)) break;
if (!empty_element_parsed_guard_(b, "json", c)) break;
c = current_position_(b);
}

Тут цикл з умовою по локальній змінній
r
, значення якої в циклі не змінюється, тому або в цикл взагалі не зайдемо, або ніколи не вийдемо за умовою (тільки по
break
). Якщо це дійсно малося на увазі, то в таких випадках краще писати
if® { while(true) { ... } }
, щоб підкреслити намір зробити нескінченний цикл.
The switch operator has identical branches
Повторювані гілки оператора
switch
іноді виглядають розумно, але коли в них великий шматок коду, як тут, варто придивитися:
switch ((((PsiWildcardType)x).isExtends() ? 0 : 1) + (((PsiWildcardType)y).isExtends() ? 0 : 2)) {
case 0: /* ? super T1, ? super T2 */
if (constraints != null && xType != null && yType != null) {
constraints.add(new Subtype(yType, xType));
}
return balance(xType, yType, balancer, constraints);

case 1: /* ? extends T1, ? super T2 */
if (constraints != null && xType != null && yType != null) {
constraints.add(new Subtype(xType, yType));
}
return balance(xType, yType, balancer, constraints);

case 2: /* ? super T1, ? extends T2*/
return null;

case 3: /* ? extends T1, ? extends T2*/
if (constraints != null && xType != null && yType != null) {
constraints.add(new Subtype(xType, yType));
}
return balance(xType, yType, balancer, constraints);
}

Не відразу помітно, але
case 1
та
case 3
абсолютно однакові (і відрізняються від case 0). Якщо це і малося на увазі, можливо, доцільніше об'єднати
case 1
та
case 3
, щоб було простіше читати і підтримувати код.
The same condition is repeatedly checked
цьому коді навіщо-то одне і те ж умова перевіряється два рази:
if (offsetToScroll < 0) {
if (offsetToScroll < 0) {
...
}
}

Може, просто внутрішню перевірку треба прибрати, а може і щось інше хотіли перевірити. Ось ще аналогічний випадок. Або ось ще цікавий випадок:
return o instanceof PsiElement && ((PsiElement)o).isValid() && ((PsiElement)o).isPhysical() ||
o instanceof ProjectRootModificationTracker ||
o instanceof PsiModificationTracker ||
o == PsiModificationTracker.MODIFICATION_COUNT ||
o == PsiModificationTracker.OUT_OF_CODE_BLOCK_MODIFICATION_COUNT|| // <<<
o == PsiModificationTracker.OUT_OF_CODE_BLOCK_MODIFICATION_COUNT|| // <<<
o == PsiModificationTracker.JAVA_STRUCTURE_MODIFICATION_COUNT;

ось тут повторні умови не зовсім поруч і їх помітити ще складніше:
return SUPPORTED_TYPES.contains(token) || StdArrangementTokens.Regexp.NAME.equals(token) 
|| StdArrangementTokens.Regexp.XML_NAMESPACE.equals(token) || KEEP.equals(token)
|| BY_NAME.equals(token) || SUPPORTED_TYPES.contains(token);

Умова
SUPPORTED_TYPES.contains(token)
перевіряється двічі. Зрозуміло, HuntBugs уважно стежить, щоб між цими умовами нічого не змінилося. Якщо б у проміжних умовах
token
переприсваивался, така конструкція мала б право на існування.
Numeric comparison is always true or false
Ось тут, радше просто надмірна перевірка, ніж реальна помилка:
int size = myPanels.length;
final Dimension preferredSize = super.getPreferredSize();
if (size >= 0 && size <= 20) {
return preferredSize;
}

В змінну
size
записана довжина масиву, яка ніколи не може бути від'ємною. Неясно, навіщо перевіряти
size >= 0
. Навіть якщо помилки немає, я вважаю, що такі перевірки треба видаляти, тому що вони бентежать читача. Невідомо, чи може автор мав на увазі
size > 0
, тоді це помилка.
The chain of private methods is never called
Зазвичай IDE без праці знаходять приватні методи, які ніколи не викликаються, і пропонують їх видалити. Але ось такий випадок визначається не завжди:
@Nullable
private static JsonSchemaObject getChild(JsonSchemaObject current, String name) {
JsonSchemaObject schema = current.getProperties().get(name);
if (schema != null) return schema;

schema = getChildFromList(name, current.getAnyOf()); // <<<
if (schema != null) return schema;
...
}

@Nullable
private static JsonSchemaObject getChildFromList(String name, List<JsonSchemaObject> of) {
if (of == null) return null;
JsonSchemaObject schema;
for (JsonSchemaObject object : of) {
schema = getChild(object, name); // <<<
if (schema != null) return schema;
}
return null;
}

Ці два приватних методу викликають один одного рекурсивно, але от зовні їх ніхто не викликає. HuntBugs бачить цю ситуацію і каже, що обидва методи насправді не використовуються.
Useless String.substring(0)
Чесно кажучи, не очікував побачити таку діагностику у production-коді, занадто вже вона тривіальна. Але ні, бувають і тривіальні помилки:
String str = (String)value;
if (str.startsWith("\"")) {
str = str.substring(0);
str = StringUtil.trimEnd(str, "\"");
}

Мабуть, автор мав на увазі видалити перший символ рядка, але з якоїсь причини не написав
substring(1)
, а
substring(0)
(цей виклик просто повертає вихідну рядок). Це другий випадок (крім dropped exception), коли сама IDEA теж підсвічує проблемне місце.
Result of integer multiplication promoted to long
Це попередження не завжди веде до реальної небезпеки, але тим не менш хочеться показати приклад:
final long length = myIndexStream.length();
long totalCount = length / INDEX_ENTRY_SIZE; // INDEX_ENTRY_SIZE = 26
for(int i=0; i<totalCount; i++) {
final long indexOffset = length - (i + 1) * INDEX_ENTRY_SIZE;

По-перше, вже підозріло, що змінна циклу має тип
int
, а не
long
(можливо, на таку ситуацію варто зробити окрему діагностику). Якщо totalCount перевищує 231, то цикл ніколи не завершиться. Але гаразд, це можливо тільки при довжині індексу length більше 52 гігабайт, що все-таки чимало. Однак проблеми в цьому коді почнуться вже при довжині більше 2 гігабайт. Так як
i
та
INDEX_ENTRY_SIZE
мають тип
int
, то множення буде виконуватися над 32-бітними знаковими цілими і успішно переповниться. Вже після цього результат множення буде приведений до
long
і після виконання віднімання зсув може цілком виявитися більше довжини. Ймовірно, таких великих кешей ніколи тут не було, але неприємно, коли вони з'являться. Виправити просто — оголосити змінну циклу
long
.
А що з Котлином?
Відомо, що частина IntelliJ IDEA написана на Котлине, який також компілюється в Java-байткод. Статичні аналізатори байткода формально можуть аналізувати будь-яку мову, але фактично якщо аналізатор заточений на Java, то для інших мов буде багато помилкових спрацьовувань. Часто вони беруться тому, що компілятор генерує якісь специфічні конструкції (наприклад, неявні перевірки). Іноді, втім, що таке помилкове спрацьовування — привід придивитися до кодогенератору компілятора. Ось, наприклад, клас
com.intellij.configurationStore.FileBasedStorageKt
. У самому класі є така рядок:
private val XML_PROLOG = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>".toByteArray()

В класі
java.lang.String
методи
toByteArray()
, як відомо, немає. Це extension-method Котлина, причому inline-метод (який компілятор вбудовується прямо в місце використання), за замовчуванням він виконує
String.getBytes(Charsets.UTF_8)
. Давайте подивимося, у що ця рядок скомпилировалась в Котлине. Я не буду показувати прямо байткод, а перетворю його в більш зрозумілий код на Java:
String str = "<?xml version=\"1.0\" encoding=\"UTF-8\"?>";
Charset charset = null;
int mask = 1;
Object obj = null; // навіщо потрібна ця змінна і якого вона типу — я не знаю
// тут навіщо-то nop - порожня операція
if(obj != null) {
throw new UnsupportedOperationException("Super calls with default arguments not supported in this target, function: toByteArray");
}
if(mask & 1 == 0) {
charset = kotlin.text.Charsets.UTF_8;
}
// тут знову навіщось nop
XML_PROLOG = kotlin.jvm.internal.Intrinsics.checkExpressionValueIsNotNull(((String)str).getBytes(charset), 
"(as this java.lang.String).getBytes(charset)");

Видно, що рядок неймовірно розрослася. Змінна
mask
пов'язана з передачею дефолтного параметра (про це розповідав Дмитро Жемеров на JPoint — дивіться слайд 40 і нижче. Тут, очевидно, багато зайвого, і HuntBugs справедливо лається і на
obj != null
(порівняння
null
,
null
), і на
mask & 1
. Хоча автор коду зовсім не винен. Треба вважати, з часом компілятор Котлина буде розумнішим і буде генерувати менше сміття.
Висновок
Тут можна написати звичайний текст про важливість статичного аналізу, який пише Andrey2008 з колегами після своїх статей, але ви і так все знаєте. Цікаво, що навіть в коді, який розробляється з використанням статичного аналізу, вдалося знайти чимало підозрілих місць, просто перевіривши його новим інструментом. Зрозуміло, статтю потрапило не всі. Крім помилкових спрацювань є чимало важливих повідомлень, але нудних. Багато повідомлень про продуктивність. Наприклад, конкатенація рядків у циклі — 59 штук. Або обхід значень Map через
keySet()
+
get()
, коли швидше через
values()
— 18 штук. Велика кількість потенційних проблем з багатопоточністю. Скажімо, неатомарные оновлення volatile-полів — 50 штук. Або підозрілі сценарії
wait()
/
notify()
— 8 штук.
Користуйтеся статичним аналізом і слідкуйте за новинами!
Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.