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 1 commit
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
42 changes: 33 additions & 9 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,8 @@ private sealed class Parser

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

private readonly HashSet<ValueTuple<Type, DiagnosticDescriptor, string[]>> _typeLevelDiagnostics = new();
layomia marked this conversation as resolved.
Show resolved Hide resolved

private JsonKnownNamingPolicy _currentContextNamingPolicy;

private static DiagnosticDescriptor ContextClassesMustBePartial { get; } = new DiagnosticDescriptor(
Expand Down Expand Up @@ -285,15 +287,18 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene
INamedTypeSymbol contextTypeSymbol = (INamedTypeSymbol)compilationSemanticModel.GetDeclaredSymbol(classDeclarationSyntax);
Debug.Assert(contextTypeSymbol != null);

Location contextLocation = contextTypeSymbol.Locations[0];
layomia marked this conversation as resolved.
Show resolved Hide resolved

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,6 +321,22 @@ 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>();
Expand All @@ -324,6 +345,7 @@ public Parser(Compilation compilation, in JsonSourceGenerationContext sourceGene
// Clear the cache 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 +509,8 @@ private static bool TryGetClassDeclarationList(INamedTypeSymbol typeSymbol, [Not
typeGenerationSpec.GenerationMode = generationMode;
}

typeGenerationSpec.AttributeLocation = attributeSyntax.GetLocation();

return typeGenerationSpec;
}

Expand Down Expand Up @@ -877,7 +901,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 +968,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 +979,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 +996,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 +1011,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;
}
}
22 changes: 22 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,26 @@ private static bool HasJsonConstructorAttribute(ConstructorInfo constructorInfo)

return false;
}

public static Location? GetDiagnosticLocation(this Type type)
{
TypeWrapper? typeWrapper = type as TypeWrapper;
layomia marked this conversation as resolved.
Show resolved Hide resolved
Debug.Assert(typeWrapper != null);
return typeWrapper.Location;
}

public static Location? GetDiagnosticLocation(this PropertyInfo propertyInfo)
{
PropertyInfoWrapper? propertyInfoWrapper = propertyInfo as PropertyInfoWrapper;
Debug.Assert(propertyInfoWrapper != null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this invariant is always true, it begs the question why we need all the System.Reflection wrappers for Roslyn symbols. Not in scope for this PR, but in 7.0.0 I would recommend mapping roslyn symbols directly to sourcegen metadata to avoid abstraction leaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a few cases, we are able to share code between source-gen and reflection serializers since the symbols are expressed as reflection wrappers, e.g. the code in https://github.com/dotnet/runtime/blob/4cf86c2abfe9b2fad5ab2a48e4b0717ff0daf27a/src/libraries/System.Text.Json/Common/ReflectionExtensions.cs

return propertyInfoWrapper.Location;
}

public static Location? GetDiagnosticLocation(this FieldInfo fieldInfo)
{
FieldInfoWrapper? fieldInfoWrapper = fieldInfo as FieldInfoWrapper;
Debug.Assert(fieldInfoWrapper != null);
return fieldInfoWrapper.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
19 changes: 13 additions & 6 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 @@ -11,6 +11,7 @@
using System.Text.Encodings.Web;
using Microsoft.CodeAnalysis;
using Microsoft.CodeAnalysis.CSharp;
using Microsoft.CodeAnalysis.Text;
using Xunit;

namespace System.Text.Json.SourceGeneration.UnitTests
Expand Down Expand Up @@ -336,22 +337,28 @@ 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,
(TextSpan Location, string Message)[] expectedDiags)
{
string[] actualMessages = diagnostics.Where(diagnostic => diagnostic.Severity == level).Select(diagnostic => diagnostic.GetMessage()).ToArray();
(TextSpan Location, string Message)[] actualDiags = diagnostics
.Where(diagnostic => diagnostic.Severity == level)
.Select(diagnostic => (diagnostic.Location.SourceSpan, diagnostic.GetMessage()))
layomia marked this conversation as resolved.
Show resolved Hide resolved
.ToArray();

// Can't depend on reflection order when generating type metadata.
Array.Sort(actualMessages);
Array.Sort(expectedMessages);
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