Xml comment generation for generated signature files

[baronfel]

Today, the compiler doesn't include XmlDoc comments in generated signatures, which can present a maintainability/usability problem. With the advent of compiler performance improvements in the presence of signature files, we should ensure the onboarding experience for authoring signatures is as easy as possible. To that end, this PR adds in emit of xml documentation where is is already known.

This initial implementation is quite simple, the entire complexity comes from auditing where comments are known and ensuring that the matching call to layoutXmlDoc is called.

I've been using the following sample file for testing, and manually comparing the generated output:

Script.fs

/// namespace comments
namespace Sample

/// exception comments
exception MyEx of reason: string

/// module-level docs
module Inner =
    /// type-level docs
    type Farts
        /// primary ctor docs
        (name: string) =
        /// constructor-level docs
        new() = Farts("default name")
        /// member-level docs
        member x.blah() = [1;2;3]
        /// auto-property-level docs
        member val Name = name with get, set

    /// module-level binding docs
    let module_member = ()

    /// record docs
    type TestRecord =
        {
            /// record field docs
            RecordField: int
        }
        /// record member docs
        member x.Data = 1
        /// static record member docs
        static member Foo = true

    /// union docs
    type TestUnion =
    /// docs for first case
    | FirstCase of thing: int
        /// union member
        member x.Thing = match x with | FirstCase thing -> thing

Output at this point is:

generated signature

namespace Sample
  /// exception comments
  exception MyEx of reason: string
  /// module-level docs
  module Inner = begin
    /// type-level docs
    type Farts =
      class
        /// constructor-level docs
        new : unit -> Farts
        /// primary ctor docs
        new : name:string -> Farts
        /// member-level docs
        member blah : unit -> int list
        /// auto-property-level docs
        member Name : string
      end
    /// module-level binding docs
    val module_member : unit
    /// record docs
    type TestRecord =
      { /// record field docs
        RecordField: int }
      with
        /// record member docs
        member Data : int
        /// static record member docs
        static member Foo : bool
      end
    /// union docs
    type TestUnion =
      /// docs for first case
      | FirstCase of thing: int
      with
        /// union member
        member Thing : int
      end
  end

Open questions:

• are there display options flags that should be introduced along with this work
• what other language constructs are missing?
• where should tests go, and what should they cover? simple text comparison/equality?
• (language spec question) namespaces can have xmldocs (ModuleOrNamespace is an Entity, which has XmlDoc), but in every case I've tried so far the XmlDoc is always empty, so I wonder if the parser is incorrect/malformed here?

[baronfel]

I've done a pass through all of the functions in NicePrint and I think I've covered all of the relevant ones, but it's tough to know that I've hit them all of course.

[TIHan]

are there display options flags that should be introduced along with this work

Probably. Adding another field to DisplayEnv such as: showDocumentation: bool would be the flag to enable or disable generating the xml doc comments.

what other language constructs are missing?

I wouldn't worry about what else is missing; they can be addressed separately from this PR.

where should tests go, and what should they cover? simple text comparison/equality?

I think a few simple equality tests proving that the comments actually get emitted is sufficient for now. Use this PR as an example/guidance on where to put tests: #11303 - but if you find a better place to put them, that's also fine too :).

(language spec question) namespaces can have xmldocs (ModuleOrNamespace is an Entity, which has XmlDoc), but in every case I've tried so far the XmlDoc is always empty, so I wonder if the parser is incorrect/malformed here?

I would add a check to not emit the documentation if the Entity is a namespace.

[baronfel]

Thanks for the comments @TIHan, I'll look into addressing them.

[baronfel]

Alright, made a test by copying the framework in #11303 and adding my own method to FSharpCheckResults that mimics the way fsc.exe actually writes the signatures. We're going to need a way to disambiguate this member from the one being implemented in #11303.

[baronfel]

The failing tests right now are:

Failed Regression.MemberDefinition.DocComments.Bug5856_4 [178 ms]
  Error Message:
   Expected:
/// Union comment
type Union = /// Case comment
             | Case of int
<summary>

Union comment</summary>


to contain:
type Union = | Case of int

after index: 0.
  Stack Trace:
     at UnitTests.TestLib.Salsa.containsInOrderFrom@26(String s, Int32 fromIndex, FSharpList`1 expects) in D:\workspace\_work\1\s\vsintegration\tests\UnitTests\TestLib.Salsa.fs:line 32
   at Tests.LanguageService.QuickInfo.UsingMSBuild.AssertMemberDataTipContainsInOrder(FSharpList`1 code, String marker, String completionName, FSharpList`1 rhsContainsOrder) in D:\workspace\_work\1\s\vsintegration\tests\UnitTests\LegacyLanguageService\Tests.LanguageService.QuickInfo.fs:line 1695
   at Tests.LanguageService.QuickInfo.UsingMSBuild.Regression.MemberDefinition.DocComments.Bug5856_4() in D:\workspace\_work\1\s\vsintegration\tests\UnitTests\LegacyLanguageService\Tests.LanguageService.QuickInfo.fs:line 1778

and this is probably a reasonable failure, but I'm not sure what the resolution is. I'm not familiar with this area of the codebase. Any pointers?

[cartermp]

Since these are VS tests, and the older ones that are more difficult to work with, it'll be annyoing. The test is more or less asserting that a tooltip in VS will show the signature, followed by the summary comment. That's apparently no longer true with this work. It's a weird abstraction being used so it's kind of hard to build a model for how to make the test pass, but a quick way to see if you've fundamentally borked tooltips is to build this version of the tools and have the following code:

module MyModule =
    /// Union comment
    type Union = /// Case comment
        | Case of int

let x () =
    Module.

And see what the tooltip for Union looks like.

[baronfel]

So I've gotta go install VS and learn how to build the vsix/run the debug hive stuff? That makes me le sad.

[cartermp]

Yep :)

Luckily it's not too bad. Just set VisualFSharpDebug as the startup project and hit ctrl+f5 (or f5 to debug if you like it slow).

[baronfel]

Definitely doing weird stuff now:

The entire union definition is inlined into the tooltip, which when xml comments are included can make the tooltip unusable. My guess is I'll have to modify the displayenv that's used in whatever routine this is to toggle the documentation generation off for that pathway.

[baronfel]

Alright, so this is because the Description property of a DeclarationListItem renders unions and other type constructors wholesale into tooltips. The chain is:

• DeclarationListItem.Description =>
• DeclarationListHelpers.FormatStructuredDescriptionOfItem =>
• DeclarationListHelpers.FormatItemDescriptionToToolTipElement =>
• NicePrint.LayoutTycon (because item.Item is Item.Types and matches

There are a few potential fixes here:

• remove comments entirely from type rendering in tooltips (very easy, we just alter the denv for the scope of DeclarationListHelpers.FormatItemDescriptionToToolTipElement)
• render type-level comments in NicePrint.layoutTycon and then alter the display environment to not render any more comments (also easy, but may have impacts outside of just tooltips, since layoutTycon is also used to layout Tycon lists and Entitys)

Of these two, I prefer option 1, but am open to other ideas.

[cartermp]

Option 1 sounds much more preferable to me.

[baronfel]

I just tested option 1, and the end result is just that xml comments aren't rendered in the types. They are still part of the tooltip overall for the member being examined:

(see the Union comment) text here as an example. So I think that's the way we'll go. This will also preserve tooltip behavior compared to prior releases.

[baronfel]

Thinking a bit more on this, the reason I even had to make a change in the VS tools was because I opted documentation generation on by default for layout. I now think this is a mistake.

Instead, I want to:

• default doc generation in layouts to off
• explicitly opt in to it on code paths that generate signatures (the new FCS endpoint and the existing FSC generation routines)
• keep the explicit opt out in the VS tooling, to ensure that that behavior is noted/documented for future versions of us.

[baronfel]

It looks like the only test failure is a timeout on linux :-/

[cartermp]

I like this a bunch, but will definitely want a thorough @TIHan review.

[TIHan]

Yes, by default it should be false.

[TIHan]

We can get rid of this I think.

[TIHan]

Great work! This is going to be extremely helpful.

[TIHan]

I'm going to merge it in. If anything, I'll make a PR with some minor tweaks if they come up as I'm still working in this space.