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.

Sign up for GitHub

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

Jump to bottom

Provide locations for src-gen diagnostic output #61430

Merged
merged 3 commits into from
Nov 16, 2021
Merged

Provide locations for src-gen diagnostic output #61430

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4eadba4
Provide locations for src-gen diagnostic output
layomia Nov 10, 2021
97452f7
Fix tests and address feedback
layomia Nov 15, 2021
29d2da4
Add defensive check for context type location
layomia Nov 15, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions src/libraries/System.Text.Json/gen/ContextGenerationSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Text.Json.Serialization;
using System.Text.Json.Reflection;
using System.Diagnostics;
using Microsoft.CodeAnalysis;

namespace System.Text.Json.SourceGeneration
{
Expand All @@ -15,6 +16,8 @@ namespace System.Text.Json.SourceGeneration
[DebuggerDisplay("ContextTypeRef={ContextTypeRef}")]
internal sealed class ContextGenerationSpec
{
public Location Location { get; init; }

public JsonSourceGenerationOptionsAttribute GenerationOptions { get; init; }

public Type ContextType { get; init; }
Expand Down
6 changes: 4 additions & 2 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -274,8 +274,9 @@ private void GenerateTypeInfo(TypeGenerationSpec typeGenerationSpec)
break;
case ClassType.TypeUnsupportedBySourceGen:
{
Location location = typeGenerationSpec.Type.GetDiagnosticLocation() ?? typeGenerationSpec.AttributeLocation ?? _currentContext.Location;
_sourceGenerationContext.ReportDiagnostic(
Diagnostic.Create(TypeNotSupported, Location.None, new string[] { typeGenerationSpec.TypeRef }));
Diagnostic.Create(TypeNotSupported, location, new string[] { typeGenerationSpec.TypeRef }));
return;
}
default:
Expand All @@ -293,7 +294,8 @@ private void GenerateTypeInfo(TypeGenerationSpec typeGenerationSpec)
}
else
{
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(DuplicateTypeName, Location.None, new string[] { typeGenerationSpec.TypeInfoPropertyName }));
Location location = typeGenerationSpec.AttributeLocation ?? _currentContext.Location;
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(DuplicateTypeName, location, new string[] { typeGenerationSpec.TypeInfoPropertyName }));
}
}

Expand Down
55 changes: 45 additions & 10 deletions src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,14 @@ private sealed class Parser

private readonly HashSet<TypeGenerationSpec> _implicitlyRegisteredTypes = new();

/// <summary>
/// A list of diagnostics emitted at the type level. This is cached and emitted between the processing of context types
/// in order to properly retrieve [JsonSerializable] attribute applications for top-level types (since a type might occur
/// both at top-level and in nested object graphs, with no guarantee of the order that we will see the type).
/// The element tuple types specifies the serializable type, the kind of diagnostic to emit, and the diagnostic message.
/// </summary>
private readonly List<(Type, DiagnosticDescriptor, string[])> _typeLevelDiagnostics = new();

private JsonKnownNamingPolicy _currentContextNamingPolicy;

private static DiagnosticDescriptor ContextClassesMustBePartial { get; } = new DiagnosticDescriptor(
Expand Down Expand Up @@ -244,6 +252,11 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene

foreach (ClassDeclarationSyntax classDeclarationSyntax in classDeclarationSyntaxList)
{
// Ensure context-scoped metadata caches are empty.
Debug.Assert(_typeGenerationSpecCache.Count == 0);
Debug.Assert(_implicitlyRegisteredTypes.Count == 0);
Debug.Assert(_typeLevelDiagnostics.Count == 0);

CompilationUnitSyntax compilationUnitSyntax = classDeclarationSyntax.FirstAncestorOrSelf<CompilationUnitSyntax>();
SemanticModel compilationSemanticModel = compilation.GetSemanticModel(compilationUnitSyntax.SyntaxTree);

Expand Down Expand Up @@ -285,15 +298,18 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene
INamedTypeSymbol contextTypeSymbol = (INamedTypeSymbol)compilationSemanticModel.GetDeclaredSymbol(classDeclarationSyntax);
Debug.Assert(contextTypeSymbol != null);

Location contextLocation = contextTypeSymbol.Locations.Length > 0 ? contextTypeSymbol.Locations[0] : Location.None;

if (!TryGetClassDeclarationList(contextTypeSymbol, out List<string> classDeclarationList))
{
// Class or one of its containing types is not partial so we can't add to it.
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(ContextClassesMustBePartial, Location.None, new string[] { contextTypeSymbol.Name }));
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(ContextClassesMustBePartial, contextLocation, new string[] { contextTypeSymbol.Name }));
continue;
}

ContextGenerationSpec contextGenSpec = new()
{
Location = contextLocation,
GenerationOptions = options ?? new JsonSourceGenerationOptionsAttribute(),
ContextType = contextTypeSymbol.AsType(_metadataLoadContext),
ContextClassDeclarationList = classDeclarationList
Expand All @@ -316,14 +332,31 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene
continue;
}

// Emit type-level diagnostics
foreach ((Type Type, DiagnosticDescriptor Descriptor, string[] MessageArgs) diagnostic in _typeLevelDiagnostics)
{
Type type = diagnostic.Type;
Location location = type.GetDiagnosticLocation();

if (location == null)
{
TypeGenerationSpec spec = _typeGenerationSpecCache[type];
location = spec.AttributeLocation;
}

location ??= contextLocation;
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(diagnostic.Descriptor, location, diagnostic.MessageArgs));
}

contextGenSpec.ImplicitlyRegisteredTypes.UnionWith(_implicitlyRegisteredTypes);

contextGenSpecList ??= new List<ContextGenerationSpec>();
contextGenSpecList.Add(contextGenSpec);

// Clear the cache of generated metadata between the processing of context classes.
// Clear the caches of generated metadata between the processing of context classes.
_typeGenerationSpecCache.Clear();
_implicitlyRegisteredTypes.Clear();
_typeLevelDiagnostics.Clear();
layomia marked this conversation as resolved.
Show resolved Hide resolved
}

if (contextGenSpecList == null)
Expand Down Expand Up @@ -487,6 +520,8 @@ private static bool TryGetClassDeclarationList(INamedTypeSymbol typeSymbol, [Not
typeGenerationSpec.GenerationMode = generationMode;
}

typeGenerationSpec.AttributeLocation = attributeSyntax.GetLocation();

return typeGenerationSpec;
}

Expand Down Expand Up @@ -877,7 +912,7 @@ private TypeGenerationSpec GetOrAddTypeGenerationSpec(Type type, JsonSourceGener
if (!type.TryGetDeserializationConstructor(useDefaultCtorInAnnotatedStructs, out ConstructorInfo? constructor))
{
classType = ClassType.TypeUnsupportedBySourceGen;
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(MultipleJsonConstructorAttribute, Location.None, new string[] { $"{type}" }));
_typeLevelDiagnostics.Add((type, MultipleJsonConstructorAttribute, new string[] { $"{type}" }));
layomia marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
Expand Down Expand Up @@ -944,7 +979,7 @@ private TypeGenerationSpec GetOrAddTypeGenerationSpec(Type type, JsonSourceGener
}

spec = GetPropertyGenerationSpec(propertyInfo, isVirtual, generationMode);
CacheMemberHelper();
CacheMemberHelper(propertyInfo.GetDiagnosticLocation());
}

foreach (FieldInfo fieldInfo in currentType.GetFields(bindingFlags))
Expand All @@ -955,10 +990,10 @@ private TypeGenerationSpec GetOrAddTypeGenerationSpec(Type type, JsonSourceGener
}

spec = GetPropertyGenerationSpec(fieldInfo, isVirtual: false, generationMode);
CacheMemberHelper();
CacheMemberHelper(fieldInfo.GetDiagnosticLocation());
}

void CacheMemberHelper()
void CacheMemberHelper(Location memberLocation)
{
CacheMember(spec, ref propGenSpecList, ref ignoredMembers);

Expand All @@ -972,13 +1007,13 @@ void CacheMemberHelper()
{
if (dataExtensionPropGenSpec != null)
{
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(MultipleJsonExtensionDataAttribute, Location.None, new string[] { type.Name }));
_typeLevelDiagnostics.Add((type, MultipleJsonExtensionDataAttribute, new string[] { type.Name }));
}

Type propType = spec.TypeGenerationSpec.Type;
if (!IsValidDataExtensionPropertyType(propType))
{
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(DataExtensionPropertyInvalid, Location.None, new string[] { type.Name, spec.ClrName }));
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(DataExtensionPropertyInvalid, memberLocation, new string[] { type.Name, spec.ClrName }));
}

dataExtensionPropGenSpec = GetOrAddTypeGenerationSpec(propType, generationMode);
Expand All @@ -987,13 +1022,13 @@ void CacheMemberHelper()

if (!hasInitOnlyProperties && spec.CanUseSetter && spec.IsInitOnlySetter)
{
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InitOnlyPropertyDeserializationNotSupported, Location.None, new string[] { type.Name }));
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InitOnlyPropertyDeserializationNotSupported, memberLocation, new string[] { type.Name }));
hasInitOnlyProperties = true;
}

if (spec.HasJsonInclude && (!spec.CanUseGetter || !spec.CanUseSetter || !spec.IsPublic))
{
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InaccessibleJsonIncludePropertiesNotSupported, Location.None, new string[] { type.Name, spec.ClrName }));
_sourceGenerationContext.ReportDiagnostic(Diagnostic.Create(InaccessibleJsonIncludePropertiesNotSupported, memberLocation, new string[] { type.Name, spec.ClrName }));
}
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/gen/Reflection/FieldInfoWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,5 +100,7 @@ public override void SetValue(object obj, object value, BindingFlags invokeAttr,
{
throw new NotImplementedException();
}

public Location? Location => _field.Locations.Length > 0 ? _field.Locations[0] : null;
layomia marked this conversation as resolved.
Show resolved Hide resolved
}
}
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/gen/Reflection/PropertyInfoWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,5 +92,7 @@ public override void SetValue(object obj, object value, BindingFlags invokeAttr,
{
throw new NotSupportedException();
}

public Location? Location => _property.Locations.Length > 0 ? _property.Locations[0] : null;
}
}
19 changes: 19 additions & 0 deletions src/libraries/System.Text.Json/gen/Reflection/ReflectionExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using System.Diagnostics;
using System.Linq;
using System.Reflection;
using Microsoft.CodeAnalysis;

namespace System.Text.Json.Reflection
{
Expand Down Expand Up @@ -45,5 +46,23 @@ private static bool HasJsonConstructorAttribute(ConstructorInfo constructorInfo)

return false;
}

public static Location? GetDiagnosticLocation(this Type type)
{
Debug.Assert(type is TypeWrapper);
return ((TypeWrapper)type).Location;
}

public static Location? GetDiagnosticLocation(this PropertyInfo propertyInfo)
{
Debug.Assert(propertyInfo is PropertyInfoWrapper);
return ((PropertyInfoWrapper)propertyInfo).Location;
}

public static Location? GetDiagnosticLocation(this FieldInfo fieldInfo)
{
Debug.Assert(fieldInfo is FieldInfoWrapper);
return ((FieldInfoWrapper)fieldInfo).Location;
}
}
}
2 changes: 2 additions & 0 deletions src/libraries/System.Text.Json/gen/Reflection/TypeWrapper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,5 +600,7 @@ public override bool Equals(Type o)
}
return base.Equals(o);
}

public Location? Location => _typeSymbol.Locations.Length > 0 ? _typeSymbol.Locations[0] : null;
}
}
6 changes: 6 additions & 0 deletions src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using System.Reflection;
using System.Text.Json.Reflection;
using System.Text.Json.Serialization;
using Microsoft.CodeAnalysis;

namespace System.Text.Json.SourceGeneration
{
Expand All @@ -18,6 +19,11 @@ internal class TypeGenerationSpec
/// </summary>
public string TypeRef { get; private set; }

/// <summary>
/// If specified as a root type via <c>JsonSerializableAttribute</c>, specifies the location of the attribute application.
/// </summary>
public Location? AttributeLocation { get; set; }

/// <summary>
/// The name of the public <c>JsonTypeInfo&lt;T&gt;</c> property for this type on the generated context class.
/// For example, if the context class is named MyJsonContext, and the value of this property is JsonMessage;
Expand Down
24 changes: 17 additions & 7 deletions .../System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/CompilationHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,22 +336,32 @@ public partial class MyJsonContext : JsonSerializerContext
return CreateCompilation(source);
}

internal static void CheckDiagnosticMessages(ImmutableArray<Diagnostic> diagnostics, DiagnosticSeverity level, string[] expectedMessages)
internal static void CheckDiagnosticMessages(
DiagnosticSeverity level,
ImmutableArray<Diagnostic> diagnostics,
(Location Location, string Message)[] expectedDiags,
bool sort = true)
{
string[] actualMessages = diagnostics.Where(diagnostic => diagnostic.Severity == level).Select(diagnostic => diagnostic.GetMessage()).ToArray();
(Location Location, string Message)[] actualDiags = diagnostics
.Where(diagnostic => diagnostic.Severity == level)
.Select(diagnostic => (diagnostic.Location, diagnostic.GetMessage()))
.ToArray();

// Can't depend on reflection order when generating type metadata.
Array.Sort(actualMessages);
Array.Sort(expectedMessages);
if (sort)
{
// Can't depend on reflection order when generating type metadata.
Array.Sort(actualDiags);
Array.Sort(expectedDiags);
}

if (CultureInfo.CurrentUICulture.Name.StartsWith("en", StringComparison.OrdinalIgnoreCase))
{
Assert.Equal(expectedMessages, actualMessages);
Assert.Equal(expectedDiags, actualDiags);
}
else
{
// for non-English runs, just compare the number of messages are the same
Assert.Equal(expectedMessages.Length, actualMessages.Length);
Assert.Equal(expectedDiags.Length, actualDiags.Length);
}
}
}
Expand Down
Loading