Skip to content

refactor(references): разорвать цикл references↔types через унификацию OScript-резолва по mdoRef#4233

Open
nixel2007 wants to merge 4 commits into
developfrom
claude/break-references-types-cycle
Open

refactor(references): разорвать цикл references↔types через унификацию OScript-резолва по mdoRef#4233
nixel2007 wants to merge 4 commits into
developfrom
claude/break-references-types-cycle

Conversation

@nixel2007

@nixel2007 nixel2007 commented Jun 29, 2026

Copy link
Copy Markdown
Member

Зачем

references ↔ types — последний реальный кросс-доменный цикл ядра. Естественное направление —
references снизу (type-agnostic разрешение имён), types сверху (вывод типов опирается на
разрешение имён). Значит оставляем types → references, убираем references → types целиком.

Этих рёбер было 6: пять давали три «type-aware» finder'а, шестое — ReferenceIndexFiller,
которому нужно было сматчить имя OneScript-класса/модуля в его .os-документ.

Что сделано

1. Перенос type-aware finder'ов (5 рёбер). KeywordReferenceFinder,
NewExpressionReferenceFinder, PlatformMemberReferenceFinder резолвят ссылки, консультируясь с
системой типов
(TypeService, GlobalScopeProvider, символы типов) — это адаптеры порта
ReferenceFinder, а не ядро разрешения имён. Порт и чистые finder'ы остались в references, эти
три переехали в types.references, к своим зависимостям. Они по-прежнему реализуют
references.ReferenceFinder и собираются ReferenceResolver'ом через List
(Spring, независимо от пакета).

2. Унификация OScript-резолва по mdoRef (6-е ребро). Раньше BSL-ссылки резолвились по
логическому mdoRef (MdoRefBuilder/findCommonModule из context) через
ServerContext.getDocument(mdoRef, moduleType), а OScript был исключением — ключевался по URI,
потому что у .os-доков библиотек нет объекта метаданных, а значит и логического mdoRef. Привёл
OScript к BSL — теперь у library-.os-документа mdoRef = его каноничное lib-имя:

  • OScriptLibraryIndex при registerEntry до addDocument сеет привязку URI → каноничное имя в ServerContext (registerOScriptLibraryName); MdoRefBuilder для .os-файла без
    MD-объекта берёт mdoRef из этой привязки (а не из URI). Так документ попадает в
    documentsByMDORef под своим же mdoRef — механизмом addMdoRefByUri, единообразно с BSL.
    Мультиидентичность (один файл — несколько lib-имён/ролей) схлопывается в одну стабильную
    идентичность документа (первое имя выигрывает, как и ModuleType в OScriptModuleTypeResolver);
    под каждый ModuleType-роль документ доступен через тот же mdoRef.
  • ServerContext держит каталог lib-сущностей (имя → (ModuleType, DocumentContext),
    регистронезависимый) для резолва имени класса/модуля в документ — параллельно findCommonModule.
    findLibraryClass/findLibraryModule теперь возвращают DocumentContext (а не строку-имя):
    вызывающему нужен именно документ (конструктор, символы), а ключ индекса он берёт из
    doc.getMdoRef().
  • ReferenceIndexFiller берёт документ через findLibrary…, пишет mdoRef = doc.getMdoRef() и
    синтаксический ModuleType; лишний round-trip getDocument(mdoRef, moduleType) и хелпер
    libraryClassConstructor убраны. Зависимость references → types из филлера ушла.

3. Починка висячих javadoc-@link. Три ссылки на KeywordReferenceFinder в
context.symbol/hover/providers остались указывать на старый пакет после переезда финдера в
types.references (шаг 1) — обновлены на новый FQN (из-за них CI check падал на javadoc).

Граф

references → types = 0цикл разорван, references стал нижним слоем. Из реальных циклов
ядра не осталось ни одного (после configuration↔context в #4229). Модель ArchitectureTest
обновлена: References больше не обращается к Types, осталось только types → references.

Инварианты (по ревью-замечаниям)

  • context не зависит от types: каталог/привязки на ServerContext используют только
    контекст-безопасные типы (String/URI/DocumentContext/внешний bsl.types.ModuleType) —
    цикла context↔types не возникает.
  • API поиска модулей симметричен: findLibraryClass/Module — в том же домене context, что и
    findCommonModule (не переведён на API типов).
  • BSL-путь регистрации (addMdoRefByUri/mdoRefs) концептуально не тронут — .os теперь просто
    идёт по нему же, получив каноничный mdoRef.

Проверки

Локально зелёные: полный ./gradlew testBUILD SUCCESSFUL, 0 падений (включая
ArchitectureTest с layer_dependencies_are_respected/acyclic_domains_stay_free_of_cycles и все
OScript-тесты резолюции/определения/hover/completion/signature), плюс spotlessCheck и javadoc.

Дальше (отдельно)

Консолидация OScriptLibraryIndexGlobalScopeProvider (оба держат имя/тип ↔ URI в types) —
отложена в отдельный шаг.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Enhanced navigation for OneScript library classes and modules with more robust matching (case- and Unicode-normalized).
    • Improved library resolution consistency so library-backed symbols map reliably across documents.
  • Bug Fixes

    • Reference resolution remains correct after workspace reindexing, including when library-related files are added, updated, or removed.
    • Reduced missed and stale references for library-based code navigation, with improved handling for library documents that may lack certain internal anchors.

claude added 2 commits June 29, 2026 17:30
…ences

KeywordReferenceFinder, NewExpressionReferenceFinder and
PlatformMemberReferenceFinder resolve references by consulting the type
system (TypeService, GlobalScopeProvider, type symbols) — they are
type-aware adapters of the ReferenceFinder port, not part of the
type-agnostic name-resolution core. The port (ReferenceFinder) and the
pure finders stay in `references`; these three move to `types.references`,
next to their dependencies, so `references` no longer depends on `types`
through them. They keep implementing references.ReferenceFinder and are
still collected by ReferenceResolver via List<ReferenceFinder> (Spring,
package-independent).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkYRgB6NZZRRNZmBrdL54M
…oRef unification

The last references→types edge was ReferenceIndexFiller depending on
types.oscript.OScriptLibraryIndex to map an OScript library class/module
name to its file URI (used as the reference mdoRef). BSL references resolve
differently: the filler computes a logical mdoRef from metadata via
context (MdoRefBuilder/findCommonModule) and resolves through
ServerContext.getDocument(mdoRef, moduleType). OScript was the odd one out,
keying by URI because .os library docs had no logical mdoRef.

Unify OScript onto the same path:
- ServerContext gains a workspace-level catalog of OScript library symbols
  (name → canonical name + ModuleType + URI), case-insensitive, with
  findLibraryClass/findLibraryModule — parallel to findCommonModule and
  using only context-safe types (no dependency on the `types` package).
- OScriptLibraryIndex populates the catalog (types→context) during reindex
  and registerEntry, and clears/removes it in lockstep with its own entries.
- .os documents are additionally registered in documentsByMDORef under
  each of their canonical library names, so getDocument(name, moduleType)
  resolves them — multi-identity (one file, several library names/roles) is
  handled by registering the doc under each name. The BSL registration path
  (addMdoRefByUri / mdoRefs) is untouched.
- ReferenceIndexFiller now canonicalizes via ServerContext.findLibrary…,
  uses the canonical name as mdoRef and the syntactic ModuleType, and drops
  OScriptLibraryIndex and actualLibraryModuleType — so references no longer
  depends on types.

The ArchitectureTest model is updated accordingly (References no longer
accesses Types; only types→references remains).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkYRgB6NZZRRNZmBrdL54M
@coderabbitai

coderabbitai Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Walkthrough

ServerContext now tracks canonical OneScript library names and symbols. OScriptLibraryIndex registers, removes, and clears those mappings as library documents change. ReferenceIndexFiller resolves library references through ServerContext, and type-aware reference classes move into types.references with matching architecture and Javadoc updates.

Changes

OScript library catalog and reference resolution

Layer / File(s) Summary
OScript library catalog in ServerContext
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java, src/main/java/com/github/_1c_syntax/bsl/languageserver/context/MdoRefBuilder.java
Adds canonical library-name tracking, normalized symbol lookup, catalog cleanup, and a mdoRef fallback that uses the stored canonical name.
OScriptLibraryIndex synchronizes ServerContext
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java
Removes stale registrations on reindex and document removal, and registers library names and symbols as documents are rebuilt.
Reference lookup and types.references move
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java, src/main/java/com/github/_1c_syntax/bsl/languageserver/types/references/*, src/test/java/com/github/_1c_syntax/bsl/languageserver/architecture/ArchitectureTest.java, src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/KeywordSymbol.java, src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/KeywordSymbolMarkupContentBuilder.java, src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/HoverProvider.java
ReferenceIndexFiller now resolves library classes and modules from ServerContext, the reference-finder classes move into types.references, and the architecture and Javadoc references are updated to match.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related PRs

Suggested reviewers

  • theshadowco
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 71.43% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed Title clearly matches the main change: breaking the references↔types cycle by unifying OScript resolution via mdoRef.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed Описание соответствует изменениям: оно объясняет разрыв цикла references↔types и унификацию резолва OScript.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/break-references-types-cycle

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@nixel2007 nixel2007 changed the title refactor(references): move type-aware ReferenceFinders to types.references refactor(references): разорвать цикл references↔types через унификацию OScript-резолва по mdoRef Jun 29, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java (1)

571-572: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Rename the URI-named library-class flow to mdoRef.

findLibraryClass(...) now returns the canonical mdoRef, but this value still flows through variableToLibraryClassUriMap, libClassUri, and comments that say URI. Rename these to ...MdoRef... to keep the new ServerContext contract explicit.

🤖 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/references/ReferenceIndexFiller.java`
around lines 571 - 572, The library-class lookup in ReferenceIndexFiller should
be renamed to reflect the new canonical mdoRef value instead of URI. Update the
flow around findLibraryClass(...) and the related variable/method names such as
variableToLibraryClassUriMap and libClassUri so they use MdoRef naming
consistently, and adjust any comments or local symbols that still describe the
value as a URI.
🤖 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/ServerContext.java`:
- Around line 128-130: `ServerContext` currently indexes `OScriptLibrarySymbol`
only by `nameKey`, so a library exported as both module and class can overwrite
one entry and break `findLibraryClass(...)` or `findLibraryModule(...)`. Update
the `oscriptLibrariesByNameKey` / `oscriptLibraryNameKeysByUri` storage and the
related registration and cleanup flow in `ServerContext` to key symbols by both
name and module type (for example, via a composite key or nested map) so both
variants can coexist. Also make removal URI-aware and exact to the stored symbol
key so cleanup does not unregister a replacement from another URI sharing the
same `nameKey`.

---

Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java`:
- Around line 571-572: The library-class lookup in ReferenceIndexFiller should
be renamed to reflect the new canonical mdoRef value instead of URI. Update the
flow around findLibraryClass(...) and the related variable/method names such as
variableToLibraryClassUriMap and libClassUri so they use MdoRef naming
consistently, and adjust any comments or local symbols that still describe the
value as a URI.
🪄 Autofix (Beta)

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

Run ID: fad3e7ec-63b5-424e-9855-94a5c0f65ff0

📥 Commits

Reviewing files that changed from the base of the PR and between 2311db8 and d6705c4.

📒 Files selected for processing (8)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/references/KeywordReferenceFinder.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/references/NewExpressionReferenceFinder.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/references/PlatformMemberReferenceFinder.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/references/package-info.java
  • src/test/java/com/github/_1c_syntax/bsl/languageserver/architecture/ArchitectureTest.java

Comment thread src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java Outdated
@github-actions

github-actions Bot commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Test Results

 3 582 files   3 582 suites   1h 31m 40s ⏱️
 3 535 tests  3 517 ✅  18 💤 0 ❌
21 210 runs  21 098 ✅ 112 💤 0 ❌

Results for commit 4687137.

♻️ This comment has been updated with latest results.

…not generic doc lifecycle

The first cut registered .os documents in documentsByMDORef via a hook in
the generic createDocumentContext/removeDocument paths (fired for every
document, BSL included, no-op for non-OScript) and kept a URI→names reverse
map in ServerContext to make that hook cheap. That leaked OScript-specific
logic into the generic document lifecycle.

OScriptLibraryIndex already owns the .os document lifecycle (it creates and
removes them in registerEntry/handleDocumentRemoved and has its own
URI→entries index). So drive the documentsByMDORef registration from there:
ServerContext now exposes only registerOScriptLibrary(name, moduleType, doc)
and removeOScriptLibrary(name, moduleType) (catalog entry + documentsByMDORef
key together), the index calls them per entry, and the generic
createDocumentContext/removeDocument hooks and the URI→names reverse map are
gone. ServerContext keeps just the case-insensitive name catalog (the
findLibraryClass/Module canonicalizer). No behaviour change.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkYRgB6NZZRRNZmBrdL54M

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (1)

128-128: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Key the OScript catalog by both normalized name and ModuleType.

This still stores only one symbol per nameKey, so registering the same library name as both OScriptClass and OScriptModule overwrites one variant. After that, either findLibraryClass(...) or findLibraryModule(...) can start returning empty incorrectly, and removeOScriptLibrary(...) removes by name only, so it can also drop the surviving variant. A composite key like (nameKey, moduleType) or a nested map fixes both problems.

Also applies to: 493-544

🤖 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/ServerContext.java`
at line 128, The OScript library cache in ServerContext is keyed only by the
normalized name, so registering the same library as both OScriptClass and
OScriptModule overwrites one entry and makes findLibraryClass(...),
findLibraryModule(...), and removeOScriptLibrary(...) behave incorrectly. Update
oscriptLibrariesByNameKey to use a composite key or nested map that includes
both nameKey and ModuleType, and adjust the lookup and removal logic in
findLibraryClass(...), findLibraryModule(...), and removeOScriptLibrary(...) so
each variant is stored, found, and removed independently.
🤖 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.

Duplicate comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java`:
- Line 128: The OScript library cache in ServerContext is keyed only by the
normalized name, so registering the same library as both OScriptClass and
OScriptModule overwrites one entry and makes findLibraryClass(...),
findLibraryModule(...), and removeOScriptLibrary(...) behave incorrectly. Update
oscriptLibrariesByNameKey to use a composite key or nested map that includes
both nameKey and ModuleType, and adjust the lookup and removal logic in
findLibraryClass(...), findLibraryModule(...), and removeOScriptLibrary(...) so
each variant is stored, found, and removed independently.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 4f85bf7c-b0ce-41d5-a0a8-1b3fd6929358

📥 Commits

Reviewing files that changed from the base of the PR and between d6705c4 and 90637fc.

📒 Files selected for processing (2)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java

…dule

Complete the OScript mdoRef unification: a library .os document's mdoRef is
now its canonical library name (not the URI), so it is indexed in
documentsByMDORef under its own mdoRef — uniformly with BSL objects.

ServerContext.findLibraryClass/findLibraryModule now return the resolved
DocumentContext instead of the canonical name string; callers get the doc
directly and read the reference-index key from doc.getMdoRef(), dropping the
extra getDocument(mdoRef, moduleType) round-trip in ReferenceIndexFiller.

The library name is seeded into ServerContext (registerOScriptLibraryName)
before the document is added, so MdoRefBuilder resolves the mdoRef of an .os
file without an MD object to that name.

Also fix three dangling javadoc @link references to KeywordReferenceFinder,
left in context.symbol/hover/providers after the finder moved to
types.references.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01LkYRgB6NZZRRNZmBrdL54M

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java (2)

141-143: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Clear prior registrations from entriesByUri, not entriesByName.

Line 142 iterates the collapsed entriesByName map. For a .os file exported under the same name as both module and class, only one entry is removed from ServerContext, leaving the other documentsByMDORef registration stale across reindex.

Proposed fix
-    entriesByName.values().forEach(entry ->
-      serverContext.removeOScriptLibrary(entry.qualifiedName(), moduleTypeOf(entry.kind())));
+    entriesByUri.values().stream()
+      .flatMap(Collection::stream)
+      .forEach(entry ->
+        serverContext.removeOScriptLibrary(entry.qualifiedName(), moduleTypeOf(entry.kind())));
🤖 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/types/oscript/OScriptLibraryIndex.java`
around lines 141 - 143, The cleanup in OScriptLibraryIndex should remove all
prior registrations from ServerContext using entriesByUri, not the collapsed
entriesByName map. Update the reindex cleanup loop to iterate the URI-indexed
entries so both module and class registrations are cleared, and keep using
serverContext.removeOScriptLibrary with each entry’s qualifiedName() and
moduleTypeOf(entry.kind()).

428-453: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Rollback the URI→library-name binding on load failure.

Line 431 registers the canonical name before the try, but the catch only logs. If addDocument or rebuildDocument fails, ServerContext.findOScriptLibraryName(uri) remains populated for an unregistered library and putIfAbsent can block a corrected name on a later reindex.

Proposed direction
-    serverContext.registerOScriptLibraryName(uri, qualifiedName);
-
     // Добавляем .os-файл в ServerContext как обычный документ. SymbolTreeComputer,
...
     try {
+      serverContext.registerOScriptLibraryName(uri, qualifiedName);
       var dc = serverContext.addDocument(uri);
       serverContext.rebuildDocument(dc);
...
     } catch (RuntimeException e) {
+      serverContext.removeOScriptLibraryName(uri, qualifiedName);
       LOGGER.warn("Failed to load oscript library file: {}", osFile, e);
     }
🤖 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/types/oscript/OScriptLibraryIndex.java`
around lines 428 - 453, The URI-to-library-name mapping is being registered
before the load work succeeds, so a failed load leaves stale state in
ServerContext. In OScriptLibraryIndex around
registerOScriptLibraryName/addDocument/rebuildDocument, move the binding into
the successful path or explicitly undo it in the RuntimeException catch so a
failed library load does not keep findOScriptLibraryName(uri) populated. Make
sure the cleanup restores the pre-load state before logging the warning.
♻️ Duplicate comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java (1)

129-129: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Keep OneScript catalog entries keyed by both name and module type.

Line 531 still overwrites oscriptLibrariesByNameKey by nameKey only. When the same library name is exported as both <module> and <class>, the later registration hides the earlier one, so either findLibraryClass(...) or findLibraryModule(...) returns empty; line 546 can also remove the wrong symbol. This matches the earlier catalog-keying concern and still appears present in the current code.

Proposed direction
-  private final Map<String, OScriptLibrarySymbol> oscriptLibrariesByNameKey = new ConcurrentHashMap<>();
+  private final Map<OScriptLibrarySymbolKey, OScriptLibrarySymbol> oscriptLibrariesByKey = new ConcurrentHashMap<>();
...
-    oscriptLibrariesByNameKey.put(oscriptNameKey(qualifiedName), new OScriptLibrarySymbol(moduleType, documentContext));
+    var key = new OScriptLibrarySymbolKey(oscriptNameKey(qualifiedName), moduleType);
+    oscriptLibrariesByKey.put(key, new OScriptLibrarySymbol(moduleType, documentContext));
...
-    var symbol = oscriptLibrariesByNameKey.remove(oscriptNameKey(qualifiedName));
+    var symbol = oscriptLibrariesByKey.remove(new OScriptLibrarySymbolKey(oscriptNameKey(qualifiedName), moduleType));
...
-    return Optional.ofNullable(oscriptLibrariesByNameKey.get(oscriptNameKey(name)))
-      .filter(symbol -> symbol.moduleType() == moduleType)
+    return Optional.ofNullable(oscriptLibrariesByKey.get(new OScriptLibrarySymbolKey(oscriptNameKey(name), moduleType)))
       .map(OScriptLibrarySymbol::documentContext);
...
+  private record OScriptLibrarySymbolKey(String nameKey, ModuleType moduleType) {
+  }

Also applies to: 530-587

🤖 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/ServerContext.java`
at line 129, The OneScript library index in ServerContext is still keyed only by
name, so entries for the same library can overwrite each other when one is a
module and the other is a class. Update the registration and removal logic
around oscriptLibrariesByNameKey to use a composite key that includes both the
library name and module type, and make the lookup paths in findLibraryClass(...)
and findLibraryModule(...) read from that same key scheme. Also ensure the
cleanup code removes only the exact symbol that was registered so one variant
does not delete the other.
🧹 Nitpick comments (1)
src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java (1)

542-566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Rename the library-class “URI” plumbing to mdoRef.

Line 566 now stores DocumentContext::getMdoRef, but the JavaDoc, variableToLibraryClassUriMap, libClassUri, and processLibraryClass... parameters still say URI. Please align the names/docs to avoid future misuse. As per coding guidelines: use meaningful Java names and keep documentation up to date.

Also applies to: 613-620, 783-800

🤖 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/references/ReferenceIndexFiller.java`
around lines 542 - 566, Rename the library-class “URI” plumbing to mdoRef
throughout ReferenceIndexFiller: update the JavaDoc on
extractLibraryClassUriFromExpression, the variableToLibraryClassUriMap naming,
the libClassUri local/parameter names, and the processLibraryClass... method
parameters/usages so they consistently refer to mdoRef instead of URI. Also
adjust any related comments and call sites in the referenced sections to keep
the naming aligned with DocumentContext::getMdoRef and prevent misleading
documentation.

Source: Coding guidelines

🤖 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.

Outside diff comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java`:
- Around line 141-143: The cleanup in OScriptLibraryIndex should remove all
prior registrations from ServerContext using entriesByUri, not the collapsed
entriesByName map. Update the reindex cleanup loop to iterate the URI-indexed
entries so both module and class registrations are cleared, and keep using
serverContext.removeOScriptLibrary with each entry’s qualifiedName() and
moduleTypeOf(entry.kind()).
- Around line 428-453: The URI-to-library-name mapping is being registered
before the load work succeeds, so a failed load leaves stale state in
ServerContext. In OScriptLibraryIndex around
registerOScriptLibraryName/addDocument/rebuildDocument, move the binding into
the successful path or explicitly undo it in the RuntimeException catch so a
failed library load does not keep findOScriptLibraryName(uri) populated. Make
sure the cleanup restores the pre-load state before logging the warning.

---

Duplicate comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java`:
- Line 129: The OneScript library index in ServerContext is still keyed only by
name, so entries for the same library can overwrite each other when one is a
module and the other is a class. Update the registration and removal logic
around oscriptLibrariesByNameKey to use a composite key that includes both the
library name and module type, and make the lookup paths in findLibraryClass(...)
and findLibraryModule(...) read from that same key scheme. Also ensure the
cleanup code removes only the exact symbol that was registered so one variant
does not delete the other.

---

Nitpick comments:
In
`@src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java`:
- Around line 542-566: Rename the library-class “URI” plumbing to mdoRef
throughout ReferenceIndexFiller: update the JavaDoc on
extractLibraryClassUriFromExpression, the variableToLibraryClassUriMap naming,
the libClassUri local/parameter names, and the processLibraryClass... method
parameters/usages so they consistently refer to mdoRef instead of URI. Also
adjust any related comments and call sites in the referenced sections to keep
the naming aligned with DocumentContext::getMdoRef and prevent misleading
documentation.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 941ae410-841d-4a61-8671-4fda3c7c08ae

📥 Commits

Reviewing files that changed from the base of the PR and between 90637fc and 4687137.

📒 Files selected for processing (7)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/context/MdoRefBuilder.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/context/ServerContext.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/KeywordSymbol.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/KeywordSymbolMarkupContentBuilder.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/HoverProvider.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/references/ReferenceIndexFiller.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/types/oscript/OScriptLibraryIndex.java
✅ Files skipped from review due to trivial changes (3)
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/context/symbol/KeywordSymbol.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/providers/HoverProvider.java
  • src/main/java/com/github/_1c_syntax/bsl/languageserver/hover/KeywordSymbolMarkupContentBuilder.java

@sonarqubecloud

sonarqubecloud Bot commented Jul 1, 2026

Copy link
Copy Markdown

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants