-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Address feedback from STJ source-gen PR #51300 #52147
Conversation
layomia
commented
May 1, 2021
- Adopt parser/emitter pattern in source generator (similar to logging and event source generators)
- Enable localization for diagnostic descriptors
- Move IsExternalInit.cs to common folder
Tagging subscribers to this area: @eiriktsarpalis, @layomia Issue Details
|
src/libraries/System.Text.Json/gen/System.Text.Json.SourceGeneration.csproj
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/Reflection/ReflectionExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/System.Text.Json/gen/JsonSourceGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
MetadataLoadContextInternal metadataLoadContext = new(compilation); | ||
Parser parser = new(executionContext.Compilation); | ||
SyntaxReceiver receiver = (SyntaxReceiver)executionContext.SyntaxReceiver; | ||
_rootTypes = parser.GetRootSerializableTypes(receiver.CompilationUnits); |
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.
Do you need to check if reciever
is null here?
Comparing with
runtime/src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.cs
Lines 27 to 33 in da580be
public void Execute(GeneratorExecutionContext context) | |
{ | |
if (context.SyntaxReceiver is not SyntaxReceiver receiver || receiver.ClassDeclarations.Count == 0) | |
{ | |
// nothing to do yet | |
return; | |
} |
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.
Per the source generators cookbook, it appears we don't have to check if the receiver
is null
:
// the generator infrastructure will create a receiver and populate it
// we can retrieve the populated instance via the context
MySyntaxReceiver syntaxReceiver = (MySyntaxReceiver)context.SyntaxReceiver;
// get the recorded user class
ClassDeclarationSyntax userClass = syntaxReceiver.ClassToAugment;
cc @jaredpar, @chsienki - do we need null
checks for syntax receivers?
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.