Repository navigation
Fix false-positive --enforce-docs on byref (ref/out/in) parameters - #40
Merged
Merged
Conversation
Mono.Cecil encodes byref parameter types with a trailing "&" in TypeReference.FullName, but the XML doc ID convention uses a trailing "@" instead. ToXmlDocTypeName had no handling for "&", so it built an XML doc ID that never matched the compiler-generated XML, causing XmlDocReader.GetSummary to return null and the member to be reported as [Undocumented] even when fully documented. ToXmlDocTypeName now strips a trailing "&" before transforming the type name and appends "@" to the result, matching the C# compiler's XML doc ID encoding for ref/out/in parameters. Added test cases covering simple and generic byref parameter types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The byref handling change is localized and covered by new unit tests, with only a minor optional test-name clarity nit.
Pull request overview
Fixes a --enforce-docs false positive in ApiMark’s .NET XML-doc ID generation by correctly translating Mono.Cecil byref (ref/out/in) parameter type names from trailing & to the XML-doc trailing @, allowing documented members to be matched and not incorrectly reported as [Undocumented].
Changes:
- Update
DotNetEmitter.ToXmlDocTypeNameto detect a trailing&, strip it for the existing normalization pass, then append@. - Add unit test cases verifying
&→@conversion for simple and generic byref types.
File summaries
| File | Description |
|---|---|
| src/ApiMark.DotNet/DotNetEmitter.cs | Adds byref-aware handling in ToXmlDocTypeName to align Cecil type names with XML doc ID conventions. |
| test/ApiMark.DotNet.Tests/DotNetEmitterTests.cs | Extends the existing theory data to cover byref parameter type encodings, including a generic case. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Renamed DotNetEmitter_ToXmlDocTypeName_ConvertsGenericNotation to DotNetEmitter_ToXmlDocTypeName_ConvertsCecilEncodingToXmlDocId since it now also covers nested-type separators and byref (ref/out/in) parameter encoding, not just generic notation. Updated the matching verification doc entry to reference the renamed test and describe the byref coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The change is small and targeted, includes unit tests for the new behavior, and the updated implementation matches the documented XML doc ID encoding rules for byref parameters.
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0 new
- Review effort level: Lite
The CI Quality Checks build failed on this branch because the term 'byref' (used in the verification doc and code comments describing Cecil/XML-doc-ID byref parameter encoding) was not recognized by cspell. Adding it as a legitimate technical term. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The fix is localized, matches the documented XML doc ID convention, and is covered by targeted unit tests and updated verification documentation.
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0 new
- Review effort level: Lite
Malcolmnixon
added a commit
that referenced
this pull request
Sep 7, 2026
…dered docs (#42) * Fix invalid C# syntax and dead links for ref/out/in parameters in rendered 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> * Address Copilot review: handle ref-returning methods, fix stale doc comment 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> --------- Co-authored-by: Malcolm Nixon <Malcolm.Nixon@hiarc.inc> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This was referenced Sep 14, 2026
This was referenced Sep 21, 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.
Summary
Fixes a false-positive
--enforce-docsreporting on any public method withref/out/inparameters (e.g.TryResolve(string, out T)patterns).Root Cause
Mono.Cecil represents byref parameter types with a trailing
&inTypeReference.FullName(e.g.System.String&), but the C# compiler's XML doc ID convention uses a trailing@instead (e.g.System.String@).DotNetEmitter.ToXmlDocTypeNamehad no handling for&, so the built XML doc ID never matched the compiler-generated XML doc file, causingXmlDocReader.GetSummaryto return null and the member to be reported as[Undocumented]even when fully documented.Fix
ToXmlDocTypeNamenow detects a trailing&up front, strips it before running the existing transformation, and appends@to the result (byref only ever appears as the outermost modifier).Verification
public bool TryResolve(string, out int)method, ran--enforce-docs Public --enforce-docs-severity Erroronmainand observed the false positive; re-ran with this fix applied and confirmed it no longer flags the method.ApiMark.DotNet.Testssuite passes (283 tests, net8.0/net9.0/net10.0).fix.ps1applied cleanly.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com