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 malformed format strings #52226

Closed
Tracked by #77390 ...
maryamariyan opened this issue May 4, 2021 · 3 comments · Fixed by #81503
Closed
Tracked by #77390 ...

[LSG] LoggerMessage - Add diagnostic - Can't have malformed format strings #52226

maryamariyan opened this issue May 4, 2021 · 3 comments · Fixed by #81503
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Logging
Milestone

Comments

@maryamariyan
Copy link
Member

maryamariyan commented May 4, 2021

Refer to: #51064 (comment)

Proposal

The proposed diagnostic descriptor would be:

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

With the following title:

Logging method contains malformed format strings

And the following message format:

Logging method '{0}' contains malformed format strings

Note: SYSLIB1022 diagnostic 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 = 1, Level = LogLevel.Debug, Message = ""M1 {A} M1 { M3"")]
        static partial void M1(ILogger logger);

        [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = ""M2 {A} M2 } M2"")]
        static partial void M2(ILogger logger);

        [LoggerMessage(EventId = 3, Level = LogLevel.Debug, Message = ""M3 {arg1"")]
        static partial void M3(ILogger logger);

        [LoggerMessage(EventId = 4, Level = LogLevel.Debug, Message = ""M4 arg1}"")]
        static partial void M4(ILogger logger);

        [LoggerMessage(EventId = 5, Level = LogLevel.Debug, Message = ""M5 {"")]
        static partial void M5(ILogger logger);

        [LoggerMessage(EventId = 6, Level = LogLevel.Debug, Message = ""}M6 "")]
        static partial void M6(ILogger logger);

        [LoggerMessage(EventId = 7, Level = LogLevel.Debug, Message = ""M7 {{arg1}}"")]
        static partial void M7(ILogger logger);

        [LoggerMessage(EventId = 8, Level = LogLevel.Debug, Message = ""}M8{arg1}{arg2}"")]
        static partial void M8(ILogger logger);
    }
");
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Logging untriaged New issue has not been triaged by the area owner labels May 4, 2021
@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

Cases for which this diagnostic would be helpful:

[LoggerMessage(EventId = 3, Level = LogLevel.Debug, Message = ""M3 {arg1"")]
static partial void M3(ILogger logger);

[LoggerMessage(EventId = 4, Level = LogLevel.Debug, Message = ""M4 arg1}"")]
static partial void M4(ILogger logger);

[LoggerMessage(EventId = 5, Level = LogLevel.Debug, Message = ""M5 {"")]
static partial void M5(ILogger logger);

[LoggerMessage(EventId = 6, Level = LogLevel.Debug, Message = ""}M6 "")]
static partial void M6(ILogger logger);
Author: maryamariyan
Assignees: -
Labels:

area-Extensions-Logging, untriaged

Milestone: -

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label May 4, 2021
@maryamariyan maryamariyan added this to the 6.0.0 milestone May 4, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 4, 2021
@maryamariyan maryamariyan changed the title LoggerMessage - Add Diagnostic - Can't have malformed format strings LoggerMessage - Add diagnostic - Can't have malformed format strings May 4, 2021
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label May 25, 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 malformed format strings [LSG] LoggerMessage - Add diagnostic - Can't have malformed format strings Nov 16, 2022
@allantargino
Copy link
Contributor

allantargino commented Jan 20, 2023

Proposal

The proposed diagnostic descriptor would be:

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

With the following title:

Logging method contains malformed format strings

And the following message format:

Logging method '{0}' contains malformed format strings

Note: SYSLIB1022 diagnostic 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 = 1, Level = LogLevel.Debug, Message = ""M1 {A} M1 { M3"")]
        static partial void M1(ILogger logger);

        [LoggerMessage(EventId = 2, Level = LogLevel.Debug, Message = ""M2 {A} M2 } M2"")]
        static partial void M2(ILogger logger);

        [LoggerMessage(EventId = 3, Level = LogLevel.Debug, Message = ""M3 {arg1"")]
        static partial void M3(ILogger logger);

        [LoggerMessage(EventId = 4, Level = LogLevel.Debug, Message = ""M4 arg1}"")]
        static partial void M4(ILogger logger);

        [LoggerMessage(EventId = 5, Level = LogLevel.Debug, Message = ""M5 {"")]
        static partial void M5(ILogger logger);

        [LoggerMessage(EventId = 6, Level = LogLevel.Debug, Message = ""}M6 "")]
        static partial void M6(ILogger logger);

        [LoggerMessage(EventId = 7, Level = LogLevel.Debug, Message = ""M7 {{arg1}}"")]
        static partial void M7(ILogger logger);

        [LoggerMessage(EventId = 8, Level = LogLevel.Debug, Message = ""}M8{arg1}{arg2}"")]
        static partial void M8(ILogger logger);
    }
");

@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

Looks good as proposed (severity, category, ID). We should also change the generator to not emit any code rather than emitting code that won't compile with an unintelligent error.

@terrajobst terrajobst added api-approved API was approved in API review, it can be implemented and removed api-ready-for-review API is ready for review, it is NOT ready for implementation labels Jan 24, 2023
@tarekgh tarekgh removed the blocking Marks issues that we want to fast track in order to unblock other important work label Jan 24, 2023
allantargino added a commit to allantargino/dotnet-runtime that referenced this issue Feb 1, 2023
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 1, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 5, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 8, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Logging
Projects
None yet
5 participants