Allow for XML documentation comments on DU Case fields

[baronfel]

I propose we extend the parser and language spec to allow for DU Case fields to accept XML documentation comments like record fields already do. For example, allowing

type Foo =        
| Thing of 
  /// docs for first
  first: string *
  /// docs for second
   second: bool
| Other of
  /// docs for extra
  extra: string *
  /// docs for what
  what: System.DateTime

The existing way of approaching this problem in F# is to describe each of the fields in the overall DU Case type constructor, but this interacts poorly with tooling (the DU fields themselves have no documentation, so it's trickier to display the docs for the parent type when docs for the field are requested) and is less maintainable because it separates the docs from the element they describe.

The existing way can be seen on several DUs used in the compiler itself: https://github.com/dotnet/fsharp/blob/main/src/fsharp/SyntaxTree.fs#L422-L431

It would be much nicer in my opinion to be able to write something like the following instead, and have access to these details on a field-by-field basis when consuming these types:

/// Describes F# syntax for type parameter applications to a base type.
/// Can be in a prefix or a postfix form:
/// prefix form:  type<type, ..., type> or type type
/// postfix form: (type, ..., type) type
| App of
    /// the base type to which type arguments are being applied
    typeName: SynType *
    /// the range of the `<` token, signifying the start of the type arguments list
    lessRange: range option *
    /// type arguments applied to the base type name
    typeArgs: SynType list *
    /// ranges for each of the commas that separate the typeArgs. The size of this list should be
    /// one less than the size of typeArgs, if any
    commaRanges: range list *
    /// the range of the `>` token, signifying the end of the type arguments list
    greaterRange: range option *
    /// true if this type application is done in a postfix manner: (type, ..., type) baseType
    /// false if this type application is done in a prefix manner: baseType<type, ..., type>
    isPostfix: bool *
    /// the overall range of the expression, from the start of `typeName` to the end of `greaterRange`
    range: range

Pros and Cons

The advantages of making this adjustment to F# are a more uniform experience for authoring comments on Union types.

The disadvantages of making this adjustment to F# are a bit of increased work done by

• the parser (to collect documentation comments for the Case field)
• the XmlDocWriter (to create the XmlDocSig for the Case field and then to write the Case field property xmldoc entries)

Extra information

Estimated cost (XS, S, M, L, XL, XXL): S

Related suggestions: None that I could find

Affidavit (please submit!)

Please tick this by placing a cross in the box:

• This is not a question (e.g. like one you might ask on stackoverflow) and I have searched stackoverflow for discussions of this issue
• I have searched both open and closed suggestions on this site and believe this is not a duplicate
• This is not something which has obviously "already been decided" in previous versions of F#. If you're questioning a fundamental design decision that has obviously already been taken (e.g. "Make F# untyped") then please don't submit it.

Please tick all that apply:

• This is not a breaking change to the F# language design (it allows for existing data types, but in locations they were not allowed before)
• I or my company would be willing to help implement and/or test this

For Readers

If you would like to see this issue implemented, please click the 👍 emoji on this issue. These counts are used to generally order the suggestions by engagement.

[baronfel]

If this is approved, I have an RFC and implementation ready to go.

[charlesroddie]

I like this syntax. Could it apply to regular class constructors too?

[dsyme]

@baronfel Thinking about this with respect to three things

1. Is this the syntax we want and how does this play w.r.t. other language constructs (e.g. parameters and class constructors).

2. How will this play with any future "markdown docs" feature

3. What docs are actually being emitted in the XML doc file here (and are we emitting the right docs into the XML file for union types at all?)

On (1) the question is whether we should allow this on other similar language constructs, in particular

• parameters in implementation files
• parameters in signature files
• parameters for delegate definitions
• parameters in abstract member definitions

If we add what you've shown then I think there becomes an expectation that

static member M (
        /// param one
        x:int, 
        /// param two
        y:int)

also works. I'm not opposed to making this work but if we do one I feel we should do the other. However that's a big job.

An alternative is that we use what we already use for parameters, namely the <param name="Foo">...</param> syntax.

/// <summary>Describes F# syntax for type parameter applications to a base type.</summary>
/// <param name="typeName">the base type to which type arguments are being applied</param>
/// <param name="lessRange">the range of the `&lt` token, signifying the start of the type arguments list</param>
| App of
    typeName: SynType *
    lessRange: range option *
    ...

To be honest this does feel like the orthogonal way to do things from a language design consistency point of view. There are some complications though - the implementation would have to extract and associate these docs with the union field specs. Also we would need the completeness checking for parameter names for docs to apply to union cases as well.

On (3), some relevant code is here and here and here.

Today, for a multi-case union type

module M
type A = 
  /// ABC
  | ABC of xyz: int * def: string
  /// DEF
  | ABC2 of xyz: int * def: string

we're emitting the docs into the XML file as the generated type for each case:

<member name="T:M.A.ABC2"><summary>DEF</summary></member>
<member name="T:M.A.ABC"><summary>ABC</summary></member>

This is not really perfect from - it should also really be attached to the NewABC and NewABC2 methods to aid the C# programmer. However for a single-case union type:

module M
type A = 
  /// ABC
  | ABC of xyz: int * def: string

we're actually emitting a bad documentation file referencing a non-existent type T:M.A.ABC (because only M.A is generated in this case). The fact that the type doesn't actually exist is not a problem for F#-to-F# references (where both writer and reader key on this XML doc signature), for for C#-to-F# references you won't see these docs anywhere,

<member name="T:M.A.ABC"><summary>ABC</summary></member>

The underlying problem here is that the "Erase Unions" part of the compiler just isn't doing anything with the docs, and the CML doc file writer was written to just emit whatever information.

Now, looking at your addition I have a similar concern - currently these docs will not be written to the XML file at all. So I think at the very minimum this code would need to be adjusted to write docs for the union field to the XML file. You would also need to adjust this code to actually populate the rfield_xmldocsig field of each union case field. This should be enough for the docs to be found for F#-to-F# references (note - I mean referencing a DLL or nuget pacakge, not in-memory cross project references). I guess we can treat the problem of C#-to-F# docs as separate.

[dsyme]

So basically, this suggestion raises questions about both union case docs and parameter docs - and whatever we do we should do systematically

[baronfel]

Your comments echo a lot of what @charlesroddie and I were discussing offline with his comment.

For what it's worth, I was looking at this mainly as a way to plug a gap that currently exists in terms of DU documentation compared to how record fields can be independenly documented, as an intermediate step towards the kind of holistic parameter-handling that you speak of. It would be lovely to fold parameter documentation into constructors where appropriate in a more general fashion, but it would in my opinion require a significant chunk of work around the structure of xml documentation as you point out.

Regarding the technical aspects, I have been able to get the parser and xmldoc writer plumbed through and have an RFC describing the changes. It's pretty much exactly as you describe in terms of work, the only other change is slightly modifying the parser to collect and flow through the PreXmlDoc.

My overall thought process was something like:

• start collecting DU field these documentation elements (this suggestion)
• expand xml handling to fold these kind of docs for parameters into the constructors of DUs
• expand this folding into other constructors (records, classes)
• expand into general method calls?

[baronfel]

On the alternative of allowing the <param name=""> syntax on the DU case constructor itself, I decided against proposing that direction because there's currently not a concept of transforming the source XML based on different view modes. If we added parameter declarations to the DU case itself, then viewing XmlDoc for the DU would always show these parameter nodes, even in cases where they are not relevant (ie where the DU case is not being used in a constructor context. I'm thinking here of Ionide's Info Panel display for a DU case, or the Object Explorer in VS).

If we allowed that syntax then I'd want to be able to 'project' subsets of the source XML based on the viewer's context, I think.

[baronfel]

I've got a prototype implementation of this proposal at my fork for reference.

Should the 'need to attach Union Case docs to the NewABC' members generated by the compiler portion of your comments be treated as a bug in the existing implementation? If so I'd be happy to write it up and begin looking at an implementation independently of this suggestion.

[dsyme]

@baronfel TBH the prototype looks good. In balance I think I'm ok with going ahead with your proposal even if the most "natural" approach would be to use <param... or perhaps a specific construct like <field.... But because these are ultimately both parameters and fields and properties they deserve a full XML doc section (because properties get a full XML doc section).

Should the 'need to attach Union Case docs to the NewABC' members generated by the compiler portion of your comments be treated as a bug in the existing implementation?

Yes, I think so

[srid]

Can we generalize this feature to make it work on not only DU fields, but also on parts of any declaration in general? i.e.

• record fields
• function arguments
• etc.

cf. how Haskell does it: https://haskell-haddock.readthedocs.io/en/latest/markup.html#documenting-parts-of-a-declaration

[baronfel]

@srid currently this can be done for record fields.

Given the F# code:

    /// An event describing a change to a text document. If range and rangeLength are omitted
    /// the new text is considered to be the full content of the document.
    type TextDocumentContentChangeEvent = {
        /// The range of the document that changed.
        Range: Range option

        /// The length of the range that got replaced.
        RangeLength: int option

        /// The new text of the range/document.
        Text: string
    }

hovering over the definition or usage of the .Range record field anywhere it's used in a codebase shows the docs for that field:

[dsyme]

I'll mark this as approved, though it is a surprisingly difficult area to make progress on