Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Changes from 2 commits
d7497fc
15643ec
9fc6ce4
2d6748c
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 thatprotected
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 newLookupSymbols
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 theusing
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:
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.
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.
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.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.
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.
(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.
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.
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.
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.
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 have added the three test cases suggested by @333fred, as well as one that shadows the primary constructor parameter with an
ILogger
field:I've changed the code to support these cases by:
ILogger
field.ILogger
primary constructor parameter.In practice, this means that:
I think this is a reasonable behavior, though one could argue that it would make sense to have a specific warning when a
ILogger
primary constructor parameter is found that is shadowed by a non-ILogger
field.How should this behavior be documented? Just on the learn page?
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.
Yes, we can have a diagnostic for that. Would it be better to have it as
Info
instead ofwarning
?Right, this is the right place to document the behavior. We'll need to update this doc anyway for primary constructor usage with a sample.
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.
@ericstj do you see a problem if we upgrade
runtime/src/libraries/Microsoft.Extensions.Logging.Abstractions/gen/Microsoft.Extensions.Logging.Generators.Roslyn4.4.csproj
Line 5 in 017593d
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.
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.
Looks like it doesn't matter in the end. I don't actually need to have code in the generator that's conditionally compiled for Roslyn 4.8. It's only in the test project I need to be sure primary constructors are supported. @tarekgh, is this approach of just upgrading the
RoslynApiVersion
inMicrosoft.Extensions.Logging.Generators.Roslyn4.0.Tests.csproj
OK?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.
Can you add a new
Microsoft.Extensions.Logging.Generators.Roslyn4.8.Tests.csproj
that target Roslyn 4.8?