-
Notifications
You must be signed in to change notification settings - Fork 1.7k
C#: Tolerate missing call targets in LogMessageSink #14855
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
Conversation
DCA shows no changes (in the nightly suite). |
|
||
public class C | ||
{ | ||
public ILogger logger; |
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.
OOI, which type does Roslyn then assign to this field?
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.
In this case it's A.ILogger
. What's odd is that when we process the field, there's no sign of ambiguity.
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's also what I don't understand, I would have expected that Roslyn didn't assign any type at all. Or is it perhaps because we just take the first candidate symbol?
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.
No, it's not us.
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.
To conclude, we found that there's no ambiguity when processing the field or the reference to the field. But the field's type symbol is actually an ambiguous error symbol.
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 good to me!
Will this improve the logging injection query alert retention rate on our standalone suite?
Yes it will. The issue was found through the lost Let me kick off a DCA experiment to see the changes in Standalone... |
DCA shows 86 new |
In the standalone mode, we're automatically adding implicit usings to the compilation. If we detect any web application in the repo, we're adding the asp.net core specific implicit usings, which include
Microsoft.Extensions.Logging
. This namespace containsILogger
, which is quite a common name, and oftentimes conflict withILogger
interfaces defined in the repo. As a result, we're facing ambiguities in the standalone extracted compilation, and we end up with an incomplete DB. This PR adjusts theLogMessageSink
class to work in cases when "logger-like" method calls have no extracted target.