Redesign XmlDoc collecting mechanism #11973

DedSec256 · 2021-08-14T12:45:55Z

This PR contains a redesign of the XmlDoc collecting mechanism to allow more predictable and C# consistent positions for XmlDoc.

Current approach

At the moment, doc-comments are sequentially collected and attached to the nearest (after comments) declaration that supports XmlDoc. This leads to errors when comments that should not be considered at all are attached to declarations:

Also at the moment, it is allowed to leave comments up to the name of the binding/type definition, which seems redundant:

Redesign

The basic idea is to set the grab points for doc-comments after the non-comment expressions encountered. Then we can assume that the comments block belongs to the declaration if its grab point coincides with the correct position in the declaration (for example the beginning of the declaration).

This PR offers to collect comments according to the following rules

1. Delimiter expressions

Similar to C#, if another expression (expression, simple comment, (*), etc.) is encountered when collecting doc-comments, doc-comments before this expression cannot be attached to the declaration

/// A1
1 + 1
/// A2
type A     // xmlDoc: A2

2. Attributes

Similar to C#, if the declaration has attributes, then the comments before the attributes are attached to the declaration

/// A1
[<Attribute>]
/// A2
type A     // xmlDoc: A1

3. type/let-and

Since there is a real use case #11489 (comment), it is allowed to add doc-comments before and

type A

/// B
and B     // xmlDoc: B

Also, doc-comments are allowed to be left after and

type A

and /// B
       B     // xmlDoc: B

However, if comments before and after and are present at the same time, then the priority is given to the comments before and

type A

/// B1
and /// B2
       B     // xmlDoc: B1

4. Declaration parts

It is allowed to leave comments only at the beginning of the declarations

let rec f x = g x

/// A
and /// B
    inline /// C
           private
                  g x = f x     // xmlDoc: A

type A =
    /// B1
    member
            ///B2
            private x.B = ()     // xmlDoc: B1

Fix for #11488

Support XmlDoc for union case without bar

type MyUnion =
    /// A
    A      // xmlDoc: A
    | B

TODO

Support let-bindings (and other unsupported constructions)
Extend XmlDoc-alalyzer with checks for invalid documentation location (probably in a separate PR)
More tests
Fix docs in FSharp.Core & FSharp.Compiler.Service
Include XmlDoc range to the declaration range? (in a separate PR)

Discussion

PR in progress, we will be glad to hear your opinion, join the discussion (:

dsyme · 2021-08-14T13:52:37Z

Yes this is all reasonable.

Support XmlDoc for union case without bar

I'm not particularly concerned to support this case - omitting the first | from the first union case is very non-idiomatic F#

Similar to C#, if the declaration has attributes, then the comments before the attributes are attached to the declaration

What if the declaration doesn't have attributes? I think I've authored types where the attributes come first

auduchinok · 2021-08-14T14:34:32Z

I'm not particularly concerned to support this case - omitting the first | from the first union case is very non-idiomatic F#

We want to have it properly marked in tree anyway, so the tooling is able to analyze it correctly.

dsyme · 2021-08-14T14:54:17Z

We want to have it properly marked in tree anyway, so the tooling is able to analyze it correctly.

Got it

auduchinok

Just a few small comments.

src/fsharp/XmlDoc.fs

tests/FSharp.Compiler.Service.Tests/FSharp.Compiler.Service.Tests.fsproj

auduchinok · 2021-08-14T14:53:27Z

tests/service/XmlDocTests.fs

+"""
+    checkResults
+    |> checkXml "A" [|"B"|]
+


Could you please also add a test case for attributes before an xml doc in a type declaration?

It's already there!
https://github.com/dotnet/fsharp/pull/11973/files/3ad240c448a9b49502cb011eca39a7ece0dc7a9b#diff-6353bbdf824b76a75ac480672baa99c8fb36c0379ec4170959c479acb4954bf5R270

Ah, sorry, I didn't spot it there, thanks!

DedSec256 · 2021-08-14T18:54:22Z

@dsyme,

What if the declaration doesn't have attributes? I think I've authored types where the attributes come first

Do I understand correctly that you mean the case

[<Attr>]
/// A
type A

In this case, type A will not have XmlDoc

DedSec256 · 2021-08-14T19:22:06Z

I would also like to discuss the XmlDoc collecting mechanism for get/set

At the moment, it is allowed to leave additional doc-comments for get/set.
Then the documentation for get (get_X) / set (set_X) is merged with the documentation for the property itself, and the documentation for the property is merged with the documentation for get

/// A
member x.A with ///GET
                get() = 5
            and /// SET
                set (y: int) = ()
                
// A     xmlDoc: "A GET"
// get_A xmlDoc: "A GET"
// set_A xmlDoc: "A SET"

Which seems overcomplicated unnecessarily.
Are there any cases when it is essential to add documentation for accessors?

In this PR, it is proposed to leave the possibility of adding documentation only to the declaration of the property itself,
property documentation will also apply to accessors.

dsyme · 2021-08-15T16:09:42Z

Are there any cases when it is essential to add documentation for accessors?

What is the C# spec here? Can you document them separately? We should follow what they do.

DedSec256 · 2021-08-15T17:18:38Z

For C#, the documentation can be added only for properties

Documentation comments must immediately precede a user-defined type (such as a class, delegate, or interface) or a member (such as a field, event, property, or method)

https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/documentation-comments

https://sharplab.io/#v2:EYLgxg9gTgpgtADwGwBYA+ABAzAAgwJhwGEAbAQwGcKBGAWACgBvBnVnAek5wB4KBXALYCyUAJ4A+Fm07tWABSgQADlNYye7fkJETVeXAEsAdgBccC5Xub02tjlwDiAUQAqe2wHMYZxngDsOACsANw4AL4M7tJcAMquUawU3ji+ETZsaWFAA

dsyme · 2021-08-15T17:37:04Z

For C#, the documentation can be added only for properties

OK, yes, we should follow that

auduchinok · 2021-08-16T10:58:18Z

Extend XmlDoc-alalyzer with checks for invalid documentation location (probably in a separate PR)

We can mark each comment block when grabbing it (add a bool field to a block?), and then report syntax warnings for blocks that haven't been grabbed after parsing finishes.

DedSec256 · 2021-08-23T18:10:16Z

I think it is more or less ready.

DedSec256 · 2021-08-27T11:49:07Z

@dsyme, can you please take a look at this 👀

dsyme · 2021-08-27T16:41:08Z

@dsyme, can you please take a look at this 👀

Will do!

dsyme · 2021-08-27T16:42:18Z

src/fsharp/pars.fsy

+        | h :: _ -> h.Range
+    LexbufLocalXmlDocStore.GrabXmlDocBeforeMarker(parseState.LexBuffer, grabPoint)
+
+let grabXmlDoc(parseState: IParseState, optAttributes: SynAttributeList list, elemIdx) =


Generally best to move all helper functions outside pars.fsy, at least for new code or whenever we touch this code.

dsyme · 2021-08-31T12:36:13Z

I'm still uneasy about us dropping the XML doc on this case: #11973 (comment)

Am I right in thinking the XML doc is just dropped on the floor? Is it possible to emit an informational warning in this case, indeed whenever any /// doc is discarded, unattached?

DedSec256 · 2021-08-31T12:48:27Z

Yes, in the described case, the comment is discarded.
We want to add an analyzer that generates warnings (as described here #11973 (comment)) in a separate PR.

Or would you like the analyzer to be part of this PR?

dsyme · 2021-08-31T13:15:49Z

Or would you like the analyzer to be part of this PR?

Yes I think we should have an informational warning informationalWarning(...) on discarded /// as part of this PR unless it's technically really difficult. Even for FSharp.Core there may be cases where we're now dropping docs, and I'd like that checked. I was going to ask you to check manually but we should really just automate it, it's better. So yes, please add the informational warning and also add --warnon:NNN for the informational message number you choose to FSharp.Core.fsproj

(Aside: It can't be an "analyzer" in the sense of #11057 and it should be on for the command-line compiler by default)

dsyme

Request for informational warning on dropped XML doc

auduchinok · 2021-09-08T10:36:03Z

src/fsharp/FSharp.Core/async.fs

@@ -305,7 +305,7 @@ namespace Microsoft.FSharp.Control
            contents.aux.ccont (OperationCanceledException (contents.aux.token))

        /// Check for trampoline hijacking.
-        // Note, this must make tailcalls, so may not be an instance member taking a byref argument,
+        /// Note, this must make tailcalls, so may not be an instance member taking a byref argument,


Looks like an internal comment to me. @dsyme Could you advice?

@auduchinok, here it is rather suspicious since the line below is a continuation of this one and it begins with ///

Yeah, I thought maybe it was better to change all comments below to // here?

oh, below in the same file there is a similar comment that uses //

DedSec256 · 2021-09-08T11:05:04Z

@dsyme, FSharp.Compiler.Service has a lot of documentation, where the part of the documentation after // looks internal. We can replace all comments after // with /// -> //, then they will disappear from public access. Or how can these conflicts be resolved?

dsyme · 2021-09-08T12:13:48Z

@dsyme, FSharp.Compiler.Service has a lot of documentation, where the part of the documentation after // looks internal. We can replace all comments after // with /// -> //, then they will disappear from public access. Or how can these conflicts be resolved?

As long as it isn't public FCS API I don't really mind

So yes, replace all comments after // with /// -> // is fine

DedSec256 · 2021-09-13T13:50:22Z

@dsyme , it's done! (:

dsyme · 2021-09-17T18:06:09Z

@DedSec256 This is fantastic work, thank you

dsyme · 2021-09-17T18:06:45Z

@DedSec256 Thank you for everything you did on this, including the extensive testing and multiple iterations to take into account code feedback. It's really great to have this addressed

DedSec256 · 2021-11-08T11:48:41Z

@dsyme, I thought about the fact that we currently have XmlDoc allowed for local bindings. They do not participate in the generation of the documentation file, and XmlDoc is not allowed for local variables in C#. How do you feel about disallowing XmlDoc for local bindings in the same way?

dsyme · 2021-11-08T12:48:36Z

@dsyme, I thought about the fact that we currently have XmlDoc allowed for local bindings. They do not participate in the generation of the documentation file, and XmlDoc is not allowed for local variables in C#. How do you feel about disallowing XmlDoc for local bindings in the same way?

XML doc is allowed for local bindings in F#. They should show in autocomplete/intellisense. I assume this PR didn't change that?

DedSec256 · 2021-11-08T12:56:34Z

XML doc is allowed for local bindings in F#. They should show in autocomplete/intellisense. I assume this PR didn't change that?

yes, in this PR the XmlDoc is still allowed for local bindings.

auduchinok · 2021-11-08T13:08:28Z

@dsyme Could you tell more about why XmlDoc is preferred over normal comments in local scopes?
Maybe you have any insight why it wasn't enabled in C#? 🙂

dsyme · 2021-11-08T13:43:48Z

The only rationale is to have them show in tooling.

I guess it wasn't enabled in C# because scopes were generally much smaller, especially in C# 1.0 coding styles. In F# you can easily have a class 500 lines long and in that situation it pays to document a let function that has scope over the whole class.

dsyme · 2021-11-08T13:44:23Z

(Also, in F# XML doc comments are much lighter with the implicit <summary>)

auduchinok · 2021-11-08T13:55:43Z

(Also, in F# XML doc comments are much lighter with the implicit <summary>)

(Hm, sounds just like in modern C#. 🙂)

dsyme · 2021-11-08T14:04:53Z

I wonder where they got that idea!

auduchinok reviewed Aug 14, 2021

View reviewed changes

dsyme approved these changes Aug 31, 2021

View reviewed changes

dsyme requested changes Aug 31, 2021

View reviewed changes

dsyme added the Theme-Simple-F# A cross-community initiative called "Simple F#", keeping people in the sweet spot of the language. label Aug 31, 2021

DedSec256 added 11 commits August 31, 2021 21:22

wip

53e4a5d

let bindings and 'extern' support

9f6a77e

fix SurfaceArea

761b923

fix for val & member val

52a31c3

tests + fixes

8d10fa6

yet another tests

41cf202

_

614d152

simplify

0c7bc4d

test for implicit ctor

10c1536

fix

0e9f61f

improve test

5f44b78

auduchinok mentioned this pull request Sep 7, 2021

Print XML Document comments from AST fsprojects/fantomas#1878

Closed

DedSec256 mentioned this pull request Sep 7, 2021

Redesign XML-documentation collecting mechanism fsharp/fslang-suggestions#1071

Closed

5 tasks

fix xml-doc in FSharp.Core

407fc1f

auduchinok reviewed Sep 8, 2021

View reviewed changes

Merge branch 'main' into ber.a/xmlDocCollecting

863a9d4

fix tests

4015469

DedSec256 added 4 commits September 8, 2021 15:47

fix docs in FSharp.Compiler.Service

81a4f99

yet another comment fix

2298a6f

try to fix tests

7c4cab7

yet another test fix

889c2f9

DedSec256 requested a review from dsyme September 9, 2021 09:29

DedSec256 added 2 commits September 17, 2021 00:08

Merge branch 'main' into ber.a/xmlDocCollecting

5b90144

compilation fix

79e9afa

dsyme approved these changes Sep 17, 2021

View reviewed changes

dsyme merged commit 85fe280 into dotnet:main Sep 17, 2021

dsyme mentioned this pull request Nov 5, 2021

XmlDoc is allowed before and in type definitions #11489

Closed

nojaf mentioned this pull request Mar 4, 2022

Update FCS to 41.0.3 fsprojects/fantomas#2122

Merged

Redesign XmlDoc collecting mechanism #11973

Redesign XmlDoc collecting mechanism #11973

Conversation

DedSec256 commented Aug 14, 2021 • edited Loading

Current approach

Redesign

1. Delimiter expressions

2. Attributes

3. type/let-and

4. Declaration parts

Fix for #11488

TODO

Discussion

dsyme commented Aug 14, 2021

auduchinok commented Aug 14, 2021

dsyme commented Aug 14, 2021

auduchinok left a comment

Choose a reason for hiding this comment

auduchinok Aug 14, 2021

Choose a reason for hiding this comment

DedSec256 Aug 14, 2021

Choose a reason for hiding this comment

auduchinok Aug 14, 2021

Choose a reason for hiding this comment

DedSec256 commented Aug 14, 2021

DedSec256 commented Aug 14, 2021 • edited Loading

dsyme commented Aug 15, 2021

DedSec256 commented Aug 15, 2021

dsyme commented Aug 15, 2021

auduchinok commented Aug 16, 2021

DedSec256 commented Aug 23, 2021

DedSec256 commented Aug 27, 2021

dsyme commented Aug 27, 2021

dsyme Aug 27, 2021

Choose a reason for hiding this comment

dsyme commented Aug 31, 2021 • edited Loading

DedSec256 commented Aug 31, 2021 • edited Loading

dsyme commented Aug 31, 2021 • edited Loading

dsyme left a comment

Choose a reason for hiding this comment

auduchinok Sep 8, 2021

Choose a reason for hiding this comment

DedSec256 Sep 8, 2021

Choose a reason for hiding this comment

auduchinok Sep 8, 2021

Choose a reason for hiding this comment

DedSec256 Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

DedSec256 commented Sep 8, 2021

dsyme commented Sep 8, 2021 • edited Loading

DedSec256 commented Sep 13, 2021

dsyme commented Sep 17, 2021

dsyme commented Sep 17, 2021 • edited Loading

DedSec256 commented Nov 8, 2021

dsyme commented Nov 8, 2021

DedSec256 commented Nov 8, 2021

auduchinok commented Nov 8, 2021

dsyme commented Nov 8, 2021

dsyme commented Nov 8, 2021

auduchinok commented Nov 8, 2021 • edited Loading

dsyme commented Nov 8, 2021

DedSec256 commented Aug 14, 2021 •

edited

Loading

DedSec256 commented Aug 14, 2021 •

edited

Loading

dsyme commented Aug 31, 2021 •

edited

Loading

DedSec256 commented Aug 31, 2021 •

edited

Loading

dsyme commented Aug 31, 2021 •

edited

Loading

DedSec256 Sep 8, 2021 •

edited

Loading

dsyme commented Sep 8, 2021 •

edited

Loading

dsyme commented Sep 17, 2021 •

edited

Loading

auduchinok commented Nov 8, 2021 •

edited

Loading