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] Logging Source Generator fails to compile when out parameter modifier is present #64665

Closed
1 task
Tracked by #77390 ...
maryamariyan opened this issue Feb 2, 2022 · 9 comments · Fixed by #80458
Closed
1 task
Tracked by #77390 ...
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Logging blocking Marks issues that we want to fast track in order to unblock other important work
Milestone

Comments

@maryamariyan
Copy link
Member

maryamariyan commented Feb 2, 2022

As part of issue #62644 we fixed compile errors with using ref or in.

But for out usage, we would need to add a diagnostic that explains out usage is not supported for LoggerMessageAttribute annotated logging methods.

Proposal

The proposed diagnostic descriptor would be:

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

With the following title:

Argument is using the unsupported out parameter modifier

And the following message format:

Argument '{0}' is using the unsupported out parameter modifier

Code Sample

IReadOnlyList<Diagnostic> diagnostics = await RunGenerator(@$"
                partial class C
                {{
                    [LoggerMessage(EventId = 0, Level = LogLevel.Debug, Message = ""Parameter {{P1}}"")]
                    static partial void M(ILogger logger, out int p1);
                }}");

Assert.Single(diagnostics);
Assert.Equal(DiagnosticDescriptors.InvalidLoggingMethodParameterOut.Id, diagnostics[0].Id);

TODO:

  • Take diagnostic message/severity/category to API review
@dotnet-issue-labeler dotnet-issue-labeler bot added area-Extensions-Logging untriaged New issue has not been triaged by the area owner labels Feb 2, 2022
@ghost
Copy link

ghost commented Feb 2, 2022

Tagging subscribers to this area: @dotnet/area-extensions-logging
See info in area-owners.md if you want to be subscribed.

Issue Details

As part of issue #62644 we fixed compile errors with using ref or in.

But for out usage, we would need to add a diagnostic that explains out usage is not supported for LoggerMessageAttribute annotated logging methods.

Author: maryamariyan
Assignees: -
Labels:

untriaged, area-Extensions-Logging

Milestone: -

@maryamariyan maryamariyan removed the untriaged New issue has not been triaged by the area owner label Feb 2, 2022
@maryamariyan maryamariyan added this to the 7.0.0 milestone Feb 2, 2022
@eerhardt eerhardt modified the milestones: 7.0.0, Future Jul 20, 2022
@maryamariyan maryamariyan modified the milestones: Future, 8.0.0 Sep 28, 2022
@maryamariyan maryamariyan changed the title Logging Source Generator fails to compile when out parameter modifier is present [LSG] Logging Source Generator fails to compile when out parameter modifier is present Nov 16, 2022
@allantargino
Copy link
Contributor

Hi @maryamariyan, I'd like to be assigned to this one :)

@allantargino
Copy link
Contributor

allantargino commented Jan 5, 2023

The proposed diagnostic descriptor would be:

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

With the following title:

Argument is using the unsupported out parameter modifier

And the following message format:

Argument '{0}' is using the unsupported out parameter modifier

@tarekgh
Copy link
Member

tarekgh commented Jan 5, 2023

@allantargino feel free to submit a PR for that and I can help reviewing. Thanks for willing to help with this issue.

@tarekgh
Copy link
Member

tarekgh commented Jan 5, 2023

Forgot to mention, we need to do "Take diagnostic message/severity/category to API review" first. I'll try to edit the issue description and then take it to the API review first. Please hold creating the PR till we get the design review approval.

@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 5, 2023
@allantargino
Copy link
Contributor

allantargino commented Jan 7, 2023

Thanks @tarekgh. I just updated the proposal in my comment above to have both message title and format (instead of just message format). Would you please update the description? Thanks!

@tarekgh
Copy link
Member

tarekgh commented Jan 7, 2023

With the following message format:

Did you mean this will be the title?

@allantargino
Copy link
Contributor

allantargino commented Jan 7, 2023

Yes, my bad 😄 I just updated it again.

@terrajobst
Copy link
Member

terrajobst commented Jan 10, 2023

Video

Looks good as proposed.

@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 10, 2023
allantargino added a commit to allantargino/dotnet-runtime that referenced this issue Jan 10, 2023
Adds a diagnostic that explains out usage is not supported
for LoggerMessageAttribute annotated logging methods

fixes dotnet#64665
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2023
tarekgh pushed a commit that referenced this issue Jan 10, 2023
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 10, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 10, 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 blocking Marks issues that we want to fast track in order to unblock other important work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants