dotnet/runtime

[LSG] LoggerMessage - Add diagnostic - Can't have the same template with different casing

maryamariyan opened this issue · 4 comments

maryamariyan commented

LoggerMessage supports case insensitive parameters. But we need to add a diagnostic when different casing of the same parameter is specified in the same message template like in the below sample:

[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {p1} {P1}"")]
public static partial void M1(ILogger logger, int p1, int P1);

Refer to: #51064 (comment)

  • Note: case insensitive support against on LoggerMessage.Define is also supported.

Proposal

The proposed diagnostic descriptor would be:

public static DiagnosticDescriptor InconsistentTemplateCasing { get; } = new DiagnosticDescriptor(
    id: "SYSLIB1021",
    title: new LocalizableResourceString(nameof(SR.InconsistentTemplateCasingTitle), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    messageFormat: new LocalizableResourceString(nameof(SR.InconsistentTemplateCasingMessage), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    category: "LoggingGenerator",
    DiagnosticSeverity.Error,
    isEnabledByDefault: true);

With the following title:

Logging method have the same template with different casing

And the following message format:

Logging method '{0}' have the same template with different casing

Note: SYSLIB1021 diagnostic descriptor is already merged on main, but it is not being triggered.

Code Sample

The diagnostic would be triggered for case such as the following ones:

IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@"
    partial class C
    {
        [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {par1} {PAr1} {a}"")]
        static partial void M1(ILogger logger, int par1, int a);
    }
");
Deleted user commented

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

Issue Details

LoggerMessage supports case insensitive parameters. But we need to add a diagnostic when different casing of the same parameter is specified in the same message template like in the below sample:

[LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {p1} {P1}"")]
public static partial void M1(ILogger logger, int p1, int P1);
Author: maryamariyan
Assignees: -
Labels:

area-Extensions-Logging
Milestone: 6.0.0
allantargino commented

Proposal

The proposed diagnostic descriptor would be:

public static DiagnosticDescriptor InconsistentTemplateCasing { get; } = new DiagnosticDescriptor(
    id: "SYSLIB1021",
    title: new LocalizableResourceString(nameof(SR.InconsistentTemplateCasingTitle), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    messageFormat: new LocalizableResourceString(nameof(SR.InconsistentTemplateCasingMessage), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    category: "LoggingGenerator",
    DiagnosticSeverity.Error,
    isEnabledByDefault: true);

With the following title:

Parameter has inconsistent template casing

And the following message format:

Parameter '{0}' has inconsistent template casing

Note: SYSLIB1021 diagnostic descriptor is already merged on main, but it is not being triggered.

Code Sample

The diagnostic would be triggered for case such as the following ones:

IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@"
    partial class C
    {
        [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""M1 {par1} {PAr1} {a}"")]
        static partial void M1(ILogger logger, int par1, int a);
    }
");
terrajobst commented

Video

It seems we generally want to allow message parameters and parameters in C# to be matched case-insensitively such that folks can use Pascal-casing in the message for example. However, when one does it in the case of parameters that have the same name but differ in case the result is ill-defined today.

We can either do a diagnostic and disallow this (proposal) or we can change the source generator to first match case-sensitively and if that doesn't produce a match fall back to case-insensitive matching. We're leaning towards the latter.

tarekgh commented

I looked at the details and I agree it is possible we fix the issue inside the source generator and not necessary adding the diagnostics.

  • 👍1