Provide locations for src-gen diagnostic output (#61430)

* Provide locations for src-gen diagnostic output * Fix tests and address feedback * Add defensive check for context type location
dotnet · Nov 16, 2021 · a89debf · a89debf
1 parent 4d82463
commit a89debf
Show file tree

Hide file tree

Showing 11 changed files with 232 additions and 120 deletions.
diff --git a/src/libraries/System.Text.Json/gen/ContextGenerationSpec.cs b/src/libraries/System.Text.Json/gen/ContextGenerationSpec.cs
@@ -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
 {
@@ -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; }

diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
@@ -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:
@@ -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 }));
                 }
             }
 

diff --git a/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs b/src/libraries/System.Text.Json/gen/JsonSourceGenerator.Parser.cs
@@ -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(
@@ -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);
 
@@ -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
@@ -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();
                 }
 
                 if (contextGenSpecList == null)
@@ -487,6 +520,8 @@ private static bool TryGetClassDeclarationList(INamedTypeSymbol typeSymbol, [Not
                     typeGenerationSpec.GenerationMode = generationMode;
                 }
 
+                typeGenerationSpec.AttributeLocation = attributeSyntax.GetLocation();
+
                 return typeGenerationSpec;
             }
 
@@ -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}" }));
                     }
                     else
                     {
@@ -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))
@@ -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);
 
@@ -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);
@@ -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 }));
                                 }
                             }
                         }

diff --git a/src/libraries/System.Text.Json/gen/Reflection/FieldInfoWrapper.cs b/src/libraries/System.Text.Json/gen/Reflection/FieldInfoWrapper.cs
@@ -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;
     }
 }
diff --git a/src/libraries/System.Text.Json/gen/Reflection/PropertyInfoWrapper.cs b/src/libraries/System.Text.Json/gen/Reflection/PropertyInfoWrapper.cs
@@ -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;
     }
 }
diff --git a/src/libraries/System.Text.Json/gen/Reflection/ReflectionExtensions.cs b/src/libraries/System.Text.Json/gen/Reflection/ReflectionExtensions.cs
@@ -5,6 +5,7 @@
 using System.Diagnostics;
 using System.Linq;
 using System.Reflection;
+using Microsoft.CodeAnalysis;
 
 namespace System.Text.Json.Reflection
 {
@@ -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;
+        }
     }
 }
diff --git a/src/libraries/System.Text.Json/gen/Reflection/TypeWrapper.cs b/src/libraries/System.Text.Json/gen/Reflection/TypeWrapper.cs
@@ -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;
     }
 }
diff --git a/src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs b/src/libraries/System.Text.Json/gen/TypeGenerationSpec.cs
@@ -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
 {
@@ -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;

diff --git a/.../System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/CompilationHelper.cs b/.../System.Text.Json/tests/System.Text.Json.SourceGeneration.Unit.Tests/CompilationHelper.cs
@@ -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);
             }
         }
     }