Issue diagnostic warnings when context types don't indicate serializable types #58768
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -33,7 +33,7 @@ private sealed class Parser | |
| private const string JsonPropertyNameAttributeFullName = "System.Text.Json.Serialization.JsonPropertyNameAttribute"; | ||
| private const string JsonPropertyOrderAttributeFullName = "System.Text.Json.Serialization.JsonPropertyOrderAttribute"; | ||
| private const string JsonSerializerContextFullName = "System.Text.Json.Serialization.JsonSerializerContext"; | ||
| private const string JsonSerializableAttributeFullName = "System.Text.Json.Serialization.JsonSerializableAttribute"; | ||
| private const string JsonSourceGenerationOptionsAttributeFullName = "System.Text.Json.Serialization.JsonSourceGenerationOptionsAttribute"; | ||
|
|
||
| private readonly Compilation _compilation; | ||
|
|
@@ -94,6 +94,14 @@ private sealed class Parser | |
| defaultSeverity: DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true); | ||
|
|
||
| private static DiagnosticDescriptor ContextClassesMustIndicateSerializableTypes { get; } = new DiagnosticDescriptor( | ||
| id: "SYSLIB1032", | ||
| title: new LocalizableResourceString(nameof(SR.ContextClassesMustIndicateSerializableTypesTitle), SR.ResourceManager, typeof(FxResources.System.Text.Json.SourceGeneration.SR)), | ||
| messageFormat: new LocalizableResourceString(nameof(SR.ContextClassesMustIndicateSerializableTypesMessageFormat), SR.ResourceManager, typeof(FxResources.System.Text.Json.SourceGeneration.SR)), | ||
| category: JsonConstants.SystemTextJsonSourceGenerationName, | ||
| defaultSeverity: DiagnosticSeverity.Warning, | ||
| isEnabledByDefault: true); | ||
|
|
||
| private static DiagnosticDescriptor MultipleJsonConstructorAttribute { get; } = new DiagnosticDescriptor( | ||
| id: "SYSLIB1033", | ||
| title: new LocalizableResourceString(nameof(SR.MultipleJsonConstructorAttributeTitle), SR.ResourceManager, typeof(FxResources.System.Text.Json.SourceGeneration.SR)), | ||
|
|
@@ -149,7 +157,7 @@ public Parser(Compilation compilation, in SourceProductionContext sourceProducti | |
| { | ||
| Compilation compilation = _compilation; | ||
| INamedTypeSymbol jsonSerializerContextSymbol = compilation.GetBestTypeByMetadataName(JsonSerializerContextFullName); | ||
| INamedTypeSymbol jsonSerializableAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonSerializableAttributeFullName); | ||
| INamedTypeSymbol jsonSourceGenerationOptionsAttributeSymbol = compilation.GetBestTypeByMetadataName(JsonSourceGenerationOptionsAttributeFullName); | ||
|
|
||
| if (jsonSerializerContextSymbol == null || jsonSerializableAttributeSymbol == null || jsonSourceGenerationOptionsAttributeSymbol == null) | ||
|
|
@@ -193,19 +201,27 @@ public Parser(Compilation compilation, in SourceProductionContext sourceProducti | |
| } | ||
| } | ||
|
|
||
| INamedTypeSymbol contextTypeSymbol = (INamedTypeSymbol)compilationSemanticModel.GetDeclaredSymbol(classDeclarationSyntax); | ||
| Debug.Assert(contextTypeSymbol != null); | ||
| string[] diagnosticMessageArgs = new string[] { contextTypeSymbol.Name }; | ||
|
|
||
| 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()) | ||
| { | ||
| _sourceProductionContext.ReportDiagnostic(Diagnostic.Create(ContextClassesMustIndicateSerializableTypes, Location.None, diagnosticMessageArgs)); | ||
| } | ||
|
|
||
| continue; | ||
| } | ||
|
|
||
| if (!TryGetClassDeclarationList(contextTypeSymbol, out List<string> classDeclarationList)) | ||
| { | ||
| // Class or one of its containing types is not partial so we can't add to it. | ||
| _sourceProductionContext.ReportDiagnostic(Diagnostic.Create(ContextClassesMustBePartial, Location.None, diagnosticMessageArgs)); | ||
| continue; | ||
| } | ||
|
|
||
|
|
@@ -407,31 +423,25 @@ private static bool TryGetClassDeclarationList(INamedTypeSymbol typeSymbol, [Not | |
| return typeGenerationSpec; | ||
| } | ||
|
|
||
| internal static bool IsSyntaxTargetForGeneration(SyntaxNode node) => node is ClassDeclarationSyntax { BaseList: { Types : { Count : > 0 } } }; | ||
|
There was a problem hiding this comment. 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. I believe this change is fine since we'll have about the same number of types to consider in most source gen scenarios. There was a problem hiding this comment. Note that this could impact perf. I would test the before/after of this change and ensure for large solutions that target I can get you a large solution offline, if you need it to test. There was a problem hiding this comment. 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? There was a problem hiding this comment. @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. There was a problem hiding this comment. 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. |
||
|
|
||
| internal static ClassDeclarationSyntax? GetSemanticTargetForGeneration(GeneratorSyntaxContext context) | ||
| { | ||
| var classDeclarationSyntax = (ClassDeclarationSyntax)context.Node; | ||
|
|
||
| foreach (BaseTypeSyntax baseTypeSyntax in classDeclarationSyntax.BaseList.Types) | ||
| { | ||
| INamedTypeSymbol? baseTypeSymbol = context.SemanticModel.GetSymbolInfo(baseTypeSyntax.Type).Symbol as INamedTypeSymbol; | ||
| if (baseTypeSymbol == null) | ||
| { | ||
| continue; | ||
| } | ||
|
|
||
| string fullName = baseTypeSymbol.ToDisplayString(); | ||
| if (fullName == JsonSerializerContextFullName) | ||
| { | ||
| return classDeclarationSyntax; | ||
| } | ||
| } | ||
|
|
||
| return null; | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My feeling is that we should refine this to check for the existence of
GetTypeInfooverrides. A user that unintentionally adds any member to their context class (be it a helper method, type, etc.) would see this diagnostic disappear completely.