diff --git a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md b/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md index ff1c9aae2dd..cebbf1bf563 100644 --- a/docs/release-notes/.FSharp.Compiler.Service/11.0.100.md +++ b/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` / `true` 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 diff --git a/src/Compiler/SyntaxTree/SyntaxTreeOps.fs b/src/Compiler/SyntaxTree/SyntaxTreeOps.fs index da070557119..1fb56dd697d 100644 --- a/src/Compiler/SyntaxTree/SyntaxTreeOps.fs +++ b/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 diff --git a/src/Compiler/SyntaxTree/XmlDoc.fs b/src/Compiler/SyntaxTree/XmlDoc.fs index ed189aba1a5..3207b89905c 100644 --- a/src/Compiler/SyntaxTree/XmlDoc.fs +++ b/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,6 +253,7 @@ 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 @@ -252,10 +261,12 @@ type PreXmlDoc = | 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) + [] type XmlDocumentationInfo private (tryGetXmlDocument: unit -> XmlDocument option) = diff --git a/src/Compiler/SyntaxTree/XmlDoc.fsi b/src/Compiler/SyntaxTree/XmlDoc.fsi index 561436cfa93..c7ad8d3cac0 100644 --- a/src/Compiler/SyntaxTree/XmlDoc.fsi +++ b/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 diff --git a/tests/FSharp.Compiler.ComponentTests/Language/XmlComments.fs b/tests/FSharp.Compiler.ComponentTests/Language/XmlComments.fs index 47aee9362b7..1ca410a56e2 100644 --- a/tests/FSharp.Compiler.ComponentTests/Language/XmlComments.fs +++ b/tests/FSharp.Compiler.ComponentTests/Language/XmlComments.fs @@ -257,4 +257,126 @@ module M = """ |> withXmlCommentChecking |> compile - |> shouldSucceed \ No newline at end of file + |> shouldSucceed + +module XmlCommentCheckingGetSetProperty = + + [] + let ``Fully documented get-set property produces no xmldoc warning`` () = + Fsx """ +type MyClass() = + /// A property + /// The index + /// The value + member _.Item + with get(index: int) = index + and set (index: int) (value: int) = () + """ + |> withXmlCommentChecking + |> ignoreWarnings + |> compile + |> shouldSucceed + |> withDiagnostics [] + + [] + let ``Simple get-set property with xmldoc no false warning`` () = + Fsx """ +type MyClass() = + /// Gets or sets the value + /// The value + member _.Prop + with get() = 0 + and set (v: int) = () + """ + |> withXmlCommentChecking + |> ignoreWarnings + |> compile + |> shouldSucceed + |> withDiagnostics [] + + [] + let ``Get-only property xmldoc check unaffected`` () = + Fsx """ +type MyClass() = + /// A property + /// The index + member _.Item with get(index: int) = index + """ + |> withXmlCommentChecking + |> ignoreWarnings + |> compile + |> shouldSucceed + |> withDiagnostics [] + + [] + let ``Actually missing param doc still warns for get-set`` () = + Fsx """ +type MyClass() = + /// A property + /// The index + 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'") ] + + [] + let ``Documented param that exists in neither accessor warns`` () = + Fsx """ +type MyClass() = + /// A property + /// The index + /// The value + /// Does not exist + 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'") ] + + [] + [] + [] + let ``Get-set with different types fully documented is clean`` (getType: string) (setType: string) = + let source = + "\ntype MyClass() =\n" + + " /// Prop\n" + + " /// The value\n" + + " member _.Prop\n" + + " with get() : " + getType + " = Unchecked.defaultof<_>\n" + + " and set (v: " + setType + ") = ()\n " + Fsx source + |> withXmlCommentChecking + |> ignoreWarnings + |> compile + |> shouldSucceed + |> withDiagnostics [] + + [] + let ``Issue 13684 repro indexed property fully documented is clean`` () = + Fsx """ +type A = + /// + /// + /// + /// + member x.A with get (j: int) : int = 3 + and set (k: int) (l: int) = () + """ + |> withXmlCommentChecking + |> ignoreWarnings + |> compile + |> shouldSucceed + |> withDiagnostics [] \ No newline at end of file diff --git a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl b/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl index 9b33caf5d71..93f733ef373 100644 --- a/tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.SurfaceArea.netstandard20.bsl +++ b/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()