Skip to content

Conversation

@TIHan
Copy link
Contributor

@TIHan TIHan commented Apr 21, 2021

This adds the ability for FCS to read XML documentation without having to rely on a third-party; it's just integrated in.
Not only will this allow other editors to leverage reading XML documentation easily from FCS, it will also allow metadata-as-source features to print out the XML documentation.

This is a pretty big change mainly due to having to plumb InfoReader around.

  • Cleanup layout calls to ensure they take a *Ref version of properties/meths/fields/etc.
  • Weak cache for XMLDocument
  • Tests

/// Try to load xml documentation associated with an assembly by the same file path with the extension ".xml".
/// If that fails, look to see if it exists in ILModuleDef's resources.
member _.TryLoad(assemblyFileName, ilModule) =
let xmlFileName = Path.ChangeExtension(assemblyFileName, ".xml")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should first look for the localized XML doc file, see https://docs.microsoft.com/en-us/dotnet/core/install/localized-intellisense

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e.g. if FSharp.Core.dll is at

C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\FSharp.Core.dll

then if we actually shipped localized XML doc for FSHarp.Core then it would be located at

C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\de\FSharp.Core.xml

C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\zh-hans\FSharp.Core.xml

etc. I don't know the exact rules for how to look, e.g. does it search through preferred English language versions:

C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\en-us\FSharp.Core.xml
C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\en-gb\FSharp.Core.xml
C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\en\FSharp.Core.xml
C:\Users\Administrator\.nuget\packages\fsharp.core\5.0.1\lib\netstandard2.0\FSharp.Core.xml

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After talking with @brettfo and @KevinRansom - it seems .NET as a whole does not handle localizations for XML documentation on libraries.

We could, in theory, add support for it ourselves, but I would wait until .NET as a whole decides what the rules are, whenever or if that happens.

@TIHan TIHan changed the title [WIP] Integrated XML documentation reading in FCS Integrated XML documentation reading in FCS Apr 27, 2021
@TIHan
Copy link
Contributor Author

TIHan commented Apr 27, 2021

This is ready.

@TIHan TIHan requested a review from cartermp April 27, 2021 20:11
open FSharp.Compiler.SyntaxTreeOps
open FSharp.Compiler.Text
open FSharp.Compiler.Text.Range
open FSharp.Compiler.Xml
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't notice the addition of this namespace. I don't think we should be using it as it's not about XML as a thing, but about F# documentation

In any case I'd like to make sure all FCS API changes get carefully reviewed - please make sure I sign off on them for now - we need this to be iterating towards being fully stable

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see the logic in moving out of "Syntax" since it's not purely syntax but also additional logic.

Perhaps either of these:

namespace FSharp.Compiler.XmlDocumentation
namespace FSharp.Compiler.XmlDocs

though we could also look forward to if we one day support Markdown docs, in which case

namespace FSharp.Compiler.CodeDocumentation

may be best (to also have corresponding types like MarkdownDoc etc.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants