dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.06k stars 4.69k forks source link

Use of JsonSerializerContext Can Cause CS1591 #61379

Closed RehanSaeed closed 2 years ago

RehanSaeed commented 2 years ago

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:

ghost commented 2 years ago

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. ``` CS1591 ``` ### Configuration .NET SDK 6.0.100 ### Other information See also: - https://github.com/dotnet/roslyn/issues/12702
Author: RehanSaeed
Assignees: -
Labels: `area-System.Text.Json`, `untriaged`
Milestone: -
eiriktsarpalis commented 2 years ago

Would marking your class internal work around the issue?

RehanSaeed commented 2 years ago

Would marking your class internal work around the issue?

Thats another great workaround.

eiriktsarpalis commented 2 years ago

Related to https://github.com/dotnet/roslyn/issues/54119

layomia commented 2 years ago

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.

JuanZamudioGBM commented 2 years ago

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?

Youssef1313 commented 2 years ago

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 https://github.com/dotnet/roslyn/pull/56419.

Youssef1313 commented 2 years ago

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 commented 2 years ago

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.

JakeYallop commented 2 years ago

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 commented 2 years ago

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.

eiriktsarpalis commented 2 years ago

Moving to the .NET 8 milestone.