Один из самых больших проектов на Java в Open Source. Elasticsearch используется во многих крупных организациях, таких как GitHub, Альфа-Банк, Тинькофф, Netflix и Amazon. Шесть лет назад мы уже проверяли проект, но интересно, какие новые ошибки появились за столь долгое время?

О чём будем говорить?

Как указано в аннотации, проект мы проверяли уже целых шесть (!) лет назад. Ознакомиться с прошлой статьёй вы можете по этой ссылке.

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

Для начала вспомним, что же такое Elasticsearch. А это один из наиболее успешных поисковых движков, который могут использовать как крупные организации, так и стартапы.

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

Стоит также отметить и огромную кодовую базу проекта (три миллиона строк Java кода), которая и позволяет ему работать. Перейдём к её проверке.

Дисклеймер: форматирование кода (пробелы, переносы строк) может немного отличаться от оригинала. Можете не переживать из-за этого, смысл написанного никак не искажён.

Очепятки

Начнём с одного из самых страшных врагов разработчика — опечаток.

Поле за поле

Часто ли вы пишете геттеры? Надеемся, что не очень, поскольку это не самое интересное занятие. От бойлерплейт-кода спасают генераторы кода или такие удобные инструменты, как Lombok.

private final boolean isRunning;
private final boolean isAsync;
....
public boolean isRunning() {  
    return isRunning;  
}

public boolean isAsync() {  
    return isRunning;  
}

Посмотрим на переменные isRunning и isAsync, а точнее на их геттеры. #isRunning() возвращает значение isRunning, а #isAsync() возвращает... тоже значение isRunning. Очевидно, что второй геттер должен был возвращать isAsync, но автор ошибся и получилась простая, но довольно опасная опечатка.

Предупреждение анализатора PVS-Studio: V6091 Suspicious getter implementation. The 'isAsync' field should probably be returned instead. EsqlQueryResponse.java 180

Мы были бы рады сказать, что более одного раза такая ошибка не встретилась, но нашёлся ещё один геттер, который делает не то, что от него ожидают:

boolean isDirectory;
boolean isSymbolicLink;
....
@Override
public boolean isDirectory() {  
    return isDirectory;  
}

@Override
public boolean isSymbolicLink() {  
    return isDirectory;  
}

Здесь автор кода уже перепутал переменную isDirectory с isSymbolicLink.

PVS-Studio выдал аналогичное предупреждение: V6091 Suspicious getter implementation. The 'isSymbolicLink' field should probably be returned instead. DockerFileAttributes.java 67

"equals" == "=="

У новичков в Java появляется вполне логичный вопрос: "В чём разница между equals и ==?" Рассмотрим на конкретном примере:

private void assertValidJsonInput(String content) {
    if (testResponse && ("js" == language || "console-result" == language)
            && null == skip) {
        .....
    }
}

На код выше анализатор PVS-Studio выдаёт следующие предупреждения:

V6013 Strings '"js"' and 'language' are compared by reference. Possibly an equality comparison was intended. SnippetBuilder.java 232

V6013 Strings '"console-result"' and 'language' are compared by reference. Possibly an equality comparison was intended. SnippetBuilder.java 232

Можно сказать: "Но в чём проблема? Я писал такой код, и он работал правильно". От проблем вас спас String Pool, который минимизирует количество экземпляров строк, чтобы не выделять место в памяти каждый раз. Однако вам никак не гарантируется, что случайно взятая строка будет находиться в пуле. Вполне возможна ситуация, что две одинаковые по содержанию строки будут ссылаться на разные экземпляры, и сравнение по ссылке (==) вернёт false. Поэтому рекомендуется использовать equals для сравнения строк.

Если со строками сравнение по ссылке может и сработать, то с коллекциями оно никогда не даст верного результата (если не считать сравнение одного и того же экземпляра с самим собой).

private final List values;
....
@Override
public final Pipe resolveAttributes(AttributeResolver resolver) {
    List newValues = new ArrayList<>(values.size());
    for (Pipe v : values) {
        newValues.add(v.resolveAttributes(resolver));
    }
    if (newValues == values) {  // <=
        return this;
    }
    return replaceChildrenSameSize(newValues);
}

Здесь новорождённый объект newValues сравнивается с полем values, что, конечно, всегда будет false. Скорее всего, автор этого кода хотел проверить, не содержит ли новый список точно такие же элементы, как и существующий, чтобы не перезаписывать его лишний раз.

Анализатор указал на эту ошибку в предупреждении: V6013 Objects 'newValues' and 'values' are compared by reference. Possibly an equality comparison was intended. ConcatFunctionPipe.java 41

Лев всегда прав

Code Completion сильно закрепился в процессе разработки. Вместе с той пользой, что он приносит, он также везёт за собой маленькую тележку, в которой лежат новые опечатки. Взглянем на одну такую:

public void testReadSlices() throws IOException {
    ....
    try (StreamInput input1 = bytesReference.streamInput();
         StreamInput input2 = bytesReference.streamInput()) {
        for (int i = 0; i < refs; i++) {
            boolean sliceLeft = randomBoolean();
            BytesReference left = sliceLeft ?
                                        input1.readSlicedBytesReference()
                                        : input1.readBytesReference();
            if (sliceLeft && bytesReference.hasArray()) {
                assertSame(left.array(), bytesReference.array());  // <=
            }
            boolean sliceRight = randomBoolean();
            BytesReference right = sliceRight ?
                                    input2.readSlicedBytesReference()
                                    : input2.readBytesReference();
            assertEquals(left, right);
            if (sliceRight && bytesReference.hasArray()) {
                assertSame(right.array(), right.array());  // <=
            }
        }
    }
}

Сравнив первую отмеченную строку кода со второй, можно предположить, что во второй предусматривалось следующее:

assertSame(right.array(), bytesReference.array());

Однако сейчас код сравнивает результат функции array() у одного и того же объекта right.

Поблагодарим PVS-Studio за предупреждение: V6009 Function 'assertSame' receives an odd argument. The 'right.array()' argument was passed several times. AbstractBytesReferenceTestCase.java 713

Предположим, это будет equals

Мы уже поговорили о различии между equals и ==. Теперь рассмотрим equals более подробно, а точнее окунёмся в возможные ошибки при его реализации:

@Override
public boolean equals(Object obj) {
    ....
    KeyedFilter other = (KeyedFilter) obj;
    return Objects.equals(keys, other.keys)
        && Objects.equals(timestamp, other.timestamp)
        && Objects.equals(tiebreaker, other.tiebreaker)
        && Objects.equals(child(), other.child())
        && isMissingEventFilter == isMissingEventFilter; // <=
}

Здесь забыли поставить other перед вторым isMissingEventFilter. Из-за такой небольшой опечатки при сравнении будет полностью игнорироваться параметр isMissingEventFilter.

Посмотрим на предупреждение PVS-Studio: V6001 There are identical sub-expressions 'isMissingEventFilter' to the left and to the right of the '==' operator. KeyedFilter.java 116

Рассмотрим ещё одну опечатку в реализации equals:

@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (o == null || getClass() != o.getClass()) return false;
    IndexError that = (IndexError) o;
    return indexName.equals(that.indexName)
        && Arrays.equals(shardIds, that.shardIds)
        && errorType == that.errorType
        && message.equals(that.message)
        && stallTimeSeconds == stallTimeSeconds;    // <=
}

Аналогично предыдущему equals, здесь забыли приписать ко второму stallTimeSeconds префикс that.

Примечательно, что в обоих случаях опечатка произошла в последней строчке. Это реальный феномен в разработке. Если вам интересно разобраться в нём подробнее, можете ознакомиться с этой публикацией.

Анализатор PVS-Studio выдал следующее предупреждение на этот код: V6001 There are identical sub-expressions 'stallTimeSeconds' to the left and to the right of the '==' operator. IndexError.java 147

Любите математику? А в коде?

Независимо от того, как вы ответили на эти вопросы, согласитесь с тем, что в таком коде легко опечататься. Посмотрим на один из таких случаев:

private static LinearRing parseLinearRing(
            ByteBuffer byteBuffer,
            boolean hasZ, boolean coerce
            ) {
    ....
    double[] lons = new double[length];
    double[] lats = new double[length];
    ....
    if (linearRingNeedsCoerced(lats, lons, alts, coerce)) { // <=
        lons = coerce(lons);
        lats = coerce(lats);
        if (hasZ) {
            alts = coerce(alts);
        }
    }
    ....
}

Но что не так с этим кодом? Чтобы разобраться, взглянем на аргументы метода linearRingNeedsCoerced.

private static boolean linearRingNeedsCoerced(
                    double[] lons, double[] lats,
                    double[] alts,
                    boolean coerce
                    ) {
    assert lats.length == lons.length
                && (alts == null || alts.length == lats.length);
    assert lats.length > 0;
    if (coerce == false) {
        return false;
    }
    final int last = lons.length - 1;
    return lons[0] != lons[last] 
                || lats[0] != lats[last]
                || (alts != null && alts[0] != alts[last]);
}

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

Предупреждение PVS-Studio: V6029 Possible incorrect order of arguments passed to method: 'lats', 'lons'. WellKnownBinary.java 370

null null'ом а что там дальше?

Разберём небольшой фрагмент кода:

public static void assertDeepEquals(ElasticsearchException expected,
                                    ElasticsearchException actual) {
    do {
        if (expected == null) {
            assertNull(actual);
        } else {
            assertNotNull(actual);
        }
        assertEquals(expected.getMessage(), actual.getMessage());
        ....
    }
    ....
}

При expected == null проверяется, является ли actual тоже null. На этом проверка на сходство должна была закончиться, но нет. Дальше проверяются свойства объекта, и в случае, если actual оказался null, возможно разыменование нулевого указателя.

Посмотрим, что на это говорит анализатор: V6008 Potential null dereference of 'expected'. ElasticsearchExceptionTests.java 1354

Проверим? Нет

Во время разработки многое приходится держать в голове и легко забыть одну маленькую тонкость, как в коде ниже:

private FinishFunction(....) {
    ....
    for (int p = 1; p < fn.getParameters().size(); p++) {
        VariableElement param = fn.getParameters().get(p);
        if (p == 0) {     // <=
            if (false == TypeName.get(param.asType()).equals(workType)) {
                throw new IllegalArgumentException(
                    "First argument of "
                        + declarationType + "#" + fn.getSimpleName()
                        + " must have type " + workType
                );
            }
            continue;
        }
        ....
    }
    invocationPattern = pattern.append(")").toString();
}

Здесь for начинается с единицы, но в самом теле есть проверка типа первого элемента. Скорее всего, автор забыл поменять границы цикла и добавил проверку первого элемента, которая никогда не сработает.

Анализатор PVS-Studio не забывает о таких тонкостях и даёт предупреждение, если разработчик забыл: V6007 Expression 'p == 0' is always false. MvEvaluatorImplementer.java 456

Корабли коммитили, коммитили, да перекоммитили

Без лишних слов посмотрим на код ниже:

public void testDLS() throws Exception {
    ....

    int numDocs = scaledRandomIntBetween(32, 128);
    int commitAfter = scaledRandomIntBetween(1, numDocs);
    
    ....

    for (int doc = 1; doc <= numDocs; doc++) {
        ....
        if (doc % 11 == 0) {
            iw.deleteDocuments(new Term("id", id));
        } else {
            if (commitAfter % commitAfter == 0) {   // <=
                iw.commit(); 
            }
            valuesHitCount[valueIndex]++;
        }
    }
    ....
}

В этом фрагменте можно заметить, что берётся остаток от деления commitAfter на самого себя (что всегда будет равняться нулю) и сравнивается с 0. Это условие всегда верно, из-за чего iw.commit() исполняется после каждого создания документа.

PVS-Studio помечает подозрительную операцию со следующим предупреждением: V6001 There are identical sub-expressions 'commitAfter' to the left and to the right of the '%' operator. SecurityIndexReaderWrapperIntegrationTests.java 157

Что происходит в этом коде? Переменная commitAfter генерируется случайно в диапазоне от 1 до numDocs. После перебираются все числа от 1 до numDocs, и если остаток от деления commitAfter равен нулю, то коммитятся созданные документы. Остаток от деления тут отвечает за то, чтобы коммитились каждые commitAfter элементов. Исправленное условие выглядит следующим образом:

doc % commitAfter == 0

Расставаться не хочется

Посмотрим сразу на два фрагмента кода:

@Override
public void setAutoCommit(boolean autoCommit) throws SQLException {
    checkOpen();
    if (autoCommit == false) {
        new SQLFeatureNotSupportedException("Non auto-commit is not supported");
    }
}

private String getProperty(String propertyName) {  
    final String[] settings = getConnectString().split(";");  
    for (int i = 0; i < settings.length; i++) {  
        String setting = settings[i].trim();  
        if (setting.length() > 0) {  
            final int idx = setting.indexOf('=');  
            if (idx == -1 || idx == 0 || idx == settings[i].length() - 1) {  
                new IllegalArgumentException("Invalid connection string: " // <=
                                                + getConnectString());
            }  
            if (propertyName.equals(setting.substring(0, idx))) {  
                return setting.substring(idx + 1);  
            }  
        }  
    }  
    return null;  
}

Вы наверняка уже заметили, что объединяет оба фрагмента: создаётся исключение, но не выбрасывается, и ему остаётся только лежать на месте так и неиспользованным, пока JIT-компилятор не поймёт, что эта строчка является мёртвым кодом.

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

PVS-Studio дал два предупреждения соответственно:

V6006 The object was created but it is not being used. The 'throw' keyword could be missing. JdbcConnection.java 93

V6006 The object was created but it is not being used. The 'throw' keyword could be missing. AzureStorageSettings.java 352

Стать копией себя

Рассмотрим ошибку, возникшую в результате перекрытия переменной:

private final PatternSet patternSet = new PatternSet().include("**/*.class");

....

public void setPatternSet(PatternSet patternSet) {
    patternSet.copyFrom(patternSet);
}

По задумке автора, приведённый выше сеттер должен копировать состояние переданного patternSet в поле класса, но из-за забытого ключевого слова this объект копирует собственное состояние.

Посмотрим на предупреждение анализатора, который счёл этот вызов метода подозрительным: V6100 An object is used as an argument to its own method. Consider checking the first actual argument of the 'copyFrom' method. CheckForbiddenApisTask.java 157

Ctrl+C, Ctrl+V, Ctrl+V, Ctrl+V, Ctr... Ой

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

private void test(Snippet test) {
    setupCurrent(test);
    if (test.continued()) {
        /* Catch some difficult to debug errors with // TEST[continued]
         * and throw a helpful error message. */
        if (previousTest == null
                || previousTest.path().equals(test.path()) == false) {
            throw new InvalidUserDataException("// TEST[continued] "
                + "cannot be on first snippet in a file: " + test);
        }
        if (previousTest != null && previousTest.testSetup()) {
            throw new InvalidUserDataException("// TEST[continued] "
                + "cannot immediately follow // TESTSETUP: " + test);
        }
        if (previousTest != null && previousTest.testSetup()) {
            throw new InvalidUserDataException("// TEST[continued] "
                + "cannot immediately follow // TEARDOWN: " + test);
        }
    }
    ....
}

Если у вас получилось найти опечатку — отлично, если нет, то объясняем.

Взглянем на условия в двух последних блоках if. Они идентичны. Но, может, это сделано специально? Тогда взглянем на блоки then, а точнее на сообщения исключений, которые там выкидываются. Они уже разнятся. Скорее всего, автор этого кода хотел проверить два разных условия, но вследствие копипасты получилось, что получилось.

Можем предположить, что второе условие должно было выглядеть следующим образом (не забудем, что можно опустить проверку на null):

if (previousTest.testTearDown()) {
    ....
}

Также посмотрим на предупреждения анализатора PVS-Studio, которые подсказали нам проблемное место:

V6007 Expression 'previousTest != null' is always true. RestTestsFromDocSnippetTask.java 247

V6007 Expression 'previousTest != null' is always true. RestTestsFromDocSnippetTask.java 249

Проверка, проверка и ещё проверка

Проверять входные данные стоит, особенно если они пришли от пользователя. В этом разделе посмотрим на проверки, которые зашли слишком далеко.

Токен или токен?

Начнём со странной проверки текущего токена.

protected BulkByScrollTask.Status doParseInstance(
                        XContentParser parser
                        ) throws IOException {
    XContentParser.Token token;
    if (parser.currentToken() == XContentParser.Token.START_OBJECT) {
        token = parser.nextToken(); // <=
    } else {
        token = parser.nextToken(); // <=
    }
    ensureExpectedToken(XContentParser.Token.START_OBJECT, token, parser);
    token = parser.nextToken();
    ensureExpectedToken(XContentParser.Token.FIELD_NAME, token, parser);
    return innerParseStatus(parser);
}

Разберёмся в вышеприведённом методе. Приходит объект parser, из него мы проверяем текущий токен и, если он является началом объекта, читаем следующий, а если нет, делаем то же самое! И, самое интересное, после этого сразу проверяем, что и этот токен является началом объекта!

Может быть, такое поведение сложилось из-за того, что несколько человек вносили правки? Нет, этот метод появился в одном коммите и больше не менялся.

Предупреждение PVS-Studio: V6004 The 'then' statement is equivalent to the 'else' statement. BulkByScrollTaskStatusTests.java 189

Сейчас строк нет. И сейчас нет

Взглянем на ещё одну лишнюю проверку:

@Override
protected StringBuilder contentToWKT() {
    final StringBuilder sb = new StringBuilder();
    if (lines.isEmpty()) {          // <=
        sb.append(GeoWKTParser.EMPTY);
    } else {
        sb.append(GeoWKTParser.LPAREN);
        if (lines.size() > 0) {     // <=
            sb.append(ShapeBuilder
                .coordinateListToWKT(lines.get(0).coordinates)
            );
        }
        for (int i = 1; i < lines.size(); ++i) {
            sb.append(GeoWKTParser.COMMA);
            sb.append(ShapeBuilder
                .coordinateListToWKT(lines.get(i).coordinates)
            );
        }
        sb.append(GeoWKTParser.RPAREN);
    }
    return sb;
}

В коде присутствует явно лишняя проверка lines.size() > 0. Поскольку мы уже проверили lines на пустоту, эта ветвь исполнения будет выполнена только при наличии хотя бы одного элемента.

Посмотрим на предупреждение PVS-Studio: V6007 Expression 'lines.size() > 0' is always true. MultiLineStringBuilder.java 81

Скобка? Точно?

Теперь немного пороемся в коде. Для начала взглянем на следующий метод:

private static List parseCoordinateList(
                        StreamTokenizer stream,
                        final boolean ignoreZValue,
                        final boolean coerce
                    ) throws IOException, ElasticsearchParseException {
    ....
    while (nextCloserOrComma(stream).equals(COMMA)) {
        ....
        if (isOpenParen && nextCloser(stream).equals(RPAREN) == false) {  // <=
            throw new ElasticsearchParseException(
                                "expected: " + RPAREN
                                    + " but found: " + tokenString(stream),
                                stream.lineno()
                            );
        }
    }
    return coordinates.build();
}

Если вы не помните весь код Elasticsearch наизусть, то вы, скорее всего, не поймёте, в чём здесь ошибка.

Взглянем на предупреждение анализатора: V6007 Expression 'nextCloser(stream).equals(RPAREN) == false' is always false. GeoWKTParser.java 164

Появилось представление об ошибке, но почему результат функции nextCloser всегда будет эквивалентен RPAREN? Посмотрим на тело метода:

private static String nextCloser(StreamTokenizer stream) 
                    throws IOException, ElasticsearchParseException {
    if (nextWord(stream).equals(RPAREN)) {
        return RPAREN;
    }
    throw new ElasticsearchParseException(
                                "expected: " + RPAREN
                                    + " but found: " + tokenString(stream),
                                stream.lineno()
                            );

}

Выяснилось, что nextCloser либо возвращает RPAREN, либо кидает исключение! Подметим также, что оно идентично тому, которое находится в методе с предупреждением.

Код можно упростить до следующего:

private static List parseCoordinateList(
                        StreamTokenizer stream,
                        final boolean ignoreZValue,
                        final boolean coerce
                    ) throws IOException, ElasticsearchParseException {
    ....
    while (nextCloserOrComma(stream).equals(COMMA)) {
        ....
        if (isOpenParen) {
            nextCloser(stream);
        }
    }
    return coordinates.build();
}

Пустота x2

Посмотрим на простой метод объединения списков:

public static  List combine(
                    List left,
                    List right
                    ) {
    if (right.isEmpty()) {
        return (List) left;
    }
    if (left.isEmpty()) {
        return (List) right;
    }
    List list = new ArrayList<>(left.size() + right.size());
    if (left.isEmpty() == false) {  // <=
        list.addAll(left);
    }
    if (right.isEmpty() == false) {  // <=
        list.addAll(right);
    }
    return list;
}

Заранее проверив списки на пустоту, незачем делать это ещё раз при заполнении объединения. Стоит отметить, что это нельзя назвать ошибкой, но написать более простой и компактный код здесь можно.

PVS-Studio заметил лишние проверки и выдал два предупреждения:

V6007 Expression 'left.isEmpty() == false' is always true. CollectionUtils.java 33

V6007 Expression 'right.isEmpty() == false' is always true. CollectionUtils.java 36

Содержишь? Содержу. А если проверю?

Разгоняемся и смотрим не самый очевидный кейс:

@TaskAction
public void checkDependencies() {
    ....
    for (File file : licensesDirAsFile.listFiles()) {
        String name = file.getName();
        if (name.endsWith("-LICENSE") || name.endsWith("-LICENSE.txt")) {
            // TODO: why do we support suffix of LICENSE *and* LICENSE.txt??
            licenses.put(name, false);
        } else if (name.contains("-NOTICE")
                        || name.contains("-NOTICE.txt")) { // <=
            notices.put(name, false);
        } else if (name.contains("-SOURCES")
                        || name.contains("-SOURCES.txt")) { // <=
            sources.put(name, false);
        }
    }
    ....
}

Этот код работает правильно, но его также можно упростить, убрав лишние проверки. Разберём представленную логику, а чтобы точно не возникло путаницы, воспользуемся конкретным примером.

У нас есть строка "FILENAME-NOTICE.txt". Содержит ли она подстроку "-NOTICE.txt"? А "-NOTICE"? Обе проверки положительны. Получается, что нет смысла отдельно проверять "-NOTICE.txt" и "-NOTICE".

Полностью аналогичная ситуация у нас и с подстроками "-SOURCES.txt" и "-SOURCES".

Посмотрим на предупреждения анализатора:

V6007 Expression 'name.contains("-NOTICE.txt")' is always false. DependencyLicensesTask.java 228

V6007 Expression 'name.contains("-SOURCES.txt")' is always false. DependencyLicensesTask.java 230

Сложилось исторически

Под конец раздела посмотрим на код одного из многочисленных тестов:

public void testMinimumPerNode() {
    int negativeShardsPerNode = between(-50_000, 0);
    try {
        if (frequently()) {
            clusterAdmin().prepareUpdateSettings(....)
                .setPersistentSettings(
                    Settings.builder()
                    .put(shardsPerNodeKey, negativeShardsPerNode)
                    .build()
                )
                .get();
        } else {
            clusterAdmin().prepareUpdateSettings(....)
                .setPersistentSettings(
                    Settings.builder()
                    .put(shardsPerNodeKey, negativeShardsPerNode)
                    .build()
                )
                .get();
        }
        fail("should not be able to set negative shards per node");
    } catch (IllegalArgumentException ex) {
        ....
    }
}

Предупреждение анализатора PVS-Studio: V6004 The 'then' statement is equivalent to the 'else' statement. ClusterShardLimitIT.java 52

Чтобы выяснить, как получилось так, что then эквивалентен else, обратимся к Git History. Сам метод появился примерно шесть лет назад. If-else тогда ещё имел смысл, поскольку настройки могли устанавливаться двумя способами: текущим, а также с помощью setTransientSettings. Однако через некоторое время после того, как последний метод был помечен устаревшим, его заменили на setPersistentSettings, но не заметили, что then и else стали идентичны.

Это так работает?

Для начала взглянем на метод для получения случайного числа со случайным типом (double или float):

protected Number randomNumber() {
    /*
     * The source parser and doc values round trip will both reduce
     * the precision to 32 bits if the value is more precise.
     * randomDoubleBetween will smear the values out across a wide
     * range of valid values.
     */
    return randomBoolean() ?
        randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE, true)
        : randomFloat();
}

Вроде бы ничего необычного: метод случайно возвращает либо float, либо double.

Взглянем на предупреждение PVS-Studio: V6088 Result of this expression will be implicitly cast to 'double'. Check if program logic handles it correctly. ScaledFloatFieldMapperTests.java 505

Но как?! Может, проблема в том, что #randomFloat() на самом деле возвращает double? Посмотрим и на него:

public static float randomFloat() {
    return random().nextFloat();
}

И тут всё верно. Может, что-то не так с возвращаемым типом у #randomDoubleBetween?

public static double randomDoubleBetween(
                double start, double end,
                boolean lowerInclusive
                ) {
    double result = 0.0;
    ....
    return result;
}

Анализатор ошибся? Можем вас заверить, что нет. Пусть возвращаемый тип и является ссылочным, сначала вычисляется примитивное значение тернарного оператора, преобразование которого соответствует числовым контекстам (float приводится к double). И только после полученное значение упаковывается.

Стоит также подметить, что в Elasticseach есть множество подобных ошибок. Однако не стоит винить разработчиков в невнимательности, поведение и вправду непредсказуемое. От таких ситуаций спасает статический анализатор, который знает о подобный тонкостях реализации.

Исправить такую ошибку можно преобразованием тернарного оператора в обычный if-else.

if (randomBoolean()) {
    return randomDoubleBetween(-Float.MAX_VALUE, Float.MAX_VALUE, true);
} else {
    return randomFloat();
}

Пример аналогичной ошибки приведён ниже:

public static Number truncate(Number n, Number precision) {
    ....
    return n instanceof Float ? result.floatValue() : result;
}

Анализатор PVS-Studio даёт следующее предупреждение: V6088 Result of this expression will be implicitly cast to 'double'. Check if program logic handles it correctly. Maths.java 122

Подводим итоги

Выражаем уважение разработчикам: разрабатывать и поддерживать такой огромный проект, как Elasticsearch (напомним, три миллиона строк Java кода), стоит таких же огромных усилий. Однако все мы люди и все мы допускаем ошибки, которые, к сожалению, найти вручную не всегда получается.

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

Если вы хотите попробовать PVS-Studio, можете скачать триальную версию здесь. Для Open Source проектов есть вариант бесплатного лицензирования.

Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Egor Pishii. Searching in a search: let's check Elasticsearch.

Поиск в поиске: проверка Elasticsearch