[dds/dap] Provide reason/message for unverified breakpoints

This adds some additional information to breakpoint response/events so that in VS Code a grey breakpoint has a tooltip that explains the reason.

Fixes Dart-Code/Dart-Code#5314

Change-Id: I1defdbc9b62b8536b81efbe3f01626e6faf8db50
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/409280
Reviewed-by: Ben Konyi <[email protected]>
Commit-Queue: Derek Xu <[email protected]>
Reviewed-by: Derek Xu <[email protected]>

Author: DanTup

pkg/dds/lib/src/dap/adapters/dart.dart

@@ -1583,7 +1583,11 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
           // Send breakpoints back as unverified and with our generated IDs so we
           // can update them with a 'breakpoint' event when we get the
           // 'BreakpointAdded'/'BreakpointResolved' events from the VM.
-          .map((bp) => Breakpoint(id: bp.id, verified: false))
+          .map((bp) => Breakpoint(
+              id: bp.id,
+              verified: false,
+              message: 'Breakpoint has not yet been resolved',
+              reason: 'pending'))
           .toList(),
     ));
     completer.complete();

pkg/dds/lib/src/dap/isolate_manager.dart

@@ -131,6 +131,11 @@ class IsolateManager {
   /// Any leading character matched in place of the dollar is in the first capture.
   final _braceNotPrefixedByDollarOrBackslashPattern = RegExp(r'(^|[^\\\$]){');
 
+  /// A [RegExp] to extract the useful part of an error message when adding
+  /// breakpoints so that the tooltip shown in editors can be less wordy.
+  final _terseBreakpointFailureRegex =
+      RegExp(r'Error occurred when resolving breakpoint location: (.*?)\.?$');
+
   IsolateManager(this._adapter);
 
   /// A list of all current active isolates.
@@ -853,6 +858,48 @@ class IsolateManager {
     );
   }
 
+  /// Queues a breakpoint event that passes an error reason from the VM back to
+  /// the client.
+  ///
+  /// This queue will be processed only after the client has been given the ID
+  /// of this breakpoint. If that has already happened, the event will be
+  /// processed on the next task queue iteration.
+  void queueFailedBreakpointEvent(
+    Object error,
+    ClientBreakpoint clientBreakpoint,
+  ) {
+    // Attempt to clean up the message to something that fits better in a
+    // tooltip.
+    //
+    // An example failure is:
+    //
+    //    addBreakpointWithScriptUri: Cannot add breakpoint at line 8.
+    //    Error occurred when resolving breakpoint location:
+    //    No debuggable code where breakpoint was requested.
+    var userMessage = error is vm.RPCError
+        ? error.details ?? error.toString()
+        : error.toString();
+    var terseMessageMatch =
+        _terseBreakpointFailureRegex.firstMatch(userMessage);
+    if (terseMessageMatch != null) {
+      userMessage = terseMessageMatch.group(1) ?? userMessage;
+    }
+
+    final updatedBreakpoint = Breakpoint(
+      id: clientBreakpoint.id,
+      verified: false,
+      message: userMessage,
+      reason: 'failed',
+    );
+    // Ensure we don't send the breakpoint event until the client has been
+    // given the breakpoint ID by queueing it.
+    clientBreakpoint.queueAction(
+      () => _adapter.sendEvent(
+        BreakpointEventBody(breakpoint: updatedBreakpoint, reason: 'changed'),
+      ),
+    );
+  }
+
   /// Attempts to resolve [uris] to file:/// URIs via the VM Service.
   ///
   /// This method calls the VM service directly. Most requests to resolve URIs
@@ -1017,6 +1064,7 @@ class IsolateManager {
           // request as it's very easy for editors to send us breakpoints that
           // aren't valid any more.
           _adapter.logger?.call('Failed to add breakpoint $e');
+          queueFailedBreakpointEvent(e, bp);
         }
       });
     }

pkg/dds/test/dap/integration/debug_breakpoints_test.dart

@@ -75,6 +75,69 @@ main() {
       expect(resolvedBreakpoints, addedBreakpoints);
     });
 
+    testWithUriConfigurations(
+        () => dap, 'provides reason for failed breakpoints', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(debuggerPauseProgram);
+      final invalidBreakpointLine = 9999;
+
+      // Start running the app.
+      await Future.wait([
+        client.initialize(),
+        client.launch(testFile.path),
+      ], eagerError: true);
+
+      // Collect reasons from all breakpoint events.
+      final breakpointReasons = <String?>[];
+      final breakpointChangedSubscription =
+          client.breakpointChangeEvents.listen((event) {
+        breakpointReasons
+            .add('${event.breakpoint.reason}: ${event.breakpoint.message}');
+      });
+
+      // Set the breakpoint and also collect the original reason.
+      var bps = await client.setBreakpoint(testFile, invalidBreakpointLine);
+      var bp = bps.breakpoints.single;
+      breakpointReasons.add('${bp.reason}: ${bp.message}');
+
+      // Wait up to a few seconds for the change events to come through to
+      // allow for slow CI bots, but exit early if they all arrived.
+      final testUntil = DateTime.now().toUtc().add(const Duration(seconds: 5));
+      while (DateTime.now().toUtc().isBefore(testUntil) &&
+          breakpointReasons.length < 2) {
+        await pumpEventQueue(times: 5000);
+      }
+
+      expect(
+          breakpointReasons,
+          equals([
+            'pending: Breakpoint has not yet been resolved',
+            'failed: No debuggable code where breakpoint was requested'
+          ]));
+
+      await breakpointChangedSubscription.cancel();
+    });
+
+    testWithUriConfigurations(
+        () => dap, 'provides reason for not-yet-resolved breakpoints',
+        () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(debuggerPauseProgram);
+      final breakpointLine = lineWith(testFile, breakpointMarker);
+
+      // Start running the app.
+      await Future.wait([
+        client.initialize(),
+        client.launch(testFile.path),
+      ], eagerError: true);
+
+      // Set a breakpoint and verify the result.
+      var bps = await client.setBreakpoint(testFile, breakpointLine);
+      expect(bps.breakpoints.single.reason, 'pending');
+      expect(bps.breakpoints.single.message,
+          'Breakpoint has not yet been resolved');
+    });
+
     test('responds to setBreakpoints before any breakpoint events', () async {
       final client = dap.client;
       final testFile =