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

Use of JsonSerializerContext Can Cause CS1591 #61379

Closed
Tracked by #63918
RehanSaeed opened this issue Nov 9, 2021 · 12 comments · Fixed by #72761
Closed
Tracked by #63918

Use of JsonSerializerContext Can Cause CS1591 #61379

RehanSaeed opened this issue Nov 9, 2021 · 12 comments · Fixed by #72761
Assignees
Labels
area-System.Text.Json Cost:S Work that requires one engineer up to 1 week Priority:1 Work that is critical for the release, but we could probably ship without source-generator Indicates an issue with a source generator feature
Milestone

Comments

@RehanSaeed
Copy link

RehanSaeed commented Nov 9, 2021

Description

Using JsonSerializerContext can cause a CS1591 "Missing XML comment" error on the generated code.

[JsonSerializable(typeof(Foo[]))]
public partial class CustomJsonSerializerContext : JsonSerializerContext
{
}

You cannot suppress CS1591 from a global suppressions file because this is a compiler error. Disabling CS1591 from an .editorconfig file still causes the error.

dotnet_diagnostic.CS1591.severity = none

You also cannot disable the warning in source because the source code is generated in a separate file. It looks like the generated source code needs to include some suppressions.

#pragma warning disable CS1591
[JsonSerializable(typeof(Foo[]))]
public partial class CustomJsonSerializerContext : JsonSerializerContext
{
}
#pragma warning enable CS1591

The only workaround seems to be to globally disable this warning in the project .csproj file, which is not great since I would like to keep this warning.

<PropertyGroup>
    <NoWarn>CS1591</NoWarn>
</PropertyGroup>

Configuration

.NET SDK 6.0.100

Other information

See also:

@mairaw mairaw transferred this issue from dotnet/core Nov 9, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added area-System.Text.Json untriaged New issue has not been triaged by the area owner labels Nov 9, 2021
@ghost
Copy link

ghost commented Nov 9, 2021

Tagging subscribers to this area: @dotnet/area-system-text-json
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Using JsonSerializerContext can cause a CS1591 "Missing XML comment" error on the generated code.

[JsonSerializable(typeof(Foo[]))]
public partial class CustomJsonSerializerContext : JsonSerializerContext
{
}

You cannot suppress CS1591 from a global suppressions file because this is a compiler error. Disabling CS1591 from an .editorconfig file still causes the error.

dotnet_diagnostic.CS1591.severity = none

You also cannot disable the warning in source because the source code is generated in a separate file. It looks like the generated source code needs to include some suppressions.

#pragma warning disable CS1591
[JsonSerializable(typeof(Foo[]))]
public partial class CustomJsonSerializerContext : JsonSerializerContext
{
}
#pragma warning enable CS1591

The only workaround seems to be to globally disable this warning in the project .csproj file, which is not great since I would like to keep this warning.

<PropertyGroup>
    <NoWarn>CS1591</NoWarn>
</PropertyGroup>

Configuration

.NET SDK 6.0.100

Other information

See also:

Author: RehanSaeed
Assignees: -
Labels:

area-System.Text.Json, untriaged

Milestone: -

@eiriktsarpalis
Copy link
Member

Would marking your class internal work around the issue?

@RehanSaeed
Copy link
Author

Would marking your class internal work around the issue?

Thats another great workaround.

@eiriktsarpalis
Copy link
Member

Related to dotnet/roslyn#54119

@layomia
Copy link
Contributor

layomia commented Nov 15, 2021

Marking as 7.0 since the workaround here seems acceptable. We could augment the generator to emit documentation for generated members, but also have to consider scenarios where users want to provide their own documentation. Standing by for community feedback on this.

@layomia layomia removed the untriaged New issue has not been triaged by the area owner label Nov 15, 2021
@layomia layomia added this to the 7.0.0 milestone Nov 15, 2021
@JuanZamudioGBM
Copy link

JuanZamudioGBM commented Jan 19, 2022

If I make the class internal and I making a library, how my clients can invoke the serialization with JsonSerializerContext? Do I have to expose a Serialize method for all my classes (or one with a case per type)? Does every one of the people using my library have to create their own MyContext : JsonSerializerContext?

Or what is the recommend way to expose JsonSerializerContext derived classes so external clients can use it?

@layomia layomia added the source-generator Indicates an issue with a source generator feature label Jan 21, 2022
@Youssef1313
Copy link
Member

The generated file doesn't have to have a documentation comment. You can add the documentation on the partial that's not generated starting with dotnet/roslyn#56419.

@eiriktsarpalis eiriktsarpalis added Priority:1 Work that is critical for the release, but we could probably ship without Cost:M Work that requires one engineer up to 2 weeks labels Feb 7, 2022
@Youssef1313
Copy link
Member

My previous comment is misleading. I thought the diagnostic is issued for the partial CustomJsonSerializerContext type.

The issue is that generator can generate public members without doc comments. The generator should either add #pragma warning disables or properly generate documentation comments.

@JakeYallop
Copy link
Contributor

I'm running into this issue as well.

To allow users to provide their own documentation, would this not require (at least) something like partial properties in order to work, or a fundamental re-design of the generator? At the moment, the way that code takes advantage of the library is via source-generated public properties.

Of the 2 options (either adding comments or adding #pragma warning disable CS1591) I would lean towards the latter for simplicity - but that reflects my experience in that I'm usually using the source generator in the context of ASP .NET Core so I don't directly use the properties on the JsonSerializerContext very often.

@eiriktsarpalis eiriktsarpalis added Cost:S Work that requires one engineer up to 1 week and removed Cost:M Work that requires one engineer up to 2 weeks labels May 10, 2022
@JakeYallop
Copy link
Contributor

What would an acceptable PR look like for this? Are we happy to go with #pragma warning disable CS1591 at the top of each generated file?

@eiriktsarpalis
Copy link
Member

After discussion with the @dotnet/area-system-text-json crew consensus seems to be that we would need to emit relevant XML docs for every generated member that is public.

@layomia layomia self-assigned this May 17, 2022
@eiriktsarpalis
Copy link
Member

Moving to the .NET 8 milestone.

@eiriktsarpalis eiriktsarpalis modified the milestones: 7.0.0, 8.0.0 Jul 19, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jul 25, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jul 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Aug 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Text.Json Cost:S Work that requires one engineer up to 1 week Priority:1 Work that is critical for the release, but we could probably ship without source-generator Indicates an issue with a source generator feature
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants