Repository navigation
Render type-only cref references as inline code, matching member crefs - #54
Merged
Merged
Conversation
… crefs A see-cref reference to a type (e.g. ArgumentValidator) rendered as plain prose, while a see-cref to one of its members right next to it rendered as code -- an inconsistency reported against ToolRegistry.ExecuteAsync. - FormatCref's type branch is no longer forced to the escaped, never-wrapped form; it now participates in the same forCodeSpan/wrap decision as member crefs, so a type-only reference is wrapped in a backtick code span (with raw, unescaped angle brackets for generics) the same way a member reference already is. Renamed the FormatCref/FormatMemberReference tuple field from IsMemberReference to ShouldWrapInCodeSpan to reflect the broadened semantics (constructors remain the only unwrapped case). - Updated/renamed the two existing tests whose expectations assumed type-only crefs stayed unwrapped, and added a dedicated regression test for the plain (non-generic) type-cref case from the report. - Amended the xml-doc-reader verification/reqstream docs for the renamed and new tests. True cross-reference links (to the target's page) were considered as a further enhancement, but require threading per-call link-resolution context (current page folder, resolver) into XmlDocReader, which has no such context today; the inline-code-span fix addresses the reported inconsistency without that larger architectural change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Documentation artifacts still contain contradictory or outdated cref behavior and naming.
2 open findings
What changed in this PR
Updates .NET XML documentation rendering so type-only cref references use inline code spans consistently with member references.
Changes:
- Wraps type-only crefs in code spans, including generic types.
- Updates implementation and tuple naming.
- Adds and adjusts regression tests.
- Updates verification and requirements documentation.
| File | Summary |
|---|---|
test/ApiMark.DotNet.Tests/XmlDocReaderTests.cs |
Tests plain and generic type-cref rendering. |
test/ApiMark.DotNet.Tests/DotNetGeneratorTests.cs |
Updates generated Markdown expectations. |
src/ApiMark.DotNet/XmlDocReader.cs |
Implements broadened cref wrapping behavior. |
docs/verification/api-mark-dot-net/xml-doc-reader.md |
Documents revised verification scenarios. |
docs/reqstream/api-mark-dot-net/xml-doc-reader.yaml |
Updates the rendering requirement and test list. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Addresses code-review and formal-review findings on PR #54: GetInlineReferenceText's XML doc comment and the design document both still described the pre-fix behavior (type-only crefs left unwrapped, old IsMemberReference field name).
Malcolmnixon
pushed a commit
that referenced
this pull request
Oct 8, 2026
The verification checklist still claimed type-only crefs were unwrapped, and two design-doc passages still pointed at the old Member-cref inline code span wrapping section name or excluded type-only crefs from the wrap condition. These were flagged in PR #54 review comments but missed by that PR's fix commit.
Malcolmnixon
added a commit
that referenced
this pull request
Oct 8, 2026
…ks (#55) * Extract GetCandidateMembers from DotNetEmitter.GetVisibleMembers Behavior-preserving refactor: splits the structural member-eligibility filtering (special-name accessor exclusion, compiler-generated exclusion, value__ field exclusion) out of GetVisibleMembers into a new static GetCandidateMembers helper, with GetVisibleMembers now simply applying ShouldIncludeMember on top of it. This is preparation for the upcoming BuildMemberPageIndex function (full cref cross-reference linking work), which needs the same structural member-eligibility rules under a caller-supplied visibility predicate instead of the instance-bound ShouldIncludeMember, without duplicating the filtering rules in a second, drift-prone copy. No behavior change: full existing ApiMark.DotNet.Tests suite (404 tests) passes unchanged. * Add CrefTargetResolver, link helpers, and member page index wiring - Add CrefTargetResolver: visibility-agnostic index resolving a cref XML-doc-ID string to the TypeDefinition/IMemberDefinition that defines it within a single assembly, built once from AssemblyDefinition.MainModule.GetTypes(). Used later to determine whether a <see cref>/<seealso cref> target exists in-assembly at all (external types/members simply are not found). - Add TypeLinkResolver.LinkifyResolvedType/LinkifyResolvedMember, reusing the existing GetTypePageKey/ComputeRelativePath helpers so resolved cref targets can be rendered as real relative Markdown links consistent with existing intra-assembly type links. - Add DotNetEmitterGradualDisclosure.BuildMemberPageIndex: a pure function that reproduces the existing member grouping/page-naming decisions (overload groups, operator page, case-collision groups, single-member pages) for every emitted member of a type, returning {member XML-doc-ID -> relative page path}. This is an independently written duplicate of the grouping logic in ProcessTypeMembers rather than a true single-source refactor (scope reduction, see planning-full-cref-linking-a1f3e9.md); mitigated by a dedicated cross-check test to be added against real written page paths. - Wire CrefTargetResolver and the merged MemberPageIndex/ EmittedTypeIds into DotNetAstModel/DotNetGenerator.Parse: CrefTargetResolver is built eagerly in the DotNetAstModel constructor (needs only the assembly); MemberPageIndex and EmittedTypeIds are computed after DotNetEmitter construction (they need its visibility predicates) via new BuildCrefLinkIndices/ CollectTypeCrefLinkIndex helpers and populated through a new DotNetAstModel.SetMemberPageIndex setter, preserving the existing one-pass Parse contract. No behavior change: these additions are not yet consulted by any rendering code path. * Thread optional CrefLinkContext through XmlDocReader rendering chain Add a new internal CrefLinkContext record bundling everything needed to resolve and linkify a <see cref>/<seealso cref> target: the CrefTargetResolver and TypeLinkResolver for the current assembly, the merged member page index, delegates to test whether a resolved type/ member is actually emitted, and the current page's folder (needed to compute a relative path to the link target). Thread a new optional, trailing, nullable linkContext parameter through the entire XmlDocReader rendering chain: GetDocumentationText/GetSingleLineDocumentationText -> AppendNodeText (both overloads) -> AppendElementText -> list rendering helpers (AppendListText/AppendInlineMarkerList/AppendMarkerList/ AppendTableList/RenderTermDescription/RenderInlineElement) -> GetInlineReferenceText. Because XmlDocReader is a public type, its public GetSummary/ GetSummaryMarkdown/GetRemarks/GetParams/GetReturns/GetExampleParts methods cannot directly expose an internal CrefLinkContext parameter (CS0051). Each is split into its existing public single-argument overload (behavior unchanged, delegates to the new overload with linkContext: null) plus a new internal two-argument overload that accepts the context. GetExample is deliberately left without an internal overload: it returns the raw element value without going through AppendNodeText/AppendElementText, so threading a link context through it would be a no-op; this is documented in its XML remarks. GetExceptions/GetExceptionDetails are explicitly out of scope per the approved plan and are untouched. In GetInlineReferenceText, a new TryLinkifyCref helper implements the fallback semantics: when linkContext is null, or the cref cannot be resolved via CrefTargetResolver.TryResolveType/TryResolveMember, or the resolved symbol's IsTypeEmitted/IsMemberEmitted delegate returns false, the existing plain-text/code-span behavior is preserved unchanged. Otherwise the resolved target is linkified via TypeLinkResolver.LinkifyResolvedType/LinkifyResolvedMember using the existing display text, and the resulting Markdown link is wrapped in the same code span the plain text would have been wrapped in. All ~100 existing call sites across both emitters and XmlDocReaderTests.cs continue to call the public overloads unchanged (passing no link context), so existing behavior for both single-file and gradual-disclosure modes is unaffected by this commit; CrefLinkContext is not yet constructed or passed by either emitter. Also disambiguates now-ambiguous <see cref> references in XmlDocReader.cs's own doc comments and in DocumentationCoverageChecker.cs after the overload split. Full existing ApiMark.DotNet.Tests suite (404 tests) passes unchanged. * Add unit tests for CrefTargetResolver, link helpers, and link context - Add CrefTargetResolverTests.cs: covers resolving top-level and nested types, methods and fields, resolving a private member (confirms no visibility filtering), and failing to resolve unknown identifiers, external-assembly types, and malformed/unresolvable cref strings. - Extend TypeLinkResolverTests.cs with cases for LinkifyResolvedType/LinkifyResolvedMember: link generation enabled vs. disabled, and a member found vs. not found in the supplied member page index. - Extend XmlDocReaderTests.cs with a dedicated CrefLinkContext region covering every fallback/success path from the approved plan, exercised directly against the fixture assembly (no full emitter run required): null link context (baseline unchanged), a resolved and emitted type, a resolved and emitted member, a resolved member whose IsMemberEmitted delegate returns false (filtered out), an external framework type, a malformed cref, a constructor cref (never linkified, matching existing precedent), and a seealso cref sharing the same resolution path as see. All 426 tests pass (404 pre-existing + 22 new) on net8.0. * Wire CrefLinkContext through the gradual-disclosure emitter Thread CrefLinkContext into every call site in DotNetEmitterGradualDisclosure.cs that reads XmlDocReader Get* documentation text (GetSummary/GetSummaryMarkdown/GetRemarks/ GetReturns/GetParams/GetExampleParts), so <see cref>/<seealso cref> references now render as real relative Markdown links when their target is an intra-assembly type or member that will actually be emitted, and keep today's code-span-only fallback otherwise. - DotNetAstModel.cs: add a required CrefLinkContext LinkContext parameter to TypePageWriteContext and MethodDocContext so page writers carry a folder-scoped link context without needing access to the emitter instance. - DotNetEmitterGradualDisclosure.cs: add BuildCrefLinkContext(folder) to construct a CrefLinkContext bound to the model's CrefTargets, Resolver, and MemberPageIndex plus IsTypeEmitted/IsMemberEmitted delegates; wire it through WriteNamespacePage, WriteTypeHeaderSections, WriteNestedTypesSection, ProcessSingleMember, ProcessOverloadGroup, ProcessCollisionMember, WriteMemberPage, WriteMethodOverloadPage, WriteTypeOperatorsPage, WriteCombinedMemberPage, WriteNonMethodMemberContent, and WriteMethodDocumentation. Page writers at a deeper folder than their caller derive a re-scoped context via `linkContext with { CurrentFolder = ... }` rather than needing the emitter instance passed in. - DotNetEmitterSingleFile.cs is intentionally untouched: single-file mode never supplies a CrefLinkContext, so it keeps rendering crefs exactly as it does today (code-span-only, never linked). Tests: - test/ApiMark.DotNet.Fixtures/CrefLinkingClass.cs: new fixture exercising every cref resolution outcome — a <see cref> to a visible in-assembly type, a <see cref> to a visible in-assembly member, a <see cref> to a member filtered out by visibility, a <see cref> to an external framework type, a malformed/unresolvable cref string, and a <seealso cref> to a visible type (nested in <remarks>, since top-level <seealso> siblings of <summary> are not read by this pipeline). - DotNetEmitterGradualDisclosureTests.cs: new end-to-end tests proving each of the above renders correctly in generated gradual-disclosure output, plus a test asserting every DotNetAstModel.MemberPageIndex entry corresponds to an actually-written page (mitigating the BuildMemberPageIndex duplicated-logic scope reduction). - DotNetEmitterSingleFileTests.cs: new regression test proving the same cref still renders as plain code-span text with no link in single-file mode. - DotNetGeneratorTests.cs: updated DotNetGenerator_Generate_InlineReferences_PreserveSymbolNamesInMarkdown expectations, since the SampleStatus/SampleStatus.Active/ SampleStatus.Pending crefs it asserts on are now resolvable intra-assembly symbols and correctly render as real links under the new feature. * Add DotNetAstModel tests for CrefTargets and MemberPageIndex Cover the new DotNetAstModel.CrefTargets, MemberPageIndex, and EmittedTypeIds members added for cross-reference linking: - CrefTargets is constructed eagerly during DotNetGenerator.Parse and can resolve a known fixture type by its XML-doc cref ID. - MemberPageIndex and EmittedTypeIds are populated by the gradual-disclosure emitter's post-construction setup step and contain entries for known visible fixture symbols. * Add companion documentation for cref cross-reference linking Update design, verification, and reqstream docs for DotNetAstModel, DotNetGenerator, DotNetEmitterGradualDisclosure, DotNetEmitterSingleFile, TypeLinkResolver, and XmlDocReader to cover the new cref cross-reference linking feature. Add a new CrefTargetResolver unit's design, verification, and reqstream docs. Register the new unit and requirement IDs in docs/reqstream/api-mark-dot-net.yaml and requirements.yaml. Add a new ApiMark-DotNet-CrefTargetResolver entry to .reviewmark.yaml and extend the existing XmlDocReader entry to cover the new CrefLinkContext.cs file. Update the subsystem-level docs/design/api-mark-dot-net.md with the new unit, its dependency edges, and a cref-linking data-flow step. * Fix remaining stale cref doc references flagged on PR #54 The verification checklist still claimed type-only crefs were unwrapped, and two design-doc passages still pointed at the old Member-cref inline code span wrapping section name or excluded type-only crefs from the wrap condition. These were flagged in PR #54 review comments but missed by that PR's fix commit. * Fix critical cref-link nesting bug and address review findings Critical fix: GetInlineReferenceText/TryLinkifyCref were composing `[Type.Member](path.md)` (a Markdown link nested inside a code span, which CommonMark renders literally) instead of [`Type.Member`](path.md) (code span inside the link label). This made the cref cross-reference linking feature produce zero working links despite the existing tests passing, because those tests asserted the broken shape as correct. Fixed the implementation, all affected test assertions, and the design docs that described the broken nesting. Also addresses findings from the code-review and formal-review agents run across the affected review-sets: - Corrected design docs describing public XmlDocReader methods as threading CrefLinkContext directly; the actual change added internal overloads and left the public API surface unchanged. - Reworded several HOW-oriented requirement titles (method/parameter names, refactor mandates) to state only observable behavior, moving implementation detail into their justification fields. - Documented DotNetAstModelArgs (the constructor parameter-bundling record) in the DotNetAstModel design doc, including its Data Model entry and corrected constructor description. - Documented GetCandidateMembers in the DotNetEmitter design doc as the structural superset consumed by both GetVisibleMembers and DotNetEmitterGradualDisclosure.BuildMemberPageIndex. - Documented that DotNetEmitterSingleFile always uses parameterless XmlDocReader overloads so cref references never link in single-file output. - Added missing file-to-unit map entries (CrefTargetResolver, DocumentationCoverageChecker) to the DotNetGenerator design doc. - Added Acceptance Criteria and Test Scenario coverage for cref cross-reference linking to the api-mark-dot-net verification doc. - Added a "Cross-Reference Linking" subsection to the dotnet user guide describing the new linking behavior and its single-file limitation. Validation: dotnet build (0 warnings/errors); ApiMark.DotNet.Tests 436/436 passing on net8.0/net9.0/net10.0; ApiMark.Cpp.Tests 195/195 passing (with clang on PATH); fix.ps1 and lint.ps1 clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix GitHub AI review findings and formal-review follow-ups for cref linking GitHub AI review fixes: - Extract shared GroupMembersByFileName/GetOrderedOverloads/ DecideGroupPageFileName helpers in DotNetEmitterGradualDisclosure so the cref-link index builder and the page writer can no longer silently drift out of sync on member grouping/overload-ordering/page-naming decisions. - Apply CrefLinkContext to namespace-level documentation: NamespaceDescription now stores the winning carrier's summary/remarks/example member IDs instead of pre-rendered text, deferring rendering to WriteNamespacePage once a folder-scoped CrefLinkContext is available, so <see cref>/<seealso cref> in a NamespaceDoc carrier's remarks/summary/example now link like type/member summaries do. DotNetEmitterSingleFile keeps passing no CrefLinkContext, preserving its no-links-in-single-file behavior. - Add the missing SysML2 model entry for CrefTargetResolver, mirroring the existing TypeLinkResolver pattern. Formal-review follow-ups: - Correct stale design/verification/reqstream documentation that still referenced a non-existent WriteMembersSection function, an inverted cref-link nesting description, and stale NamespaceDescription field names. - Document the (pre-existing, already-fixed) unconditional WriteNestedTypesSection call and its rationale in the gradual-disclosure design doc, and add verification/reqstream coverage for the memberless-container-type and generic-method-arity regression tests that already existed but weren't documented. - Add a single-file-mode regression test confirming a NamespaceDoc remarks cref also renders as plain code-span text with no link, matching the type/member behavior. - Remove a duplicate CrefLinkContext construction in WriteNamespacePage. Other fixes: - Exclude the gitignored artifacts/ build-output folder from markdownlint-cli2 scanning; it was never part of any PR diff but was still being linted locally. - Add "Memberless" and "desync" to the cspell word list for new test/comment identifiers. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix formal-review findings on DotNetGenerator and CrefTargetResolver review-sets - Relocate DotNetGenerator_BuildMethodIdFromReference_ExplicitGenericInterfaceOverride_MatchesInterfaceMethodId from CrefTargetResolverTests.cs to DotNetGeneratorTests.cs (it exercises DotNetGenerator, not CrefTargetResolver) - Add ExplicitIndexerClass fixture and widen MapAccessorReferenceToPropertyId to internal (with rationale remarks, matching existing convention) to cover the indexer parameter-list handling for explicit accessor overrides - Add remarks rationale to BuildMethodIdFromReference's internal visibility - Correct cref-target-resolver.md verification doc (remove misattributed acceptance criteria/test scenario) and dot-net-generator.md verification doc (add acceptance criteria/test scenarios for the new/relocated tests) - Fix stale "unmodified" wording in cref-target-resolver.md design doc now that BuildMemberId/BuildTypeId formatting is reused as-is rather than duplicated - Link reqstream requirements to the new/moved tests All findings from the DotNetGenerator and CrefTargetResolver formal reviews are resolved; both review-sets are now Approved. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Resolve cref links for explicit-label see/seealso references - GetInlineReferenceText now resolves and links the cref attribute even when the <see>/<seealso> element has explicit display text, wrapping that explicit label in the target link instead of leaving it unlinked. Falls back to the unlinked label exactly as before when there is no link context or the cref does not resolve. - Add 3 unit tests covering the resolved/linked, null-context, and unresolved-cref fallback cases for the explicit-label path. - Update xml-doc-reader.md design/verification docs and reqstream test links to reflect the new behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Never link a labeled constructor cref in GetInlineReferenceText - The explicit-label branch now checks FormatCref's ShouldWrapInCodeSpan classification before calling TryLinkifyCref, so a constructor cref (M:...#ctor) is never linked even when the <see>/<seealso> element has an explicit display label - matching the no-label branch's existing precedent that constructor crefs are never linked. - Add a regression test (XmlDocReader_GetSummary_ExplicitLabelConstructorCref_RendersLabelOnlyNoLink) covering a labeled constructor cref whose declaring type is otherwise resolvable and emitted. - Update xml-doc-reader.md design/verification docs and reqstream test links accordingly. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Malcolm Nixon <Malcolm.Nixon@hiarc.inc> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Oct 12, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes a reported inconsistency: a see-cref reference to a type (e.g. ArgumentValidator) rendered as plain prose, while a see-cref to one of its members right next to it (e.g. ITool.Parameters) rendered as inline code.
Change
Not done (follow-up candidate)
Actual cross-reference links to the target's page (the 'better' option raised in the report) would require threading per-call link-resolution context (current page folder, TypeLinkResolver) into XmlDocReader, which has no such context today -- a larger architectural change than this fix. Happy to pick that up separately if wanted.