-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Fixed LoggerMessageGenerator message escaping. #123787
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
Fixed LoggerMessageGenerator message escaping. #123787
Conversation
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.
Pull request overview
Fixes LoggerMessageGenerator emitting malformed C# string literals by escaping backslashes and all non-printable characters (< 0x20) when generating source, and adds a regression test covering these escaping scenarios.
Changes:
- Update the emitter to escape
\and all control characters (< 0x20) using\xNN(while keeping\r/\nas named escapes). - Add a new theory test validating the generated source is syntactically valid and contains the expected escaped message literal.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs | Extends message escaping to include backslashes and all control characters so generated string literals remain well-formed. |
| src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs | Adds a theory-based regression test for message escaping (backslash, quotes, CR/LF, and \xNN escapes). |
...ns/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs
Outdated
Show resolved
Hide resolved
...ns/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
…Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorEmitterTests.cs Co-authored-by: Copilot <[email protected]>
Moved the diagnostics retrieval after the source text assertion to ensure proper validation of generated diagnostics.
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
Outdated
Show resolved
Hide resolved
We should get rid of this method and just call Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs:580 in 2f6a0f4. [](commit_id = 2f6a0f4, deletion_comment = False) |
|
@tarekgh, pushed your suggested changes and added some additional test coverage. |
tarekgh
left a comment
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.
Thanks @John-Leitch. I pushed a small test fix, LGTM otherwise.
This PR addresses bug #123786 by escaping all non-printable characters (
< 0x20) in the emitted message.\rand\nare emitted using the same escape sequence, while other non-printable characters are emitted as hex escape sequences e.g.\x00. Escaping of\is also addressed. Finally, an additional test was added to cover these behaviors.