service/dap: support clearing breakpoints and setting breakpoint conditions (#2188)

* Support clearing breakpoints and setting conditions

* Return unverified breakpoints with errors

Co-authored-by: Polina Sokolova <polinasok@users.noreply.github.com>

service/dap/daptest/client.go

@@ -329,6 +329,11 @@ func (c *Client) DisconnectRequest() {
 
 // SetBreakpointsRequest sends a 'setBreakpoints' request.
 func (c *Client) SetBreakpointsRequest(file string, lines []int) {
+	c.SetConditionalBreakpointsRequest(file, lines, nil)
+}
+
+// SetBreakpointsRequest sends a 'setBreakpoints' request with conditions.
+func (c *Client) SetConditionalBreakpointsRequest(file string, lines []int, conditions map[int]string) {
 	request := &dap.SetBreakpointsRequest{Request: *c.newRequest("setBreakpoints")}
 	request.Arguments = dap.SetBreakpointsArguments{
 		Source: dap.Source{
@@ -340,6 +345,10 @@ func (c *Client) SetBreakpointsRequest(file string, lines []int) {
 	}
 	for i, l := range lines {
 		request.Arguments.Breakpoints[i].Line = l
+		cond, ok := conditions[l]
+		if ok {
+			request.Arguments.Breakpoints[i].Condition = cond
+		}
 	}
 	c.send(request)
 }

service/dap/error_ids.go

@@ -12,6 +12,7 @@ const (
 	// values below are inspired the original vscode-go debug adaptor.
 	FailedToLaunch            = 3000
 	FailedtoAttach            = 3001
+	UnableToSetBreakpoints    = 2002
 	UnableToDisplayThreads    = 2003
 	UnableToProduceStackTrace = 2004
 	UnableToListLocals        = 2005

service/dap/server.go

@@ -380,6 +380,7 @@ func (s *Server) onInitializeRequest(request *dap.InitializeRequest) {
 	// TODO(polina): Respond with an error if debug session is in progress?
 	response := &dap.InitializeResponse{Response: *newResponse(request.Request)}
 	response.Body.SupportsConfigurationDoneRequest = true
+	response.Body.SupportsConditionalBreakpoints = true
 	// TODO(polina): support this to match vscode-go functionality
 	response.Body.SupportsSetVariable = false
 	// TODO(polina): support these requests in addition to vscode-go feature parity
@@ -535,27 +536,57 @@ func (s *Server) onDisconnectRequest(request *dap.DisconnectRequest) {
 }
 
 func (s *Server) onSetBreakpointsRequest(request *dap.SetBreakpointsRequest) {
+	// TODO(polina): handle this while running by halting first.
+
 	if request.Arguments.Source.Path == "" {
-		s.log.Error("ERROR: Unable to set breakpoint for empty file path")
+		s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", "empty file path")
+		return
 	}
-	response := &dap.SetBreakpointsResponse{Response: *newResponse(request.Request)}
-	response.Body.Breakpoints = make([]dap.Breakpoint, len(request.Arguments.Breakpoints))
-	// Only verified breakpoints will be set and reported back in the
-	// response. All breakpoints resulting in errors (e.g. duplicates
-	// or lines that do not have statements) will be skipped.
-	i := 0
-	for _, b := range request.Arguments.Breakpoints {
-		bp, err := s.debugger.CreateBreakpoint(
-			&api.Breakpoint{File: request.Arguments.Source.Path, Line: b.Line})
-		if err != nil {
-			s.log.Error("ERROR:", err)
+
+	// According to the spec we should "set multiple breakpoints for a single source
+	// and clear all previous breakpoints in that source." The simplest way is
+	// to clear all and then set all.
+	//
+	// TODO(polina): should we optimize this as follows?
+	// See https://github.com/golang/vscode-go/issues/163 for details.
+	// If a breakpoint:
+	// -- exists and not in request => ClearBreakpoint
+	// -- exists and in request => AmendBreakpoint
+	// -- doesn't exist and in request => SetBreakpoint
+
+	// Clear all existing breakpoints in the file.
+	existing := s.debugger.Breakpoints()
+	for _, bp := range existing {
+		// Skip special breakpoints such as for panic.
+		if bp.ID < 0 {
 			continue
 		}
-		response.Body.Breakpoints[i].Verified = true
-		response.Body.Breakpoints[i].Line = bp.Line
-		i++
+		// Skip other source files.
+		// TODO(polina): should this be normalized because of different OSes?
+		if bp.File != request.Arguments.Source.Path {
+			continue
+		}
+		_, err := s.debugger.ClearBreakpoint(bp)
+		if err != nil {
+			s.sendErrorResponse(request.Request, UnableToSetBreakpoints, "Unable to set or clear breakpoints", err.Error())
+			return
+		}
+	}
+
+	// Set all requested breakpoints.
+	response := &dap.SetBreakpointsResponse{Response: *newResponse(request.Request)}
+	response.Body.Breakpoints = make([]dap.Breakpoint, len(request.Arguments.Breakpoints))
+	for i, want := range request.Arguments.Breakpoints {
+		got, err := s.debugger.CreateBreakpoint(
+			&api.Breakpoint{File: request.Arguments.Source.Path, Line: want.Line, Cond: want.Condition})
+		response.Body.Breakpoints[i].Verified = (err == nil)
+		if err != nil {
+			response.Body.Breakpoints[i].Line = want.Line
+			response.Body.Breakpoints[i].Message = err.Error()
+		} else {
+			response.Body.Breakpoints[i].Line = got.Line
+		}
 	}
-	response.Body.Breakpoints = response.Body.Breakpoints[:i]
 	s.send(response)
 }
 

service/dap/server_test.go

@@ -261,9 +261,9 @@ func TestContinueOnEntry(t *testing.T) {
 	})
 }
 
-// TestSetBreakpoint corresponds to a debug session that is configured to
+// TestPreSetBreakpoint corresponds to a debug session that is configured to
 // continue on entry with a pre-set breakpoint.
-func TestSetBreakpoint(t *testing.T) {
+func TestPreSetBreakpoint(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
 		client.InitializeRequest()
 		client.ExpectInitializeResponse(t)
@@ -272,7 +272,7 @@ func TestSetBreakpoint(t *testing.T) {
 		client.ExpectInitializedEvent(t)
 		client.ExpectLaunchResponse(t)
 
-		client.SetBreakpointsRequest(fixture.Source, []int{8, 100})
+		client.SetBreakpointsRequest(fixture.Source, []int{8})
 		sResp := client.ExpectSetBreakpointsResponse(t)
 		if len(sResp.Body.Breakpoints) != 1 {
 			t.Errorf("got %#v, want len(Breakpoints)=1", sResp)
@@ -479,8 +479,7 @@ func expectVarRegex(t *testing.T, got *dap.VariablesResponse, i int, name, value
 	return expectVar(t, got, i, name, value, false, hasRef)
 }
 
-// TestStackTraceRequest executes to a breakpoint (similarly to TestSetBreakpoint
-// that includes more thorough checking of that sequence) and tests different
+// TestStackTraceRequest executes to a breakpoint and tests different
 // good and bad configurations of 'stackTrace' requests.
 func TestStackTraceRequest(t *testing.T) {
 	runTest(t, "increment", func(client *daptest.Client, fixture protest.Fixture) {
@@ -1134,6 +1133,104 @@ func TestLaunchRequestWithStackTraceDepth(t *testing.T) {
 	})
 }
 
+// TestSetBreakpoint executes to a breakpoint and tests different
+// configurations of setBreakpoint requests.
+func TestSetBreakpoint(t *testing.T) {
+	runTest(t, "loopprog", func(client *daptest.Client, fixture protest.Fixture) {
+		runDebugSessionWithBPs(t, client,
+			// Launch
+			func() {
+				client.LaunchRequest("exec", fixture.Path, !stopOnEntry)
+			},
+			// Set breakpoints
+			fixture.Source, []int{16}, // b main.main
+			[]onBreakpoint{{
+				execute: func() {
+					handleStop(t, client, 1, 16)
+
+					type Breakpoint struct {
+						line      int
+						verified  bool
+						msgPrefix string
+					}
+					expectSetBreakpointsResponse := func(bps []Breakpoint) {
+						t.Helper()
+						got := client.ExpectSetBreakpointsResponse(t)
+						if len(got.Body.Breakpoints) != len(bps) {
+							t.Errorf("got %#v,\nwant len(Breakpoints)=%d", got, len(bps))
+							return
+						}
+						for i, bp := range got.Body.Breakpoints {
+							if bp.Line != bps[i].line || bp.Verified != bps[i].verified ||
+								!strings.HasPrefix(bp.Message, bps[i].msgPrefix) {
+								t.Errorf("got breakpoints[%d] = %#v, \nwant %#v", i, bp, bps[i])
+							}
+						}
+					}
+
+					// Set two breakpoints at the next two lines in main
+					client.SetBreakpointsRequest(fixture.Source, []int{17, 18})
+					expectSetBreakpointsResponse([]Breakpoint{{17, true, ""}, {18, true, ""}})
+
+					// Clear 17, reset 18
+					client.SetBreakpointsRequest(fixture.Source, []int{18})
+					expectSetBreakpointsResponse([]Breakpoint{{18, true, ""}})
+
+					// Skip 17, continue to 18
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 18)
+
+					// Set another breakpoint inside the loop in loop(), twice to trigger error
+					client.SetBreakpointsRequest(fixture.Source, []int{8, 8})
+					expectSetBreakpointsResponse([]Breakpoint{{8, true, ""}, {8, false, "Breakpoint exists"}})
+
+					// Continue into the loop
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 8)
+					client.VariablesRequest(1001) // Locals
+					locals := client.ExpectVariablesResponse(t)
+					expectVarExact(t, locals, 0, "i", "0", noChildren) // i == 0
+
+					// Edit the breakpoint to add a condition
+					client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: "i == 3"})
+					expectSetBreakpointsResponse([]Breakpoint{{8, true, ""}})
+
+					// Continue until condition is hit
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 8)
+					client.VariablesRequest(1001) // Locals
+					locals = client.ExpectVariablesResponse(t)
+					expectVarExact(t, locals, 0, "i", "3", noChildren) // i == 3
+
+					// Edit the breakpoint to remove a condition
+					client.SetConditionalBreakpointsRequest(fixture.Source, []int{8}, map[int]string{8: ""})
+					expectSetBreakpointsResponse([]Breakpoint{{8, true, ""}})
+
+					// Continue for one more loop iteration
+					client.ContinueRequest(1)
+					client.ExpectContinueResponse(t)
+					client.ExpectStoppedEvent(t)
+					handleStop(t, client, 1, 8)
+					client.VariablesRequest(1001) // Locals
+					locals = client.ExpectVariablesResponse(t)
+					expectVarExact(t, locals, 0, "i", "4", noChildren) // i == 4
+
+					// Set at a line without a statement
+					client.SetBreakpointsRequest(fixture.Source, []int{1000})
+					expectSetBreakpointsResponse([]Breakpoint{{1000, false, "could not find statement"}}) // all cleared, none set
+				},
+				// The program has an infinite loop, so we must kill it by disconnecting.
+				disconnect: true,
+			}})
+	})
+}
+
 func TestNextAndStep(t *testing.T) {
 	runTest(t, "testinline", func(client *daptest.Client, fixture protest.Fixture) {
 		runDebugSessionWithBPs(t, client,
@@ -1313,11 +1410,11 @@ func runDebugSessionWithBPs(t *testing.T, client *daptest.Client, launchRequest
 	client.ExpectDisconnectResponse(t)
 }
 
-// runDebugSesion is a helper for executing the standard init and shutdown
+// runDebugSession is a helper for executing the standard init and shutdown
 // sequences for a program that does not stop on entry
 // while specifying unique launch criteria via parameters.
-func runDebugSession(t *testing.T, client *daptest.Client, launchRequest func()) {
-	runDebugSessionWithBPs(t, client, launchRequest, "", nil, nil)
+func runDebugSession(t *testing.T, client *daptest.Client, launchRequest func(), source string) {
+	runDebugSessionWithBPs(t, client, launchRequest, source, nil, nil)
 }
 
 func TestLaunchDebugRequest(t *testing.T) {
@@ -1328,7 +1425,7 @@ func TestLaunchDebugRequest(t *testing.T) {
 			// Use the default output directory.
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "debug", "program": fixture.Source})
-		})
+		}, fixture.Source)
 	})
 }
 
@@ -1341,7 +1438,7 @@ func TestLaunchTestRequest(t *testing.T) {
 			testdir, _ := filepath.Abs(filepath.Join(fixtures, "buildtest"))
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "test", "program": testdir, "output": "__mytestdir"})
-		})
+		}, fixture.Source)
 	})
 }
 
@@ -1355,7 +1452,7 @@ func TestLaunchRequestWithArgs(t *testing.T) {
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "exec", "program": fixture.Path,
 				"args": []string{"test", "pass flag"}})
-		})
+		}, fixture.Source)
 	})
 }
 
@@ -1371,7 +1468,7 @@ func TestLaunchRequestWithBuildFlags(t *testing.T) {
 			client.LaunchRequestWithArgs(map[string]interface{}{
 				"mode": "debug", "program": fixture.Source,
 				"buildFlags": "-ldflags '-X main.Hello=World'"})
-		})
+		}, fixture.Source)
 	})
 }