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

[release/6.0] Fix Logging source generator failures with various syntax #64779

Merged
merged 3 commits into from
Feb 9, 2022

Conversation

maryamariyan
Copy link
Member

@maryamariyan maryamariyan commented Feb 4, 2022

Customer Impact

Presenting 3 customer-reported issues, being blocked from using the Logging Source Generator when using the following use cases below. (Workarounds require customers to significantly change their usage which prevents them from adopting the feature).


If a C# keyword is used as a parameter name for a [LoggerMessage] method using the logging source generator prefixed with an @, such as the example below, the application will fail to compile due to the source generator creating invalid C#.

[LoggerMessage(1, LogLevel.Information, "Event: {event}")]
public static partial void LogEvent(ILogger logger, object @event);

If the @ is included in the template to match the parameter, it will also fail to compile.


For example, as originally reported, the generator fails to compile due to CS0246 and CS0265 errors if type for generic constraint is in a different namespace. There's more constraint-related use cases covered within the PR.


[LoggerMessage(2, LogLevel.Information, "Foo: {my}")]
public static partial void LogSomething1(this ILogger logger, in MyReadOnlyStruct my);

Regression

No

Testing

Automated test cases added to cover the missed syntax. Included in the PRs:

Risk

Low. Very targeted changes were selected to handle missed syntax.
Test cases were added and more variants to reported repro cases were also explored.

When fixing for generic constrains, we reduced risk by avoiding duplication of constraints/derived types.

The other two fixes (ref/in support and @ prefixed parameter support) only impact use cases for which we are otherwise unsupported and produce ambiguous compilation errors, has also been reviewed by Roslyn members and therefore are clear improvements with low risk.

Packaging impact:

  • Requires packaging of Microsoft.Extensions.Logging.Abstractions

Ref pack impact:

Yes, modifies inbox source-generator contained in ASP.NET ref-pack.

@ghost
Copy link

ghost commented Feb 4, 2022

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

Issue Details

Recently we added a few post 6.0 bug fixes to main branch. This PR is cherry picking the following ones:

Customer Impact

Bugfix in PR #64663 was the most requested by community as part of post release 6.0 feedback so would be good to backport it to 6.0.

Also, the bugfix in PR helps with the logging source generator robustness and to developers wanting to migrate to the new logging approach fixes a variety of use cases having to do with constraints.

Testing

The PRs have combined test use cases with them.

Risk

Author: maryamariyan
Assignees: maryamariyan
Labels:

area-Extensions-Logging

Milestone: -

… / when dealing with constraints (dotnet#64593)

* Fixes some logging source gen bugs:

- Supports usage of "in" modifier
- Improves support for generic constraints

Fixes dotnet#58550, dotnet#62644

* Apply PR feedback

* Add another test
…ters (dotnet#64663)

* Adds support to `@` signed prefixed parameters

Fixes dotnet#60968

* Move repetitive logic to a new property

* Remove NeedsAtSign
@maryamariyan maryamariyan force-pushed the combined-sgen-prs branch 2 times, most recently from aab4f8c to 124c7c1 Compare February 4, 2022 02:33
@maryamariyan maryamariyan marked this pull request as ready for review February 4, 2022 02:37
@maryamariyan
Copy link
Member Author

To make things clear for tactics and servicing, I only am keeping this one release/6.0 PR for logging source generator improvements.

@maryamariyan maryamariyan added the Servicing-consider Issue for next servicing release review label Feb 4, 2022
@ericstj ericstj changed the title Cherry picks for a few logging source generator bug fixes onto 6.0 servicing branch [release/6.0] Fix Logging source generator failures with various syntax Feb 4, 2022
@ericstj
Copy link
Member

ericstj commented Feb 8, 2022

To make things clear for tactics and servicing, I only am keeping this one release/6.0 PR

Typically this is not preferred. It makes things a bit harder since it becomes an all or nothing decision to take the PR. The commits are also not independently revertible. I think here we can say these are all low risk changes and we kept them together to avoid merge conflicts with each other.

@maryamariyan
Copy link
Member Author

This received tactics approval. I think it should be OK to merge

@ericstj ericstj added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Feb 9, 2022
@ericstj
Copy link
Member

ericstj commented Feb 9, 2022

Approved over mail

@safern
Copy link
Member

safern commented Feb 9, 2022

Failures are unrelated. We can merge this one (got approval offline).

@safern safern merged commit 3334f3b into dotnet:release/6.0 Feb 9, 2022
@snakefoot
Copy link

@safern Missing assignment of .NET 6.0 milestone ? (Since merged into dotnet:release/6.0)

@ghost ghost locked as resolved and limited conversation to collaborators Apr 5, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Extensions-Logging Servicing-approved Approved for servicing release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants