Handle unsupported diagnostics reported by analyzers #1286
Conversation
|
❗ This will break some analyzers, and I'm not sure it's what you want to do in all cases. For a specific example, our original implementation of StyleCop rule SA1119 only really reported SA1119, which is reported for a complete parenthesized expression. To avoid marking the entire expression as unnecessary, we created an additional diagnostic for the purpose of marking just the parentheses themselves as unnecessary code. You can see this in DotNetAnalyzers/StyleCopAnalyzers#363. The helper diagnostic is hidden, enabled by default, and uses This weekend we updated the implementation of this analyzer to report both diagnostics as supported (in DotNetAnalyzers/StyleCopAnalyzers@264eb6d) in order to simplify our testing. I was a bit uncomfortable with this change because I don't want the supporting rule to cause this analyzer to run even if the user disables SA1119. To ensure the analyzer is only triggered when SA1119 itself is enabled, I marked the supporting diagnostic as disabled by default when I added it to the list of supported diagnostics (it already has the tag Please make sure to consider (and/or correct) my final assumptions, as we are relying on this for our diagnostic analyzer. |
| @@ -36,15 +37,17 @@ internal class AnalyzerExecutor | |||
| /// Optional delegate which is invoked when an analyzer throws an exception. | |||
There was a problem hiding this comment.
Is the name AnalyzerActionsExecutor in the <summary> element above up-to-date? Consider using cref for it.
|
👍 |
| @@ -303,7 +305,7 @@ public CompilationAnalysisContext(Compilation compilation, AnalyzerOptions optio | |||
| /// <param name="diagnostic"><see cref="Diagnostic"/> to be reported.</param> | |||
| public void ReportDiagnostic(Diagnostic diagnostic) | |||
| { | |||
| DiagnosticAnalysisContextHelpers.VerifyArguments(diagnostic); | |||
| DiagnosticAnalysisContextHelpers.VerifyArguments(diagnostic, _isSupportedDiagnostic); | |||
| lock (_reportDiagnostic) | |||
There was a problem hiding this comment.
Why do we lock on a delegate object passed to the constructor from outside? It introduces an implicit dependency on the calling code (e.g. it might pass the same object to other unrelated methods that can also lock on it).
There was a problem hiding this comment.
The constructor is only invoked from the within the IDE and compiler analyzer drivers, and #1285 tracks internalizing these constructors.
We need to lock the reportDiagnostic callback as the analyzer might decide to kick off multiple threads and report diagnostics from different threads simultaneously. We can probably guarantee that all analyzer hosts use thread safe diagnostic bags and the Add delegate that they provide for reported diagnostics is thread safe. However, this code is not maintainable and likely to introduce future race conditions. The only foolproof approach here is to make ReportDiagnostic itself intrinsically thread-safe by using a lock.
There was a problem hiding this comment.
Is this robust? If the passed-in delegate comes from a lambda expression, is there a guarantee that the lambda doesn't create a new instance at each evaluation and thereby defeat the locking?
There was a problem hiding this comment.
You are right, Sri is going to open a new issue to fix this in a better way.
|
@sharwell Thanks for sharing this use case, I hadn't considered this and probably it was overlooked by our team as well. However, I think your current strategy is relying on a bug on our side as being an unintentional way to support a feature. I think the analyzer driver is right in assuming that an analyzer only ever reports diagnostics that it was told are supported, and not guaranteeing this can lead to other unforeseen bugs. Having said that I agree that we can't checkin this change unless we add first class support for representing linked descriptors, we don't want to break analyzer authors without giving an alterative. I am going to propose we do the following:
@srivatsn @JohnHamby @tmeschter @heejaechang @jmarolf @shyamnamboodiripad whats your view for this scenario? |
|
I like the idea of a In the absence of a |
|
Another alternative would be add an additional optional parameter "linkedDescriptors" to the DiagnosticDescriptor constructor and let the analyzer engine handle all this internally. We have decided to put this PR on hold and discuss the design at a team meeting. I'll give an update here once we finalize the design. This PR is not super critical for us anyways, so we are fine with taking our time to design this properly. Thanks again @sharwell |
|
@mavasani @sharwell linked diagnostic is interesting. and usage case seems interesting. but kind of feels like we are trying to use diagnostic to send extra information to the system. since the linked diagnostic is not actually a diagnostic but something that is used for other purpose like having extra tagging on screen, having different code fix area than squiggles and etc. we have 3 different system to deliver such information now, custom tag, diagnostic property, trigger diagnostic. we might need to think a bit deeper about those mechanisms and sort things out more clearly. @srivatsn @JohnHamby @tmeschter @jmarolf how do you think? |
|
Yes I agree that we need to think more about this scenario. What @sharwell is really trying do is to have a custom presentation. He needs to squiggle the entire expression but also grey out the surrounding parentheses for that issue. Because diagnostic authors cannot provide their own presentation of the issue and are limited to the only ones that are implemented (squiggle or fading out for unnecessary), this "linked" diagnostic is being used as a way to hack that in. #1224 is the feature request for giving diagnostic authors the ability to define custom presentations. I imagine it would look like this:
|
|
@mavasani @sharwell @srivatsn by the way, I think we need to check this in for RC. so that people can't to rely on this behavior (our bug). it is unfortunate that this will remove something people could do due to our bug but I think we should stop from more people doing this sooner and provide proper way to do this. by the way, what @sharwell doing is clearly violation of API contract due to us not enforcing it. |
|
I will need to think (and probably discuss in person) before having anything meaningful to say about custom presentations or linked diagnostics, but analyzers should not be able to report diagnostics that they don't advertise. The original intent of this pull request is fundamentally correct. |
| internal bool IsSupportedDiagnostic(DiagnosticAnalyzer analyzer, Diagnostic diagnostic, AnalyzerExecutor analyzerExecutor) | ||
| { | ||
| var supportedDescriptors = GetSupportedDiagnosticDescriptors(analyzer, analyzerExecutor); | ||
| foreach (var descriptor in supportedDescriptors) |
There was a problem hiding this comment.
this is iterating through supported descriptions, can we think about a bit more performant way to do this at right position?
There was a problem hiding this comment.
The linear scan is surely OK, given that this runs only if a diagnostic is being reported and a given analyzer is quite unlikely to have hundreds of thousands of supported diagnostics, but maybe a comment to this effect is appropriate.
There was a problem hiding this comment.
I wrote my comment before reading HeeJae's, but mine read as a response to his. I was actually responding to asking myself the same question.
There was a problem hiding this comment.
@JohnHamby but we already have compiler analyzer that has many descriptors. for each compiler diagnostic reported we will linearly scan all those. and assumption seems a bit risky. how about analyzer manager who cache the descriptor anyway, check the count of the descriptor when cache and if it is more than 10 or something, we cache them in hashset rather than array?
There was a problem hiding this comment.
I agree with both of your views.
For compiler analyzer we don't even want to invoke SupportedDiagnostics to determine whether or not to run the analyzer AND whether or not diagnostics it reports are supported. We already did the former, and I will modify the PR to do the later as well by special casing the compiler analyzer.
I also agree with John's comment that for rest of the analyzers, we should be fine doing a linear scan here, doesn't buy us much by adding any more complexity.
|
@srivatsn It's possible that diagnostic isn't even running due to the incorrect layout of the dependencies in the NuGet associated with CTP6. :-( |
|
Since we added a property bag to |
|
I sign off. We do need to discuss the objection, and come up with a decent way of covering it. |
|
@srivatsn We did not see that diagnostic. In addition to the issue mentioned by @tmeschter, it's possible the diagnostic does not support our pattern of declaring the |
|
@sharwell @heejaechang @JohnHamby @srivatsn let me try to summarize:
Does this sound reasonable? |
|
@mavasani +1 |
|
That sounds reasonable to me too. |
|
I agree that Manish's summary and proposal represent a reasonable position. |
…orted diagnostic ID, i.e. no descriptor returned by SupportedDiagnostics has that ID, then throw an ArgumentException in ReportDiagnostic method. This exception would be turned into an analyzer diagnostic by the driver and reported back to the analyzer host. Also fix a few tests that were reporting diagnostics with unsupported ID!
|
@srivatsn @sharwell @tmeschter the code analysis analyzer diagnostic that recommends not to report diagnostics with unsupported descriptors only scans for identifiers that bind to DiagnosticDescriptor type within the body of SupportedDiagnostics property. We should expand it look for backing fields of type ImmutableArray of descriptor. Note that the analyzer still cannot be guaranteed to catch 100% of misuse cases until we have complete dataflow analysis support. |
Handle unsupported diagnostics reported by analyzers. Fixes #252 : If an analyzer reports a diagnostic with an unsupported diagnostic ID, i.e. no descriptor returned by SupportedDiagnostics has that ID, then throw an ArgumentException in ReportDiagnostic method. This exception would be turned into an analyzer diagnostic by the driver and reported back to the analyzer host. Also fix a few tests that were reporting diagnostics with unsupported ID!
|
@suprak Take a look at DotNetAnalyzers/StyleCopAnalyzers#930. Originally we just reported a diagnostic without declaring support for it. This pull request updates the analyzer to properly declare the existence of both diagnostics, and avoids performing the actual analysis if the "main" diagnostic is suppressed (a test for this performance issue was later added in DotNetAnalyzers/StyleCopAnalyzers#951). |
|
@sharwell @suprak A better way to check for suppression of primary diagnostic is to use the newly added public API: |
|
I don't think we have it in our plans for the next update. /cc @Pilchie @srivatsn @ManishJayaswal might have more information about the timeline for #1224 |
|
@mavasani Is what you mentioned earlier in this thread #1286 (comment) still currently the best way to add rich adornments to diagnostics? |
|
@suprak Yes, you are correct. |
Fixes #252 : If an analyzer reports a diagnostic with an unsupported diagnostic ID, i.e. no descriptor returned by SupportedDiagnostics has that ID, then throw an ArgumentException in ReportDiagnostic method. This exception would be turned into an analyzer diagnostic by the driver and reported back to the analyzer host.
Also fix a few tests that were reporting diagnostics with unsupported ID!
@srivatsn @tmeschter @shyamnamboodiripad @JohnHamby @jmarolf @heejaechang can you please review?