-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Add LoggingGenerator #51064
Add LoggingGenerator #51064
Conversation
The `ISyntaxReceiver` is a syntax model that is more IDE friendly than the pull model that was previously implemented. This allows the IDE to push data to the generator as it is processing it. This is important because the IDE is constantly "abandoning" analysis as the customer types in the IDE which invalidates state and they quickly want to move to calculating the new state
- Use the logger factory and console logger in the sample - Support overriding the event name via the logger message attribute - Added ToString override - Enable dumping generated code in the sample for easy debugging
- GetEnumerator is now implemented by calling the indexer to avoid some redundant code. - Logging methods without arguments now share a common log state struct, which eliminates redundant code to JIT.
- Add error checking to ensure the first argument to a logging method implements the ILogger interface. - Can now specify logging methods with generic ILogger<T> as logger. - Add error checking to prevent generic logging method parameters. - Add error checking to ensure logging methods are static and partial - Can now specify logging methods which different access modifiers. - Eliminate or reduce cascading errors in many cases. - Ensure generated symbol names all start with __ so as not to conflict with user-specified symbols
int index = 0; | ||
while (index < s.Length) | ||
{ | ||
if (s[index] == '\n' || s[index] == '\r' || s[index] == '"') |
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.
Can we use string.IndexOfAny
?
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.
Good idea, I could try on another PR or post preview 4.
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
- Feedback on ConvertEndOfLineAndQuotationCharactersToEscapeForm
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
- Add comment in versions.props - Nit code style feedback - Dont keep method when starts with underscore
reviewers can I get a sign off on this PR if your blocking feedback is addressed? cc @eerhardt @tarekgh @ericstj @safern I will have a follow up to reduce source generator support (not allow source generation for larger than 6 arguments, etc.) but that would not drastically change all the changes reviewed here. |
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.
I think this is great work that will benefit our customers. Let's get this feature in.
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging/gen/LoggerMessageGenerator.Parser.cs
Show resolved
Hide resolved
@maryamariyan I generated the fwlinks and have pointed all of them to this issue for now. Once you create proper docs pages for all the newly introduced analyzer rules, feel free to change the fwlinks yourself (you should have write permission), or ping me and I can assist with changing them. Thanks! |
Fixes #36022
More Info: https://github.com/dotnet/designs/blob/main/accepted/2021/logging-generator.md
CI Health:
Stack trace
Workaround: Exclude gen test project when TargetsMono set to true
TODO:
Separate from this PR:
Microsoft.CodeAnalysis.CSharp.Workspaces
dependency (Add LoggingGenerator #51064 (comment))Next PR
Remove support for generating more than 6 arguments (refer to Compile-time source generation for structured log messages designs#195 (comment)) and would involve adding another diagnostic here. (Added baselineTestBaseline_TestWithMoreThan6Params_Success
to illustrate this use case)context.AddSource(nameof(Generator), SourceText.From(result, Encoding.UTF8)); // replace nameof(Generator) with "LoggerMessage"