Skip to content

Tags in a free-text XML doc comment are silently escaped; add a warning and XML doc refactorings #20661

Description

@xperiandri

A /// comment that does not start with < is treated as free text: the compiler escapes every line of the block and wraps the result in <summary>. So tags written after free text, or inside it, are silently turned into literal text:

/// Typecheck a single file.
/// <returns>A callback that adds the checked result to the state.</returns>
let CheckOneInputWithCallback ... = ...

Here <returns> ends up as &lt;returns&gt;... inside the summary. The same happens to <param>, <remarks>, and inline <see cref="..."/>, <c> or <paramref>. The consequences:

  • Nothing is reported, not even with --warnon:3390. XmlDoc.Check only sees the already-escaped text, so it finds no <param> to validate.
  • QuickInfo shows the tags as raw markup in the summary, and signature help shows no parameter docs.
  • The generated .xml file has no <param>/<returns> elements.

The repository has instances of this, for example src/Compiler/Driver/ParseAndCheckInputs.fs (CheckOneInputWithCallback) and src/Compiler/Facilities/DiagnosticsLogger.fsi (a <remarks> after free text). Related: #11611.

Proposal

  1. A warning when a free-text doc comment contains XML tags, either inline or on following lines. The free-text form stays valid and gets no warning when it has no tags.

  2. Wrap in <summary>: a code fix for that warning, and the same action offered as a refactoring on any free-text comment. The reverse refactoring turns a comment that is only a <summary> back into the short form.

  3. Add missing <param> tags, modelled on Roslyn's "Add missing param nodes" (CS1573). Each new tag goes after the tag of the previous parameter in declaration order, or after <summary>. For a primary constructor, the tags go on the constructor's own doc between the type name and (, never on the type's comment:

    /// <summary>Translates the events of one subscription into payloads.</summary>
    type internal SubscriptionPayloads
        /// <param name="logger">The logger of the connection.</param>
        /// <param name="options">The options of the middleware.</param>
        (logger: ILogger, options: Options) =

Questions

@T-Gro, before I implement this:

  1. Where should the warning live?
    • (a) In the compiler, as a new message in the FS3390 family, checked on UnprocessedLines before escaping. It would be shared by builds, VS and other IDEs, but it only shows where 3390 is on (the VS templates and this repository turn it on). The instances above would need fixing in the same PR.
    • (b) An IDE-only analyzer in FSharp.Editor with its own number, like FS3583, always on in VS but invisible to builds.
  2. Code fix or refactoring? Per your rule in Refactoring for _.Property / _.MethodCall() / _.IndexerAccess[idx] shorthand for accessor functions #16234, I would make the wrap action a code fix only for the warning case and a refactoring otherwise. "Add missing param tags" would be a refactoring, since Roslyn's trigger (CS1573) needs at least one existing <param>, while FS3390's "no documentation for parameter" already fires in that case. Would you rather bind it to that FS3390 message as a code fix, like Roslyn does?
  3. Primary constructor docs: is the placement above (constructor <param>s between the name and () the one you want the tooling to generate? The compiler accepts constructor <param>s on the type's comment too. Separately, the ///< template generator (XmlDocParser.GetXmlDocables) produces nothing for a primary constructor today. Should that be fixed in the same change?

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Type

    No type

    Projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions