dotnet/runtime

[LSG] LoggerMessage - Add diagnostic when unexpected order provided in message template

maryamariyan opened this issue · 3 comments

maryamariyan commented

Today, using the LoggerMessage attribute, we can support mismatched orders, like:

[LoggerMessage(.., Message="message with foo: {foo}, and bar: {bar}"]
public void LogMethod(..., string bar, string foo)

But this is not supported with LoggerMessage.Define. The best way to keep the feature set coherent here is to add an INFO diagnostic for LoggerMessage source generator, and mention that "the order from message template arguments did not match the expected order from the method arguments.

Proposal

The proposed diagnostic descriptor would be:

public static DiagnosticDescriptor ParametersOutOfOrder { get; } = new DiagnosticDescriptor(
    id: "SYSLIB1026",
    title: new LocalizableResourceString(nameof(SR.ParametersOutOfOrderTitle), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    messageFormat: new LocalizableResourceString(nameof(SR.ParametersOutOfOrderMessage), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    category: "LoggingGenerator",
    DiagnosticSeverity.Info,
    isEnabledByDefault: true);

With the following title:

Logging method contains parameters out of order provided in the message template

And the following message format:

Logging method '{0}' contains parameters out of order provided in the message template

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=""{a3},{a1},{a4},{a2}"")]
        static partial void M1(ILogger logger, int a1, int a2, int a3, int a4);

        [LoggerMessage(EventId = 1, Message=""{a2},{a3},{a1}"")]
        static partial void M2(ILogger logger, int a1, LogLevel l, int a2, System.Exception e, int a3);
    }
");
Deleted user commented

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

Issue Details

Today, using the LoggerMessage attribute, we can support mismatched orders, like:

[LoggerMessage(.., Message="message with foo: {foo}, and bar: {bar}"]
public void LogMethod(..., string bar, string foo)

But this is not supported with LoggerMessage.Define. The best way to keep the feature set coherent here is to add an INFO diagnostic for LoggerMessage source generator, and mention that "the order from message template arguments did not match the expected order from the method arguments.

Author: maryamariyan
Assignees: -
Labels:

area-Extensions-Logging
Milestone: 6.0.0
allantargino commented

Proposal

The proposed diagnostic descriptor would be:

public static DiagnosticDescriptor ParametersOutOfOrder { get; } = new DiagnosticDescriptor(
    id: "SYSLIB1026",
    title: new LocalizableResourceString(nameof(SR.ParametersOutOfOrderTitle), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    messageFormat: new LocalizableResourceString(nameof(SR.ParametersOutOfOrderMessage), SR.ResourceManager, typeof(FxResources.Microsoft.Extensions.Logging.Generators.SR)),
    category: "LoggingGenerator",
    DiagnosticSeverity.Info,
    isEnabledByDefault: true);

With the following title:

Logging method contains parameters out of order provided in the message template

And the following message format:

Logging method '{0}' contains parameters out of order provided in the message template

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=""{a3},{a1},{a4},{a2}"")]
        static partial void M1(ILogger logger, int a1, int a2, int a3, int a4);

        [LoggerMessage(EventId = 1, Message=""{a2},{a3},{a1}"")]
        static partial void M2(ILogger logger, int a1, LogLevel l, int a2, System.Exception e, int a3);
    }
");
terrajobst commented

Video

We concluded that since the source generator already does the right thing, we don't need to add the diagnostic. While the old model was positional, there is no confusion because the old model had no named parameter. We believe this is good enough to avoid confusion. We don't need to force users of the source generator to be positional.