[dds] Support changing DAP debug settings after the session has started

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

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

@@ -318,6 +318,15 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
         sendResponse(protocols?.toJson());
         break;
 
+      /// Used to toggle debug settings such as whether SDK/Packages are
+      /// debuggable while the session is in progress.
+      case 'updateDebugOptions':
+        if (args != null) {
+          await _updateDebugOptions(args.args);
+        }
+        sendResponse(null);
+        break;
+
       default:
         await super.customRequest(request, args, sendResponse);
     }
@@ -1009,7 +1018,29 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
     // Notify IsolateManager if we'll be debugging so it knows whether to set
     // up breakpoints etc. when isolates are registered.
     final debug = !(args.noDebug ?? false);
-    _isolateManager.setDebugEnabled(debug);
+    _isolateManager.debug = debug;
+    _isolateManager.debugSdkLibraries = args.debugSdkLibraries ?? true;
+    _isolateManager.debugExternalPackageLibraries =
+        args.debugExternalPackageLibraries ?? true;
+  }
+
+  /// Updates the current debug options for the session.
+  ///
+  /// Clients may not know about all debug options, so anything not included
+  /// in the map will not be updated by this method.
+  Future<void> _updateDebugOptions(Map<String, Object?> args) async {
+    // TODO(dantup): Document this - it's a public API we expect to be used
+    //   by editors that can support it (although it will require custom
+    //   code as it's there's no DAP standard for this, or the settings it
+    //   toggles).
+    if (args.containsKey('debugSdkLibraries')) {
+      _isolateManager.debugSdkLibraries = args['debugSdkLibraries'] as bool;
+    }
+    if (args.containsKey('debugExternalPackageLibraries')) {
+      _isolateManager.debugExternalPackageLibraries =
+          args['debugExternalPackageLibraries'] as bool;
+    }
+    await _isolateManager.applyDebugOptions();
   }
 
   /// A wrapper around the same name function from package:vm_service that

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

@@ -65,14 +65,7 @@ abstract class BaseDebugAdapter<TLaunchArgs extends LaunchRequestArguments> {
     RawRequestArguments? args,
     void Function(Object?) sendResponse,
   ) async {
-    final response = Response(
-      success: false,
-      requestSeq: request.seq,
-      seq: _sequence++,
-      command: request.command,
-      message: 'Unknown command: ${request.command}',
-    );
-    sendResponse(response);
+    throw DebugAdapterException('Unknown command  ${request.command}');
   }
 
   Future<void> disconnectRequest(
@@ -251,7 +244,7 @@ abstract class BaseDebugAdapter<TLaunchArgs extends LaunchRequestArguments> {
     } else if (message is Response) {
       _handleIncomingResponse(message);
     } else {
-      throw Exception('Unknown Protocol message ${message.type}');
+      throw DebugAdapterException('Unknown Protocol message ${message.type}');
     }
   }
 

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

@@ -23,15 +23,33 @@ class IsolateManager {
   final Map<int, ThreadInfo> _threadsByThreadId = {};
   int _nextThreadNumber = 1;
 
-  /// Whether debugging is enabled.
+  /// Whether debugging is enabled for this session.
   ///
   /// This must be set before any isolates are spawned and controls whether
   /// breakpoints or exception pause modes are sent to the VM.
   ///
+  /// If false, requests to send breakpoints or exception pause mode will be
+  /// dropped. Other functionality (handling pause events, resuming, etc.) will
+  /// all still function.
+  ///
   /// This is used to support debug sessions that have VM Service connections
   /// but were run with noDebug: true (for example we may need a VM Service
   /// connection for a noDebug flutter app in order to support hot reload).
-  bool _debug = false;
+  bool debug = false;
+
+  /// Whether SDK libraries should be marked as debuggable.
+  ///
+  /// Calling [sendLibraryDebuggables] is required after changing this value to
+  /// apply changes. This allows applying both [debugSdkLibraries] and
+  /// [debugExternalPackageLibraries] in one step.
+  bool debugSdkLibraries = true;
+
+  /// Whether external package libraries should be marked as debuggable.
+  ///
+  /// Calling [sendLibraryDebuggables] is required after changing this value to
+  /// apply changes. This allows applying both [debugSdkLibraries] and
+  /// [debugExternalPackageLibraries] in one step.
+  bool debugExternalPackageLibraries = true;
 
   /// Tracks breakpoints last provided by the client so they can be sent to new
   /// isolates that appear after initial breakpoints were sent.
@@ -72,6 +90,18 @@ class IsolateManager {
   /// not exited between accessing this list and trying to use the results.
   List<ThreadInfo> get threads => _threadsByIsolateId.values.toList();
 
+  /// Re-applies debug options to all isolates/libraries.
+  ///
+  /// This is required if options like debugSdkLibraries are modified, but is a
+  /// separate step to batch together changes to multiple options.
+  Future<void> applyDebugOptions() async {
+    await Future.wait(_threadsByThreadId.values.map(
+      // debuggable libraries is the only thing currently affected by these
+      // changable options.
+      (isolate) => _sendLibraryDebuggables(isolate.isolate),
+    ));
+  }
+
   Future<T> getObject<T extends vm.Response>(
       vm.IsolateRef isolate, vm.ObjRef object) async {
     final res = await _adapter.vmService?.getObject(isolate.id!, object.id!);
@@ -219,19 +249,6 @@ class IsolateManager {
         .map((isolate) => _sendBreakpoints(isolate.isolate, uri: uri)));
   }
 
-  /// Sets whether debugging is enabled for this session.
-  ///
-  /// If not, requests to send breakpoints or exception pause mode will be
-  /// dropped. Other functionality (handling pause events, resuming, etc.) will
-  /// all still function.
-  ///
-  /// This is used to support debug sessions that have VM Service connections
-  /// but were run with noDebug: true (for example we may need a VM Service
-  /// connection for a noDebug flutter app in order to support hot reload).
-  void setDebugEnabled(bool debug) {
-    _debug = debug;
-  }
-
   /// Records exception pause mode as one of 'None', 'Unhandled' or 'All'. All
   /// existing isolates will be updated to reflect the new setting.
   Future<void> setExceptionPauseMode(String mode) async {
@@ -371,13 +388,13 @@ class IsolateManager {
 
   /// Checks whether a library should be considered debuggable.
   ///
-  /// This usesthe settings from the launch arguments (debugSdkLibraries
-  /// and debugExternalPackageLibraries) against the type of library given.
+  /// Initial values are provided in the launch arguments, but may be updated
+  /// by the `updateDebugOptions` custom request.
   bool _libaryIsDebuggable(vm.LibraryRef library) {
     if (_isSdkLibrary(library)) {
-      return _adapter.args.debugSdkLibraries ?? false;
+      return debugSdkLibraries;
     } else if (_isExternalPackageLibrary(library)) {
-      return _adapter.args.debugExternalPackageLibraries ?? false;
+      return debugExternalPackageLibraries;
     } else {
       return true;
     }
@@ -391,7 +408,7 @@ class IsolateManager {
   /// newly-created isolates).
   Future<void> _sendBreakpoints(vm.IsolateRef isolate, {String? uri}) async {
     final service = _adapter.vmService;
-    if (!_debug || service == null) {
+    if (!debug || service == null) {
       return;
     }
 
@@ -425,7 +442,7 @@ class IsolateManager {
   /// Sets the exception pause mode for an individual isolate.
   Future<void> _sendExceptionPauseMode(vm.IsolateRef isolate) async {
     final service = _adapter.vmService;
-    if (!_debug || service == null) {
+    if (!debug || service == null) {
       return;
     }
 
@@ -436,7 +453,7 @@ class IsolateManager {
   /// on the debug settings.
   Future<void> _sendLibraryDebuggables(vm.IsolateRef isolateRef) async {
     final service = _adapter.vmService;
-    if (!_debug || service == null) {
+    if (!debug || service == null) {
       return;
     }
 

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

@@ -155,7 +155,14 @@ void main(List<String> args) async {
       final stepLine = lineWith(testFile, '// STEP');
 
       // Hit the initial breakpoint.
-      final stop = await client.hitBreakpoint(testFile, breakpointLine);
+      final stop = await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        launch: () => client.launch(
+          testFile.path,
+          debugSdkLibraries: false,
+        ),
+      );
 
       // Step in and expect stopping on the next line (don't go into print).
       await Future.wait([
@@ -203,12 +210,39 @@ void main(List<String> args) async {
       // TODO(dantup): Support for debugExternalPackageLibraries
     }, skip: true);
 
-    test('allows changing debug settings during session', () {
-      // TODO(dantup): !
-      // Dart-Code's DAP has a custom method that allows an editor to change
-      // the debug settings (debugSdkLibraries/debugExternalPackageLibraries)
-      // during a debug session.
-    }, skip: true);
+    test('allows changing debug settings during session', () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile(r'''
+void main(List<String> args) async {
+  print('Hello!'); // BREAKPOINT
+  print('Hello!'); // STEP
+}
+    ''');
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+      final stepLine = lineWith(testFile, '// STEP');
+
+      // Start with debugSdkLibraryes _enabled_ and hit the breakpoint.
+      final stop = await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        launch: () => client.launch(
+          testFile.path,
+          debugSdkLibraries: true,
+        ),
+      );
+
+      // Turn off debugSdkLibraries.
+      await client.custom('updateDebugOptions', {
+        'debugSdkLibraries': false,
+      });
+
+      // Step in and expect stopping on the next line (don't go into print
+      // because we turned off SDK debugging).
+      await Future.wait([
+        client.expectStop('step', file: testFile, line: stepLine),
+        client.stepIn(stop.threadId!),
+      ], eagerError: true);
+    });
     // These tests can be slow due to starting up the external server process.
   }, timeout: Timeout.none);
 }