[dds/dap] Support translating VM Instance IDs -> DAP variablesReferences and back for DAP-over-DDS

This adds two custom requests to the DAP-over-DDS handler to translate between VM/DAP instance IDs:

- $/createVariableForInstance (String isolateId, String instanceId)
- $/getVariablesInstanceId (int variablesReference)

Because DAP's variables request only fetches _child_ variables (eg. fields) but we'd likely want to align the top-level string display of a variable, the wrapped variable will first be returned as a single variable named "value", which contains both the string display value and also a variablesReference to then get the child variables (usually invoked when expanded).

These methods currently live directly in the DDS DAP adapter since I figure they're only useful for clients using the VM Service, but we can move them to the base adapter if in future this turns out to not be the case.

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

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

@@ -346,8 +346,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
 
   /// Manages VM Isolates and their events, including fanning out any requests
   /// to set breakpoints etc. from the client to all Isolates.
-  @visibleForTesting
-  late IsolateManager isolateManager;
+  late final IsolateManager isolateManager;
 
   /// A helper that handlers converting to/from DAP and VM Service types.
   late ProtocolConverter _converter;
@@ -1897,6 +1896,30 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
         value: '<inspected variable>', // Shown to user, expandable.
         variablesReference: instance != null ? thread.storeData(instance) : 0,
       ));
+    } else if (data is WrappedInstanceVariable) {
+      // WrappedInstanceVariables are used to support DAP-over-DDS clients that
+      // had a VM Instance ID and wanted to convert it to a variable for use in
+      // `variables` requests.
+      final response = await isolateManager.getObject(
+        storedData.thread.isolate,
+        vm.ObjRef(id: data.instanceId),
+        offset: childStart,
+        count: childCount,
+      );
+      // Because `variables` requests are a request for _child_ variables but we
+      // want DAP-over-DDS clients to be able to get the whole variable (eg.
+      // including toe initial string representation of the variable itself) the
+      // initial request will return a list containing a single variable named
+      // `value`. This will contain both the `variablesReference` to get the
+      // children, and also a `value` field with the display string.
+      final variable = await _converter.convertVmResponseToVariable(
+        thread,
+        response,
+        name: 'value',
+        evaluateName: null,
+        allowCallingToString: evaluateToStringInDebugViews,
+      );
+      variables.add(variable);
     } else if (data is vm.MapAssociation) {
       final key = data.key;
       final value = data.value;

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

@@ -10,7 +10,9 @@ import 'package:async/async.dart';
 import 'package:dap/dap.dart';
 import 'package:vm_service/vm_service.dart' as vm;
 
+import '../constants.dart';
 import '../protocol_stream.dart';
+import '../variables.dart';
 import 'dart.dart';
 import 'mixins.dart';
 
@@ -98,6 +100,99 @@ class DdsHostedAdapter extends DartDebugAdapter<DartLaunchRequestArguments,
     unawaited(connectDebugger(ddsUri!));
   }
 
+  /// Handles custom requests that are specific to the DDS-hosted adapter, such
+  /// as translating between VM IDs and DAP IDs.
+  @override
+  Future<void> customRequest(
+    Request request,
+    RawRequestArguments? args,
+    void Function(Object?) sendResponse,
+  ) async {
+    switch (request.command) {
+      case Command.createVariableForInstance:
+        sendResponse(_createVariableForInstance(request.arguments));
+        break;
+
+      case Command.getVariablesInstanceId:
+        sendResponse(_getVariablesInstanceId(request.arguments));
+        break;
+
+      default:
+        await super.customRequest(request, args, sendResponse);
+    }
+  }
+
+  /// Creates a DAP variablesReference for a VM Instance ID.
+  Map<String, Object?> _createVariableForInstance(Object? arguments) {
+    if (arguments is! Map<String, Object?>) {
+      throw DebugAdapterException(
+        '${Command.createVariableForInstance} arguments must be Map<String, Object?>',
+      );
+    }
+    final isolateId = arguments[Parameters.isolateId];
+    final instanceId = arguments[Parameters.instanceId];
+    if (isolateId is! String) {
+      throw DebugAdapterException(
+        'createVariableForInstance requires a valid String ${Parameters.isolateId}',
+      );
+    }
+    if (instanceId is! String) {
+      throw DebugAdapterException(
+        'createVariableForInstance requires a value String ${Parameters.instanceId}',
+      );
+    }
+
+    final thread = isolateManager.threadForIsolateId(isolateId);
+    if (thread == null) {
+      throw DebugAdapterException('Isolate $isolateId is not valid');
+    }
+
+    // Create a new reference for this instance ID.
+    final variablesReference =
+        thread.storeData(WrappedInstanceVariable(instanceId));
+
+    return {
+      Parameters.variablesReference: variablesReference,
+    };
+  }
+
+  /// Tries to extract a VM Instance ID from a DAP variablesReference.
+  Map<String, Object?> _getVariablesInstanceId(Object? arguments) {
+    if (arguments is! Map<String, Object?>) {
+      throw DebugAdapterException(
+        '${Command.getVariablesInstanceId} arguments must be Map<String, Object?>',
+      );
+    }
+    final variablesReference = arguments[Parameters.variablesReference];
+    if (variablesReference is! int) {
+      throw DebugAdapterException(
+        '${Command.getVariablesInstanceId} requires a valid int ${Parameters.variablesReference}',
+      );
+    }
+
+    // Extract the stored data. This should generally always be a
+    // `WrappedInstanceVariable` (created by `_createVariableForInstance`) but
+    // for possible future compatibility, we'll also handle `VariableData` and
+    // other variables we can extract IDs for.
+    var data = isolateManager.getStoredData(variablesReference)?.data;
+
+    // Unwrap if it was wrapped for formatting.
+    if (data is VariableData) {
+      data = data.data;
+    }
+
+    // Extract the ID.
+    final instanceId = data is WrappedInstanceVariable
+        ? data.instanceId
+        : data is vm.ObjRef
+            ? data.id
+            : null;
+
+    return {
+      Parameters.instanceId: instanceId,
+    };
+  }
+
   /// Called by [terminateRequest] to request that we gracefully shut down the
   /// app being run (or in the case of an attach, disconnect).
   @override

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

@@ -6,8 +6,16 @@ class Command {
   static const initialize = 'initialize';
   static const configurationDone = 'configurationDone';
   static const attach = 'attach';
+  static const createVariableForInstance = r'$/createVariableForInstance';
+  static const getVariablesInstanceId = r'$/getVariablesInstanceId';
 }
 
 class ErrorMessageType {
   static const general = 1;
 }
+
+class Parameters {
+  static const isolateId = 'isolateId';
+  static const instanceId = 'instanceId';
+  static const variablesReference = 'variablesReference';
+}

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

@@ -430,7 +430,10 @@ class IsolateManager {
   }
 
   ThreadInfo? threadForIsolate(vm.IsolateRef? isolate) =>
-      isolate?.id != null ? _threadsByIsolateId[isolate!.id!] : null;
+      isolate?.id != null ? threadForIsolateId(isolate!.id!) : null;
+
+  ThreadInfo? threadForIsolateId(String isolateId) =>
+      _threadsByIsolateId[isolateId];
 
   /// Evaluates breakpoint condition [condition] and returns whether the result
   /// is true (or non-zero for a numeric), sending any evaluation error to the

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

@@ -53,6 +53,15 @@ class InspectData {
   InspectData(this.instance);
 }
 
+/// A wrapper around an Instance ID that will result in a variable with a single
+/// field `value` that can be used by DAP-over-DDS clients wanting to use
+/// variables requests for variable display.
+class WrappedInstanceVariable {
+  final String instanceId;
+
+  WrappedInstanceVariable(this.instanceId);
+}
+
 /// Formatting preferences for a variable.
 class VariableFormat {
   /// Whether to supress quotes around [String]s.

pkg/dds/lib/src/dap_handler.dart

@@ -16,12 +16,14 @@ import 'dds_impl.dart';
 class DapHandler {
   DapHandler(this.dds);
 
+  final _initializedCompleter = Completer<void>();
+
   Future<Map<String, dynamic>> sendRequest(
     DdsHostedAdapter adapter,
     json_rpc.Parameters parameters,
   ) async {
     if (adapter.ddsUri == null) {
-      _startAdapter(adapter);
+      await _startAdapter(adapter);
     }
 
     // TODO(helin24): Consider a sequence offset for incoming messages to avoid
@@ -44,6 +46,9 @@ class DapHandler {
   }
 
   _handleEvent(Event event) {
+    if (event.event == 'initialized') {
+      _initializedCompleter.complete();
+    }
     dds.streamManager.streamNotify(DapEventStreams.kDAP, {
       'streamId': DapEventStreams.kDAP,
       'event': {
@@ -61,8 +66,18 @@ class DapHandler {
     // TODO(helin24): Most likely we'll want the client to do these
     // initialization steps so that clients can differentiate capabilities. This
     // may require a custom stream for the debug adapter.
-    int seq = 1;
-    // TODO(helin24): Add waiting for `InitializedEvent`.
+
+    // Each DAP request has a `seq` number (essentially a message ID) which
+    // should be unique.
+    //
+    // We send a few requsets to initialize the adapter, but these are not
+    // visible to the DDS client so if we start at 1, the IDs will be
+    // reused.
+    //
+    // To avoid that, for our own initialization requests, use negative numbers
+    // (though they must still ascend) so there's no overlay with the messages
+    // we'll forward from the DDS client.
+    int seq = -1000;
     await adapter.initializeRequest(
       Request(
         command: Command.initialize,
@@ -73,6 +88,7 @@ class DapHandler {
       ),
       (capabilities) {},
     );
+    await _initializedCompleter.future;
     await adapter.configurationDoneRequest(
       Request(
         arguments: const {},
@@ -93,6 +109,10 @@ class DapHandler {
       ),
       noopCallback,
     );
+    // Wait for the debugger to fully initialize, because the request that
+    // triggered this initialization may require things like isolates that will
+    // only be known after the debugger has initialized.
+    await adapter.debuggerInitialized;
   }
 
   final DartDevelopmentServiceImpl dds;

pkg/dds/test/dap_handler_test.dart

@@ -7,59 +7,150 @@ import 'dart:io';
 
 import 'package:dap/dap.dart';
 import 'package:dds/dds.dart';
+import 'package:dds/src/dap/constants.dart';
 import 'package:dds_service_extensions/dap.dart';
 import 'package:test/test.dart';
+import 'package:vm_service/vm_service.dart';
 import 'package:vm_service/vm_service_io.dart';
 
 import 'common/test_helper.dart';
 
 void main() {
-  late Process process;
+  Process? process;
   DartDevelopmentService? dds;
+  late VmService service;
 
-  setUp(() async {
+  Future<void> createProcess({bool pauseOnStart = true}) async {
     process = await spawnDartProcess(
-      'get_cached_cpu_samples_script.dart',
+      'dap_over_dds_script.dart',
       disableServiceAuthCodes: true,
+      pauseOnStart: pauseOnStart,
     );
-  });
+  }
 
   tearDown(() async {
     await dds?.shutdown();
-    process.kill();
+    process?.kill();
+    process = null;
   });
 
-  test('DDS responds to DAP message', () async {
-    Uri serviceUri = remoteVmServiceUri;
-    dds = await DartDevelopmentService.startDartDevelopmentService(
-      remoteVmServiceUri,
-    );
-    serviceUri = dds!.wsUri!;
-    expect(dds!.isRunning, true);
-    final service = await vmServiceConnectUri(serviceUri.toString());
-
-    final setBreakpointsRequest = Request(
-      command: 'setBreakpoints',
-      seq: 9,
-      arguments: SetBreakpointsArguments(
-        breakpoints: [
-          SourceBreakpoint(line: 20),
-          SourceBreakpoint(line: 30),
-        ],
-        source: Source(
-          name: 'main.dart',
-          path: '/file/to/main.dart',
+  var nextSeq = 1;
+  Future<DapResponse> sendDapRequest(String request, Object? arguments) async {
+    final result = await service.sendDapRequest(
+      jsonEncode(
+        Request(
+          command: request,
+          seq: nextSeq++,
+          arguments: arguments,
         ),
       ),
     );
 
-    // TODO(helinx): Check result format after using better typing from JSON.
-    final result =
-        await service.sendDapRequest(jsonEncode(setBreakpointsRequest));
     expect(result.dapResponse, isNotNull);
     expect(result.dapResponse.type, 'response');
+    expect(result.dapResponse.message, isNull);
     expect(result.dapResponse.success, true);
-    expect(result.dapResponse.command, 'setBreakpoints');
-    expect(result.dapResponse.body, isNotNull);
+    expect(result.dapResponse.command, request);
+
+    return result;
+  }
+
+  Future<String?> instanceToString(String isolateId, String instanceId) async {
+    final result = await service.invoke(isolateId, instanceId, 'toString', []);
+    return result is InstanceRef ? result.valueAsString : null;
+  }
+
+  Future<String> variableToString(int variablesReference) async {
+    // Because variables requests are for _child_ variables, a converted
+    // Instance->Variable will return an initial container that has a single
+    // 'value' field, which contains the string representation of the variable
+    // as its value, and a further 'variablesReference' that can be used to
+    // fetch child variables.
+    final variablesResult = await sendDapRequest(
+      'variables',
+      VariablesArguments(variablesReference: variablesReference),
+    );
+    final variablesBody = VariablesResponseBody.fromMap(
+      variablesResult.dapResponse.body as Map<String, Object?>,
+    );
+    expect(variablesBody.variables, hasLength(1));
+    final variable = variablesBody.variables.single;
+    expect(variable.name, 'value');
+    expect(variable.variablesReference, isPositive);
+    return variable.value;
+  }
+
+  test('DDS responds to DAP message', () async {
+    await createProcess();
+    dds = await DartDevelopmentService.startDartDevelopmentService(
+      remoteVmServiceUri,
+    );
+    expect(dds!.isRunning, true);
+    final serviceUri = dds!.wsUri!;
+    service = await vmServiceConnectUri(serviceUri.toString());
+
+    final breakpointArguments = SetBreakpointsArguments(
+      breakpoints: [
+        SourceBreakpoint(line: 20),
+        SourceBreakpoint(line: 30),
+      ],
+      source: Source(
+        name: 'main.dart',
+        path: '/file/to/main.dart',
+      ),
+    );
+
+    final result = await sendDapRequest('setBreakpoints', breakpointArguments);
+    final response = SetBreakpointsResponseBody.fromMap(
+        result.dapResponse.body as Map<String, Object?>);
+    expect(response.breakpoints, hasLength(2));
+    expect(response.breakpoints[0].verified, isFalse);
+    expect(response.breakpoints[1].verified, isFalse);
+  });
+
+  test('DAP can map between variableReferences and InstanceRefs', () async {
+    await createProcess(pauseOnStart: false);
+    dds = await DartDevelopmentService.startDartDevelopmentService(
+      remoteVmServiceUri,
+    );
+    service = await vmServiceConnectUri(dds!.wsUri!.toString());
+
+    final isolate = (await service.getVM()).isolates!.first;
+    final isolateId = isolate.id!;
+
+    // Get the variable for 'myInstance'.
+    final originalInstanceRef = (await service.evaluateInFrame(
+        isolateId, 0, 'myInstance')) as InstanceRef;
+    final originalInstanceId = originalInstanceRef.id!;
+
+    // Ask DAP to make a variableReference for it.
+    final createVariableResult = await sendDapRequest(
+      Command.createVariableForInstance,
+      {
+        Parameters.isolateId: isolateId,
+        Parameters.instanceId: originalInstanceId,
+      },
+    );
+    final createVariablesBody =
+        createVariableResult.dapResponse.body as Map<String, Object?>;
+    final variablesReference =
+        createVariablesBody[Parameters.variablesReference] as int;
+
+    // And now ask DAP to convert it back to an instance ID.
+    final getInstanceResult = await sendDapRequest(
+      Command.getVariablesInstanceId,
+      {
+        Parameters.variablesReference: variablesReference,
+      },
+    );
+    final getInstanceRefBody =
+        getInstanceResult.dapResponse.body as Map<String, Object?>;
+    final mappedInstanceId =
+        getInstanceRefBody[Parameters.instanceId] as String;
+
+    // Now verify that the string value of these are all the same.
+    expect(await instanceToString(isolateId, originalInstanceId), 'MyClass');
+    expect(await variableToString(variablesReference), 'MyClass');
+    expect(await instanceToString(isolateId, mappedInstanceId), 'MyClass');
   });
 }

pkg/dds/test/dap_over_dds_script.dart

@@ -0,0 +1,22 @@
+// Copyright (c) 2023, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// This script is used by tests in `test/dap_handler_test.dart`.
+
+import 'dart:developer';
+
+void main() async {
+  final myInstance = MyClass('myFieldValue');
+  debugger();
+  print(myInstance);
+}
+
+class MyClass {
+  final String myField;
+
+  MyClass(this.myField);
+
+  @override
+  String toString() => 'MyClass';
+}