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

Figure out XML docs for positional records #44571

Closed
jcouv opened this issue May 26, 2020 · 24 comments · Fixed by #49134
Closed

Figure out XML docs for positional records #44571

jcouv opened this issue May 26, 2020 · 24 comments · Fixed by #49134

Comments

@jcouv
Copy link
Member

jcouv commented May 26, 2020

How do we specify xml docs for positional members (which define both parameters and properties)?

record Person(string First, string Last);
@jcouv
Copy link
Member Author

jcouv commented Jul 16, 2020

From discussion on 7/15/2020, we're going to go with a minimalist approach to start:

  1. compiler will carry over <param> tags from record types to constructor parameters
  2. IDE may choose to adjust inheritance logic so that properties show some information from associated parameter
  3. synthesized methods (Equals and such) won't include any boilerplate documentation

@333fred
Copy link
Member

333fred commented Aug 27, 2020

Make sure that CS1591 (missing XML comments) is not reported for these carry-overs.

@RikkiGibson
Copy link
Contributor

Is further action needed by the compiler team for 16.8?

@jcouv
Copy link
Member Author

jcouv commented Sep 18, 2020

@RikkiGibson Yes, we'd like to do (1) from plan above: "compiler will carry over tags from record types to constructor parameters".
This can be pushed out to 16.9 if necessary.

@jaredpar jaredpar modified the milestones: 16.8, 16.9 Oct 12, 2020
@buvinghausen
Copy link

@jcouv is there currently any way to get the compiler not to throw the CS1591 warning other than doing a pragma warning disable?

I've tried lots of options and usually they add even more warnings than just the CS1591.

@jcouv jcouv self-assigned this Oct 29, 2020
@jcouv
Copy link
Member Author

jcouv commented Oct 31, 2020

@buvinghausen The CS1591 is complaining that the properties don't have documentation. One solution at the moment is to use explicit properties (public C Name { get; init; } = Name;) and place documentation on it.

@buvinghausen
Copy link

@jcouv I understand the context of CS1591. I was hoping there was a workaround other than using explicit properties or not having any documentation (via pragma disable) at all. Bummer. Hopefully this gets sorted soon. Thank you for the clarification.

@RikkiGibson
Copy link
Contributor

RikkiGibson commented Nov 14, 2020

Let's make sure that completion works properly on nominal records as well, particularly in the following scenarios analogous to method parameters:

param-doc-completion-1

param-doc-completion-2

That is, typing a /// on the line before a record declaration should include <param> tags for all the record primary constructor parameters, and within a /// block the <param> elements should be suggested with appropriate parameter names, based on which parameters haven't been documented yet.

@jcouv jcouv modified the milestones: 16.9, 16.9.P3 Jan 5, 2021
@jcouv jcouv removed the 4 - In Review A fix for the issue is submitted for review. label Jan 5, 2021
@maxild
Copy link

maxild commented Jan 26, 2021

@jcouv Two questions

  1. Is this the correct way of using this feature in todays sdk-5.0.102 compiler version (and of course have no IDE support or anything for the 'xml doc strings' on the compiler generated properties)
#pragma warning disable CS1572
#pragma warning disable CS1573
/// <summary>
/// Some summary...
/// </summary>
/// <param name="SomeNumber">SomeNumber xml doc string</param>
public record SomeRecord(int SomeNumber);
#pragma warning restore CS1572
#pragma warning restore CS1573

I would like to avoid writing,

/// <summary>
/// Some summary...
/// </summary>
public record SomeRecord(int SomeNumber) 
{

        /// <summary>
        /// SomeNumber xml doc string
        /// </summary>
        public int SomeNumber { get; } = SomeNumber; // workaround
}

and just what for the above to work properly in a future compiler release

  1. Is it planned for what compiler/sdk version?

@jcouv
Copy link
Member Author

jcouv commented Jan 26, 2021

@maxild Yes, I think that's the best you can do in sdk-5.0.102.
One way to check that the compiler is sufficiently recent is to add #error version in your source and see that the version is 3.9 or above, but that's still only available in preview (the fix went into preview 3 of 3.9).

Tagging @marcpopMSFT to confirm: I expect the fix (ie. roslyn/compiler version 3.9) will be ship via .NET SDK 5.0.2xx.

With that fix, the following two forms will be allowed without pragma suppressions:

/// <summary>Summary <paramref name="I1"/></summary>
/// <param name="I1">Description for I1</param>
public record C(int I1);


/// <summary>Summary</summary>
/// <param name="I1">Description for I1</param>
public record C(int I1)
{
    /// <summary>Property summary</summary>
    public int I1 { get; init; } = I1;
}

@maxild
Copy link

maxild commented Jan 26, 2021

@jcouv I added #error version to my source.

XXXXXXXXXX.cs(5, 8): [CS8304] Compiler version: '3.8.0-5.20570.14 (6559f38c)'. Language version: 9.0.

Nice way to inspect compiler version

dotnet /usr/share/dotnet/sdk/5.0.101/Roslyn/bincore/csc.dll -version
3.8.0-5.20570.14 (6559f38c)

It would be sweet to add msbuild/nuget/roslyn version to dotnet --info. I always strugle to understand github milestones (16.9, 3.9, 5.0.2xx etc) on dotnet/xxxx repos. Of course I am begining to understand how the massive sdk is a container of sub-components and align with visual studio releases using the feature-band(-semver-hack)....

@marcpopMSFT
Copy link
Member

I can confirm that the most recent version of Roslyn shipping with 5.0.2xx is 3.9.0-3.21056.4. I also filed a bug for your suggestion about including more information in dotnet --info on versions of the various tools included in the SDK. Good idea.

@maxild
Copy link

maxild commented Jan 27, 2021

@marcpopMSFT Thanks for your quick response...Looking forward to 5.0.2xx. 🚀

I also filed a bug for your suggestion about including more information in dotnet --info on versions of the various tools included in the SDK. Good idea.

Can you send me a link to the issue, or is it internal?

@marcpopMSFT
Copy link
Member

@maxild , do you see the link to dotnet/sdk#15496 above here in the GH UI?

@danstur
Copy link

danstur commented Feb 28, 2021

@marcpopMSFT Is there any way how to figure out in what Visual Studio version this will be available? Or will it be enough to install the right dotnet SDK and get the goodness even without VS update?

@jcouv
Copy link
Member Author

jcouv commented Feb 28, 2021

@danstur This was fixed in Visual Studio 16.9

@maxild
Copy link

maxild commented Feb 28, 2021

@jcouv and/or @marcpopMSFT I see here https://docs.microsoft.com/en-us/visualstudio/releases/2019/release-notes-preview that we are at preview5. But I couldn't find any info about when 16.9 will RTM?

@maxild
Copy link

maxild commented Mar 3, 2021

I can confirm that the most recent version of Roslyn shipping with 5.0.2xx is 3.9.0-3.21056.4.

After using 5.0.200 released today the version is actually greater

$ dotnet /usr/share/dotnet/sdk/5.0.200/Roslyn/bincore/csc.dll -version  
3.9.0-5.21120.8 (accdcb77)

@adamjones1
Copy link

Using the .Net SDK 5.0.1xx to document properties I was able to do:

public record MyRecord(
    /// <summary>
    /// The first field
    /// </summary>
    string Field1,

    /// <summary>
    /// The second field
    /// </summary>
    string Field2
);

I got a CS1587 warning that the XML comments were on invalid language elements but it still worked fine in the generated doc and I was able to suppress the warning, assuming it would later get fixed down the line to stop being emitted. In 5.0.2xx this doesn't work and the field descriptions aren't written to the XML doc.

Could we reenable this form as well as the <param></param> form? I find it a far more readable and cohesive style of adding descriptions (also more consistent with regular C# classes and FWIW F# records) when the description is alongside the symbol it's describing, especially with larger records (which is my predominant use case), and it doesn't have the obvious drawbacks of redeclaring the property.

@danstur
Copy link

danstur commented Apr 11, 2021

@jcouv I just tried it with VS 16.9.3 and it still seems to not work as expected. I guess I'm not using the correct syntax for the declaration?

Using the following definitions:

/// <summary>
/// Class description.
/// </summary>
/// <param name="Baz">Foo-Baz description</param>
public sealed record Foo(string Baz)
{
}

public sealed class Bar
{
	/// <summary>
	/// Bar-Baz description
	/// </summary>
	public string Baz { get; init; }

	public Bar(string baz)
	{
		Baz = baz;
	}
}

I get the following results:
Record
Classic

The generated documentation file also does not include the information:

<?xml version="1.0"?>
<doc>
    <assembly>
        <name>Test</name>
    </assembly>
    <members>
        <member name="T:Test.Foo">
            <summary>
            Class description.
            </summary>
            <param name="Baz">Foo-Baz description</param>
        </member>
        <member name="M:Test.Foo.#ctor(System.String)">
            <summary>
            Class description.
            </summary>
            <param name="Baz">Foo-Baz description</param>
        </member>
        <member name="P:Test.Bar.Baz">
            <summary>
            Bar-Baz description
            </summary>
        </member>
    </members>
</doc>

@erichiller
Copy link

erichiller commented Apr 13, 2021

Same here, that is, the docs for the shorthand property are written as <param... on both the type (T:Test.Foo), and the constructor (M:Test.Foo.#ctor(System.String)).
This means that <inheritdoc cref="Test.Foo.Baz"/> does not work.

@InspiringCode
Copy link

I just reproduced this behavior with 16.10.2. Please fix this, it would really make records more useful.

@jcouv I just tried it with VS 16.9.3 and it still seems to not work as expected. I guess I'm not using the correct syntax for the declaration?

Using the following definitions:

/// <summary>
/// Class description.
/// </summary>
/// <param name="Baz">Foo-Baz description</param>
public sealed record Foo(string Baz)
{
}

public sealed class Bar
{
	/// <summary>
	/// Bar-Baz description
	/// </summary>
	public string Baz { get; init; }

	public Bar(string baz)
	{
		Baz = baz;
	}
}

I get the following results:
Record
Classic

The generated documentation file also does not include the information:

<?xml version="1.0"?>
<doc>
    <assembly>
        <name>Test</name>
    </assembly>
    <members>
        <member name="T:Test.Foo">
            <summary>
            Class description.
            </summary>
            <param name="Baz">Foo-Baz description</param>
        </member>
        <member name="M:Test.Foo.#ctor(System.String)">
            <summary>
            Class description.
            </summary>
            <param name="Baz">Foo-Baz description</param>
        </member>
        <member name="P:Test.Bar.Baz">
            <summary>
            Bar-Baz description
            </summary>
        </member>
    </members>
</doc>

@AgentFire
Copy link

Same in 16.11.1, unfortunately.

@jcouv
Copy link
Member Author

jcouv commented Oct 15, 2021

@danstur @InspiringCode @AgentFire Track issue #52663 for upcoming design change (aiming for 17.1).

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

Successfully merging a pull request may close this issue.

13 participants