Fix link resolution in symbols converter#4574
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes doc-comment link resolution in ZeroC.Slice.Symbols by deferring link resolution until after all symbols have been converted and cached, avoiding missed forward/self/cyclic references in doc comments.
Changes:
- Reworked doc-comment conversion to record links as
UnresolvedCommentLinkduring symbol conversion and resolve them in a second pass once_cacheis fully populated. - Added recursive traversal to resolve comments across nested entities (operations/fields/variants/enumerators).
- Introduced small supporting API/internal changes (mutable
Entity.Commentinternally;BasicEnum.EnumeratorEntitiesfor traversal; minor refactors/collection expressions).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/ZeroC.Slice.Symbols/SymbolConverter.cs | Implements two-phase doc-comment link handling and resolves links after symbol table population. |
| src/ZeroC.Slice.Symbols/Entity.cs | Allows internal post-construction updates of Comment so links can be resolved in phase 2. |
| src/ZeroC.Slice.Symbols/BasicEnum.cs | Exposes enumerators as entities internally so comment resolution can traverse and fix enumerator comments. |
| // Phase 1: convert all named symbols in dependency order. Doc-comment links are recorded as | ||
| // UnresolvedCommentLink and resolved in phase 2 — eager resolution here would miss any reference | ||
| // that targets a symbol not yet in _cache (forward references, self-references, or cycles between | ||
| // entities that aren't related by type dependencies). | ||
| foreach (string typeId in TopologicalSort()) | ||
| { | ||
| (Compiler.SliceFile file, Compiler.Symbol symbol) = _named[typeId]; | ||
| Module module = ConvertModule(file.ModuleDeclaration); | ||
| _cache[typeId] = ConvertSymbol(symbol, file, module); | ||
| } | ||
|
|
||
| // Phase 2: resolve doc-comment links now that the symbol table is fully populated. | ||
| foreach (ISymbol symbol in _cache.Values) | ||
| { | ||
| if (symbol is Entity entity) | ||
| { | ||
| ResolveEntityComments(entity); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new two-phase doc-comment link resolution logic (record unresolved links during conversion, then resolve after the symbol table is populated) doesn’t appear to have any regression tests that validate resolved vs unresolved links (e.g., forward/self/cyclic links, and links to sub-entities like operations/fields/enumerators). Consider adding a generator/symbols test that asserts these links become ResolvedCommentLink where appropriate so future refactors don’t silently revert to unresolved links.
InsertCreativityHere
left a comment
InsertCreativityHere
left a comment
There was a problem hiding this comment.
All looks good to me!
This PR reworks how doc comment links are handled in the symbols converter. We cannot resolve links until all symbols are converted, the eager resolution assume that there were not circular dependencies which is true for Slice definitions but not for links in the doc copmments.