Allow use of overloaded methods when using LoggerMessage #64573
Conversation
Calculates and assigns a unique LoggerMethod name as required, and uses this new unique name for generating types in the source generator. This allows using the LoggerMessageAttribute on methods that share the same name but have different method signatures. Fix dotnet#61814
ghost
added
area-Extensions-Logging
community-contribution
|
Tagging subscribers to this area: @dotnet/area-extensions-logging Issue DetailsFixes #61814 Computes a unique name for
|
|
|
...ts/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MethodOverloadTestExtensions.cs
Outdated
Show resolved
Hide resolved
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
...ts/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MethodOverloadTestExtensions.cs
Show resolved
Hide resolved
...ts/Microsoft.Extensions.Logging.Generators.Tests/TestClasses/MethodOverloadTestExtensions.cs
Outdated
Show resolved
Hide resolved
|
This could be merged once you get a chance to apply the minor feedback in this PR. |
Also resolves other PR feedback, an extra blank line and incorrect use of an implicit type.
|
I've updated the PR based on feedback. The only thing left is the last two LoggerMessage overload tests - I've written a comment with my thinking on that - let me know if you still want me to remove them, |
|
@JakeYallop Thanks for the quick turnaround. I would still be tempted to remove the last two overloads. event name uniqueness is not currently being respected and I was thinking we should fix that first and just leave this PR to focus on overloads for now. |
|
I've removed the tests. |
|
/backport to release/6.0 |
|
Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1786697916 |
|
@maryamariyan backporting to release/6.0 failed, the patch most likely resulted in conflicts: $ git am --3way --ignore-whitespace --keep-non-patch changes.patch
Applying: Compute unique method names for generated code
Using index info to reconstruct a base tree...
M src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
M src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Falling back to patching base and 3-way merge...
Auto-merging src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Auto-merging src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
CONFLICT (content): Merge conflict in src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Emitter.cs
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Compute unique method names for generated code
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128Please backport manually! |
|
@maryamariyan - are we sure this needs to be backported to 6.0? It doesn't feel like this limitation is critical to fix in 6, but I guess I'd like your opinion/thoughts. |
|
I assume it seems like a blocker to developers wanting to migrate to the logging source generator approach. Looking back at the issue doesn't seem like there'd been a lot of hits with this bug as much as there were for the |
ghost
locked as resolved and limited conversation to collaborators
4 participants
Fixes #61814
Computes a unique name for
LoggerMethods, allowing for overloaded methods to be used with the source generator.