Add code fix to convert ///-comments to Xml-tagged comments

[dawedawe]

Fixes #1033

Recording.2023-03-05.132753.mp4

ToDo: Make the traversal work for nested let bindings.

[baronfel]

This fix is great, especially the tests, but I wanted to dig in a bit more into the details. From was I can see, there is a lot of direct text/range manipulation going on, and I think we can get a lot of the same information in a less-tightly-bound way by using the AST.

What is the 'detection' rule

I propose the following rule:

• if we are on a language element that supports comments, and
• if there are comments present, and
• if those comments are not already xml-based, then
  • we are in a valid position for the fix, else
  • we are not in a valid position for the fix

How do we answer each of these questions?

I propose that instead of walking lines backwards from trigger positions, we should use the untyped AST to determine the answers to the first 2 (at least!) questions.

Many elements of the AST have a field xmlDoc of type PreXmlDoc. We should be able to use a syntax walker to answer the question 'are we on a language element that supports comments' (based on if there is an xmlDoc field on the AST element), and the question 'are the comments present' based on the value of the IsEmpty member of the xmlDoc. That leaves the third question - 'are the comments already xml-based'. This we can get by converting the PreXmlDoc to an XmlDoc and checking the UnprocessedLines field - if this field contains <summary> tags already then we know we are already xml-based, otherwise we know we are not.

If we use the AST instead of more low-level walking we can be insulated from subtle changes. What do you think of this idea?

Separately, I noticed that your fix also determines params/return elements - might I suggest breaking that out into another codefix that could be used more generally on already-xml comments? That would be really useful to help keep xmldocs up-to-date as parameters and return types change.

[dawedawe]

Hey Chet,
thanks a lot for your review.

Yes, splitting this up in two codefixes and relying more on the AST is a better approach.
I'll try to come up with something. It may take a little while as I'm travelling next week 🚄 🇬🇧 😃

[dawedawe]

It's based now on the AST.
Unfortunately, I'm struggling a lot with VisitModuleOrNamespace and VisitInheritSynMemberDefn - SynMemberDefn.AutoProperty. I don't see what I'm doing wrong there. Anything obvious to you?
Otherwise I'll try to digg deeper at the weekend.

/// works on NamedModule
module M1

    /// fails on NestedModule
    module M2 =

        let f x = x

    type MyClass() =
        /// fails on AutoProperty
        member val Name = "" with get, set

[dawedawe]

Ok, all AST Nodes with XmlDoc should work now.
I had to use two visitors as I was getting lost in all the nesting. If there's a way to make this work with just one visitor I'd love to see that.
I'll do a second PR for the second code fix to add params/return elements.

[baronfel]

do you think this is a bug in the FCS Walker that makes auto-properties not get traversed?

[dawedawe]

The whole VisitInheritSynMemberDefn member seems not to be needed, so I removed it.

I added the following test to TreeVisitorTests.fs in the fsharp repo and it fails. But I have really no idea if this API is even meant to be used this way.

[<Test>]
let ``Visit AutoProperty`` () =
    let pos = mkPos 2 13
    let containsPosAndNotEmptyAndNotElaborated (pos: FSharp.Compiler.Text.Position) (xmlDoc: PreXmlDoc) =
      let containsPosAndNoSummaryPresent (xd: PreXmlDoc) =
        let d = xd.ToXmlDoc(false, None)

        if SyntaxTraversal.rangeContainsPosEdgesExclusive d.Range pos then
          let summaryPresent =
            d.UnprocessedLines |> Array.exists (fun s -> s.Contains("<summary>"))

          not summaryPresent
        else
          false

      not xmlDoc.IsEmpty && containsPosAndNoSummaryPresent xmlDoc
  
    let visitor =
        { new SyntaxVisitorBase<_>() with
            member _.VisitInheritSynMemberDefn(_, _, _, _, members, _) =
                let isInLine c =
                    match c with
                    | SynMemberDefn.AutoProperty(xmlDoc = xmlDoc) when containsPosAndNotEmptyAndNotElaborated pos xmlDoc -> Some xmlDoc
                    | SynMemberDefn.ImplicitCtor(xmlDoc = xmlDoc) when containsPosAndNotEmptyAndNotElaborated pos xmlDoc -> Some xmlDoc
                    | _ -> None
                members |> List.tryPick isInLine
                     }

    let source = """
type MyClass() =
    /// fails on AutoProperty
    member val Name = "" with get, set
"""
    let parseTree = parseSourceCode("C:\\test.fs", source)

    match SyntaxTraversal.Traverse(pos, parseTree, visitor) with
    | Some xml -> ()
    | _ -> failwith "Did not visit AutoProperty"

[baronfel]

This is great, thank you for adding it!