fix(types): перечитывание того же содержимого не сбрасывает типы - #4441
Conversation
Разбор документа не всегда означает правку: в пакетном анализе тот же самый текст перечитывается заново после освобождения вторичных данных. Индексы типов трактовали такой разбор как изменение — стирали записи документа и считали их заново, уже параллельно с чужими чтениями. На ssl_3_1 это 2162 сноса за прогон и 2035 чтений, вернувших пустоту по методу, у которого мгновением раньше был непустой тип. DocumentContext держит отпечаток разобранного содержимого (переживает освобождение вторичных данных) и признак «содержимое изменилось», который уходит в событие разбора. SymbolTypeIndex и MethodReturnTypeIndexer по перечитыванию ничего не трогают: символы построенного заново дерева равны прежним, поэтому находятся по старым записям. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…имволам Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 30 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthrough
ChangesContent-aware document rebuilds
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant DocumentContext
participant EventPublisherAspect
participant DocumentIndexes
DocumentContext->>DocumentContext: rebuild and compare SHA-256 fingerprint
EventPublisherAspect->>DocumentIndexes: publish content-change event
DocumentIndexes->>DocumentIndexes: skip indexing when content is unchanged
DocumentIndexes->>DocumentIndexes: reindex when content changes or entries are missing
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java`:
- Around line 393-395: The contentFingerprint implementation in DocumentContext
must stop relying on the collision-prone 32-bit String.hashCode(); replace it
with full-content identity or a cryptographic digest such as SHA-256 so
different same-length document contents cannot be treated as unchanged. Update
the related rebuild-change detection to use the stronger identity, and add a
regression test covering distinct same-length strings such as “Aa” and “BB” to
verify SymbolTypeIndex and MethodReturnTypeIndexer records are refreshed.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9e3736dc-f1d9-4519-8ac6-e20389461bc5
📒 Files selected for processing (8)
src/main/java/com/github/_1c_syntax/bsl/languageserver/aop/EventPublisherAspect.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/CLAUDE.mdsrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/context/events/DocumentContextContentChangedEvent.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/MethodReturnTypeIndexer.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/SymbolTypeIndex.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/MethodReturnTypeIndexerTest.java
У String.hashCode() 32 бита, и совпадения у него не случайность, а конструируемое свойство: "Aa" и "BB" равны по нему при равной длине. По совпадению отпечатков документ считается неизменившимся и сохраняет записи, посчитанные по прежнему тексту, — ошибка в эту сторону тихая. Отпечаток считается SHA-256 по содержимому. Тест берёт ровно такую пару текстов и проверяет, что правка распознаётся как правка. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cu7S3zYn7n6GMsdYf5v1q
|
Принято, исправлено в 40cc383. Замечание по делу: у Отпечаток теперь считается SHA-256 по содержимому, поле стало Про цену: свёртка считается один раз на разбор документа. На ssl_3_1 это 2162 файла общим объёмом порядка 246 МБ, то есть доли секунды на весь прогон. Точные числа добавлю вместе с замерами времени — сейчас на машине идёт корпоративный сканер, съедающий до трёх ядер, и прогоны одной сборки давали разброс от 44 до 98 секунд. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java (1)
105-120: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn the change flag from each rebuild.
EventPublisherAspect.documentContextRebuildreads sharedcontentChangedOnLastRebuildafterrebuildreleases its locks. A concurrent rebuild can overwrite the flag before the advice reads it. The event can then makeSymbolTypeIndexandMethodReturnTypeIndexerkeep stale data or clear valid data. Return the completed rebuild result and bind it with@AfterReturning(returning = ...).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java` around lines 105 - 120, The rebuild operation in DocumentContext must return its completed content-change result instead of relying on the shared contentChangedOnLastRebuild field after locks are released. Update rebuild and EventPublisherAspect.documentContextRebuild to bind the returned value with `@AfterReturning`(returning = ...), and use that invocation-specific result when updating SymbolTypeIndex and MethodReturnTypeIndexer.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java`:
- Around line 357-360: Update rebuild in DocumentContext so the candidate
fingerprint and content-changed flag remain local while validating the version
and running computeSymbolTree(). Assign them to contentFingerprint and
contentChangedOnLastRebuild only after computeSymbolTree() succeeds, preserving
the previous committed state on same-version returns or failures. Add coverage
for repeated-version content changes and retrying after a failed rebuild.
---
Outside diff comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java`:
- Around line 105-120: The rebuild operation in DocumentContext must return its
completed content-change result instead of relying on the shared
contentChangedOnLastRebuild field after locks are released. Update rebuild and
EventPublisherAspect.documentContextRebuild to bind the returned value with
`@AfterReturning`(returning = ...), and use that invocation-specific result when
updating SymbolTypeIndex and MethodReturnTypeIndexer.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9348496d-14e6-484c-ad70-b11677ecfdc6
📒 Files selected for processing (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java
Перечитывание того же текста — не правка, и это касается не только типов. ReferenceIndexFiller держал ровно такой же механизм собственной постройки: карту URI → отпечаток, с той же 32-битной свёрткой. Вместо неё — признак из события; от карты остаётся множество URI, доведённых до конца индексации, чтобы упавший обход переиндексировался при следующем событии. ConfigurationModuleMembersProvider, OScriptModuleMembersProvider и WorkspaceSymbolIndex на перечитывании делали работу заново; у OScript-провайдера это вдобавок сбрасывало memo членов у всех наследников документа. Пропуск разрешён только когда записи по документу на месте: закрытие документа их убирает, и тогда перечитывание обязано наполнить индекс снова. Заморозка вычисленных данных — политика хранения, а изменение содержимого — факт, и факт сильнее. Замороженным остаётся документ, перечитанный из-за правки файла на диске: его прежние диагностики и метрики посчитаны по другому тексту, а сбрасывались только у незамороженных. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cu7S3zYn7n6GMsdYf5v1q
|
Снял обещанные замеры на сети и без сканера — и они меняют выводы, поэтому выкладываю целиком. Время. Шесть прогонов на сторону вперемежку: develop 42/71/67 и ветка 48/63/60 в одной серии, 35–60 с в другой. Одна и та же сборка даёт от 35 до 71 секунды, поэтому по времени вывода нет: разброс перекрывает любую разницу. Деградации не видно, ускорения тоже. Недетерминированность — а вот тут результат неприятный. По шесть прогонов на сборку, все пятнадцать попарных расхождений подписи:
Разбор по устойчивости (замечание считается устойчивым, если оно есть во всех шести прогонах):
И происхождение мерцающих у ветки:
То есть правка не расшатывает что-то постороннее: она чинит через раз. Уцелеют ли записи типов к моменту чтения — зависит от порядка, а порядок и есть предмет #4429. Отсюда предложение: этот PR отдельно не вливать. Для CI устойчиво-ложное замечание неприятно, но предсказуемо, а мерцающее ломает воспроизводимость прогона — ровно то, из-за чего #4429 и заведён. Ценность правки (сходимость итерации по циклам: 42 → 11 → 2 → 2… против 115 → 24 → 0) реализуется только вместе с расписанием расчёта. Перевожу в черновик и понесу в стеке #4429; если считаете иначе — верну в готовые. |
Разбор с тем же номером версии содержимое не применяет — дерево остаётся прежним, — а отпечаток записывался до этой проверки, уже от нового текста. Следующий разбор того же текста после этого выглядел бы перечитыванием, и записи, построенные по прежнему дереву, остались бы жить. То же самое при падении построения дерева символов: сорвавшийся разбор выглядел состоявшимся, и повторная попытка не переделывала работу. Отпечаток и признак изменения фиксируются последними, когда содержимое уже применено и дерево построено. В неприменённом разборе признак — «не менялось»: дерево то же, значит и записи по нему верны. Заодно сняты пропуски у ConfigurationModuleMembersProvider и OScriptModuleMembersProvider: их register попутно сбрасывает memo членов типа, то есть это не повтор работы, а необходимое действие. Замер выигрыша от них не показал. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017cu7S3zYn7n6GMsdYf5v1q
|
Оба замечания закрыты. Отпечаток до применения содержимого — исправлено в 50b5589. Разбор с тем же номером версии уходит по раннему возврату, не применяя текст, поэтому теперь отпечаток и признак изменения фиксируются последними: после того как содержимое применено и дерево символов построено. В неприменённом разборе признак — «не менялось», это честно: дерево осталось прежним, значит и записи по нему верны. Сорвавшийся разбор больше не выглядит состоявшимся — повторная попытка увидит текст новым и переделает работу. Тест на повтор номера версии добавлен: без правки он краснеет. Разрядность отпечатка — исправлено ранее в 40cc383 (SHA-256 плюс тест на паре Заодно снял из PR пропуски у |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java`:
- Around line 114-120: Add regression tests for the event skip behavior: in
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java:114-120,
extend ReferenceIndexFillerTest to verify an unchanged event triggers indexing
before a successful fill but is skipped afterward; in
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.java:226-236,
extend WorkspaceSymbolIndexTest to verify unchanged content is skipped while
entries remain and indexing occurs after clear(URI) removes them. Use the
existing JUnit, AssertJ, and Mockito patterns, and run the tests.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1f0a70a6-a8b4-4507-976c-3ab7aa2e2990
📒 Files selected for processing (4)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.javasrc/main/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndex.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContextTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/github/_1c_syntax/bsl/languageserver/context/DocumentContext.java
Пропуск разрешён только когда записи по документу уже есть: у ReferenceIndexFiller это признак дошедшей до конца индексации, у WorkspaceSymbolIndex — наличие записей по URI. Тесты закрепляют обе ветки: перечитывание того же текста записи сохраняет, а после удаления документа либо очистки индекса тот же текст индексируется заново. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Тесты добавлены в fa01d32 — по обеим веткам пропуска, как и просили.
Оба сценария с очисткой краснеют, если убрать проверку «записи по документу на месте», — то есть тесты закрепляют именно условие пропуска, а не просто проходят. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.java`:
- Around line 83-95: Add assertions immediately after the initial indexing event
in ReferenceIndexFillerTest.java lines 83-95 and 97-114 to verify the reference
exists before unchanged-content handling or removal. In
WorkspaceSymbolIndexTest.java lines 94-109 and 111-130, assert the symbol exists
after initial indexing and before the unchanged-content event or index.clear
call. Use the existing reference and symbol lookup assertions.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 37eb743c-33a5-4244-8f38-cf4c14bfe73b
📒 Files selected for processing (2)
src/test/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFillerTest.javasrc/test/java/com/github/_1c_syntax/bsl/languageserver/types/index/WorkspaceSymbolIndexTest.java
Без проверки после первого разбора тест проходил бы и в случае, когда первая индексация ничего не записала, а пропуск сработал неверно. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Принято, поправил в этом же прогоне: во все четыре теста добавлена проверка исходного состояния — что первый разбор действительно записал вхождения и символы. Без неё тест прошёл бы и тогда, когда первая индексация ничего не сделала, а пропуск сработал неверно. |
|
1 participant
Проблема
Разбор документа не всегда означает правку. В пакетном анализе один и тот же текст перечитывается заново после освобождения вторичных данных — у каждого файла. Индексы типов трактовали такой разбор как изменение: стирали записи документа и считали их заново, уже параллельно с чужими чтениями.
Замер на ssl_3_1 (инструментация): 2162 сноса записей за прогон и 2035 чтений, вернувших пустоту по методу, у которого мгновением раньше был непустой тип.
Отсюда ложные замечания вида «У типа "Структура" нет метода или свойства "Результат"»: поля структуры были известны, но запись стёрли посреди работы.
Что сделано
DocumentContextдержит отпечаток разобранного содержимого — он переживает освобождение вторичных данных — и выставляетisContentChangedOnLastRebuild(). Признак уходит вDocumentContextContentChangedEvent(isContentChanged()).Дальше действует правило, записанное в
context/CLAUDE.md:SymbolTypeIndex,MethodReturnTypeIndexer), по перечитыванию того же текста не делает ничего: символы построенного заново дерева равны прежним (имя, документ, позиция имени), поэтому находятся по старым записям. Снос и сборка заново оставляли окно, в котором чужой поток не находит уже посчитанного;InferredExpressionTypeIndex): после разбора это другие объекты. Их поведение не меняется.Замеры на ssl_3_1
Пакетный анализ, репортер SARIF,
mode: onlyсUnknownMember+EventHandlerInvalidSignature.Разница по замечаниям: 953 исчезло, 318 появилось, чистое −635. Верхушка исчезнувших — характерная: «У типа "Структура" нет метода или свойства "Результат"» (86), «…"Выполнено"» (62), «…"СтатусОшибки"», «…"Ошибка"».
Время не привожу: на этой машине идёт корпоративный сканер, съедающий до трёх ядер, и прогоны одной и той же сборки давали от 44 до 98 секунд. Как появится тишина — добавлю числа отдельным комментарием. Счётчики выше от этого не зависят.
Чего это не чинит
Недетерминированность анализа сама по себе не уходит: расхождение подписи между двумя прогонами остаётся 292 строки при базовых 273–363 у develop. Стирка записей — одна из трёх причин, найденных по #4429; остальные (межмодульные факты, посчитанные по неполной области, и немонотонность передачи) лечатся отдельно.
Зато без этой правки не работает то, что дальше: итерация по циклам вызовов на develop не сходится вовсе (42 → 11 → 2 → 2 → 2…, упор в предохранитель), потому что значения зависимостей обнуляются между волнами. С правкой она сходится за две волны (115 → 24 → 0).
Связанные задачи
Часть работы по #4429.
Остальные подписчики того же события
Признак «содержимое изменилось» касается не только типов — по подписчикам прошёлся целиком.
ReferenceIndexFillerдержал ровно такой же механизм собственной постройки: картуURI → отпечатокс той же 32-битной свёрткой, что чинилась выше. Заменён признаком из события; от карты остаётся множество URI, у которых индексация дошла до конца, — упавший на обходе документ переиндексируется при следующем событии, как и раньше.Ещё трое делали на перечитывании работу заново, и теперь её пропускают:
ConfigurationModuleMembersProviderOScriptModuleMembersProviderWorkspaceSymbolIndexПропуск везде разрешён только когда записи по документу на месте: закрытие документа их убирает, и тогда перечитывание обязано наполнить индекс снова. Без этой оговорки поиск по символам после закрытия и перечитывания оставался бы пустым.
Не тронуты сознательно:
EventContractsIndex— его база стирает записи уже при освобождении вторичных данных, поэтому пропускать нечего: наполнять придётся в любом случае;AnnotationReferenceFinder— для BSL-файлов он выходит сразу, а для OS смотрит только аннотацию конструктора, то есть работы там нет. Вдобавок репозиторий аннотаций чистится целиком в обработчике наполнения области, и локальный признак «уже проиндексирован» после этого врал бы.Заморозка вычисленных данных
isComputedDataFrozen— политика хранения («этот документ не редактируют, его вторичные данные держим»), а изменение содержимого — факт. Факт сильнее, и до сих пор это не учитывалось:rebuildсбрасывал вторичные данные только у незамороженных. Между тем замороженным остаётся документ, перечитанный по событию правки файла на диске (BSLWorkspaceService.handleChangedFileEvent) — его прежние диагностики и метрики посчитаны уже по другому тексту.Теперь сброс идёт, если содержимое действительно изменилось, независимо от заморозки. Два теста закрепляют обе стороны: правка замороженного документа данные сбрасывает, перечитывание того же текста — нет.
Summary by CodeRabbit
Bug Fixes
Tests