Обновление зависимостей + null check#51
Conversation
|
WalkthroughОбновлён проект с переходом от Lombok и FindBugs на JSpecify для аннотаций нулевых ссылок, обновлены версии Gradle и зависимостей (JUnit, AssertJ), заменён лицензионный плагин и изменены правила включения/исключения файлов для лицензирования. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
build.gradle.kts (1)
13-13: Закомментирован плагинvalidate-poms.Плагин
io.freefair.maven-central.validate-pomsзакомментирован. Если это временная мера (например, несовместимость с Gradle 9), рекомендуется оставить TODO-комментарий с пояснением причины и планом возврата.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@build.gradle.kts` at line 13, Плагин "io.freefair.maven-central.validate-poms" сейчас закомментирован в build.gradle.kts без объяснения причины; добавьте рядом с закомментированной строкой TODO-комментарий, объясняющий причину (например, несовместимость с Gradle 9 или временный баг), указав ожидаемое условие для восстановления и план действий (например, цель версии плагина или тикет/дата), либо восстановите плагин, если он уже совместим; ссылаться в комментарии на идентификатор плагина "io.freefair.maven-central.validate-poms" для ясности.src/main/java/com/github/_1c_syntax/utils/Lazy.java (1)
55-64:lock.unlock()не защищёнtry-finally— утечка блокировки при исключении.Если
maybeComputeвыбросит исключение (включая новыйrequireNonNull(value)на строке 84 при конкурентном вызовеclear()),lock.unlock()не будет вызван, и экземплярLazyокажется перманентно заблокирован.Проблема существовала и раньше, но добавление
requireNonNull(value)вmaybeComputeсоздаёт дополнительную точку выброса исключения.♻️ Предлагаемое исправление
public T getOrCompute(Supplier<T> supplier) { final T result = value; // Just one volatile read if (result == null) { lock.lock(); - var localResult = maybeCompute(supplier); - lock.unlock(); - return localResult; + try { + return maybeCompute(supplier); + } finally { + lock.unlock(); + } } return result; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/utils/Lazy.java` around lines 55 - 64, The getOrCompute method currently calls lock.lock(), then invokes maybeCompute(supplier) and separately calls lock.unlock(), which can leak the lock if maybeCompute throws (e.g., requireNonNull in maybeCompute when clear() races). Change getOrCompute to acquire the lock and execute maybeCompute inside a try block with a finally that always calls lock.unlock() so the lock is released on success or exception; keep returning the computed value from the try or rethrow the exception as-is. Ensure references to getOrCompute, maybeCompute and lock.unlock are updated accordingly.src/main/java/com/github/_1c_syntax/utils/Absolute.java (1)
48-48: Миграция с Lombok@NonNullна JSpecify@NullMarkedменяет поведение при null-значениях в публичном API.Удаление
@NonNullозначает отсутствие runtime-проверок видаif (param == null) throw new NullPointerException("param is null")в начале методов. Компиляция полагается на@NullMarkedиз package-info.java, что обеспечивает контроль на этапе компиляции (для JSpecify-совместимых инструментов). При передачеnullв методыuri()иpath()потребители без JSpecify-инструментов получат менее информативный NPE глубже в коде (например, наuri.replace(...)в строке 50) вместо ясного сообщения "uri is null".Если это осознанное решение как часть миграции на JSpecify — всё в порядке. Если требуется fail-fast поведение для публичных методов, можно добавить явные
Objects.requireNonNull(uri, "uri")вызовы.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/com/github/_1c_syntax/utils/Absolute.java` at line 48, The public API lost runtime null checks when migrating from Lombok `@NonNull` to JSpecify `@NullMarked`; add explicit fail-fast checks by calling Objects.requireNonNull(...) at the start of the public methods (e.g., Absolute.uri(String uri) and Absolute.path(String path)) so a clear NPE with the parameter name is thrown immediately rather than a later NPE inside operations like uri.replace(...).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@build.gradle.kts`:
- Line 13: Плагин "io.freefair.maven-central.validate-poms" сейчас
закомментирован в build.gradle.kts без объяснения причины; добавьте рядом с
закомментированной строкой TODO-комментарий, объясняющий причину (например,
несовместимость с Gradle 9 или временный баг), указав ожидаемое условие для
восстановления и план действий (например, цель версии плагина или тикет/дата),
либо восстановите плагин, если он уже совместим; ссылаться в комментарии на
идентификатор плагина "io.freefair.maven-central.validate-poms" для ясности.
In `@src/main/java/com/github/_1c_syntax/utils/Absolute.java`:
- Line 48: The public API lost runtime null checks when migrating from Lombok
`@NonNull` to JSpecify `@NullMarked`; add explicit fail-fast checks by calling
Objects.requireNonNull(...) at the start of the public methods (e.g.,
Absolute.uri(String uri) and Absolute.path(String path)) so a clear NPE with the
parameter name is thrown immediately rather than a later NPE inside operations
like uri.replace(...).
In `@src/main/java/com/github/_1c_syntax/utils/Lazy.java`:
- Around line 55-64: The getOrCompute method currently calls lock.lock(), then
invokes maybeCompute(supplier) and separately calls lock.unlock(), which can
leak the lock if maybeCompute throws (e.g., requireNonNull in maybeCompute when
clear() races). Change getOrCompute to acquire the lock and execute maybeCompute
inside a try block with a finally that always calls lock.unlock() so the lock is
released on success or exception; keep returning the computed value from the try
or rethrow the exception as-is. Ensure references to getOrCompute, maybeCompute
and lock.unlock are updated accordingly.
| value = requireNonNull(supplier.get()); | ||
| } | ||
| return value; | ||
| return requireNonNull(value); |
There was a problem hiding this comment.
@theshadowco вот тут лишний вызов. Он точно обязателен? Может быть можно чуть переписать метод, чтобы по потоку кода тут не могло быть null?
2 participants
No description provided.