DAP error response handling is confusing and inconsistent

[rehmsen]

Does this issue occur when all extensions are disabled?: Yes/No

• VS Code Version: 1.77.3
• OS Version: macOS Ventura 13.3.1

Steps to reproduce

1. Respond e.g. to the "initialize" DAP requests with the various combinations explained below.

Various combinations in the error response

Handling of debug adapter protocol error responses is confusing:

• showUser (from DAP) and showErrors (passed to send) are ignored when a url is provided - it will always show a dialog:
• if showUsers is false, showErrors is ignored - it will never show a message (this may be fine, but then showErrors could probably be named better)
• if showUser is true
  • ...showErrors confusingly controls whether to show them in a dialog (showErrors= false) or in a notification (showErrors=true)
  • ... and showErrors is false, and no error.format is send from DAP, it will still show a dialog, but the dialog will be empty, looking broken.

This is the logic I am referring to: https://github.com/microsoft/vscode/blob/c29e4b0fdb6b63c23be7055feb5a188784a86691/src/vs/workbench/contrib/debug/browser/rawDebugSession.ts#L760-L785

DAP unfortunately does not specify exactly what should happen if showUser is false but a URL is provided: https://github.com/microsoft/debug-adapter-protocol/issues/387

It also does not seem to have a way to express severity so that one could decide whether a dialog or a notification is more appropriate. In the absence of that, maybe this should be decided based on criticality of the request?

I think it would make sense that:

• failing a critical request always causes an error dialog
  • if showUser=true, it should have the text from the DAP response
  • if showUser=false, it should be a generic failure message generated on the client
• failing a non-critical request
  • ...with showUser=true should show a notification - if a url is provided, it could be a prompt notification with actions
  • ...with showUser=false should not show anything to the user

In my mind, "initialize" is a critical requests. I wonder why VSCode chose to set showErrors=false for "initialize" requests (and true for almost everything else) - maybe it is because a failure to initialize then causes a "disconnect" request, which already shows an error?

@weinand @roblourens and @isidorn Seems to have worked on this function and may have additional background as to the intentions here?

I would be happy to contribute here, but we probably need to get on the same page regarding the desired behavior first.

[roblourens]

This is even more complex than you describe, because there is one more dimension- how the debug request was triggered. I tested this by failing setBreakpoints, and when it is triggered by a keypress, the error bubbles up to the keybindings service and is shown there

https://github.com/microsoft/vscode/blob/5df31a17c8559bdb61bda296e5e7677c57a5a0e8/src/vs/platform/keybinding/common/abstractKeybindingService.ts#L367

So what I see is

• breakpoint triggered by a keypress- a notification always appears (as a warning)
• breakpoint triggered by a click- only a notification for showUser=true

And as you point out, showUser is ignored when a url is set, but that means that it still never shows an error if you clicked and it always shows one if you used a keybinding. There might be even more to that, depending on what command you were testing with and how you triggered it.

I dug into the history of the code, and I think it's clear that the intent is that showUser should be interpreted in the way you expect and be independent of the url property. I would call that aspect a bug. Really just a reference for myself later:

• It was added in this change originally https://github.com/microsoft/vscode/commit/fe9bc04a75e92c4630cd1917320b8d8827e4548b
• It was removed entirely in this change, I think on accident, since the change was just targeting canceled errors https://github.com/microsoft/vscode/commit/b7e8092b8df23b5017269335ed4a0968870a1269
• It was readded here https://github.com/microsoft/vscode/issues/89059

Then for the next part of the issue, what showErrors does, that was added for this issue https://github.com/microsoft/vscode/issues/128484 basically we are showing the error dialog somewhere else upstream for some requests, and that upstream code was changed to support showUser but we need to set that property on the error for errors with url too.

[rehmsen]

Thanks for looking into this, and great that we are aligned that this should be improved. Let me know if I can do anything to help.

[DanTup]

Seems like showUser is treated differently if not defined in different places too:

https://github.com/microsoft/vscode/blob/314c9891c3b676cf476a3f95b01492c1931b4ca7/src/vs/workbench/contrib/debug/browser/debugService.ts#L635

https://github.com/microsoft/vscode/blob/314c9891c3b676cf476a3f95b01492c1931b4ca7/src/vs/workbench/contrib/debug/browser/rawDebugSession.ts#L805

It would be good to expand the documentation here. The DAP spec only says "If true show user", but it's not clear what that means... does it mean show an extra error notification in addition to in the context of where this failed, or does it mean show an error at all?

Setting true causes this toast notification here that is unhelpful:

So it seems like false might be better (the error will still appear in the debug console, watch window, etc.). However it's not clear from the spec that that's the meaning - it could be interpreted to mean that showError: false means not to show the error anywhere.

It's important that DAP clients have similar behaviour because otherwise it's impossible for a debug adapter to know what to use (or, it'll start sniffing clients, and that would be bad for everyone :-) )