TipFormatter is stricker than before can we try to make it more tolerant/user-friendly?

[MangelMaxime]

Hello,

the new TipFormatter is now working using an XML parser this made the implementation easier but has the drawback of failing on invalid XML.

This drawback is the same as in VS2019.

In VS2019, if the doc comment cannot be parsed, then an empty tooltip is shown.

When using FSAC, we show this tooltip:

If we look at the output it will show:

[09:28:48 WRN] [TipFormatter] TipFormatter - Error while parsing the doc comment
System.Xml.XmlException: The 'summary' start tag on line 3 position 3 does not match the end tag of 'fake'. Line 3, position 51.
   at System.Xml.XmlTextReaderImpl.Throw(Exception e)
   at System.Xml.XmlTextReaderImpl.Throw(String res, String[] args)
   at System.Xml.XmlTextReaderImpl.ThrowTagMismatch(NodeData startTag)
   at System.Xml.XmlTextReaderImpl.ParseEndElement()
   at System.Xml.XmlTextReaderImpl.ParseElementContent()
   at System.Xml.XmlTextReaderImpl.Read()
   at System.Xml.XmlLoader.LoadNode(Boolean skipOverWhitespace)
   at System.Xml.XmlLoader.LoadDocSequence(XmlDocument parentDoc)
   at System.Xml.XmlLoader.Load(XmlDocument doc, XmlReader reader, Boolean preserveWhitespace)
   at System.Xml.XmlDocument.Load(XmlReader reader)
   at System.Xml.XmlDocument.LoadXml(String xml)
   at FsAutoComplete.TipFormatter.buildFormatComment(FSharpXmlDoc cmt, FormatCommentStyle formatStyle, FSharpOption`1 typeDoc) in D:\Programowanie\ionide\paket-files\github.com\fsharp\FsAutoComplete\src\FsAutoComplete.Core\TipFormatter.fs:line 975

Not all the error messages are that clear from the XML parser

Failing case

Non closed tag

/// <summary>Here is an example of a bulleted list:
let a = ""

For this case, we should be able to parse the exception text The 'summary' start tag on line 3 position 3 does not match the end tag of 'fake' and provide a user-friendly error message in the tooltip.

Usage of special characters in the text

/// <summary>Here is an example of a bulleted list: `Result<string>`</summary>
let b = ""

In this case, the official documentation says that the user should escape the XML special chars. User should have written:

/// <summary>Here is an example of a bulleted list: Result&lt;string&gt;</summary>

Should we try to identify non-escaped special chars and escape them before parsing the XML?

[Krzysztof-Cieslak]

Yeah, those additional suggestions seem fine, I guess?

[MangelMaxime]

It seems like Ionide 5.4.0 shows a warning when the doc comment is invalid.

I suppose this is coming from the latest FCS which add more warning/analyser thing.

The good news is that it is now easier for the user to access the error message. However, even if I provide a special handling for the case listed above we will still see the warning on the doc comment text.

@cartermp Will we be able to port the changes to the FCS too; to not show a warning in some cases. Or is FCS strict over the doc comment definition?

For the case I am speaking about here, it is mostly to relax/workaround the XML parser on some circumstance.

[MangelMaxime]

To give a bit more context:

One way to detect non closed tag is to use a Regex:

Disclaimer: This Regex isn't 100% safe yet it is just to demonstrate the idea

See how it detect the <string> tag. We could then decide to escape the < and > from it to generate a valid XML. Of course, we can prevent behaviour on standard/reserved tag like <summary>, <c>, etc. like that if the user forgot to close the <summary> tag he will still have the warning asking to fix it.

[MangelMaxime]

I did try to find a way to detect char we want to escape and it is not that simple.

Looking at FSharp.Core source code for reference it seems like the choice was made to respect the "Doc comment specification"

https://github.com/dotnet/fsharp/blob/4ebc0f1768ee3eaf01b898fc5e3065d0210a4852/src/fsharp/FSharp.Core/option.fsi#L403

So it seems like it is up to the user to escape the special char.

I am closing this issue as FSAC seems currently aligned with the spec and the analyzer used by FCS 39. If the situation change we will take a look again.