Type Breakpoint contains empty source

[corneliusweig]

The Breakpoint is defined as

type Breakpoint struct {
	Id                   int    `json:"id,omitempty"`
	Verified             bool   `json:"verified"`
	Message              string `json:"message,omitempty"`
	Source               Source `json:"source,omitempty"`
	Line                 int    `json:"line,omitempty"`
	Column               int    `json:"column,omitempty"`
	EndLine              int    `json:"endLine,omitempty"`
	EndColumn            int    `json:"endColumn,omitempty"`
	InstructionReference string `json:"instructionReference,omitempty"`
	Offset               int    `json:"offset,omitempty"`
}

It says "source,omitempty", but an empty object is serialised as:

data, _ := json.Marshal(dap.Breakpoint{})
fmt.Println(string(data))

// {"verified":false,"source":{}}
//                     ^^^^^^^^

This means that BreakpointEvent types with missing source are serialised with an empty source object, which clears the breakpoint location in the UI.

Can the Source field be changed to a pointer field instead?

[corneliusweig]

I haven't checked other types in this repository, but I can look up if this is also an issue elsewhere.

[hyangah]

Thanks for the bug report.
Can you help us find the part in the spec that explains the editor's behavior you were observing? More specifically, I want to understand why the editor is clearing the breakpoint location when seeing completely empty Source object.

[corneliusweig]

The spec is a bit vague, but I think it's there:

1. The documentation for BreakpointEvent (https://microsoft.github.io/debug-adapter-protocol/specification#Events_Breakpoint) says:

       /**
        * The `id` attribute is used to find the target breakpoint, the other
        * attributes are used as the new values.
        */
       breakpoint: Breakpoint;

   I think the phrase the other attributes are used as the new values means that whatever is specified in the breakpoint updates existing values.

2. The Breakpoint type has an optional source field (https://microsoft.github.io/debug-adapter-protocol/specification#Types_Breakpoint):

     /**
      * The source where the breakpoint is located.
      */
     source?: Source;

Taken together, I think it makes sense to read it as "when present, the source field updates existing values".

Note: All fields in the Source type are optional. However, if they would always be merged into the existing values, it would be impossible to clear fields. Therefore, I think it does not make sense to assume that an empty Source gets merged into an existing one.

[hyangah]

Thanks @corneliusweig for helping with interpreting the spec (cc @weinand to confirm the interpretation of the breakpoint source field)

I guess the proposed change will affect only the shape of the messages produced by debug adapter (e.g. Delve), and clients (e.g. VS Code) will continue to work with old/new versions of Delve. So, the impact on the Delve and editors that use Delve DAP is not concerning once delve is updated to work with the newer version of go-dap after the change.

There are a couple of debug adapters using go-dap outside Delve https://pkg.go.dev/github.com/google/go-dap?tab=importedby From my quick search, I believe they can quickly resolve the incompatibility when they upgrade the dependencies.

IMO the change to fix the bug is acceptable, but I want to hear opinion from @polinasok @suzmue @aarzilli @derekparker
(And go-dap is still in v0 😄 ).

[suzmue]

I agree that fixing this bug makes sense.

Some initial investigation in the vscode source VS Code seems that it handles undefined differently than zero (https://github.com/microsoft/vscode/blob/9dba5385c7aefebe3756ed413da7fedf1868a71c/src/vs/workbench/contrib/debug/common/debugModel.ts#L966).

Delve does not yet use BreakpointEvents to update breakpoints. Delve sends all information about breakpoints through the response to the setBreakpoints requests.

[corneliusweig]

Thank you! I tentatively created a PR for this trivial change.

If this is fixed for Breakpoint.Source, there are a few other places that should possibly be updated too. I found a few fields that are marked as omitempty, but are struct types, so they would be serialised as empty objects as described above. Additional fields I could imagine are problematic are:

• DisassembledInstruction.Location
• Scope.Source
• SetExpressionResponseBody.PresentationHint
• EvaluateResponseBody.PresentationHint