[dds/dap] Add 'showGettersInDebugViews' which includes getters but lazily

The existing 'evaluateGettersInDebugViews' evaluates getters eagerly which can have side-effects. This adds a setting that allows showing getters in a way that can be shown lazily instead.

I added a new setting (instead of just making evaluateGettersInDebugViews=false be lazy) to preserve the ability to have getters not shown at all (since some users are using that today and might prefer this over lots of lazy getters showing up).

Fixes Dart-Code/Dart-Code#4234

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

Author: DanTup

pkg/dds/CHANGELOG.md

@@ -1,4 +1,5 @@
 # 2.9.1
+- [DAP] A new configuration option `bool? showGettersInDebugViews` allows getters to be shown wrapped in Variables/Evaluation responses so that they can be lazily expanded by the user. `evaluateGettersInDebugViews` must be `false` for this behaviour.
 - [DAP] `runInTerminal` requests are now sent after first responding to the `launchRequest`.
 - [DAP] Skipped tests are now marked with `!` instead of `✓` in `Output` events.
 - [DAP] Implemented `pause` request.

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

@@ -114,6 +114,7 @@ class DartAttachRequestArguments extends DartCommonLaunchAttachRequestArguments
     List<String>? additionalProjectPaths,
     bool? debugSdkLibraries,
     bool? debugExternalPackageLibraries,
+    bool? showGettersInDebugViews,
     bool? evaluateGettersInDebugViews,
     bool? evaluateToStringInDebugViews,
     bool? sendLogsToClient,
@@ -127,6 +128,7 @@ class DartAttachRequestArguments extends DartCommonLaunchAttachRequestArguments
           additionalProjectPaths: additionalProjectPaths,
           debugSdkLibraries: debugSdkLibraries,
           debugExternalPackageLibraries: debugExternalPackageLibraries,
+          showGettersInDebugViews: showGettersInDebugViews,
           evaluateGettersInDebugViews: evaluateGettersInDebugViews,
           evaluateToStringInDebugViews: evaluateToStringInDebugViews,
           sendLogsToClient: sendLogsToClient,
@@ -194,13 +196,19 @@ class DartCommonLaunchAttachRequestArguments extends RequestArguments {
   /// packages as block boxes.
   final bool? debugExternalPackageLibraries;
 
-  /// Whether to evaluate getters in debug views like hovers and the variables
+  /// Whether to show getters in debug views like hovers and the variables
   /// list.
+  final bool? showGettersInDebugViews;
+
+  /// Whether to eagerly evaluate getters in debug views like hovers and the
+  /// variables list.
   ///
-  /// Invoking getters has a performance cost and may introduce side-effects,
-  /// although users may expected this functionality. null is treated like false
-  /// although clients may have their own defaults (for example Dart-Code sends
-  /// true by default at the time of writing).
+  /// If `true`, getters will be invoked automatically and included inline with
+  /// other  fields (implies [showGettersInDebugViews]).
+  ///
+  /// If `false`, getters will not be included unless [showGettersInDebugViews]
+  /// is `true`, in which case they will be wrapped and only evaluated when the
+  /// user expands them.
   final bool? evaluateGettersInDebugViews;
 
   /// Whether to call toString() on objects in debug views like hovers and the
@@ -226,6 +234,9 @@ class DartCommonLaunchAttachRequestArguments extends RequestArguments {
     required this.additionalProjectPaths,
     required this.debugSdkLibraries,
     required this.debugExternalPackageLibraries,
+    // TODO(dantup): Make this 'required' after Flutter subclasses have been
+    //  updated.
+    this.showGettersInDebugViews,
     required this.evaluateGettersInDebugViews,
     required this.evaluateToStringInDebugViews,
     required this.sendLogsToClient,
@@ -242,6 +253,8 @@ class DartCommonLaunchAttachRequestArguments extends RequestArguments {
         debugSdkLibraries = arg.read<bool?>(obj, 'debugSdkLibraries'),
         debugExternalPackageLibraries =
             arg.read<bool?>(obj, 'debugExternalPackageLibraries'),
+        showGettersInDebugViews =
+            arg.read<bool?>(obj, 'showGettersInDebugViews'),
         evaluateGettersInDebugViews =
             arg.read<bool?>(obj, 'evaluateGettersInDebugViews'),
         evaluateToStringInDebugViews =
@@ -260,6 +273,8 @@ class DartCommonLaunchAttachRequestArguments extends RequestArguments {
         if (debugSdkLibraries != null) 'debugSdkLibraries': debugSdkLibraries,
         if (debugExternalPackageLibraries != null)
           'debugExternalPackageLibraries': debugExternalPackageLibraries,
+        if (showGettersInDebugViews != null)
+          'showGettersInDebugViews': showGettersInDebugViews,
         if (evaluateGettersInDebugViews != null)
           'evaluateGettersInDebugViews': evaluateGettersInDebugViews,
         if (evaluateToStringInDebugViews != null)
@@ -1745,6 +1760,7 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     VariablesArguments args,
     void Function(VariablesResponseBody) sendResponse,
   ) async {
+    final service = vmService;
     final childStart = args.start;
     final childCount = args.count;
     final storedData = isolateManager.getStoredData(args.variablesReference);
@@ -1883,6 +1899,20 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
           variablesReference: 0,
         ));
       }
+    } else if (data is VariableGetter && service != null) {
+      final variable = await _converter.createVariableForGetter(
+        service,
+        thread,
+        data.instance,
+        // Empty names for lazy variable values because they were already shown
+        // in the parent object.
+        variableName: '',
+        getterName: data.getterName,
+        evaluateName: data.parentEvaluateName,
+        allowCallingToString: data.allowCallingToString,
+        format: format,
+      );
+      variables.add(variable);
     }
 
     sendResponse(VariablesResponseBody(variables: variables));
@@ -2558,6 +2588,7 @@ class DartLaunchRequestArguments extends DartCommonLaunchAttachRequestArguments
     List<String>? additionalProjectPaths,
     bool? debugSdkLibraries,
     bool? debugExternalPackageLibraries,
+    bool? showGettersInDebugViews,
     bool? evaluateGettersInDebugViews,
     bool? evaluateToStringInDebugViews,
     bool? sendLogsToClient,
@@ -2570,6 +2601,7 @@ class DartLaunchRequestArguments extends DartCommonLaunchAttachRequestArguments
           additionalProjectPaths: additionalProjectPaths,
           debugSdkLibraries: debugSdkLibraries,
           debugExternalPackageLibraries: debugExternalPackageLibraries,
+          showGettersInDebugViews: showGettersInDebugViews,
           evaluateGettersInDebugViews: evaluateGettersInDebugViews,
           evaluateToStringInDebugViews: evaluateToStringInDebugViews,
           sendLogsToClient: sendLogsToClient,

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

@@ -255,39 +255,36 @@ class ProtocolConverter {
         },
       ));
 
-      // Also evaluate the getters if evaluateGettersInDebugViews=true enabled.
+      // 'evaluateGettersInDebugViews' implies 'showGettersInDebugViews' so
+      // either being `true` will cause getters to be included.
+      final includeGetters = (_adapter.args.showGettersInDebugViews ?? false) ||
+          (_adapter.args.evaluateGettersInDebugViews ?? false);
+      final lazyGetters = !(_adapter.args.evaluateGettersInDebugViews ?? false);
       final service = _adapter.vmService;
-      if (service != null &&
-          (_adapter.args.evaluateGettersInDebugViews ?? false)) {
-        // Collect getter names for this instances class and its supers.
-        final getterNames =
-            await _getterNamesForClassHierarchy(thread, instance.classRef);
-
-        /// Helper to evaluate each getter and convert the response to a
-        /// variable.
-        Future<dap.Variable> evaluate(int index, String getterName) async {
+      if (service != null && includeGetters) {
+        /// Helper to create a Variable for a getter.
+        Future<dap.Variable> createVariable(
+            int index, String getterName) async {
           try {
-            final response = await service.evaluate(
-              thread.isolate.id!,
-              instance.id!,
-              getterName,
-            );
-            final fieldEvaluateName =
-                _adapter.combineEvaluateName(evaluateName, '.$getterName');
-            if (response is vm.InstanceRef) {
-              _adapter.storeEvaluateName(response, fieldEvaluateName);
-            }
-            // Convert results to variables.
-            return convertVmResponseToVariable(
-              thread,
-              response,
-              name: getterName,
-              evaluateName:
-                  _adapter.combineEvaluateName(evaluateName, '.$getterName'),
-              allowCallingToString:
-                  allowCallingToString && index <= maxToStringsPerEvaluation,
-              format: format,
-            );
+            return lazyGetters
+                ? createVariableForLazyGetter(
+                    thread,
+                    instance,
+                    getterName,
+                    evaluateName,
+                    allowCallingToString,
+                    format,
+                  )
+                : await createVariableForGetter(
+                    service,
+                    thread,
+                    instance,
+                    getterName: getterName,
+                    evaluateName: evaluateName,
+                    allowCallingToString: allowCallingToString &&
+                        index <= maxToStringsPerEvaluation,
+                    format: format,
+                  );
           } catch (e) {
             return dap.Variable(
               name: getterName,
@@ -297,7 +294,12 @@ class ProtocolConverter {
           }
         }
 
-        variables.addAll(await Future.wait(getterNames.mapIndexed(evaluate)));
+        // Collect getter names for this instances class and its supers.
+        final getterNames =
+            await _getterNamesForClassHierarchy(thread, instance.classRef);
+
+        final getterVariables = getterNames.mapIndexed(createVariable);
+        variables.addAll(await Future.wait(getterVariables));
       }
 
       // Sort the fields/getters by name.
@@ -311,6 +313,74 @@ class ProtocolConverter {
     }
   }
 
+  /// Creates a Variable for a getter after eagerly fetching its value.
+  Future<Variable> createVariableForGetter(
+    vm.VmServiceInterface service,
+    ThreadInfo thread,
+    vm.Instance instance, {
+    String? variableName,
+    required String getterName,
+    required String? evaluateName,
+    required bool allowCallingToString,
+    required VariableFormat? format,
+  }) async {
+    final response = await service.evaluate(
+      thread.isolate.id!,
+      instance.id!,
+      getterName,
+    );
+    final fieldEvaluateName =
+        _adapter.combineEvaluateName(evaluateName, '.$getterName');
+    if (response is vm.InstanceRef) {
+      _adapter.storeEvaluateName(response, fieldEvaluateName);
+    }
+    // Convert results to variables.
+    return convertVmResponseToVariable(
+      thread,
+      response,
+      name: variableName ?? getterName,
+      evaluateName: _adapter.combineEvaluateName(evaluateName, '.$getterName'),
+      allowCallingToString: allowCallingToString,
+      format: format,
+    );
+  }
+
+  /// Creates a Variable for a getter that will be lazily evaluated.
+  ///
+  /// This stores any data required to call [createVariableForGetter] later.
+  ///
+  /// Lazy getters are implemented by inserting wrapper values and setting
+  /// their presentation hint to "lazy". This will instruct clients like VS Code
+  /// that they can show the value as "..." and fetch the child value into the
+  /// placeholder when clicked.
+  Variable createVariableForLazyGetter(
+    ThreadInfo thread,
+    vm.Instance instance,
+    String getterName,
+    String? evaluateName,
+    bool allowCallingToString,
+    VariableFormat? format,
+  ) {
+    final variablesReference = thread.storeData(
+      VariableData(
+        VariableGetter(
+          instance: instance,
+          getterName: getterName,
+          parentEvaluateName: evaluateName,
+          allowCallingToString: allowCallingToString,
+        ),
+        format,
+      ),
+    );
+
+    return dap.Variable(
+      name: getterName,
+      value: '',
+      variablesReference: variablesReference,
+      presentationHint: dap.VariablePresentationHint(lazy: true),
+    );
+  }
+
   /// Decodes the bytes of a list from the base64 encoded string
   /// [instance.bytes].
   List<Object?> _decodeList(vm.Instance instance) {

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

@@ -15,6 +15,21 @@ class VariableData {
   VariableData(this.data, this.format);
 }
 
+/// Data used to lazily evaluate a getter in a Variables request.
+class VariableGetter {
+  final Instance instance;
+  final String getterName;
+  final String? parentEvaluateName;
+  final bool allowCallingToString;
+
+  VariableGetter({
+    required this.instance,
+    required this.getterName,
+    required this.parentEvaluateName,
+    required this.allowCallingToString,
+  });
+}
+
 /// A wrapper around variables for use in `variablesRequest` that can hold
 /// additional data, such as a formatting information supplied in an evaluation
 /// request.

pkg/dds/pubspec.yaml

@@ -14,7 +14,7 @@ dependencies:
   browser_launcher: ^1.0.0
   collection: ^1.15.0
   dds_service_extensions: ^1.3.0
-  dap: ^1.0.0
+  dap: ^1.1.0
   devtools_shared: ^2.14.1
   http_multi_server: ^3.0.0
   json_rpc_2: ^3.0.0

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

@@ -154,7 +154,7 @@ void main(List<String> args) {
       );
     });
 
-    test('includes public getters when evaluateGettersInDebugViews=true',
+    test('includes eager public getters when evaluateGettersInDebugViews=true',
         () async {
       final client = dap.client;
       final testFile = dap.createTestFile('''
@@ -163,8 +163,8 @@ void main(List<String> args) {
   print('Hello!'); $breakpointMarker
 }
 class A {
-  String get publicString => '';
-  String get _privateString => '';
+  String get publicString => '111';
+  String get _privateString => '222';
 }
     ''');
       final breakpointLine = lineWith(testFile, breakpointMarker);
@@ -182,13 +182,62 @@ class A {
         expectedName: 'myVariable',
         expectedDisplayString: 'A',
         expectedVariables: '''
-            publicString: "", eval: myVariable.publicString
+            publicString: "111", eval: myVariable.publicString
             runtimeType: Type (A), eval: myVariable.runtimeType
         ''',
         ignorePrivate: false,
       );
     });
 
+    test('includes lazy public getters when showGettersInDebugViews=true',
+        () async {
+      final client = dap.client;
+      final testFile = dap.createTestFile('''
+void main(List<String> args) {
+  final myVariable = A();
+  print('Hello!'); $breakpointMarker
+}
+class A {
+  String get publicString => '111';
+  String get _privateString => '222';
+}
+    ''');
+      final breakpointLine = lineWith(testFile, breakpointMarker);
+
+      final stop = await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        launch: () => client.launch(
+          testFile.path,
+          showGettersInDebugViews: true,
+        ),
+      );
+
+      // Check the first level of variables are flagged as lazy with no values.
+      final variables = await client.expectLocalVariable(
+        stop.threadId!,
+        expectedName: 'myVariable',
+        expectedDisplayString: 'A',
+        expectedVariables: '''
+            publicString: , lazy: true
+            runtimeType: , lazy: true
+        ''',
+        ignorePrivate: false,
+      );
+
+      // "Expand" publicString to ensure it resolve correctly to a single-field
+      // variable with no name and the correct value/eval information.
+      final namedRecordVariable = variables.variables
+          .singleWhere((variable) => variable.name == 'publicString');
+      expect(namedRecordVariable.variablesReference, isPositive);
+      await client.expectVariables(
+        namedRecordVariable.variablesReference,
+        r'''
+            : "111", eval: myVariable.publicString
+        ''',
+      );
+    });
+
     test('includes record fields', () async {
       final client = dap.client;
       final testFile = dap.createTestFile('''