Fix invalid C# syntax and dead links for ref/out/in parameters in rendered docs - #42
Conversation
…dered docs Root cause: DotNetEmitter.ToXmlDocTypeName (fixed in #40) only addressed the coverage-checker's XML-doc-ID matching for byref parameters. The separate doc-rendering path never handled Mono.Cecil's ByReferenceType: - TypeNameSimplifier.SimplifyCore had no case for ByReferenceType, so it fell through to the default arm and preserved Cecil's raw trailing "&" in displayed type names (e.g. "NaturalLanguageAudioTag&"). - TypeLinkResolver.Linkify had no ByReferenceType handling either, so IsIntraAssembly/GetTypePageKey computed link targets from that same "&"-suffixed name, producing dead links (e.g. "../NaturalLanguageAudioTag&.md", which never exists). - None of DotNetEmitter's signature builders (BuildMethodSignature, BuildMethodDisplayName, BuildOperatorSignature, BuildDelegateSignature) ever emitted the out/ref/in keyword, so a byref parameter rendered as bare "NaturalLanguageAudioTag&" instead of valid C# ("out NaturalLanguageAudioTag"). Fix: - TypeNameSimplifier.SimplifyCore: added a byref-unwrapping rule that simplifies a ByReferenceType by recursing into its ElementType, stripping the trailing "&" from displayed type names. - TypeLinkResolver.Linkify: added the same ByReferenceType unwrap before computing link text/href, so both use the un-suffixed type name and link to the real page. - DotNetEmitter: added GetRefKindKeyword(ParameterDefinition) returning "out ", "in ", "ref ", or "" based on ParameterType being a ByReferenceType plus the parameter's IsOut/IsIn flags. Prepended this keyword to the parameter's simplified type name in BuildMethodSignature, BuildMethodDisplayName, BuildOperatorSignature, and BuildDelegateSignature. - DotNetEmitterGradualDisclosure and DotNetEmitterSingleFile: parameter table rows now prepend GetRefKindKeyword before the linkified type in the "Type" column, so out/ref/in is visible there too. Added test/ApiMark.DotNet.Fixtures/ByRefParameterClass.cs (with a new ByRefTargetClass fixture type, to avoid colliding with the SampleClass substring match used in an existing heading-detection test) covering out/ref/in parameter methods, plus targeted unit tests for TypeNameSimplifier, TypeLinkResolver, and DotNetEmitter's signature/ display-name builders. Verification: - Full solution test suite passes on net8.0/net9.0/net10.0 (all projects, including ApiMark.Cpp.Tests and ApiMark.MSBuild.PackageTests with clang available). - End-to-end repro: built a scratch assembly with AudioTagCatalog.TryResolve(string, out T, out U) matching the bug report. Confirmed the pre-fix build renders "TryResolve(string, NaturalLanguageAudioTag&, ...)" with dead links "../NaturalLanguageAudioTag&.md"; confirmed the fixed build renders "TryResolve(string, out NaturalLanguageAudioTag, ...)" with working links to the real "NaturalLanguageAudioTag.md" pages. - pwsh ./fix.ps1 and pwsh ./lint.ps1 both run clean (0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The new ByReferenceType unwrapping can cause ref-return methods to render missing the required ref keyword unless return-type handling is updated accordingly.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes documentation rendering for C# ref/out/in parameters by unwrapping Mono.Cecil ByReferenceType in display/link logic and emitting the correct ref-kind keywords in rendered signatures and parameter tables, preventing invalid C# (&) and dead links in generated Markdown.
Changes:
- Unwrap
ByReferenceTypeinTypeNameSimplifierandTypeLinkResolverso displayed type names and link targets don’t include Cecil’s trailing&. - Emit
out/in/refkeywords for byref parameters across signature builders and parameter tables. - Add fixtures and targeted unit tests covering byref parameter rendering and linkification.
File summaries
| File | Description |
|---|---|
| test/ApiMark.DotNet.Tests/TypeNameSimplifierTests.cs | Adds a unit test ensuring byref types simplify to the element type name (no &). |
| test/ApiMark.DotNet.Tests/TypeLinkResolverTests.cs | Adds a unit test ensuring byref types linkify to the real page (no & in text/href). |
| test/ApiMark.DotNet.Tests/DotNetEmitterTests.cs | Adds tests ensuring signatures/display names render out/ref/in keywords and never &. |
| test/ApiMark.DotNet.Fixtures/ByRefParameterClass.cs | Introduces fixture types/methods for out/ref/in parameter scenarios. |
| src/ApiMark.DotNet/TypeNameSimplifier.cs | Adds ByReferenceType unwrapping rule to prevent & in simplified names. |
| src/ApiMark.DotNet/TypeLinkResolver.cs | Adds ByReferenceType unwrapping so links are computed from the element type name. |
| src/ApiMark.DotNet/DotNetEmitterSingleFile.cs | Prepends ref-kind keyword in parameter type table cells for single-file output. |
| src/ApiMark.DotNet/DotNetEmitterGradualDisclosure.cs | Prepends ref-kind keyword in parameter type table cells for gradual-disclosure output. |
| src/ApiMark.DotNet/DotNetEmitter.cs | Adds GetRefKindKeyword and uses it in signature/display-name builders for byref parameters. |
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…omment Two issues raised by the automated PR review of #42: 1. Ref-return methods now render missing the required `ref` keyword. Now that TypeNameSimplifier unwraps ByReferenceType uniformly (needed to fix byref parameter rendering), a method declared as `public ref T GetByRef()` would simplify its return type to plain `T` with no indication it returns by reference. Added DotNetEmitter.GetReturnRefKeyword(TypeReference), which returns "ref " when the (unsimplified) return type is a ByReferenceType, and used it in BuildMethodSignature and BuildDelegateSignature ahead of the simplified return type. (BuildOperatorSignature is unaffected: C# operator overloads cannot declare a ref return. BuildMethodDisplayName is unaffected: it only lists parameter types, never the return type.) 2. TypeNameSimplifier's SimplifyCore doc comment said "Applies Rules 1-6" but the method now includes Rule 0 (byref unwrapping added earlier in this PR). Corrected to "Rules 0-6". Added a GetByRef() ref-returning method to the ByRefParameterClass test fixture, plus tests for BuildMethodSignature rendering `ref` before the return type and for GetReturnRefKeyword returning an empty string for an ordinary by-value return. Verification: - Full solution test suite passes on net8.0/net9.0/net10.0, all projects (including ApiMark.Cpp.Tests and ApiMark.MSBuild.PackageTests with clang available). - pwsh ./fix.ps1 and pwsh ./lint.ps1 both run clean (0 errors). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
🟢 Approval recommended
The changes are localized, align with Cecil’s byref model, and are backed by targeted fixture-based unit tests covering the corrected rendering paths.
Review details
- Files reviewed: 9/9 changed files
- Comments generated: 0 new
- Review effort level: Lite
Summary
Fixes a rendering bug remaining after #40: although the coverage-checker
false-positive on
ref/out/inparameters was fixed by translatingMono.Cecil's byref marker in
ToXmlDocTypeName, the doc-rendering pathnever handled byref parameters at all, so generated pages for such methods
still showed invalid C# and dead links.
Repro (before fix)
For a method like
TryResolve(string, out NaturalLanguageAudioTag, out NaturalLanguageAudioTagKind):outkeyword, and&is not legal in a type reference.NaturalLanguageAudioTag&.mdnever exists; the real page isNaturalLanguageAudioTag.md.Root cause
Three separate rendering code paths never handled Mono.Cecil's
ByReferenceType:TypeNameSimplifier.SimplifyCorehad no case for it, so it fell to thedefaultarm and preserved the raw trailing&.TypeLinkResolver.Linkifyhad no case for it either, so link targets werecomputed from that same
&-suffixed name.DotNetEmitter's signature builders (BuildMethodSignature,BuildMethodDisplayName,BuildOperatorSignature,BuildDelegateSignature)ever emitted the
out/ref/inkeyword.Fix
TypeNameSimplifier.SimplifyCore: added a byref-unwrapping rule thatsimplifies a
ByReferenceTypeby recursing into itsElementType.TypeLinkResolver.Linkify: added the same unwrap so link text/href use theun-suffixed type name and link to the real page.
DotNetEmitter: addedGetRefKindKeyword(ParameterDefinition)returning"out ","in ","ref ", or""based onIsOut/IsIn. Prepended it tothe parameter type in all four signature builders.
DotNetEmitterGradualDisclosure/DotNetEmitterSingleFile: parametertable rows now prepend the ref-kind keyword before the linkified type.
After fix
Testing
test/ApiMark.DotNet.Fixtures/ByRefParameterClass.cs(with adedicated
ByRefTargetClassfixture type, to avoid a substring collisionwith
SampleClassin an existing heading-detection test) coveringout/ref/inparameter methods.TypeNameSimplifier,TypeLinkResolver, andDotNetEmitter's signature/display-name builders.(including
ApiMark.Cpp.TestsandApiMark.MSBuild.PackageTests).confirmed the pre-fix build reproduces the exact invalid-C#/dead-link
output, and confirmed the fixed build produces valid C# and working links.
pwsh ./fix.ps1andpwsh ./lint.ps1both run clean.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com