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

docs/release-notes/.FSharp.Compiler.Service/11.0.100.md

@@ -68,6 +68,7 @@
 * Reference assembly MVIDs are now deterministic across compiler invocations. Previously, `--refout` / `<ProduceReferenceAssembly>true</ProduceReferenceAssembly>` produced a different MVID every build because the implied signature hash used .NET's randomized `String.GetHashCode()`. ([Issue #19751](https://github.com/dotnet/fsharp/issues/19751), [PR #19801](https://github.com/dotnet/fsharp/pull/19801))
 * Parser: recover on unfinished if and binary expressions
 ([PR #19724](https://github.com/dotnet/fsharp/pull/19724))
+* Fix spurious XmlDoc warnings (unknown parameter / no documentation for parameter) under `--warnon:3390` when a get/set property documents the full parameter set across both accessors. ([Issue #13684](https://github.com/dotnet/fsharp/issues/13684), [PR #19884](https://github.com/dotnet/fsharp/pull/19884))
 
 ### Added
 

src/Compiler/SyntaxTree/SyntaxTreeOps.fs

@@ -1149,6 +1149,21 @@ let rec desugarGetSetMembers (memberDefns: SynMemberDefns) =
                                          GetKeyword = Some mGet
                                          SetKeyword = Some mSet
                                      }) ->
+            // Each accessor's xmlDoc must validate against the union of both accessors'
+            // parameter names; otherwise documenting the full property triggers spurious
+            // 'unknown parameter' / 'no documentation for parameter' warnings on the
+            // accessor that does not own that name. See issue #13684.
+            let argNamesOf (SynBinding(valData = SynValData(valInfo = info))) = info.ArgNames
+            let getArgs = argNamesOf getBinding
+            let setArgs = argNamesOf setBinding
+
+            let rewrap extra (SynBinding(a, k, isInline, isMutable, attrs, xmlDoc, vd, hp, ri, e, mB, sp, t)) =
+                let xmlDoc' = PreXmlDoc.WithExtraParamsForCheck(xmlDoc, extra)
+                SynBinding(a, k, isInline, isMutable, attrs, xmlDoc', vd, hp, ri, e, mB, sp, t)
+
+            let getBinding = rewrap setArgs getBinding
+            let setBinding = rewrap getArgs setBinding
+
             if Position.posLt mGet.Start mSet.Start then
                 [ SynMemberDefn.Member(getBinding, m); SynMemberDefn.Member(setBinding, m) ]
             else

src/Compiler/SyntaxTree/XmlDoc.fs

@@ -215,6 +215,7 @@ type PreXmlDoc =
     | PreXmlMerge of PreXmlDoc * PreXmlDoc
     | PreXmlDoc of pos * XmlDocCollector
     | PreXmlDocEmpty
+    | PreXmlDocPairedWith of inner: PreXmlDoc * extraParamNames: string list
 
     member x.ToXmlDoc(check: bool, paramNamesOpt: string list option) =
         match x with
@@ -235,6 +236,13 @@ type PreXmlDoc =
                     doc.Check(paramNamesOpt)
 
                 doc
+        | PreXmlDocPairedWith(inner, extra) ->
+            let paramNamesOpt =
+                match paramNamesOpt with
+                | Some names -> Some(names @ extra)
+                | None -> None
+
+            inner.ToXmlDoc(check, paramNamesOpt)
 
     member x.Range =
         match x with
@@ -245,17 +253,20 @@ type PreXmlDoc =
             else unionRanges part1.Range part2.Range
         | PreXmlDocEmpty -> range0
         | PreXmlDoc(pos, collector) -> collector.LinesRange pos
+        | PreXmlDocPairedWith(inner, _) -> inner.Range
 
     member x.IsEmpty =
         match x with
         | PreXmlDirect(lines, _) -> lines |> Array.forall String.IsNullOrWhiteSpace
         | PreXmlMerge(a, b) -> a.IsEmpty && b.IsEmpty
         | PreXmlDocEmpty -> true
         | PreXmlDoc(pos, collector) -> not (collector.HasComments pos)
+        | PreXmlDocPairedWith(inner, _) -> inner.IsEmpty
 
     member x.MarkAsInvalid() =
         match x with
         | PreXmlDoc(pos, collector) -> collector.SetXmlDocValidity(pos, false)
+        | PreXmlDocPairedWith(inner, _) -> inner.MarkAsInvalid()
         | _ -> ()
 
     static member CreateFromGrabPoint(collector: XmlDocCollector, grabPointPos) =
@@ -268,6 +279,11 @@ type PreXmlDoc =
 
     static member Merge a b = PreXmlMerge(a, b)
 
+    static member WithExtraParamsForCheck(doc: PreXmlDoc, extraParamNames: string list) =
+        match extraParamNames with
+        | [] -> doc
+        | _ -> PreXmlDocPairedWith(doc, extraParamNames)
+
 [<Sealed>]
 type XmlDocumentationInfo private (tryGetXmlDocument: unit -> XmlDocument option) =
 

src/Compiler/SyntaxTree/XmlDoc.fsi

@@ -74,6 +74,11 @@ type public PreXmlDoc =
     /// Merge two PreXmlDoc
     static member Merge: a: PreXmlDoc -> b: PreXmlDoc -> PreXmlDoc
 
+    /// Wrap a PreXmlDoc with additional parameter names that should be considered valid
+    /// when the doc is checked. Used for property get/set pairs so that each accessor's
+    /// xmldoc validation sees the union of both accessors' parameter names.
+    static member WithExtraParamsForCheck: doc: PreXmlDoc * extraParamNames: string list -> PreXmlDoc
+
     /// Create a PreXmlDoc from a collection of unprocessed lines
     static member Create: unprocessedLines: string[] * range: range -> PreXmlDoc
 

tests/FSharp.Compiler.ComponentTests/Language/XmlComments.fs

@@ -257,4 +257,126 @@ module M =
         """
         |> withXmlCommentChecking
         |> compile
-        |> shouldSucceed
+        |> shouldSucceed
+
+module XmlCommentCheckingGetSetProperty =
+
+    [<Fact>]
+    let ``Fully documented get-set property produces no xmldoc warning`` () =
+        Fsx """
+type MyClass() =
+    /// <summary>A property</summary>
+    /// <param name="index">The index</param>
+    /// <param name="value">The value</param>
+    member _.Item
+        with get(index: int) = index
+        and set (index: int) (value: int) = ()
+        """
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics []
+
+    [<Fact>]
+    let ``Simple get-set property with xmldoc no false warning`` () =
+        Fsx """
+type MyClass() =
+    /// <summary>Gets or sets the value</summary>
+    /// <param name="v">The value</param>
+    member _.Prop
+        with get() = 0
+        and set (v: int) = ()
+        """
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics []
+
+    [<Fact>]
+    let ``Get-only property xmldoc check unaffected`` () =
+        Fsx """
+type MyClass() =
+    /// <summary>A property</summary>
+    /// <param name="index">The index</param>
+    member _.Item with get(index: int) = index
+        """
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics []
+
+    [<Fact>]
+    let ``Actually missing param doc still warns for get-set`` () =
+        Fsx """
+type MyClass() =
+    /// <summary>A property</summary>
+    /// <param name="index">The index</param>
+    member _.Item
+        with get(index: int) = index
+        and set (index: int) (value: int) = ()
+        """
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics
+                [ (Warning 3390, Line 3, Col 5, Line 4, Col 46,
+                   "This XML comment is incomplete: no documentation for parameter 'value'") ]
+
+    [<Fact>]
+    let ``Documented param that exists in neither accessor warns`` () =
+        Fsx """
+type MyClass() =
+    /// <summary>A property</summary>
+    /// <param name="index">The index</param>
+    /// <param name="value">The value</param>
+    /// <param name="ghost">Does not exist</param>
+    member _.Item
+        with get(index: int) = index
+        and set (index: int) (value: int) = ()
+        """
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics
+                [ (Warning 3390, Line 3, Col 5, Line 6, Col 51,
+                   "This XML comment is invalid: unknown parameter 'ghost'") ]
+
+    [<Theory>]
+    [<InlineData("int", "int")>]
+    [<InlineData("float", "float")>]
+    let ``Get-set with different types fully documented is clean`` (getType: string) (setType: string) =
+        let source =
+            "\ntype MyClass() =\n"
+            + "    /// <summary>Prop</summary>\n"
+            + "    /// <param name=\"v\">The value</param>\n"
+            + "    member _.Prop\n"
+            + "        with get() : " + getType + " = Unchecked.defaultof<_>\n"
+            + "        and set (v: " + setType + ") = ()\n        "
+        Fsx source
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics []
+
+    [<Fact>]
+    let ``Issue 13684 repro indexed property fully documented is clean`` () =
+        Fsx """
+type A =
+    /// <summary></summary>
+    /// <param name="j"></param>
+    /// <param name="k"></param>
+    /// <param name="l"></param>
+    member x.A with get (j: int) : int = 3
+               and set (k: int) (l: int) = ()
+        """
+         |> withXmlCommentChecking
+         |> ignoreWarnings
+         |> compile
+         |> shouldSucceed
+         |> withDiagnostics []

tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl

@@ -12486,6 +12486,7 @@ FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Text.Range get_Range()
 FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Xml.PreXmlDoc Create(System.String[], FSharp.Compiler.Text.Range)
 FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Xml.PreXmlDoc Empty
 FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Xml.PreXmlDoc Merge(FSharp.Compiler.Xml.PreXmlDoc, FSharp.Compiler.Xml.PreXmlDoc)
+FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Xml.PreXmlDoc WithExtraParamsForCheck(FSharp.Compiler.Xml.PreXmlDoc, Microsoft.FSharp.Collections.FSharpList`1[System.String])
 FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Xml.PreXmlDoc get_Empty()
 FSharp.Compiler.Xml.PreXmlDoc: FSharp.Compiler.Xml.XmlDoc ToXmlDoc(Boolean, Microsoft.FSharp.Core.FSharpOption`1[Microsoft.FSharp.Collections.FSharpList`1[System.String]])
 FSharp.Compiler.Xml.PreXmlDoc: Int32 GetHashCode()