[dds] Add support for conditional breakpoints in DAP

Change-Id: I5f28337b0371f4efb52b2ba169bf27e1d61425c3
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/209702
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>

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

@@ -546,11 +546,11 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
         ),
       ],
       supportsClipboardContext: true,
-      // TODO(dantup): All of these...
-      // supportsConditionalBreakpoints: true,
+      supportsConditionalBreakpoints: true,
       supportsConfigurationDoneRequest: true,
       supportsDelayedStackTraceLoading: true,
       supportsEvaluateForHovers: true,
+      // TODO(dantup): All of these...
       // supportsLogPoints: true,
       // supportsRestartFrame: true,
       supportsTerminateRequest: true,

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

@@ -55,6 +55,10 @@ class IsolateManager {
   /// isolates that appear after initial breakpoints were sent.
   final Map<String, List<SourceBreakpoint>> _clientBreakpointsByUri = {};
 
+  /// Tracks client breakpoints by the ID assigned by the VM so we can look up
+  /// conditions/logpoints when hitting breakpoints.
+  final Map<String, SourceBreakpoint> _clientBreakpointsByVmId = {};
+
   /// Tracks breakpoints created in the VM so they can be removed when the
   /// editor sends new breakpoints (currently the editor just sends a new list
   /// and not requests to add/remove).
@@ -271,6 +275,27 @@ class IsolateManager {
   ThreadInfo? threadForIsolate(vm.IsolateRef? isolate) =>
       isolate?.id != null ? _threadsByIsolateId[isolate!.id!] : null;
 
+  /// Evaluates breakpoint condition [condition] and returns whether the result
+  /// is true (or non-zero for a numeric), sending any evaluation error to the
+  /// client.
+  Future<bool> _breakpointConditionEvaluatesTrue(
+    ThreadInfo thread,
+    String condition,
+  ) async {
+    final result =
+        await _evaluateAndPrintErrors(thread, condition, 'condition');
+    if (result == null) {
+      return false;
+    }
+
+    // Values we consider true for breakpoint conditions are boolean true,
+    // or non-zero numerics.
+    return (result.kind == vm.InstanceKind.kBool &&
+            result.valueAsString == 'true') ||
+        (result.kind == vm.InstanceKind.kInt && result.valueAsString != '0') ||
+        (result.kind == vm.InstanceKind.kDouble && result.valueAsString != '0');
+  }
+
   /// Configures a new isolate, setting it's exception-pause mode, which
   /// libraries are debuggable, and sending all breakpoints.
   Future<void> _configureIsolate(vm.IsolateRef isolate) async {
@@ -281,6 +306,44 @@ class IsolateManager {
     ], eagerError: true);
   }
 
+  /// Evaluates an expression, returning the result if it is a [vm.InstanceRef]
+  /// and sending any error as an [OutputEvent].
+  Future<vm.InstanceRef?> _evaluateAndPrintErrors(
+    ThreadInfo thread,
+    String expression,
+    String type,
+  ) async {
+    try {
+      final result = await _adapter.vmService?.evaluateInFrame(
+        thread.isolate.id!,
+        0,
+        expression,
+        disableBreakpoints: true,
+      );
+
+      if (result is vm.InstanceRef) {
+        return result;
+      } else if (result is vm.ErrorRef) {
+        final message = result.message ?? '<error ref>';
+        _adapter.sendOutput(
+          'console',
+          'Debugger failed to evaluate breakpoint $type "$expression": $message',
+        );
+      } else if (result is vm.Sentinel) {
+        final message = result.valueAsString ?? '<collected>';
+        _adapter.sendOutput(
+          'console',
+          'Debugger failed to evaluate breakpoint $type "$expression": $message',
+        );
+      }
+    } catch (e) {
+      _adapter.sendOutput(
+        'console',
+        'Debugger failed to evaluate breakpoint $type "$expression": $e',
+      );
+    }
+  }
+
   void _handleExit(vm.Event event) {
     final isolate = event.isolate!;
     final isolateId = isolate.id!;
@@ -340,6 +403,20 @@ class IsolateManager {
       if (eventKind == vm.EventKind.kPauseBreakpoint &&
           (event.pauseBreakpoints?.isNotEmpty ?? false)) {
         reason = 'breakpoint';
+        // Look up the client breakpoints that correspond to the VM breakpoint(s)
+        // we hit. It's possible some of these may be missing because we could
+        // hit a breakpoint that was set before we were attached.
+        final breakpoints = event.pauseBreakpoints!
+            .map((bp) => _clientBreakpointsByVmId[bp.id!])
+            .toSet();
+
+        // Resume if there are no (non-logpoint) breakpoints, of any of the
+        // breakpoints don't have false conditions.
+        if (breakpoints.isEmpty ||
+            !await _shouldHitBreakpoint(thread, breakpoints)) {
+          await resumeThread(thread.threadId);
+          return;
+        }
       } else if (eventKind == vm.EventKind.kPauseBreakpoint) {
         reason = 'step';
       } else if (eventKind == vm.EventKind.kPauseException) {
@@ -406,6 +483,7 @@ class IsolateManager {
             isolateId, uri, bp.line,
             column: bp.column);
         existingBreakpointsForIsolateAndUri.add(vmBp);
+        _clientBreakpointsByVmId[vmBp.id!] = bp;
       });
     }
   }
@@ -446,6 +524,33 @@ class IsolateManager {
       await service.setLibraryDebuggable(isolateId, library.id!, isDebuggable);
     }));
   }
+
+  /// Checks whether a breakpoint the VM paused at is one we should actually
+  /// remain at. That is, it either has no condition, or its condition evaluates
+  /// to something truthy.
+  Future<bool> _shouldHitBreakpoint(
+    ThreadInfo thread,
+    Set<SourceBreakpoint?> breakpoints,
+  ) async {
+    // If any were missing (they're null) or do not have a condition, we should
+    // hit the breakpoint.
+    final clientBreakpointsWithConditions =
+        breakpoints.where((bp) => bp?.condition?.isNotEmpty ?? false).toList();
+    if (breakpoints.length != clientBreakpointsWithConditions.length) {
+      return true;
+    }
+
+    // Otherwise, we need to evaluate all of the conditions and see if any are
+    // true, in which case we will also hit.
+    final conditions =
+        clientBreakpointsWithConditions.map((bp) => bp!.condition!).toSet();
+
+    final results = await Future.wait(conditions.map(
+      (condition) => _breakpointConditionEvaluatesTrue(thread, condition),
+    ));
+
+    return results.any((result) => result);
+  }
 }
 
 /// Holds state for a single Isolate/Thread.

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

@@ -331,6 +331,84 @@ void main(List<String> args) async {
         client.stepIn(stop.threadId!),
       ], eagerError: true);
     });
+  }, timeout: Timeout.none);
+
+  group('debug mode conditional breakpoints', () {
+    test('stops with condition evaluating to true', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleBreakpointProgram);
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        condition: '1 == 1',
+      );
+    });
+
+    test('does not stop with condition evaluating to false', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleBreakpointProgram);
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      await client.doNotHitBreakpoint(
+        testFile,
+        breakpointLine,
+        condition: '1 == 2',
+      );
+    });
+
+    test('stops with condition evaluating to non-zero', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleBreakpointProgram);
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        condition: '1 + 1',
+      );
+    });
+
+    test('does not stop with condition evaluating to zero', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleBreakpointProgram);
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      await client.doNotHitBreakpoint(
+        testFile,
+        breakpointLine,
+        condition: '1 - 1',
+      );
+    });
+
+    test('reports evaluation errors for conditions', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(simpleBreakpointProgram);
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      final outputEventsFuture = client.outputEvents.toList();
+
+      await client.doNotHitBreakpoint(
+        testFile,
+        breakpointLine,
+        condition: "1 + 'a'",
+      );
+
+      final outputEvents = await outputEventsFuture;
+      final outputMessages = outputEvents.map((e) => e.output);
+
+      final hasPrefix = startsWith(
+          'Debugger failed to evaluate breakpoint condition "1 + \'a\'": '
+          'evaluateInFrame: (113) Expression compilation error');
+      final hasDescriptiveMessage = contains(
+          "A value of type 'String' can't be assigned to a variable of type 'num'");
+
+      expect(
+        outputMessages,
+        containsAll([allOf(hasPrefix, hasDescriptiveMessage)]),
+      );
+    });
     // These tests can be slow due to starting up the external server process.
   }, timeout: Timeout.none);
 }

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

@@ -5,7 +5,6 @@
 import 'dart:io';
 
 import 'package:dds/src/dap/protocol_generated.dart';
-import 'package:pedantic/pedantic.dart';
 import 'package:test/test.dart';
 
 import 'test_client.dart';
@@ -61,7 +60,6 @@ void main(List<String> args) async {
       // request and capture the args.
       RunInTerminalRequestArguments? runInTerminalArgs;
       Process? proc;
-      var processExited = false;
       dap.client.handleRequest(
         'runInTerminal',
         (args) async {
@@ -78,7 +76,6 @@ void main(List<String> args) async {
             runArgs.args.skip(1).toList(),
             workingDirectory: runArgs.cwd,
           );
-          unawaited(proc!.exitCode.then((_) => processExited = true));
 
           return RunInTerminalResponseBody(processId: proc!.pid);
         },
@@ -98,7 +95,7 @@ void main(List<String> args) async {
         containsAllInOrder([Platform.resolvedExecutable, testFile.path]),
       );
       expect(proc!.pid, isPositive);
-      expect(processExited, isTrue);
+      expect(proc!.exitCode, completes);
     });
 
     test('provides a list of threads', () async {

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

@@ -391,6 +391,7 @@ extension DapTestClientExtension on DapTestClient {
   Future<StoppedEventBody> hitBreakpoint(
     File file,
     int line, {
+    String? condition,
     Future<Response> Function()? launch,
   }) async {
     final stop = expectStop('breakpoint', file: file, line: line);
@@ -400,7 +401,7 @@ extension DapTestClientExtension on DapTestClient {
       sendRequest(
         SetBreakpointsArguments(
           source: Source(path: file.path),
-          breakpoints: [SourceBreakpoint(line: line)],
+          breakpoints: [SourceBreakpoint(line: line, condition: condition)],
         ),
       ),
       launch?.call() ?? this.launch(file.path),
@@ -434,6 +435,30 @@ extension DapTestClientExtension on DapTestClient {
     return stop;
   }
 
+  /// Sets a breakpoint at [line] in [file] and expects _not_ to hit it after
+  /// running the script (instead the script is expected to terminate).
+  ///
+  /// Launch options can be customised by passing a custom [launch] function that
+  /// will be used instead of calling `launch(file.path)`.
+  Future<void> doNotHitBreakpoint(
+    File file,
+    int line, {
+    String? condition,
+    Future<Response> Function()? launch,
+  }) async {
+    await Future.wait([
+      event('terminated'),
+      initialize(),
+      sendRequest(
+        SetBreakpointsArguments(
+          source: Source(path: file.path),
+          breakpoints: [SourceBreakpoint(line: line, condition: condition)],
+        ),
+      ),
+      launch?.call() ?? this.launch(file.path),
+    ], eagerError: true);
+  }
+
   /// Returns whether DDS is available for the VM Service the debug adapter
   /// is connected to.
   Future<bool> get ddsAvailable async {