VS Code doesn't display error response from invalid exception filter conditions

[gregg-miskelly]

Issue Type: Bug

1. Configure an exception filter with an invalid condition. In my case, I am using an experimental version of the C# debugger, and I used the bogus coniditon of '==Bogus'
2. Start debugging

Expected:
VS Code should somehow tell me that I entered an invalid exception filter by displaying the text from the error response as the DAP spec indicates. My suggestion would be a warning icon next to the exception filters and display the error message as a notification.

Actual:
The error response isn't displayed anywhere.

Here is a partial protocol trace:

-> {"command":"setExceptionBreakpoints","arguments":{"filters":[],"filterOptions":[{"filterId":"all","condition":"==Bogus"},{"filterId":"user-unhandled"}]},"type":"request","seq":5}
<- {"seq":10,"type":"response","request_seq":5,"success":false,"command":"setExceptionBreakpoints","message":"Syntax error in condition for 'All Exceptions'. Unexpected character near column 0 (character '=')."}

VS Code version: Code - Insiders 1.54.0-insider (3290f9a, 2021-02-26T05:13:29.656Z)
OS version: Windows_NT x64 10.0.19042

System Info

Item	Value
CPUs	Intel(R) Core(TM) i9-9880H CPU @ 2.30GHz (16 x 2304)
GPU Status	2d_canvas: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on oop_rasterization: enabled opengl: enabled_on protected_video_decode: enabled rasterization: enabled skia_renderer: enabled_on video_decode: enabled vulkan: disabled_off webgl: enabled webgl2: enabled
Load (avg)	undefined
Memory (System)	31.79GB (9.60GB free)
Process Argv	--crash-reporter-id 8dd46428-3c33-429b-aec8-0a1119b2424f
Screen Reader	no
VM	0%

[gregg-miskelly]

CC @isidorn

[isidorn]

Showing the error via notification is easy but I think it is visually far away from breakpoints. It is in the right corner, while breakpoints are usually in the left. Same as unbound breakpoint I suggest to simply render this in the breakpoints view.

We can render the exception as "not verified" which means grayed out and on hover to show the message.
Or we need a new way to render these breakpoints which are verified but just the condition is ignored. Using a warning icon maybe. Whatever we choose here we should show the message on hover.

Let me know what you think

fyi @weinand @connor4312

[gregg-miskelly]

Sounds good to me.

[connor4312]

A warning icon would work well. For normal breakpoint I think I manually create a console output with an error but might not have done that for exception breakpoints. But this is also easy to miss if you have console spam or don't have the debug console visible.

[isidorn]

Ok, assigning to March, I will render this somehow in the Breakpoints view. Will post here.

[isidorn]

@gregg-miskelly I just noticed that in your error response you give a message specific to one exception breakpoint. However this is a setExceptionBreakpoints request. So the best VS Code can do is show that same message for every exception breakpoint.

Due to that I think this approach might not be the nicest. And I suggest the following:

1. Debug adapters send out the BreakpointEvent update with the corresponding message. VS Code currently does not support updating message for exception breakpoints but we can change this
2. If the setBreakpoint request results in an error, vscode will render breakpoints as unbounded

If we combine these two, the breakpoint will be rendered as unbounded and the debug extension can choose what message to show for each breakpoint.

Does this make sense? Or you prefer to have a global message for all breakpoint if the setBreakpoints request fails

@weinand @connor4312 let me know what you think

[gregg-miskelly]

For C#, I think it would be good enough to just handle failures from setExceptionBreakpoints -- we only define two exception breakpoints, so it is probably clear enough to users what went wrong. But I agree with you that it would be even better if there was an event to indicate that a condition is invalid. So if we think it is worth it I would be happy to update C# to send back an event instead of failing the request. I might suggest a different event from BreakpointEvent because exception filters don't have a breakpoint id (which is how I am assuming we would match up the update) or a way to send back what the breakpoint id would be. So I think I would instead suggest we define a new event for this (ExceptionFilterError? or if we want to be able to say that an exception filter condition is now valid then maybe ExceptionFilterStatusUpdate).

[isidorn]

@gregg-miskelly okey, I was more thinking in general of invalid conditions for breakpoints. So if we introduce a new invalid condition event it should cover all breakpoints, not only exception breakpoints.

But if we want to solve this only for exception breakpoints, I do not mind just showing the message from the exception breakpoints response on each exception breakpoint.

Let's see what @weinand and @connor4312 feel like.

[connor4312]

Yea, this is something that I also deal with manually for logpoint and conditional breakpoints. If there's a syntax error I return that specific breakpoint as being unbound and then log output to the console. A generalized fix would be preferred. I think having something in SetBreakpointsResult would be the cleanest way to do something, perhaps an additional array of errors: (string | undefined)[].

[gregg-miskelly]

@connor4312 Are you reporting this error to the console in addition to a BreakpointEvent with verified=false and a message? If not, are you doing it that way because you will continue to evaluate the condition/action each time it is hit?

To be clear - I think it would be great if we could use BreakpointEvent to report failures for both normal breakpoints and exception breakpoints, and if we could go back to the start of DAP and do exception filters in that way it would make sense to me. But since exception filters don't have breakpoint ids today, I feel like it doesn't quite fit.

[connor4312]

Are you reporting this error to the console in addition to a BreakpointEvent with verified=false and a message?

Yea, that's right. Simply verified=false doesn't give much info, I want to be able to tell users they have an unclosed quote that's preventing their breakpoint from setting.

[weinand]

Thanks for the discussion and your input!

I've tried to produce a summary and proposal:

There is a desire to treat all breakpoints the same.
Today Exception breakpoints are different because they don't have an ID since there is no way to return the ID from the corresponding SetExceptionBreakpoints request.

One area where this difference between breakpoints hurts a lot, is with error reporting: neither is it possible to return a "failed verification" message from SetExceptionBreakpoints, nor is it possible to send out breakpoint exceptions that contain the error message (because of the lack of an ID).

IMO the cleanest approach would be to align SetExceptionBreakpoints with SetBreakpoints and allow to optionally return an array of Breakpoints. These breakpoints can be used to return an ID (so that breakpoint events can be sent), or the error message can be directly returned via the verified and message properties.

I'd prefer the latter because I think that syntax errors in the condition are something that should be returned synchronously rather than as a async event.

@gregg-miskelly @connor4312 @isidorn what do you think?

[isidorn]

@weinand I like both your proposals. However I prefer the second one since it is sync and does not require an introduction of an ID and thus making it simpler.

[weinand]

@isidorn please note, that returning an array of breakpoints from SetExceptionBreakpoints makes it possible to assign IDs to exception breakpoints and refer to them in events later even if a "failed verification message" is used.

[gregg-miskelly]

That works for me.

[isidorn]

@weinand I see. That is still fine for me.

[isidorn]

Also assigning to @weinand to add the DAP changes needed for this
@weinand feel free to push this out to next milestone if you do not have time. Both this and next milestone work for me

[weinand]

I've created microsoft/debug-adapter-protocol#184 for the required DAP changes.

Since that work will happen in April, I've changed the milestone for this issue too.

[weinand]

@isidorn I've pushed the DAP change to VS Code.

[isidorn]

@weinand great, thanks a lot!
I can look into implementing this!

[isidorn]

I have implemented this and pushed to Insiders.
So exceptionBreakpoint will now respect the id from the response which can later also be used in the update event.

Here's how an exceptionBreakpoint which returns verified: false and a message would look like. So this is aligned with the regular breakpoint unverified experience.
I did not test out all the cases since there is not an adapter that implements this. @gregg-miskelly so it would be very cool if you give this a try and let us know how it goes. Thanks a lot!

[gregg-miskelly]

@isidorn I have been on vacation, so it will be a couple of days before I have a chance. But will do. Thanks!

[isidorn]

@gregg-miskelly no hurries, next week would be great (before we release).

[gregg-miskelly]

@isidorn Your changes worked for all the cases I can think of. Thanks for fixing this!

[isidorn]

Great, thanks for letting us know. Adding verified label.