Сделать статический анализ умным — полдела, потом его надо делать глупым

Одна из проблем статического анализа в том, что его легко сделать умнее, чем надо. В результате он начинает выдавать предупреждения в таком коде, который человеку кажется нормальным. И так и хочется сказать анализатору «Отстань, зануда! Сильно умный что ли?»

Я в очередной раз почувствовал вкус всего этого, когда работал над поиском константных выражений для Kotlin. Ранее такой анализ был для Java, но для Kotlin он впервые появится только в следующей версии IntelliJ IDEA 2021.3. Инспекция базируется на анализе потока данных и находит в коде выражения, которые всегда равны одному и тому же. Изначально такая инспекция на Java сообщала только о логических выражениях, которые всегда равны true или false. Потом мы осторожно расширили её, и она стала сообщать ещё и о выражениях, которые всегда равны null или 0. Было решено проделать тот же путь для Kotlin.

Нам успешно удалось переиспользовать ядро Java-анализатора, так что во многом анализ на Kotlin получился таким же умным как на Java. Некоторые вещи ещё недоделаны, но это вопрос времени. Однако после того как анализатор начинает работать, выясняется, что он выдаёт много мусора.

Вот начнём с простого

val x = true // 'true' is always true

Анализатор такая умница, говорит, что выражение true всегда истинно. А то мы не догадались! Ладно, запрещаем предупреждение на литералах. Дальше видим такое:

val ignoreCase = true
if (command.startsWith("--data", ignoreCase)) { // 'ignoreCase' is always true
    //...
}

Тоже супергениально. Ну захотелось нам вынести опцию в константу. Чтобы код был понятнее или мы хотим, чтобы можно было легко изменить. Мы и так знаем, что она истинна. Замолчи, анализатор.

if (x <= 0) {
    // ...
    return
}
assert(x > 0) // 'x > 0' is always true
// ...

Ай-яй-яй, говорит, анализатор, вы написали ассерт, который точно никогда не свалится. Но постойте! Ассерты ведь для того и нужны, чтобы никогда не свалиться. Если ассерт свалился, то это ошибка в программе. Как раз от ошибки и страхует ассерт. Мало ли, может я потом буду что-то менять и случайно удалю return в вышестоящем условии. Тогда ассерт быстро скажет мне, что я неправ. Окей, объяснили анализатору, что если условие в ассерте и вычисляется в true, то ругаться не надо (а вот если в false, то ещё как надо). Кстати, анализатор может вывести истинность только части выражения. Например, ассерт мог бы быть таким assert(x > 0 && somethingElse). В этом случае тоже ругаться не надо.

Продолжаем наше шоу. Посмотрим на оператор when:

when {
    a && b && c -> {}
    a && b && !c -> {} // '!c' is always true
    a && !b && c -> {} // '!b' is always true
    a && !b && !c -> {} // '!b' is always true, '!c' is always true
    // ...
}

Тут целый ворох предупреждений. Действительно, when исполняется сверху вниз и выбирается ровно одна ветка. Если мы не попали в первую, но a и b у нас истинны как и в первой, то c уже точно ложно. А если не попали ни в первую, ни во вторую, то после проверки a мы уже наверняка знаем, что b ложно. Этот же код можно переписать так:

when {
    a && b && c -> {}
    a && b -> {}
    a && c -> {}
    a -> {}
    // ...
}

Код стал короче. Но стал ли он понятнее или красивее? Вот это совсем неочевидно. В исходной версии как-то было легче разобраться, можно было читать каждую ветку отдельно от остальных. А главное, ветки были взаимно независимые, их можно было переставлять без потери семантики. В исправленной версии это не так. В общем, не очень-то полезную вещь нам анализатор насоветовал. Пришлось подавлять. Хотя, кажется, иногда предупреждения в таких цепочках ещё проскакивают.

Вот такой шаблон ещё обнаружился:

fun updatePriority(priority: String) {
    if (priority != "high" && priority != "medium" && priority != "low") return
    when (priority) {
        "high" -> setPriority(1)
        "medium" -> setPriority(2)
        "low" -> setPriority(3) // when condition is always true
        else -> throw AssertionError("Unreachable")
    }
}

Анализатор достаточно умён, чтобы отследить, что мы уже отсекли любую строку кроме трёх перечисленных, поэтому если мы не прошли в ветку "high" или "medium", то мы уж точно пройдём в ветку "low". Опять же можно сократить код до такого:

if (priority != "high" && priority != "medium" && priority != "low") return
when (priority) {
    "high" -> setPriority(1)
    "medium" -> setPriority(2)
    else -> setPriority(3)
}

Опять же стало короче, но стало ли понятнее? Кажется, не всем такое понравится. Поэтому про всегда истинную ветку в конце when или перед else мы решили тоже не предупреждать.

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

fun process(): Boolean {
    doSomething()
    return true
}

Им обязательно выпендриваться:

fun process() = doSomething().let { true }

Здесь результат выражения doSomething().let { true } всегда истинный, о чём анализатор радостно сообщает. Извини, анализатор, но люди странные существа. Тебе придётся с этим мириться. Аналогичный вариант с also тоже пришлось подавлять:

fun process() = true.also { doSomething() }

Обнаружилась ещё одна довольно удивительная вещь:

fun isOption(s: String?) : Boolean {
    return s?.let { return it.startsWith("--") } ?: false
}

Анализатор говорит, что всё выражение s?.let { return it.startsWith("--") } ?: false всегда ложно. Смысл в том, что если s == null, тело let не выполняется и благодаря elvis-оператору, оно вычисляется в false. Если же s != null, тело let выполняется, но мы выходим из выражения досрочно, так как return внутри let покидает всю функцию, а не только тело let. В итоге в этом случае выражение вообще не имеет значения. То есть если оно успешно вычислилось, то оно точно вычислилось в false.

Понятно, что это странный код и внутренний return хорошо бы убрать или в крайнем случае заменить на return@let (мы видели подобные конструкции со сложным телом let, в котором много раз сделано return). Тем не менее код работает так как задумано. Во всяком случае, если и предупреждать, то это должна быть другая инспекция, а не "condition is always false", что может быть совершенно непонятно пользователю.

Порой анализатор считает себя не только умнее пользователя, но и умнее компилятора Kotlin. Вот несколько искусственный пример, но подобное мы встречали нередко и в реальном коде:

fun processString(str: String?) {
    val empty = str?.isEmpty() ?: true
    if (!empty && str != null) { // 'str != null' is always true
        println("String: " + str.trim())
    }
}

Анализатор, конечно, соображает, что под проверкой !empty мы точно знаем, что str != null истинно. Однако этого не знает компилятор. А компилятору эта проверка нужна для смарт-каста. Если её убрать, код перестанет компилироваться. Подружить компилятор с анализатором оказалось весьма нетривиальной задачей.

Ладно, с true и false примерно разобрались. Что там с null и нулём?
Литерал null мы уже подавили, но оказалось, что люди иногда приводят null к другому типу, чтобы разрешить неоднозначность перегруженных функций:

fun process(x: Int?) {}
fun process(x: String?) {}

fun use() {
    process(null as Int?)
}

Анализатор, конечно, неглуп и радостно говорит нам, что выражение null as Int? всегда равно null. Снова пришлось объяснять ему, что тут лучше помолчать.

Иногда приходится затыкать анализатор, чтобы он не ругался несколько раз на одно и то же:

fun processString(left: String?, right: String?) {
    val empty = left == null && right == null
    if (empty) {
        //...
        if (left == null) { // 'left' is always null, 'left == null' is always true
            //...
        }
    }
}

Да, да, дорогой, ты уже сказал мне, что left == null всегда истинно в этом месте. Молодец, можно не выдавать второе предупреждение, что left всегда равно null, я и так уже понял.

Что касается целочисленного 0, мы обнаружили такой паттерн для формирования битовой маски:

var mask = 0
if (shouldUpdate()) mask = mask or SHOULD_UPDATE // 'mask' is always zero
if (shouldVerify()) mask = mask or SHOULD_VERIFY
if (shouldNotify()) mask = mask or SHOULD_NOTIFY
return mask

Ну да, в первом условии mask точно 0, можно было просто написать mask = SHOULD_UPDATE вместо mask = mask or SHOULD_UPDATE. Но ведь это будет некрасиво, потеряется аккуратная структура программы. Да и если захочется переупорядочить строки или добавить новую в самое начало, можно будет сломать программу. Анализатор хоть и умный, но ничего не понимает в программировании. Ладно, научили его не ругаться в этом случае.

Ещё мы поддержали вызов ordinal() для enum-типов и немного огребли от чрезмерно умного анализатора. Скажем, в таком коде предупреждение оказалось совершенно неуместно:

enum class Vitamin {
    A, B, C;
}

fun fromOrdinal(ord: Int) : Vitamin {
    when (ord) {
        Vitamin.A.ordinal -> Vitamin.A // 'Vitamin.A.ordinal' is always zero
        Vitamin.B.ordinal -> Vitamin.B
        Vitamin.C.ordinal -> Vitamin.C
    }
}

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

Описанная инспекция доступна в ранних сборках IntelliJ IDEA 2021.3 (только для Kotlin/JVM). Вы можете проверить свой код и поделиться результатами в комментариях. Удалось ли найти настоящие баги? Или может одни только ложные предупреждения? Расскажите, очень интересно!

Adblock test (Why?)