Skip to content
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

Logging Source Generator fails ungracefully with overloaded methods #61814

Closed
Tracked by #64015
JakeYallop opened this issue Nov 18, 2021 · 3 comments · Fixed by #64573
Closed
Tracked by #64015

Logging Source Generator fails ungracefully with overloaded methods #61814

JakeYallop opened this issue Nov 18, 2021 · 3 comments · Fixed by #64573
Assignees
Milestone

Comments

@JakeYallop
Copy link
Contributor

JakeYallop commented Nov 18, 2021

Description

The LoggerMessage source generator fails when there are overloaded logging methods.

Reproduction Steps

public static partial class LoggerMessageGeneration
{
    [LoggerMessage(EventId = 1, EventName = "LogInt", Level = LogLevel.Debug, Message = "Test message {One}")]
    public static partial void Log1(ILogger logger, int one);

    [LoggerMessage(EventId = 2, EventName = "LogBool", Level = LogLevel.Debug, Message = "Test message {Two}")]
    public static partial void Log1(ILogger logger, bool two);
}

Expected behavior

The code compiles successfully. If the LoggerMessage generator explicitly does not support logging method overloads, then a diagnostic should be emitted instead.

Consider the following code, where an overload is specified without an EventName - should this compile?

public static partial class LoggerMessageGeneration
{
    [LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = "Test message {One}")]
    public static partial void Log1(ILogger logger, int one);

    [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = "Test message {Two}")]
    public static partial void Log1(ILogger logger, bool two);
}

The current behaviour of the source generator is to use the method name as the event name, this would emit logs with different event ids that share the same event name. If there are logging providers that rely on event names being unique, this could be problematic.

Actual behavior

Emits

LoggerMessage.g.cs(19,152,19,166): error CS0102: The type 'LoggerMessageGeneration' already contains a definition for '__Log1Callback".

Regression?

No

Known Workarounds

Do not use overloaded methods when using [LoggerMessage].

Configuration

> dotnet --info

.NET SDK (reflecting any global.json):
 Version:   6.0.100
 Commit:    9e8b04bbff

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19043
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100\

Host (useful for support):
  Version: 6.0.0
  Commit:  4822e3c3aa

Other information

No response

@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Logging untriaged New issue has not been triaged by the area owner labels Nov 18, 2021
@ghost
Copy link

ghost commented Nov 18, 2021

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

The LoggerMessage source generator fails when there are overloaded logging methods, even if a unique EventName is defined for both.

Reproduction Steps

public static partial class LoggerMessageGeneration
{
    [LoggerMessage(EventId = 1, EventName = "LogInt", Level = LogLevel.Debug, Message = "Test message {One}")]
    public static partial void Log1(ILogger logger, int one);

    [LoggerMessage(EventId = 2, EventName = "LogBool", Level = LogLevel.Debug, Message = "Test message {Two}")]
    public static partial void Log1(ILogger logger, bool two);
}

Expected behavior

The code compiles successfully. If the LoggerMessage generator explicitly does not support logging method overloads, then a diagnostic should be emitted instead.

Consider the following code, where an overload is specified without an EventName - should this compile?

public static partial class LoggerMessageGeneration
{
    [LoggerMessage(EventId = 1, Level = LogLevel.Debug, Message = "Test message {One}")]
    public static partial void Log1(ILogger logger, int one);

    [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = "Test message {Two}")]
    public static partial void Log1(ILogger logger, bool two);
}

The current behaviour of the source generator is to use the method name as the event name, this would emit logs with different event ids that shared the same event name.

Actual behavior

Emits

LoggerMessage.g.cs(19,152,19,166): error CS0102: The type 'LoggerMessageGeneration' already contains a definition for '__Log1Callback".

Regression?

No

Known Workarounds

Do not use overloaded methods when using [LoggerMessage].

Configuration

> dotnet --info

.NET SDK (reflecting any global.json):
 Version:   6.0.100
 Commit:    9e8b04bbff

Runtime Environment:
 OS Name:     Windows
 OS Version:  10.0.19043
 OS Platform: Windows
 RID:         win10-x64
 Base Path:   C:\Program Files\dotnet\sdk\6.0.100\

Host (useful for support):
  Version: 6.0.0
  Commit:  4822e3c3aa

Other information

No response

Author: JakeYallop
Assignees: -
Labels:

untriaged, area-Extensions-Logging

Milestone: -

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Dec 1, 2021
@maryamariyan maryamariyan added this to the 7.0.0 milestone Dec 1, 2021
@maryamariyan maryamariyan self-assigned this Jan 18, 2022
@JakeYallop
Copy link
Contributor Author

I'd love to take a look at providing a fix for this.

JakeYallop added a commit to JakeYallop/runtime that referenced this issue Jan 31, 2022
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 ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 31, 2022
@maryamariyan
Copy link
Member

maryamariyan commented Feb 1, 2022

@JakeYallop thanks for your interest. I have a fix prepared already. It also is addressing some other source generator feedback so we would end up having to keep either one of the two. I reviewed your PR in the meantime.

maryamariyan pushed a commit that referenced this issue Feb 2, 2022
* Compute unique method names for generated code

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 #61814

* Rename MethodOverloadTestExtensions to OverloadTestExtensions

Also resolves other PR feedback, an extra blank line and incorrect use
of an implicit type.

* Remove un-needed overload tests
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 2, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants