Represent optional JSON boolean as *bool in Go to avoid altering messages when deserializing and re-serializing

[rehmsen]

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 to false 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:

   /**
   * If true show user.
   */
  showUser?: boolean;

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 using omitempty), 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?

[corneliusweig]

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.

[chingjun]

In Dart-Code/Dart-Code#4930 we have just discovered that VSCode has two different defaults for showUser, in one case it defaults to true

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

And in another, it defaults to false:

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

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.