[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 https://github.com/Dart-Code/Dart-Code/issues/4234
Change-Id: I56c2a7c8f85aa8c4cc85cfb3120a8cfec6b54c70
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/310164
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>
diff --git a/pkg/dds/CHANGELOG.md b/pkg/dds/CHANGELOG.md
index 65125f17..ca407ee 100644
--- a/pkg/dds/CHANGELOG.md
+++ b/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.
diff --git a/pkg/dds/lib/src/dap/adapters/dart.dart b/pkg/dds/lib/src/dap/adapters/dart.dart
index 500a798..1c22fa9 100644
--- a/pkg/dds/lib/src/dap/adapters/dart.dart
+++ b/pkg/dds/lib/src/dap/adapters/dart.dart
@@ -114,6 +114,7 @@
List<String>? additionalProjectPaths,
bool? debugSdkLibraries,
bool? debugExternalPackageLibraries,
+ bool? showGettersInDebugViews,
bool? evaluateGettersInDebugViews,
bool? evaluateToStringInDebugViews,
bool? sendLogsToClient,
@@ -127,6 +128,7 @@
additionalProjectPaths: additionalProjectPaths,
debugSdkLibraries: debugSdkLibraries,
debugExternalPackageLibraries: debugExternalPackageLibraries,
+ showGettersInDebugViews: showGettersInDebugViews,
evaluateGettersInDebugViews: evaluateGettersInDebugViews,
evaluateToStringInDebugViews: evaluateToStringInDebugViews,
sendLogsToClient: sendLogsToClient,
@@ -194,13 +196,19 @@
/// 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 @@
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 @@
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 @@
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 @@
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 @@
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 @@
List<String>? additionalProjectPaths,
bool? debugSdkLibraries,
bool? debugExternalPackageLibraries,
+ bool? showGettersInDebugViews,
bool? evaluateGettersInDebugViews,
bool? evaluateToStringInDebugViews,
bool? sendLogsToClient,
@@ -2570,6 +2601,7 @@
additionalProjectPaths: additionalProjectPaths,
debugSdkLibraries: debugSdkLibraries,
debugExternalPackageLibraries: debugExternalPackageLibraries,
+ showGettersInDebugViews: showGettersInDebugViews,
evaluateGettersInDebugViews: evaluateGettersInDebugViews,
evaluateToStringInDebugViews: evaluateToStringInDebugViews,
sendLogsToClient: sendLogsToClient,
diff --git a/pkg/dds/lib/src/dap/protocol_converter.dart b/pkg/dds/lib/src/dap/protocol_converter.dart
index 356eb9c..a43414f 100644
--- a/pkg/dds/lib/src/dap/protocol_converter.dart
+++ b/pkg/dds/lib/src/dap/protocol_converter.dart
@@ -255,39 +255,36 @@
},
));
- // 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 @@
}
}
- 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 @@
}
}
+ /// 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) {
diff --git a/pkg/dds/lib/src/dap/variables.dart b/pkg/dds/lib/src/dap/variables.dart
index 7faa39c..6394637 100644
--- a/pkg/dds/lib/src/dap/variables.dart
+++ b/pkg/dds/lib/src/dap/variables.dart
@@ -15,6 +15,21 @@
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.
diff --git a/pkg/dds/pubspec.yaml b/pkg/dds/pubspec.yaml
index 10bf503..4534ab5 100644
--- a/pkg/dds/pubspec.yaml
+++ b/pkg/dds/pubspec.yaml
@@ -14,7 +14,7 @@
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
diff --git a/pkg/dds/test/dap/integration/debug_variables_test.dart b/pkg/dds/test/dap/integration/debug_variables_test.dart
index 4579258..07097e0 100644
--- a/pkg/dds/test/dap/integration/debug_variables_test.dart
+++ b/pkg/dds/test/dap/integration/debug_variables_test.dart
@@ -154,7 +154,7 @@
);
});
- 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 @@
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 @@
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('''
diff --git a/pkg/dds/test/dap/integration/test_client.dart b/pkg/dds/test/dap/integration/test_client.dart
index 5edf2e1..18e613b 100644
--- a/pkg/dds/test/dap/integration/test_client.dart
+++ b/pkg/dds/test/dap/integration/test_client.dart
@@ -127,6 +127,7 @@
List<String>? additionalProjectPaths,
bool? debugSdkLibraries,
bool? debugExternalPackageLibraries,
+ bool? showGettersInDebugViews,
bool? evaluateGettersInDebugViews,
bool? evaluateToStringInDebugViews,
}) async {
@@ -150,6 +151,7 @@
additionalProjectPaths: additionalProjectPaths,
debugSdkLibraries: debugSdkLibraries,
debugExternalPackageLibraries: debugExternalPackageLibraries,
+ showGettersInDebugViews: showGettersInDebugViews,
evaluateGettersInDebugViews: evaluateGettersInDebugViews,
evaluateToStringInDebugViews: evaluateToStringInDebugViews,
// When running out of process, VM Service traffic won't be available
@@ -290,6 +292,7 @@
String? console,
bool? debugSdkLibraries,
bool? debugExternalPackageLibraries,
+ bool? showGettersInDebugViews,
bool? evaluateGettersInDebugViews,
bool? evaluateToStringInDebugViews,
bool? sendLogsToClient,
@@ -307,6 +310,7 @@
console: console,
debugSdkLibraries: debugSdkLibraries,
debugExternalPackageLibraries: debugExternalPackageLibraries,
+ showGettersInDebugViews: showGettersInDebugViews,
evaluateGettersInDebugViews: evaluateGettersInDebugViews,
evaluateToStringInDebugViews: evaluateToStringInDebugViews,
// When running out of process, VM Service traffic won't be available
@@ -1111,7 +1115,19 @@
buffer.write(', $type');
}
if (presentationHint != null) {
- buffer.write(', $presentationHint');
+ if (presentationHint.lazy != null) {
+ buffer.write(', lazy: ${presentationHint.lazy}');
+ }
+ if (presentationHint.kind != null) {
+ buffer.write(', kind: ${presentationHint.kind}');
+ }
+ if (presentationHint.visibility != null) {
+ buffer.write(', visibility: ${presentationHint.visibility}');
+ }
+ if (presentationHint.attributes != null) {
+ buffer.write(
+ ', attributes: ${presentationHint.attributes!.join(", ")}');
+ }
}
return buffer.toString();
diff --git a/pkg/dds/tool/dap/README.md b/pkg/dds/tool/dap/README.md
index 3251ef9..5e73a26 100644
--- a/pkg/dds/tool/dap/README.md
+++ b/pkg/dds/tool/dap/README.md
@@ -23,7 +23,8 @@
- `bool? debugExternalPackageLibraries` - whether to enable debugging for packages that are not inside the current workspace
- `bool? debugSdkLibraries` - whether to enable debugging for SDK libraries
-- `bool? evaluateGettersInDebugViews` - whether to evaluate getters in expression evaluation requests (inc. hovers/watch windows)
+- `bool? showGettersInDebugViews` - whether to include getters in Variables and Evaluation responses (inc. hovers/watch windows)
+- `bool? evaluateGettersInDebugViews` - whether to eagerly evaluate getters in Variables and Evaluation responses (inc. hovers/watch windows) without needing user action. Implies `showGettersInDebugViews=true`.
- `bool? evaluateToStringInDebugViews` - whether to invoke `toString()` in expression evaluation requests (inc. hovers/watch windows)
- `bool? sendLogsToClient` - used to proxy all VM Service traffic back to the client in custom `dart.log` events (has performance implications, intended for troubleshooting)
- `int? vmServicePort` - the port to bind the VM Service too
@@ -46,8 +47,8 @@
Arguments specific to `attachRequest` are:
-- `String vmServiceInfoFile` - the file to read the VM Service info from \*
-- `String vmServiceUri` - the VM Service URI to attach to \*
+- `String? vmServiceInfoFile` - the file to read the VM Service info from \*
+- `String? vmServiceUri` - the VM Service URI to attach to \*
\* Exactly one of `vmServiceInfoFile` or `vmServiceUri` should be supplied.
