Open rehmsen opened 11 months ago
It's very surprising that VSCode interprets undefined as truthy for showUser
. This goes against common practice with optional fields in JS/TS or Go. In my opinion, the DAP spec states clearly that debug adapters can only expect that the message is shown when showUser: true
:
/**
* If true show user.
*/
showUser?: boolean;
So I'm not convinced that this field should be changed to a pointer to make it optional, at least for this reason.
OTOH, you could argue that the DAP spec states that the field is optional (i.e. it is a tri-state), and this Go implementation does not support that. So this looks like a valid reason to me. However, pointer fields are much more error-prone and harder to work with. So if it is not strictly required to convert the field to pointer type, I'd prefer to keep it as-is.
In https://github.com/Dart-Code/Dart-Code/issues/4930 we have just discovered that VSCode has two different defaults for showUser
, in one case it defaults to true
And in another, it defaults to false:
Without properly handling the tristate nature of this field, it is impossible to represent the message in the lossless manner if showUser
is not specified.
Currently, go-dap reprents JSON booleans as
bool
, regardless of whether they are optional or required. As a result, a message that does not set a boolean will have it set tofalse
when deserialized and then re-serialized.Unfortunately, it is not always specified in the DAP protocol how to treat a missing boolean field. Take for example https://microsoft.github.io/debug-adapter-protocol/specification#message:
Strictly speaking (because it is not "Iff" aka "if and only if"), this comment does not specify what should happen when this is unset. And sure enough VS Code interprets missing as true for this field: https://github.com/microsoft/vscode/blob/27a0cbb26647670ad719bcb47e2d1ca4cee133bf/src/vs/workbench/contrib/debug/browser/debugService.ts#L631
We are deserializing and serializing all messages in a proxy for logging and sanitization, so go-dap turning unset into false is a problem for us.
This could be fixed by representing optional booleans as
*bool
(and then usingomitempty
), similar to how we represent optional nested messages as*struct
. Unfortunately this is a breaking change as users of the library would then need to dereference the pointer as they read and assign, and handle the nil case explicitly. However, I think this is the correct handling.What do you think?