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
Add support for primary constructors in LoggerMessageGenerator #101660
base: main
Are you sure you want to change the base?
Add support for primary constructors in LoggerMessageGenerator #101660
Conversation
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
{ | ||
string? loggerField = null; | ||
|
||
INamedTypeSymbol? classType = sm.GetDeclaredSymbol(classDec, _cancellationToken); | ||
|
||
bool onMostDerivedType = true; | ||
|
||
#if ROSLYN4_8_OR_GREATER | ||
IEnumerable<ClassDeclarationSyntax> primaryConstructors = classType.InstanceConstructors |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, there seems to be no way to identify primary constructor parameters in the semantic model, and the partial class declaration that contains the primary constructor may not contain a method annotated with [LoggerMessage]
, thereby not being picked up by the SyntaxProvider
in LoggerMessageGenerator.Roslyn4.0.cs
, so we have to work backwards from here, potentially parsing and creating semantic models for class declarations that weren't otherwise visited.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CyrusNajmabadi @eiriktsarpalis any suggestion about the best way to handle that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@333fred mentioned in the Discord that this could be potentially be done with LookupSymbols
or a speculative model, but:
LookupSymbols
doesn't return primary constructor parameters for any positions I know will exist in all cases, even in minimal class declarations with just one partial method declaration.TryGetSpeculativeSemanticModelForMethodBody
requires a position parameter within an existing method body which, once again, may not exist. Getting Speculative SemanticModel for method body method declaration without body (block or expression) is impossible roslyn#61564 suggests speculative models don't work in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I now look at the parameters straight from the constructor method symbol, which simplifies the code a lot (thanks @CyrusNajmabadi), but I still couldn't find a way to identify which constructor (if any) is the primary one without looking at the syntax.
@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review. |
src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/LoggerMessageGenerator.Parser.cs
Outdated
Show resolved
Hide resolved
…bol instead of from the semantic model
@tarekgh, with the helpful suggestion from Cyrus, I now think this is ready for review. Should I mark it as such or wait for more feedback, perhaps from @eiriktsarpalis? (I don't know when he's back from vacation.) |
@@ -632,6 +632,30 @@ private static string GenerateClassName(TypeDeclarationSyntax typeDeclaration) | |||
|
|||
bool onMostDerivedType = true; | |||
|
|||
#if ROSLYN4_8_OR_GREATER |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am seeing we are defining such constants in the csproj
Line 6 in 017593d
<DefineConstants>$(DefineConstants);ROSLYN4_0_OR_GREATER;ROSLYN4_4_OR_GREATER</DefineConstants> |
Could you try to use ROSLYN4_4_OR_GREATER
instead and look if this will work? I expect we use recent Roslyn which I expect it will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That didn't work with the previous version of this pull request, since the ParameterList
property on ClassDeclarationSyntax
isn't available until Roslyn 4.8, but it may be possible now that this uses the constructor parameters on the class symbol instead. Let me give it a try!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Look at my suggestion #101660 (comment). I am asking if we can do that instead of creating a new version of the library just to match the desired Roslyn version.
.Where(ic => ic.DeclaringSyntaxReferences | ||
.Any(ds => ds.GetSyntax() is ClassDeclarationSyntax)); | ||
|
||
foreach (IMethodSymbol primaryConstructor in primaryConstructors) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, this in itself is a great example of why I said you're not going to get lookup right if you do it by yourself. Consider this example; I've created a protected
field in the base type with a wider type than the logger parameter in the derived type, and that protected
member shadows the primary constructor parameter. Another similar example is this, where I've shadowed the primary constructor parameter with a field directly in the type itself. I'm not opposed to the idea of adding a new LookupSymbols
API here to allow SG authors to say "I need to know what names are available when I implement the body of this type", but it's also important to note that available names are by nature impacted by the using
s in the file. Let me talk with @AlekseyTs and see if we can come up with any ideas that avoid you having to try and get this correct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also test scenarios like this:
class Test(ILogger logger)
{
private readonly ILogger _logger = logger;
}
and ensure that you're using the right logger for that scenario.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@333fred is it possible to lookup the fields first and then if didn't find any try to look up the primary constructors?
Also, for detecting the primary constructor, should check constructor.Body == null && constructor.ExpressionBody == null
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private readonly ILogger _logger = logger;
good test as we don't support private fields.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@333fred is it possible to lookup the fields first and then if didn't find any try to look up the primary constructors?
Sure, that would be possible. Talking with Aleksey, that's likely the approach you want to take in general. However, I'm going to strongly recommend that you explicitly consider these cases and lay out, in comments and documentation, the exact rules you follow. IE, what happens when a primary constructor parameter is shadowed? What happens when a base type has a protected member that is named differently than the parameter (ie, has an _
)? This will make it easier to verify the behavior is what you're expecting, and to handle bugs as by-design or not.
Also, for detecting the primary constructor, should check
constructor.Body == null && constructor.ExpressionBody == null
?
You only need the check that kimsey0 put in already. It's important to note that you can only detect primary constructors from source, you cannot detect them from metadata.
good test as we don't support private fields.
Wait, what? I'll reiterate that I think you need spell out exactly what the rules are, in comments above the code, so that you can verify the code follows them. I thought I might understand the rules, but clearly I do not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@333fred, that sounds excellent. I'd be very happy to use a new LookupSymbols
API for this. The pull request isn't exactly time sensitive either. I'd just be happy for it to land in .NET 9.
For now, I'll add the test cases you've suggested and try to find ways to handle them, if nothing else, then for the sake of my own learning.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I posted the comment above before seeing 333fred's new comment at the same time.)
I'm happy to discuss, document, and test the exact rules this source generator should follow. Should that discussion happen in this pull request or perhaps in #91121? Your comments above expose some edge cases, but there are probably more that I may not think of.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait, what? I'll reiterate that I think you need spell out exactly what the rules are, in comments above the code, so that you can verify the code follows them. I thought I might understand the rules, but clearly I do not.
I am wrong here, looking more seeing we can support private fields https://learn.microsoft.com/en-us/dotnet/core/extensions/logger-message-generator#basic-usage this is because we generate the code inside the same class and LoggerMessage is not static. Sorry for confusion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should that discussion happen in this pull request or perhaps in #91121?
Either way is fine. we can continue discussing in the PR and then put the conclusion in the issue. I am fine to move the discussion to the issue too.
Your comments above expose some edge cases, but there are probably more that I may not think of.
I think when applying the suggestion which looking up the fields first will address most these corner cases. Of course we'll need to document the behavior.
@@ -0,0 +1,20 @@ | |||
<Project Sdk="Microsoft.NET.Sdk"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need to create a new library for that version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we update to that Roslyn version it means we remove the one that targets 4.4.
Updating for the inbox generators in 9.0 could be OK (if we can guarantee that 4.8 is present in all SDK/VS versions where 9.0 is supported).
Updating for the nuget package generators is ok so long as we don't take away any features / perf from folks using roslyn 4.4. Of course that's moot if noone is using 4.4-4.7.
Somewhere there is a matrix of compiler versions that are supported that could help answer this question.
@@ -15,6 +15,8 @@ Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Loggin | |||
EndProject | |||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.4", "gen\Microsoft.Extensions.Logging.Generators.Roslyn4.4.csproj", "{BF948816-45E1-4F0F-985A-0B4DB4D3BF40}" | |||
EndProject | |||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Generators.Roslyn4.8", "gen\Microsoft.Extensions.Logging.Generators.Roslyn4.8.csproj", "{40FA38ED-D2C9-4608-B85E-F731655C1864}" | |||
EndProject | |||
Project("{9A19103F-16F7-4668-BE54-9A1E7A4F7556}") = "Microsoft.Extensions.Logging.Abstractions", "ref\Microsoft.Extensions.Logging.Abstractions.csproj", "{7F536552-0E2A-4642-B7CF-863727C2F9CD}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please revert all changes in the solution files.
Fixes #91121.