Surface new Exception DAP in UI

[weinand]

In the last milestone we've added new debug adapter protocol to retrieve information about a thrown exception and we've introduced a peek UI to show this information to the user.

Now we have to bring both things together.

[weinand]

I've implemented a first cut of the new exception info request in node-debug.
Now VS Code needs to make use of this.

If the capability object returned from the InitializeRequest has a value true for the supportsExceptionInfoRequest, the exceptionInfoRequest is supported by the DA. In this case VS Code should populate the exception peek UI from the values returned from the exceptionInfoRequest instead of using the StoppedEvent.

The exceptionInfoRequest expects the threadId as the only input argument.
It returns the following structure:

The additional information (for now) is the breakMode and the details.stacktrace. Both should be surfaced in the peek UI.

[isidorn]

This has been done by @michelkaporin in #22948

[isidorn]

Tried it out and it looks good to me.
@weinand why does the protocol surround the stack trace with quotes - I think it would look nicer if no quotes were shown
@roblourens it would be cool if node2 debug adapter also picks up the support for exceptions so we do not have inconsistencies between these two expeirences
@michelkaporin can you please create a short test plan item for this feature

[weinand]

@isidorn The stack trace is surrounded by quotes because it is a "string" value (and we always do the value formatting in the backend so that VS Code does not have to second guess). So I will remove the quotes in this case.

@michelkaporin I suggest to introduce some space between the exception description ("Unhandled exception has occurred....") and the stack trace (e.g. 1/2 a line). Without this the UI looks a bit crammed.

[weinand]

I'm removing the quotes from the stacktrace now:
microsoft/vscode-node-debug@81c69ed

[roblourens]

Btw I just took a PR to apply sourcemaps to stacktraces. I think it will look very nice with this widget.

microsoft/vscode-chrome-debug-core#190

[weinand]

But this requires the ExceptionInfo request for node2...

[roblourens]

I have an issue to implement it for node2

[michelkaporin]

@weinand You've mentioned:

The additional information (for now) is the breakMode and the details.stacktrace. Both should be surfaced in the peek UI.

Sometimes details.stacktrace is not available from the DA response. What would be your suggestion to show instead of it in the exception widget then?
Currently DA response if surfaced the following way (structure with current showcase below):

breakMode-type exception has occurred. description
details.stackTrace

My proposal would be to fall back to the UI case when DA does not support exceptionInfo request, but with breakMode-type message included (see the image below).

Would be good to hear @isidorn opinion also.

This decision is needed as part of fixing #23388.

[michelkaporin]

FYI This is how it looks like when DA does not support exceptionInfo request:

[weinand]

If details (and hence the stacktrace) is missing we should still continue to use the new layout and not fallback to the old layout.

The rational behind this is as follows:
In the future there will be more details coming through the ExceptionInfo request and we will have to integrate them into the layout as well. Always falling back to the old layout doesn't make a lot of sense because there will be always some details provided that are not covered by the old layout at all. And we do not want to lose those.

[isidorn]

I agree with @weinand

[michelkaporin]

Makes perfect sense. So this is the way we go for then, omitting the stack trace completely if it is not returned by a DA:

[weinand]

the latest DA will always return an exceptionID (as spec'ed). So this will result in:

Unhandled exception has occurred. string: heello

[michelkaporin]

Currently we don't surface exceptionID, so I would be glad to hear how do we do it in a better way, because node DA description already contains exceptionID in its current implementation.

Printing it twice would make the information redundant.

[weinand]

In the original protocol change request 'Exception ID' got this example usage:

Exception ID (e.g. "CLR/System.ArgumentException")

In VS it is shown prominently as the title:

So my proposal would be:

${breakMode} exception: ${exceptionID}
${description}

${details.stackTrace}

[michelkaporin]

@weinand I agree with your proposal, and currently it looks the following way with the latest version of node DA.

• Node DA with exceptionInfo request support:
  
  

• Node DA without exceptionInfo request support:
  

It would be great to hear also @isidorn's feedback on that, and then to have your change in node DA so that it does not return redundant information as in the first image, for instance.

[weinand]

Since the description is optional, I'm planning to omit the description if it already appears in the stack trace.

@michelkaporin Please make sure that there are not two empty lines in that case.

[weinand]

@roblourens VS Code now uses the following layout for the exception peek UI:

${breakMode} exception: ${exceptionID}
${description}

${details.stackTrace}

To avoid duplication of the description ("Error: heello" from above), I'm now omitting the description from the response if it already appears at the beginning of the stack trace (I prefer this approach over removing the first line from the stacktrace because it leaves the stacktrace intact).

[roblourens]

I see 'undefined' in the exception widget now:

[michelkaporin]

@roblourens VS Code now uses the following layout for the exception peek UI:

I have not pushed my changes yet to make this layout, waiting for our UI master @isidorn's feedback and then will update the widget accordingly.

Hence, @roblourens you started seeing 'undefined' because of @weinand change above, which is not yet in sync with current widget UI implementation.

[roblourens]

Oh ok. I'll make the same change in node2.