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

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

Open
Tracked by #77390 ...
maryamariyan opened this issue May 4, 2021 · 4 comments
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Logging
Milestone

Comments

@maryamariyan
Copy link
Member

maryamariyan commented May 4, 2021

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);
    }
");
@ghost
Copy link

ghost commented May 4, 2021

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label May 4, 2021
@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label May 4, 2021
@maryamariyan maryamariyan modified the milestones: 6.0.0, 7.0.0 Aug 12, 2021
@eerhardt eerhardt modified the milestones: 7.0.0, Future Jul 14, 2022
@maryamariyan maryamariyan modified the milestones: Future, 8.0.0 Sep 28, 2022
@maryamariyan maryamariyan changed the title LoggerMessage - Add diagnostic - Can't have the same template with different casing [LSG] LoggerMessage - Add diagnostic - Can't have the same template with different casing Nov 16, 2022
@allantargino
Copy link
Contributor

allantargino commented Jan 21, 2023

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);
    }
");

allantargino added a commit to allantargino/dotnet-runtime that referenced this issue Jan 21, 2023
@tarekgh tarekgh added blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 22, 2023
@terrajobst
Copy link
Member

terrajobst commented Jan 24, 2023

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.

@terrajobst terrajobst closed this as not planned Won't fix, can't repro, duplicate, stale Jan 24, 2023
@terrajobst terrajobst reopened this Jan 24, 2023
@terrajobst terrajobst added api-needs-work API needs work before it is approved, it is NOT ready for implementation and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 24, 2023
@teo-tsirpanis teo-tsirpanis removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jan 24, 2023
@tarekgh
Copy link
Member

tarekgh commented Jan 30, 2023

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.

@tarekgh tarekgh modified the milestones: 8.0.0, Future Jan 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-needs-work API needs work before it is approved, it is NOT ready for implementation area-Extensions-Logging
Projects
None yet
Development

No branches or pull requests

6 participants