-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Show analysis context for AD0001 #6710
Comments
I will merge these two issue to one and address them in one PR - #6696 |
see #7917 for more information. basically added a way to crash process on analyzer exception using regkey. |
@heejaechang Before going in that direction, I want to clarify the request. For a syntax tree action, you might see this:
For a syntax node action, you might see this:
For a symbol action, you might see this:
|
@sharwell you can ask them to give you dump instead. that information might have let you fix a bug before, but next bug you might need yet another information to actually figure that out. |
I'm sure we've had a couple exceptions, but having been intricately involved in the development and testing of over 150 production analyzers that the information requested here represents the complete set of information typically required to address a problem. With less information we have trouble creating test cases (e.g. DotNetAnalyzers/StyleCopAnalyzers#1534), and rarely if ever have we needed additional information. I believe the request as written is reasonable (straightforward to implement and easy for users to work with) and precisely describes the overwhelming majority scenario that analyzer authors will encounter over time. I do not believe that the alternative described is a suitable replacement. In addition to obvious usability issues (we don't want to tell users to modify their registry and crash their IDE), we still have to address multiple other concerns:
I really think this issue should be reopened for additional discussion regarding directly implementing the described feedback. |
@sharwell I am fine re-opening this for further discussion, but I really don't think adding bunch of information here can be generalized. as the original bug report described, information author would want will be toward whatever analyzer they are developing and things needed for it. ... by the way, if you really want this, you can do it yourself already, catch exception in your registered action, and throw exception with original exception as inner exception with all information you want to capture, description pane will display all those information. ...
|
I re-opened the issue to further discuss this issue as @sharwell requested. |
@mavasani @srivatsn @JohnHamby so, @sharwell want to add more context related information to error list description pane. and I made it easy for users to create a dump when exception is thrown from user analyzer. and question is whether dump is enough or we should still add more info to the description pane. |
Not every user is going to have the time or willingness to set a reg key and capture a dump. For the users who are willing to do that, great, we should have that feature because that captures the maximum amount of information. However, some users might just run into a warning and report that with the text and then not follow up. For those cases, having more information that makes the error diagnosable would be useful. There's ofcourse diminishing returns to adding more and more information there but the information requested in this issue seems reasonable to me and doesn't sound like would make our code more complex. |
I echo @sharwell his request for more information in the actual error pane message. Reconstructing test scenarios often involves stepping through |
@Vannevelj that means you stepping through repro step using debugger, which require you to first construct repro step and then debug it through yourself. dump will let you straight to the problem without going through hassle of figuring out repro step first which might or might not possible or straightforward. |
@sharwell does this look okay? Analyzer 'Microsoft.CodeAnalysis.CSharp.InvokeDelegateWithConditionalAccess.InvokeDelegateWithConditionalAccessAnalyzer' threw the following exception: System.Exception: test at Microsoft.CodeAnalysis.Diagnostics.AnalyzerExecutor.ExecuteAndCatchIfThrows_NoLock(DiagnosticAnalyzer analyzer, Action analyze, Nullable`1 info)'. |
@heejaechang Oh, that looks very nice. 😄 If we changed one thing, it would be the |
@sharwell done |
When an analyzer throws an exception within a
SyntaxNodeAnalysisContext
, it would be very helpful if the message details included information about the type of syntax node which was being analyzed at the time as well as its location within the source file. In fact, the span of the node could actually be used as the span for the AD0001 diagnostic itself.This applies in various degrees to several analysis context objects which analyzers can listen to. Each of these contexts should be handled in the most meaningful way available for that context.
📝 This is related to but not a duplicate of #6696.
The text was updated successfully, but these errors were encountered: