Skip to content

Fix XmlDoc validation for get/set property pairs (#13684)#19884

Merged
T-Gro merged 2 commits into
mainfrom
fix/issue-13684
Jun 3, 2026
Merged

Fix XmlDoc validation for get/set property pairs (#13684)#19884
T-Gro merged 2 commits into
mainfrom
fix/issue-13684

Conversation

@T-Gro

@T-Gro T-Gro commented Jun 2, 2026

Copy link
Copy Markdown
Member

Summary

Each accessor's xmlDoc is now validated against the union of both accessors' parameter names, eliminating spurious 'unknown parameter' and 'no documentation for parameter' warnings under --warnon:3390 when a property documents the full parameter set across getter and setter.

Details

The fix adds a private PreXmlDocPairedWith DU case carrying extra parameter names, exposed via PreXmlDoc.WithExtraParamsForCheck. In desugarGetSetMembers, each accessor's xmlDoc is rewrapped with the other accessor's argument names before emitting the two SynBindings.

Fixes #13684

Each accessor's xmlDoc is now validated against the union of both
accessors' parameter names, eliminating spurious 'unknown parameter'
and 'no documentation for parameter' warnings under --warnon:3390 when
a property documents the full parameter set across getter and setter.

The fix adds a private PreXmlDocPairedWith DU case carrying extra
parameter names, exposed via PreXmlDoc.WithExtraParamsForCheck. In
desugarGetSetMembers, each accessor's xmlDoc is rewrapped with the
other accessor's argument names before emitting the two SynBindings.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@github-actions

github-actions Bot commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

❗ Release notes required


✅ Found changes and release notes in following paths:

Change path Release notes path Description
src/Compiler docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@github-actions github-actions Bot added the AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed label Jun 2, 2026
@T-Gro T-Gro requested a review from abonie June 3, 2026 10:14

@T-Gro T-Gro left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Review

Clean, well-scoped fix for issue #13684. The wrapper-DU approach is elegant and minimally invasive.

What works well

  • Minimal surface change: The PreXmlDocPairedWith case wraps the existing doc with extra param names, only affecting validation (not the produced XmlDoc content).
  • All match arms handled: ToXmlDoc, Range, IsEmpty, MarkAsInvalid all delegate correctly through the new case.
  • Guard against empty lists: WithExtraParamsForCheck short-circuits on [], avoiding unnecessary wrapping.
  • Good test coverage: Positive (no false warnings), negative (real warnings still fire), edge cases (ghost params, indexed properties), and the exact repro from issue #13684.

Observations (non-blocking)

  1. Duplicate warning deduplication: Both accessors share the same xmlDoc (captured once in ParseHelpers.fs:273). After rewrapping, both will emit a "missing param" warning at the same source range when a param is truly undocumented. The test Actually missing param doc still warns for get-set expects exactly one diagnostic, relying on the compiler's range-based warning deduplication. This is fine in practice — just noting the implicit dependency.

  2. Public API: WithExtraParamsForCheck is now part of the FCS public surface. This is appropriate since PreXmlDoc is already public and the method has clear utility for tooling that synthesizes or transforms bindings.

  3. paramNamesOpt = None passthrough: When paramNamesOpt is None (checking disabled), the extra names are correctly ignored — no-op behavior preserved.

Overall: solid fix, good tests, clean implementation.

@T-Gro T-Gro added the AI-reviewed PR reviewed by AI review council label Jun 3, 2026
@T-Gro T-Gro enabled auto-merge (squash) June 3, 2026 13:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI-reviewed PR reviewed by AI review council AI-Tooling-Check-Bypassed Tooling check: non-fork PR, not diff-analyzed

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Inconsistent XmlDoc parameters analysis for properties with accessors

2 participants