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 help to get the list of available commands and usage tips.}

@claude review

Claude finished @nixel2007's task in 2m 55s —— View job

Code Review

• Gather context and understand the request
• Read changed files
• Analyze code for bugs, thread-safety, performance, and correctness
• Post review findings

Отличная работа с задокументированными результатами (~20x на горячем пути). Архитектура в целом корректная. Нашёл одну критическую проблему с thread-safety (её же нашёл CodeRabbit), несколько средних и минорных.

🔴 Critical: computeMembers итерирует Collections.synchronizedList без synchronized-блока

Файл: TypeRegistry.java:354–376

registerMemberSource хранит список источников как Collections.synchronizedList(new ArrayList<>()). Javadoc этого класса явно требует: "It is imperative that the user manually synchronize on the returned list when traversing it via Iterator." Расширенный цикл for (var scoped : sources) использует итератор без синхронизации — возможен ConcurrentModificationException или несогласованное чтение в многопоточной среде.

// Текущий код — небезопасная итерация:
for (var scoped : sources) { ... }

// Исправление:
List<ScopedMemberSource> snapshot;
synchronized (sources) {
    snapshot = List.copyOf(sources);
}
for (var scoped : snapshot) { ... }

Альтернатива: заменить Collections.synchronizedList(new ArrayList<>()) на new CopyOnWriteArrayList<>() в registerMemberSource — тогда computeMembers итерирует безопасно без явной синхронизации (оптимально для read-heavy профиля).

Fix this →

🟡 Medium: Гонка between clear и computeIfAbsent в CallStatementByReceiverIndex

Файл: CallStatementByReceiverIndex.java:75

var index = byUri.computeIfAbsent(uri, k -> build(ast));

Сценарий гонки:

1. Документ изменился → handleContentChanged вызывает byUri.remove(uri)
2. Параллельно инференсер вызывает byReceiver и computeIfAbsent начинает build(ast) со старым AST
3. build завершается и сохраняет устаревший индекс в карту

В результате в byUri может оказаться индекс, построенный по старому AST. Следующий clear (при следующем изменении) его уберёт, но между этим существует окно, где выдаётся устаревший результат.

Практический эффект невелик (следующее изменение исправит ситуацию), но при активном редактировании и высокой нагрузке это может давать стабильно ненулевую частоту ложных попаданий. Можно исключить проблему, если получать ast внутри computeIfAbsent непосредственно из URI (если есть доступ к ServerContext), или документировать эту гонку как осознанный трейд-офф (аналогично тому, как InferredVariableTypeIndex документирует отсутствие кросс-документной инвалидации).

🟡 Medium: inferVariable кэширует без учёта fileType

Файл: ExpressionTypeInferencer.java:745–787

Внутри inferVariable используется ctx.documentContext.getFileType() (через getMembers, resolve, declaredParameterTypes). Кэш в InferredVariableTypeIndex ключуется только по VariableSymbol. Если переменная теоретически могла бы инферироваться в разных fileType-контекстах (например, через cross-doc вызов из .os-файла), кэшированное значение из BSL-контекста было бы возвращено для OS-контекста.

На практике variable.getOwner().getFileType() детерминирован, а ctx.documentContext всегда сеттится в variable.getOwner() (см. inferSymbol). Проблемы нет, но это неявное допущение — стоит добавить комментарий или assert ctx.documentContext == variable.getOwner().

🟢 Minor: Лишняя переменная effective в нескольких местах TypeRegistry

Файл: TypeRegistry.java:405,406 (и аналогично в registerConstructors, registerConstructorSource)

var effective = scope;
memberSources.computeIfAbsent(ref, k -> Collections.synchronizedList(new ArrayList<>()))
  .add(new ScopedMemberSource(source, effective));

effective — точная копия scope, никакой трансформации не выполняется. Либо уберите промежуточную переменную, либо выполните здесь нужную логику (например, var effective = scope != null ? scope : LanguageScope.BOTH), о которой намекает название.

🟢 Minor: Тесты не проверяют все ресиверы после событий жизненного цикла

Файл: CallStatementByReceiverIndexTest.java:64–71

После каждого из трёх событий (ContentChanged, Closed, Removed) тест проверяет только "ТЗ". CodeRabbit прав, что добавление проверок "СТР" (hasSize 1) и "Нет" (isEmpty) после каждого события дало бы уверенность в том, что весь индекс пересобирается корректно, а не только один из ключей.

✅ Что сделано хорошо

• Epoch-подход для getMembers: Корректная инвалидация при мутации memberSources и при DocumentContextContentChangedEvent (последнее важно, т.к. ленивые MemberSource читают символьное дерево без ре-регистрации). AtomicLong гарантирует видимость изменений между потоками.
• Условие кэширования ctx.visited.size() <= 1: Правильно исключает вложенные (потенциально усечённые цикл-гардом) выводы из кэша, сохраняя только корневые результаты.
• Неизменяемые результаты: List.copyOf(byName.values()) в computeMembers и HashMap→немутируемый итог в CallStatementByReceiverIndex.build — защита от случайных мутаций потребителями.
• Инвалидация по всем трём событиям (ContentChanged, Closed, Removed) — полное покрытие жизненного цикла документа в обоих индексах.
• 100% покрытие новых классов — тесты существенны и проверяют ключевые инварианты.

Итог: Критическая проблема с thread-safety в computeMembers требует исправления до merge. Остальное — улучшения надёжности и читаемости.

Test Results

 2 994 files  +12   2 994 suites  +12   1h 32m 38s ⏱️ + 2m 14s
 2 716 tests + 5   2 716 ✅ + 5  0 💤 ±0  0 ❌ ±0 
16 296 runs  +30  16 296 ✅ +30  0 💤 ±0  0 ❌ ±0 

Results for commit e82b0a6. ± Comparison against base commit 0ed27f8.

♻️ This comment has been updated with latest results.

@claude re-review

Claude finished @nixel2007's task in 2m 39s —— View job

Code Review (Re-review)

• Gather context and understand the request
• Read all changed files
• Check if previous issues were addressed
• Check SonarQube duplication findings
• Post comprehensive review

Changes since the previous review

✅ Critical thread-safety fix — computeMembers now takes a synchronized snapshot before iterating Collections.synchronizedList:

// TypeRegistry.java:368–371
List<ScopedMemberSource> snapshot;
synchronized (sources) {
    snapshot = List.copyOf(sources);
}
for (var scoped : snapshot) { ... }

The fix is correct and the comment explaining why is a good addition.

✅ CallStatementByReceiverIndexTest coverage improved — the test now uses an assertAllReceivers helper (lines 79–83) that checks all three receivers after each of the four lifecycle events. The CodeRabbit nitpick about only re-checking "ТЗ" is addressed.

✅ computeIfAbsent race in CallStatementByReceiverIndex — documented as an intentional tradeoff in the code comment at byReceiver (lines 78–82). Same model as SymbolTypeIndex. Clear and acceptable.

Remaining issues

🔴 SonarQube Quality Gate fails: duplication > 3% on new code

The four event-listener methods are verbatim identical in both InferredVariableTypeIndex (lines 105–123) and CallStatementByReceiverIndex (lines 107–125):

// Identical 4-method block in BOTH files:
@EventListener
public void handleContentChanged(DocumentContextContentChangedEvent event) {
    clear(event.getSource().getUri());
}
@EventListener
public void handleDataCleared(ServerContextDocumentClearedEvent event) {
    clear(event.getDocumentContext().getUri());
}
@EventListener
public void handleDocumentClosed(ServerContextDocumentClosedEvent event) {
    clear(event.getDocumentContext().getUri());
}
@EventListener
public void handleDocumentRemoved(ServerContextDocumentRemovedEvent event) {
    clear(event.getUri());
}

~20 duplicated lines × 2 occurrences ≈ 6% of the 668 added lines, which matches the reported 5.3% and B maintainability rating. This blocks the Quality Gate (required ≤ 3%, required ≥ A).

Fix: Extract to an abstract base class in the same package:

// types/index/AbstractDocumentLifecycleClearableIndex.java
abstract class AbstractDocumentLifecycleClearableIndex {

    public abstract void clear(URI uri);

    @EventListener
    public void handleContentChanged(DocumentContextContentChangedEvent event) {
        clear(event.getSource().getUri());
    }
    @EventListener
    public void handleDataCleared(ServerContextDocumentClearedEvent event) {
        clear(event.getDocumentContext().getUri());
    }
    @EventListener
    public void handleDocumentClosed(ServerContextDocumentClosedEvent event) {
        clear(event.getDocumentContext().getUri());
    }
    @EventListener
    public void handleDocumentRemoved(ServerContextDocumentRemovedEvent event) {
        clear(event.getUri());
    }
}

Spring's @EventListener is inherited by concrete subclasses, so both indices just extend this class and drop the four duplicated methods. This will bring duplication well below 3%.

Fix this →

🟢 Minor: dead var effective = scope assignments in TypeRegistry

Five locations in TypeRegistry.java (lines 412, 494, 689, 747, 777) create a local variable effective or effectiveScope that is identical to scope with no transformation:

var effective = scope;   // always == scope, no transform
memberSources.computeIfAbsent(...).add(new ScopedMemberSource(source, effective));

Either use scope directly, or perform the intended defaulting here (e.g. var effective = scope != null ? scope : LanguageScope.BOTH). The name implies a transformation that isn't happening, which is misleading.

🟢 Minor: no test for two calls with the same receiver

CallStatementByReceiverIndexTest still only tests one call per receiver. A case with two ТЗ.Колонки.Добавить(...) calls would verify that byReceiver returns hasSize(2) and that aggregation works correctly. Low priority, but CodeRabbit flagged it.

What looks good

• membersEpoch epoch-based invalidation is correct: even if epoch is bumped concurrently with computeMembers, the new cached entry will have the old epoch and be invalidated on the next read — no stale data, at most one extra recompute.
• InferredVariableTypeIndex cache condition ctx.visited.size() <= 1 correctly limits caching to root inference calls only.
• ServerContextDocumentClearedEvent + @AfterReturning with returning = "cleared" correctly avoids publishing on no-op clears (opened documents). The realTryClearDocumentEvictsCacheViaAop and openedDocumentIsNotClearedAndKeepsCache tests cover both branches explicitly.
• All tests pass (2994 suites, 0 failures).

Bottom line: The only blocker is the SonarQube Quality Gate failure from the duplicated event-listener block. The fix is mechanical (~15 lines) and unblocks the merge.

Quality Gate passed

Issues
6 New issues
1 Accepted issue

Measures
0 Security Hotspots
92.9% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud