Add support for primary constructors in LoggerMessageGenerator

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!

[kimsey0]

Fixes#91121.

[tarekgh]

@kimsey0 thanks for submitting the PR. I converted it to draft till it is ready for actual review.

[kimsey0]

@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.)

[333fred]

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 usings 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.

[333fred]

You should also test scenarios like this:

classTest(ILoggerlogger){privatereadonlyILogger_logger=logger;}

and ensure that you're using the right logger for that scenario.

[tarekgh]

@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?

[tarekgh]

private readonly ILogger _logger = logger;

good test as we don't support private fields.

[333fred]

@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.

[kimsey0]

Sounds good. If both the behavior described above as well as the code implementing it is acceptable to all parties, I think this is ready for review again.

[tarekgh]

I'll try to review it when I get a chance. PR is not a draft anymore. Thanks for all the help you have provided here.

[kimsey0]

Sounds good. Thanks a lot!

[kimsey0]

I forgot to commit and push the info diagnostic, but just did that. I don't know if there are any guidelines for how exactly these diagnostic messages should be written or how the translation flow goes? (We can also just revert this. I think it's useful, but not necessary.)

[tarekgh]

You don't have to worry about the translation of the added messages. I am seeing the added diagnostic is useful to catch the concerned case.

[tarekgh]

@geeknoid@joperezr to be aware about this change if need to update the extensions logging source gen too.

[joperezr]

Thanks for flagging this @tarekgh! It does seem like it is something we'll want to do in extensions. Logged dotnet/extensions#5178 to track

[tarekgh]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

---

Refers to: src/libraries/Microsoft.Extensions.Logging.Abstractions/tests/Microsoft.Extensions.Logging.Generators.Tests/LoggerMessageGeneratorParserTests.cs:1115 in 6cf2b2f. [](commit_id = 6cf2b2f, deletion_comment = False)

[tarekgh]

@kimsey0 I added a few minor comments. I think we should be good to go after addressing these. Thanks!

[kimsey0]

 Assert.Equal("SYSLIB1026", diagnostics[0].Id);

Please add similar test for the new diagnostic we added.

I added assertions that the new diagnostic is reported to the existing test cases with shadowed primary constructor parameters. Would you like any additional tests of this?

[tarekgh]

Thanks @kimsey0 providing the fix!

[kimsey0]

Thanks a lot for the help, @tarekgh!