Cleanup getObject logic for handling offsets and counts (#1936)

* Validate only needed summaries in expression_compiler_service

* Cleanup getObject code with offsets and counts

* Update comments

* Addressed CR comments

* Addressed CR comments

* Fix incorrect range count

* Added missing test cases

Author: Anna Gringauze

dwds/CHANGELOG.md

@@ -1,3 +1,8 @@
+## 17.0.1-dev
+
+- Cleanup `getObject` code for lists and maps.
+  - Now works with offset `0` and `null` count.
+
 ## 17.0.0
 
 - Include debug information in the event sent from the injected client to the

dwds/lib/src/debugging/debugger.dart

@@ -402,22 +402,41 @@ class Debugger extends Domain {
     return null;
   }
 
+  static bool _isSubRange({int? offset, int? count, int? length}) {
+    if (length == null) return false;
+    if (offset == 0 && count == null) return false;
+    return offset != null || count != null;
+  }
+
+  /// Compute the last possible element index in the range of [offset]..end
+  /// that includes [count] elements, if available.
+  static int? _calculateRangeEnd(
+          {int? count, required int offset, required int length}) =>
+      count == null ? null : math.min(offset + count, length);
+
+  /// Calculate the number of available elements in the range.
+  static int _calculateRangeCount(
+          {int? count, required int offset, required int length}) =>
+      count == null ? length - offset : math.min(count, length - offset);
+
   /// Find a sub-range of the entries for a Map/List when offset and/or count
   /// have been specified on a getObject request.
   ///
   /// If the object referenced by [id] is not a system List or Map then this
   /// will just return a RemoteObject for it and ignore [offset], [count] and
   /// [length]. If it is, then [length] should be the number of entries in the
   /// List/Map and [offset] and [count] should indicate the desired range.
-  Future<RemoteObject> _subrange(
-      String id, int offset, int? count, int length) async {
+  Future<RemoteObject> _subRange(String id,
+      {required int offset, int? count, required int length}) async {
     // TODO(#809): Sometimes we already know the type of the object, and
     // we could take advantage of that to short-circuit.
     final receiver = remoteObjectFor(id);
-    final end = count == null ? null : math.min(offset + count, length);
-    final actualCount = count ?? length - offset;
+    final end =
+        _calculateRangeEnd(count: count, offset: offset, length: length);
+    final rangeCount =
+        _calculateRangeCount(count: count, offset: offset, length: length);
     final args =
-        [offset, actualCount, end].map(dartIdFor).map(remoteObjectFor).toList();
+        [offset, rangeCount, end].map(dartIdFor).map(remoteObjectFor).toList();
     // If this is a List, just call sublist. If it's a Map, get the entries, but
     // avoid doing a toList on a large map using skip/take to get the section we
     // want. To make those alternatives easier in JS, pass both count and end.
@@ -468,11 +487,16 @@ class Debugger extends Domain {
   /// Symbol(DartClass.actualName) and will need to be converted. For a system
   /// List or Map, [offset] and/or [count] can be provided to indicate a desired
   /// range of entries. They will be ignored if there is no [length].
-  Future<List<Property>> getProperties(String objectId,
-      {int? offset, int? count, int? length}) async {
+  Future<List<Property>> getProperties(
+    String objectId, {
+    int? offset,
+    int? count,
+    int? length,
+  }) async {
     String rangeId = objectId;
-    if (length != null && (offset != null || count != null)) {
-      final range = await _subrange(objectId, offset ?? 0, count ?? 0, length);
+    if (_isSubRange(offset: offset, count: count, length: length)) {
+      final range = await _subRange(objectId,
+          offset: offset ?? 0, count: count, length: length!);
       rangeId = range.objectId ?? rangeId;
     }
     final jsProperties = await sendCommandAndValidateResult<List>(

dwds/lib/src/debugging/instance.dart

@@ -116,16 +116,15 @@ class InstanceHelper extends Domain {
       return _closureInstanceFor(remoteObject);
     }
 
-    final properties = await debugger.getProperties(objectId,
-        offset: offset, count: count, length: metaData.length);
     if (metaData.isSystemList) {
-      return await _listInstanceFor(
-          classRef, remoteObject, properties, offset, count);
+      return await _listInstanceFor(classRef, remoteObject,
+          offset: offset, count: count, length: metaData.length);
     } else if (metaData.isSystemMap) {
-      return await _mapInstanceFor(
-          classRef, remoteObject, properties, offset, count);
+      return await _mapInstanceFor(classRef, remoteObject,
+          offset: offset, count: count, length: metaData.length);
     } else {
-      return await _plainInstanceFor(classRef, remoteObject, properties);
+      return await _plainInstanceFor(classRef, remoteObject,
+          offset: offset, count: count, length: metaData.length);
     }
   }
 
@@ -171,10 +170,18 @@ class InstanceHelper extends Domain {
 
   /// Create a plain instance of [classRef] from [remoteObject] and the JS
   /// properties [properties].
-  Future<Instance?> _plainInstanceFor(ClassRef classRef,
-      RemoteObject remoteObject, List<Property> properties) async {
+  Future<Instance?> _plainInstanceFor(
+    ClassRef classRef,
+    RemoteObject remoteObject, {
+    int? offset,
+    int? count,
+    int? length,
+  }) async {
     final objectId = remoteObject.objectId;
     if (objectId == null) return null;
+
+    final properties = await debugger.getProperties(objectId,
+        offset: offset, count: count, length: length);
     final dartProperties = await _dartFieldsFor(properties, remoteObject);
     var boundFields = await Future.wait(
         dartProperties.map<Future<BoundField>>((p) => _fieldFor(p, classRef)));
@@ -200,8 +207,8 @@ class InstanceHelper extends Domain {
   }
 
   /// The associations for a Dart Map or IdentityMap.
-  Future<List<MapAssociation>> _mapAssociations(
-      RemoteObject map, int? offset, int? count) async {
+  Future<List<MapAssociation>> _mapAssociations(RemoteObject map,
+      {int? offset, int? count}) async {
     // We do this in in awkward way because we want the keys and values, but we
     // can't return things by value or some Dart objects will come back as
     // values that we need to be RemoteObject, e.g. a List of int.
@@ -241,67 +248,95 @@ class InstanceHelper extends Domain {
 
   /// Create a Map instance with class [classRef] from [remoteObject].
   Future<Instance?> _mapInstanceFor(
-      ClassRef classRef,
-      RemoteObject remoteObject,
-      List<Property> _,
-      int? offset,
-      int? count) async {
+    ClassRef classRef,
+    RemoteObject remoteObject, {
+    int? offset,
+    int? count,
+    int? length,
+  }) async {
     final objectId = remoteObject.objectId;
     if (objectId == null) return null;
+
     // Maps are complicated, do an eval to get keys and values.
-    final associations = await _mapAssociations(remoteObject, offset, count);
-    final length = (offset == null && count == null)
-        ? associations.length
-        : (await instanceRefFor(remoteObject))?.length;
+    final associations =
+        await _mapAssociations(remoteObject, offset: offset, count: count);
+    final rangeCount = _calculateRangeCount(
+        count: count, elementCount: associations.length, length: length);
     return Instance(
         identityHashCode: remoteObject.objectId.hashCode,
         kind: InstanceKind.kMap,
         id: objectId,
         classRef: classRef)
       ..length = length
       ..offset = offset
-      ..count = (associations.length == length) ? null : associations.length
+      ..count = rangeCount
       ..associations = associations;
   }
 
   /// Create a List instance of [classRef] from [remoteObject] with the JS
   /// properties [properties].
   Future<Instance?> _listInstanceFor(
-      ClassRef classRef,
-      RemoteObject remoteObject,
-      List<Property> properties,
-      int? offset,
-      int? count) async {
+    ClassRef classRef,
+    RemoteObject remoteObject, {
+    int? offset,
+    int? count,
+    int? length,
+  }) async {
     final objectId = remoteObject.objectId;
     if (objectId == null) return null;
 
-    /// TODO(annagrin): split into cases to make the logic clear.
-    /// TODO(annagrin): make sure we use offset correctly.
-    final numberOfProperties = _lengthOf(properties) ?? 0;
-    final length = (offset == null && count == null)
-        ? numberOfProperties
-        : (await instanceRefFor(remoteObject))?.length;
-    final indexed = properties.sublist(
-        0, min(count ?? length ?? numberOfProperties, numberOfProperties));
-    final fields = await Future.wait(indexed
-        .map((property) async => await _instanceRefForRemote(property.value)));
+    final elements = await _listElements(remoteObject,
+        offset: offset, count: count, length: length);
+    final rangeCount = _calculateRangeCount(
+        count: count, elementCount: elements.length, length: length);
     return Instance(
         identityHashCode: remoteObject.objectId.hashCode,
         kind: InstanceKind.kList,
         id: objectId,
         classRef: classRef)
       ..length = length
-      ..elements = fields
+      ..elements = elements
       ..offset = offset
-      ..count = (numberOfProperties == length) ? null : numberOfProperties;
+      ..count = rangeCount;
+  }
+
+  /// The elements for a Dart List.
+  Future<List<InstanceRef?>> _listElements(
+    RemoteObject list, {
+    int? offset,
+    int? count,
+    int? length,
+  }) async {
+    final properties = await debugger.getProperties(list.objectId!,
+        offset: offset, count: count, length: length);
+    final rangeCount = _calculateRangeCount(
+        count: count, elementCount: _lengthOf(properties), length: length);
+    final indexed = properties.sublist(0, rangeCount);
+
+    return Future.wait(indexed
+        .map((property) async => await _instanceRefForRemote(property.value)));
+  }
+
+  /// Return the available count of elements in the requested range.
+  /// Return `null` if the range includes the whole object.
+  /// [count] is the range length requested by the `getObject` call.
+  /// [elementCount] is the number of elements in the runtime object.
+  /// [length] is the expected length of the whole object, read from
+  /// the [ClassMetaData].
+  static int? _calculateRangeCount(
+      {int? count, int? elementCount, int? length}) {
+    if (count == null) return null;
+    if (elementCount == null) return null;
+    if (length == elementCount) return null;
+    return min(count, elementCount);
   }
 
   /// Return the value of the length attribute from [properties], if present.
   ///
   /// This is only applicable to Lists or Maps, where we expect a length
   /// attribute. Even if a plain instance happens to have a length field, we
   /// don't use it to determine the properties to display.
-  int? _lengthOf(List<Property> properties) {
+  static int? _lengthOf(List<Property> properties) {
     final lengthProperty = properties.firstWhere((p) => p.name == 'length');
     return lengthProperty.value?.value as int?;
   }

dwds/lib/src/injected/client.js

@@ -1,6 +1,6 @@
 name: dwds
 # Every time this changes you need to run `dart run build_runner build`.
-version: 17.0.0
+version: 17.0.1-dev
 description: >-
   A service that proxies between the Chrome debug protocol and the Dart VM
   service protocol.