Issue diagnostic warnings when context types don't indicate serializable types #58768

layomia · 2021-09-07T21:06:34Z

Fixes #58698. I propose porting to 6.0 for improved user experience for the src-gen feature.

With this PR we'll start issuing a diagnostic warning when a context type doesn't indicate serializable types. This warning is only logged when the context type contains no members. We assume that the user wants source generation to provide an implementation, e.g.

internal partial class MyJsonSerializerContext : JsonSerializerContext
{
}

In other cases (i.e. if the context type has at least one member), the user might have a handwritten context for which they don't need source generation. In those cases we don't issue a warning, e.g this example from our tests:

internal partial class JsonContext : JsonSerializerContext
{
    public override JsonTypeInfo? GetTypeInfo(Type type)
    {
        if (type == typeof(JsonMessage))
        {
            return JsonMessage;
        }

        return null;
    }

    public JsonTypeInfo<JsonMessage> JsonMessage => ...;
}

…le types

ghost · 2021-09-07T21:06:44Z

Tagging subscribers to this area: @eiriktsarpalis, @layomia
See info in area-owners.md if you want to be subscribed.

Issue Details

Fixes #58698. I propose porting to 6.0 for improved user experience for the src-gen feature.

With this PR we'll start issuing a diagnostic warning when a context type doesn't indicate serializable types. This warning is only logged when the context type contains no members. We assume that the user wants source generation to provide an implementation, e.g.

internal partial class MyJsonSerializerContext : JsonSerializerContext
{
}

In other cases (i.e. if the context type has at least one member), the user might have a handwritten context for which they don't need source generation. In those cases we don't issue a warning, e.g this example from our tests:

internal partial class JsonContext : JsonSerializerContext
{
    public override JsonTypeInfo? GetTypeInfo(Type type)
    {
        if (type == typeof(JsonMessage))
        {
            return JsonMessage;
        }

        return null;
    }

    public JsonTypeInfo<JsonMessage> JsonMessage => ...;
}

Author:	layomia
Assignees:	layomia
Labels:	`area-System.Text.Json`
Milestone:	6.0.0

layomia · 2021-09-07T21:14:16Z

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

@@ -407,31 +423,25 @@ private static bool TryGetClassDeclarationList(INamedTypeSymbol typeSymbol, [Not
                return typeGenerationSpec;
            }

-            internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) => node is ClassDeclarationSyntax { AttributeLists: { Count: > 0 }, BaseList: { Types : {Count : > 0 } } };
+            internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) => node is ClassDeclarationSyntax { BaseList: { Types : { Count : > 0 } } };


For diffs between here and EOF, I'm relaxing the criteria for types we consider for generation

Before: include all classes that are annotated with a type called "JsonSerializableAttribute", and also derive or implement at least one type.
After: include all classes that derive from a type called "JsonSerializerContext".

I believe this change is fine since we'll have about the same number of types to consider in most source gen scenarios.

Note that this could impact perf. I would test the before/after of this change and ensure for large solutions that target net6.0 you aren't negatively impacting the perf.

I can get you a large solution offline, if you need it to test.

It is possible this change could also improve perf, depending on the individual performance attributes vs. base classes, and of course the actual solution and its types (do they have base classes, attributes, etc).

@eerhardt how do you suggest measuring perf? Manual measuring the VS experience?

@ericstj made a micro benchmark for source generators: https://github.com/ericstj/sample-code/tree/generatorBenchmark

But honestly, I would measure it in VS with a large solution. The way I measured the time taken was to add an EventSource around the source generator implementation. And then use PerfView to see how much time was spent in the source generator when it wasn't doing any work.

But if you are using the latest Roslyn builds (as of 5 days ago), this timing measurement is built in. See dotnet/roslyn#55892.

One possible way to mitigate would be check the member count of the type. In pseudocode:

internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) => node is ClassDeclarationSyntax { { BaseList: { Types : { Count : > 0 } } and { AttributeLists: { Count: > 0 } } or { MemberList: { Count: 0 } } };

Note that this would contradict my feedback in https://github.com/dotnet/runtime/pull/58768/files#r704458155.

steveharter · 2021-09-08T14:07:07Z

src/libraries/System.Text.Json/gen/Resources/Strings.resx

@@ -130,7 +130,7 @@
    <value>Did not generate serialization metadata for type.</value>
  </data>
  <data name="ContextClassesMustBePartialMessageFormat" xml:space="preserve">
-    <value>Derived 'JsonSerializerContext' type '{0}' specifies JSON-serializable types. The type and all containing types must be made partial to kick off source generation.</value>
+    <value>Derived 'JsonSerializerContext' type '{0}' and all containing types must be made partial to kick off source generation.</value>


Suggest rewording to avoid "kick off source generation":
"..and all containing types must be made partial for source generation."

(here and elsewhere)

eiriktsarpalis · 2021-09-08T14:10:44Z

src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs

                    if (serializableAttributeList == null)
                    {
                        // No types were indicated with [JsonSerializable]
+
+                        // If there are no members on the context, assume that the user wanted source generation and notify them that there are no serializable types.
+                        if (!contextTypeSymbol.MemberNames.Any())


My feeling is that we should refine this to check for the existence of GetTypeInfo overrides. A user that unintentionally adds any member to their context class (be it a helper method, type, etc.) would see this diagnostic disappear completely.

eiriktsarpalis · 2021-09-08T14:36:54Z

src/libraries/System.Text.Json/gen/Resources/xlf/Strings.cs.xlf

@@ -3,15 +3,25 @@
  <file datatype="xml" source-language="en" target-language="cs" original="../Strings.resx">
    <body>
      <trans-unit id="ContextClassesMustBePartialMessageFormat">
-        <source>Derived 'JsonSerializerContext' type '{0}' specifies JSON-serializable types. The type and all containing types must be made partial to kick off source generation.</source>
-        <target state="translated">Odvozený typ JsonSerializerContext {0} určuje typy serializovatelné na JSON. Typ a všechny obsahující typy musí být částečné, aby se zahájilo generování zdroje.</target>
+        <source>Derived 'JsonSerializerContext' type '{0}' and all containing types must be made partial to kick off source generation.</source>


If we do backport this to release/6.0 now, isn't there a risk of translations going out of sync?

We have a 1 week SLA for translations in main. We have enough time to backport to 6.0. That backport will need to be tracked with an issue so we don't forget.

eiriktsarpalis · 2021-09-08T14:37:22Z

This is a good quality of life improvement, although I'm not necessarily convinced it meets the bar for a 6.0 backport.

layomia · 2021-09-08T17:46:15Z

although I'm not necessarily convinced it meets the bar for a 6.0 backport.

I can see that, but would we consider it a breaking change to go from no diagnostic to issuing one in a future release?

ericstj · 2021-09-08T18:08:59Z

It fails with an error today, right? I don't think adding a diagnostic to something that already fails to compile is breaking. Even cases where we add diagnostics to passing code is acceptable in a new release (this is what the roslyn-analyzers repo does with every new analyzer).

Is the case this diagnostic fires always an error case? In that sense I'm not sure anyone would even see the diagnostic as users tend to prioritize errors over warnings. Or are we filling in the abstracts with empty implementations?

layomia · 2021-09-10T13:23:09Z

@ericstj, yes this is already a failure case (due to CS0534) so adding a diagnostic in the future wouldn't be breaking.

Is the case this diagnostic fires always an error case?

It's not always an error case (depending on user intent), and we don't touch the user context unless they indicate serializable types and derive from JsonSerializerContext.

I'll close this PR for 6.0 and we can address in 7.

Issue diagnostic warning when context types don't indicate serializab…

e77c51a

…le types

layomia added the area-System.Text.Json label Sep 7, 2021

layomia added this to the 6.0.0 milestone Sep 7, 2021

layomia requested review from eiriktsarpalis, steveharter, eerhardt and chsienki September 7, 2021 21:06

layomia self-assigned this Sep 7, 2021

layomia commented Sep 7, 2021

View reviewed changes

steveharter reviewed Sep 8, 2021

View reviewed changes

eiriktsarpalis reviewed Sep 8, 2021

View reviewed changes

layomia closed this Sep 10, 2021

layomia mentioned this pull request Sep 10, 2021

Issue diagnostic warnings when custom JsonSerializerContext types don't indicate serializable types #58698

Open

ghost locked as resolved and limited conversation to collaborators Oct 10, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue diagnostic warnings when context types don't indicate serializable types #58768

Issue diagnostic warnings when context types don't indicate serializable types #58768

layomia commented Sep 7, 2021

ghost commented Sep 7, 2021

layomia Sep 7, 2021

eerhardt Sep 7, 2021

steveharter Sep 8, 2021

eerhardt Sep 8, 2021

eiriktsarpalis Sep 8, 2021

steveharter Sep 8, 2021 •

edited

Loading

eiriktsarpalis Sep 8, 2021

eiriktsarpalis Sep 8, 2021

ericstj Sep 8, 2021

eiriktsarpalis commented Sep 8, 2021

layomia commented Sep 8, 2021

ericstj commented Sep 8, 2021

layomia commented Sep 10, 2021

Issue diagnostic warnings when context types don't indicate serializable types #58768

Issue diagnostic warnings when context types don't indicate serializable types #58768

Conversation

layomia commented Sep 7, 2021

ghost commented Sep 7, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

steveharter Sep 8, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eiriktsarpalis commented Sep 8, 2021

layomia commented Sep 8, 2021

ericstj commented Sep 8, 2021

layomia commented Sep 10, 2021

steveharter Sep 8, 2021 •

edited

Loading