Consider wider default format for record declarations if any XML doc comments are present on fields

[dsyme]

From dotnet/docs#25972 (comment)

I propose we use wide formatting of records type declarations in implementation or signature file when any of the fields of the record type have XML comments.

Proposal

The proposal is to have

• record type definitions without XML doc be formatted compactly (record definition braces on same column = false)
• record type definitions with XML doc be formatted widely (record definition braces on same column = true).

This is roughly what was implemented for signature file generation in dotnet/fsharp#12072.

So default formats are as today if no XML doc:

type PostalAddress =
    { Address: string
      City: string
      Zip: string }

and wide if XML doc:

type PostalAddress =
    {
      /// The address
      Address: string

      /// The city
      City: string

      /// The zip code
      Zip: string
    }

I'd also propose that wide format be used even if only one member is XML doc commented:

type PostalAddress =
    {
      Address: string
      City: string

      /// The zip code
      Zip: string
    }

###' Background

Fantomas defaults to compact format for records

// Declaring additional members on PostalAddress
type PostalAddress =
    { Address: string
      City: string
      Zip: string }

This seems a good default. However, as soon as XML doc comments are added it sufers. That is, if we follow the style guide rule that the /// has a blank line before it, except the first, we get this:

// Declaring additional members on PostalAddress
type PostalAddress =
    { /// The address
      Address: string

      /// The city
      City: string

      /// The zip code
      Zip: string }

The only setting available to adjust this is the very widely affecting fsharp_multiline_block_brackets_on_same_column. However that would be too broadly applicable - record type definitions are the only place in the syntax where XML doc comments interact with block brackets.

Pros and cons

The main disadvantage is that this is a discontinuous approach that people may find confusing.

The main advantage is that it supports the principal that the "destination" of code be XML-doc commented.

Alternatives

Make wide formatting the default for all record expressions.

[dsyme]

@nojaf Don't rush into the change - I'm aware it creates an inconsistency and it's possible a single setting to say "wide format for record type definitions" would also be ok.

I guess I'm mainly concerned about what the defaults should be, and am aware we're caught between two things here

• it's hard to justify wide format defaults when there are no XML doc comments....
• it's hard to justify compact format defaults there are XML doc comments....

[nojaf]

Don't rush into the change

Don't worry about that 😅. I'll wait for the guide to be up to date first.
Thanks for the write-up!

[dsyme]

@nojaf Don't worry there's part of me that wants to resolve this by making this a new option for record type definition syntax :) In F# 6.1 or 7.0 maybe? :)

type PostalAddress =
    Address: string
    City: string
    Zip: string

PostalAddress(Address="A", City="B", Zip = "C")

{ Address="A"; City="B"; Zip = "C" }

{ address with Address = ""}

address.WithAddress("")
address.With(City="B", Zip = "C")

[cartermp]

PostalAddress(Address="A", City="B", Zip = "C")

This is actually covered in an approved suggestion already, so this could be done easiest I think

[dsyme]

Just to mentioned that I noticed this when applying Fantomas to the F# compiler codebase, for example the cramped formatting here: https://github.com/dotnet/fsharp/blob/7de9637f8d7839c2312748dcbbabb4cda7455548/src/fsharp/SyntaxTrivia.fsi#L43

The style guide now definitively says "if XML comments are present, then indent and add new lines", e.g. here https://docs.microsoft.com/en-us/dotnet/fsharp/style-guide/formatting#formatting-record-declarations

Also, in general, if an /// comment is present it feels like we should default to a new empty line before that comment. That's always recommended for any use of /// (except perhaps those for implicit constructors)

[nojaf]

@dsyme thanks for letting me know about this, we can have this in the next major.
Should this be active when one record declaration field has XML documentation? Or when all of them have it?

About the additional empty line, I'd like to wait a bit with that one. Users are very sensitive to empty lines, so I'm on the fence if we really want that.

Another nice thing I want to predict: a lot of issues will be opened claiming the style of record declarations is inconsistent.
Looking forward to that 😝🎉🙈 🙃

[dsyme]

Should this be active when one record declaration field has XML documentation? Or when all of them have it?

When any record field has ///

About the additional empty line, I'd like to wait a bit with that one. Users are very sensitive to empty lines, so I'm on the fence if we really want that.

I see, it makes sense. I presume we already preserve the extra line if the original code has it?

Another nice thing I want to predict: a lot of issues will be opened claiming the style of record declarations is inconsistent.

You're right, this will happen. Tricky issue

[nojaf]

I presume we already preserve the extra line if the original code has it?

Yes, that should be the case. Things can get a little ticky when there is trivia underneath some XML Doc.

Something like:

/// Xml doc stored in AST
// comment trivia
type Foo = Bar

The range of the SynTypeDefn will span from the start of the triple-slash comment till the Bar ident. So in order for us to restore the trivia (double-slash comment here) we need a range of the closest anchor (the type keyword here).

I don't think we have all of these keywords in SyntaxTreeTrivia yet, so don't hesitate to report any issues if something between the XML doc and next AST node isn't restored properly.