Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XML comments on partial methods aren't merged #60730

Open
stephentoub opened this issue Apr 13, 2022 · 12 comments
Open

XML comments on partial methods aren't merged #60730

stephentoub opened this issue Apr 13, 2022 · 12 comments
Assignees
Milestone

Comments

@stephentoub
Copy link
Member

Version Used:
4.2.0-3.22181.8 (a59a22c)

Steps to Reproduce:

class Program
{
    public static void Main() { }
}

public partial class C
{
    /// <summary>hello</summary>
    public partial void M();
}

public partial class C
{
    /// <remarks>world</remarks>
    public partial void M() { }
}

Expected Behavior:
The XML comments from both partials will be used.

Actual Behavior:
Only the XML comments from one or the other partial is used. If you look at the generated .xml file, for example, it includes just the "world" remarks from the second M():

<?xml version="1.0"?>
<doc>
    <assembly>
        <name>ReproApp</name>
    </assembly>
    <members>
        <member name="M:C.M">
            <remarks>world</remarks>
        </member>
    </members>
</doc>

cc: @RikkiGibson

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 13, 2022
@RikkiGibson RikkiGibson self-assigned this Apr 13, 2022
@jcouv
Copy link
Member

jcouv commented Apr 14, 2022

Without any spec to support my intuition, I would have expected only the docs from the declaration to count, but I guess merging them would be okay too.
I'm going to mark the current behavior as a bug, but let Rikki describe what the current logic is and confirm as needed.

@jcouv jcouv added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Apr 14, 2022
@jcouv jcouv modified the milestones: Backlog, Compiler.Next Apr 14, 2022
@stephentoub
Copy link
Member Author

Thanks, @jcouv. In my case, the motivation is a source generator being able to augment any comments the user may have written.
image

@stephentoub
Copy link
Member Author

@RikkiGibson, do we expect this to be fixed for C# 11 / .NET 7?

@Youssef1313
Copy link
Member

This current behavior is per dotnet/csharplang#5193.

@RikkiGibson Let me know what the updated rules should be (especially for when a partial definition exists without an implementation part). I'm interested in taking a look at DocumentationCommentCompiler :)

@smoogipoo
Copy link

smoogipoo commented May 30, 2023

It would also be awesome to see this for type declarations. Our project extensively uses source generators to add partial definitions to over 90% of classes, and some of those generators would benefit greatly from being able to augment the existing XML comments on the class itself.

@RikkiGibson
Copy link
Contributor

I don't have a lot of bandwidth to look at this in the immediate term. But I'd be happy to review any proposal for how augmentation of XML comments would work, which takes into account:

  • the existing discussions on this topic
  • how the multiple comments are ordered, especially on type declarations.
  • whether multiple <summary> elements, for example, would be included, or if their inner contents are somehow merged
  • how tools which consume doc comments are likely to be impacted
  • etc...

If somebody decides to write such a proposal, it would be good to post it as a discussion in csharplang.

@jaredpar jaredpar modified the milestones: Compiler.Next, Backlog Sep 12, 2023
@ThaDaVos
Copy link

Any update on this? I see that the last comment is from a year ago

@CyrusNajmabadi
Copy link
Member

There is no update on this

@ThaDaVos
Copy link

That's sad - but partially understandable

@ThaDaVos
Copy link

ThaDaVos commented Aug 1, 2024

I don't have a lot of bandwidth to look at this in the immediate term. But I'd be happy to review any proposal for how augmentation of XML comments would work, which takes into account:

* the existing discussions on this topic

* how the multiple comments are ordered, especially on type declarations.

* whether multiple `<summary>` elements, for example, would be included, or if their inner contents are somehow merged

* how tools which consume doc comments are likely to be impacted

* etc...

If somebody decides to write such a proposal, it would be good to post it as a discussion in csharplang.

@RikkiGibson

EDIT: Please vote thumbs up if you agree - or thumbs down if you don't but also comment (with a quote to mine) why you don't like it

I would make a proposal, maybe for the time being implement it the same way as done with CLASS comments, so concatenate them - maybe preferably based on alphabetical file name order or something. You could go a step further and merge the <summary tags. I think this would give the least impact to external tools and do what people already expect as this is already done for class comments.

Currently the following two partial classes:

/// MyLibary.cs
/// <summary>
/// MyLibrary class.
/// </summary>
public partial class MyLibrary
{
    /// <summary>
    /// Add two integers.
    /// </summary>
    /// <param name="a"></param>
    /// <param name="b"></param>
    /// <returns xmlns:x="@" x:type.clarion="real" x:type.javascript="int64">integer</returns>
    public static partial int Add(int a, int b);
}
// MyLibrary.part.cs
/// <generation>
///     <source>MyNative/External.cs</source>
///     <generation>1</generation>
///     <hash>1</hash>
///     <date>2021-10-01</date>
///     <author>ArjSoftware</author>
/// </generation>
public partial class MyLibrary
{
    /// <generation>
    ///     <source>MyNative/External.cs</source>
    ///     <generation>1</generation>
    ///     <hash>1</hash>
    ///     <date>2021-10-01</date>
    /// </generation>
    public static partial int Add(int a, int b)
    {
        return a + b;
    }
}

Result in the following XML documentation:

<?xml version="1.0"?>
<doc>
    <assembly>
        <name>MyNative</name>
    </assembly>
    <members>
        <member name="T:Test.MyLibrary">
            <summary>
            MyLibrary class.
            </summary>
            <generation>
                <source>MyNative/External.cs</source>
                <generation>1</generation>
                <hash>1</hash>
                <date>2021-10-01</date>
                <author>ArjSoftware</author>
            </generation>
        </member>
        <member name="M:Test.MyLibrary.Add(System.Int32,System.Int32)">
            <generation>
                <source>MyNative/External.cs</source>
                <generation>1</generation>
                <hash>1</hash>
                <date>2021-10-01</date>
            </generation>
        </member>
    </members>
</doc>

As you can see - the XML doc for the partial method is overridden - the implementation takes precedence over the definition sadly when working with source generators, where the implementatin is generated from a source generator - if it also generates XML doc, the doc added on the definition is currently lost unless there's a way for a source generator to copy this over - it still leaves it up to the author to do it.

So my proposal - just concatenate it as is already done for the CLASS - maybe order them in a logical order of <summary>, <param>, <returns> and <remarks> if you go with XML merging instead of concatenating - then I would also expect the <summary> contents to be merged - so internally concatenated. I think doc-gen tools shouldn't have issue with either way.
Maybe make implementation take precedence when duplicates are encountered - so for example duplicate for a parameter (or make this configurable through csproj if it should merge/takeDecleration/takeImplementation)

Some tags/references to hopefully speed a discussion about this up:

@alrz
Copy link
Contributor

alrz commented Aug 1, 2024

Love this idea. I wanted to add xmldocs to public methods with their impl (which is mostly configuration)

// source
public partial void ConfigureLoggingDefaults()
{
    services.AddLogging(options => { ... })
}
// generated
/// <summary>
/// <code>
/// services.AddLogging(options => { ... })
/// </code>
/// </summary>
public partial void ConfigureLoggingDefaults();

This doesn't need "merging" xml comments as long as the source doesn't have any comments of its own.

@ThaDaVos
Copy link

ThaDaVos commented Aug 1, 2024

Love this idea. I wanted to add xmldocs to public methods with their impl (which is mostly configuration)

// source
public partial void ConfigureLoggingDefaults()
{
    services.AddLogging(options => { ... })
}
// generated
/// <summary>
/// <code>
/// services.AddLogging(options => { ... })
/// </code>
/// </summary>
public partial void ConfigureLoggingDefaults();

This doesn't need "merging" xml comments as long as the source doesn't have any comments of its own.

@alrz
This seems already to be the case - it uses the doc from the declaration if there's none on the implementation - at least when you manually write both files - haven't checked for source generation. I just tried with my above shared code and removed the doc from the implementation, then dotnet build and it shows the declaration doc inside the XML

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

No branches or pull requests

9 participants