Find best locations for breakpoints, expression evaluation, and call frames. (#1590)

* Fix finding locations on breakpoints, call frames, and expression evaluation

* Update JS and Dart location search heuristic

* Fix typos in comments

* Updated heuristic for finding dart locations to work for more cases, added tests

* Addressed CR comments

* Enable accidentally disabled tests

* Format

* Fix failing tests

Author: Anna Gringauze

dwds/CHANGELOG.md

@@ -9,6 +9,7 @@
   - Fix crash when using flutter tools with web server device.
   - Remove clearing all scripts on page load for extension debugger.
 - Fix breakpoints not hitting after changing a base in index.html.
+- Find best locations for call frames, breakpoints, or expression evaluation.
 
 **Breaking changes:**
 - Add `basePath` parameter to `FrontendServerRequireStrategy`.

dwds/lib/src/debugging/debugger.dart

@@ -317,7 +317,7 @@ class Debugger extends Domain {
   }
 
   /// Returns Chrome script uri for Chrome script ID.
-  String _urlForScriptId(String scriptId) =>
+  String urlForScriptId(String scriptId) =>
       _remoteDebugger.scripts[scriptId]?.url;
 
   /// Returns source [Location] for the paused event.
@@ -331,7 +331,7 @@ class Debugger extends Domain {
     var line = location['lineNumber'] as int;
     var column = location['columnNumber'] as int;
 
-    var url = _urlForScriptId(scriptId);
+    var url = urlForScriptId(scriptId);
     return _locations.locationForJs(url, line, column);
   }
 
@@ -467,25 +467,14 @@ class Debugger extends Domain {
     var line = location.lineNumber;
     var column = location.columnNumber;
 
-    var url = _urlForScriptId(location.scriptId);
+    var url = urlForScriptId(location.scriptId);
     if (url == null) {
       logger.severe('Failed to create dart frame for ${frame.functionName}: '
           'cannot find location for script ${location.scriptId}');
       return null;
     }
 
-    // TODO(sdk/issues/37240) - ideally we look for an exact location instead
-    // of the closest location on a given line.
-    Location bestLocation;
-    for (var location in await _locations.locationsForUrl(url)) {
-      if (location.jsLocation.line == line) {
-        bestLocation ??= location;
-        if ((location.jsLocation.column - column).abs() <
-            (bestLocation.jsLocation.column - column).abs()) {
-          bestLocation = location;
-        }
-      }
-    }
+    var bestLocation = await _locations.locationForJs(url, line, column);
     if (bestLocation == null) return null;
 
     var script =
@@ -506,7 +495,11 @@ class Debugger extends Domain {
         kind: CodeKind.kDart,
         id: createId(),
       ),
-      location: SourceLocation(tokenPos: bestLocation.tokenPos, script: script),
+      location: SourceLocation(
+          line: bestLocation.dartLocation.line,
+          column: bestLocation.dartLocation.column,
+          tokenPos: bestLocation.tokenPos,
+          script: script),
       kind: FrameKind.kRegular,
     );
 
@@ -579,7 +572,7 @@ class Debugger extends Domain {
         var frame = e.params['callFrames'][0];
         var scriptId = '${frame["location"]["scriptId"]}';
 
-        var url = _urlForScriptId(scriptId);
+        var url = urlForScriptId(scriptId);
         if (url == null) {
           logger.severe('Stepping failed: '
               'cannot find location for script $scriptId');

dwds/lib/src/debugging/frame_computer.dart

@@ -4,22 +4,24 @@
 
 // @dart = 2.9
 
+import 'package:logging/logging.dart';
 import 'package:pool/pool.dart';
 import 'package:vm_service/vm_service.dart';
 import 'package:webkit_inspection_protocol/webkit_inspection_protocol.dart';
 
-import 'dart_scope.dart';
 import 'debugger.dart';
 
 class FrameComputer {
   final Debugger debugger;
 
+  final _logger = Logger('FrameComputer');
+
   // To ensure that the frames are computed only once, we use a pool to guard
   // the work. Frames are computed sequentially.
   final _pool = Pool(1);
 
   final List<WipCallFrame> _callFrames;
-  final _computedFrames = <Frame>[];
+  final List<Frame> _computedFrames = [];
 
   var _frameIndex = 0;
 
@@ -36,12 +38,6 @@ class FrameComputer {
     return frameIndex < _callFrames.length ? _callFrames[frameIndex] : null;
   }
 
-  /// Return the WipScopes for the given JavaScript frame index that are
-  /// pertinent for Dart debugging.
-  List<WipScope> getWipScopesForFrameIndex(int frameIndex) {
-    return filterScopes(jsFrameForIndex(frameIndex));
-  }
-
   /// Translates Chrome callFrames contained in [DebuggerPausedEvent] into Dart
   /// [Frame]s.
   Future<List<Frame>> calculateFrames({int limit}) async {
@@ -101,6 +97,14 @@ class FrameComputer {
           var location = WipLocation.fromValues(
               callFrame.scriptId, callFrame.lineNumber,
               columnNumber: callFrame.columnNumber);
+
+          var url = callFrame.url ?? debugger.urlForScriptId(location.scriptId);
+          if (url == null) {
+            _logger.severe(
+                'Failed to create dart frame for ${callFrame.functionName}: '
+                'cannot find location for script ${callFrame.scriptId}');
+          }
+
           var tempWipFrame = WipCallFrame({
             'url': callFrame.url,
             'functionName': callFrame.functionName,

dwds/lib/src/debugging/location.dart

@@ -152,22 +152,113 @@ class Locations {
   /// Find the [Location] for the given Dart source position.
   ///
   /// The [line] number is 1-based.
-  Future<Location> locationForDart(DartUri uri, int line, int column) async =>
-      (await locationsForDart(uri.serverPath)).firstWhere(
-          (location) =>
-              location.dartLocation.line == line &&
-              location.dartLocation.column >= column,
-          orElse: () => null);
+  Future<Location> locationForDart(DartUri uri, int line, int column) async {
+    var locations = await locationsForDart(uri.serverPath);
+    return _bestDartLocation(locations, line, column);
+  }
 
   /// Find the [Location] for the given JS source position.
   ///
   /// The [line] number is 0-based.
-  Future<Location> locationForJs(String url, int line, int column) async =>
-      (await locationsForUrl(url)).firstWhere(
-          (location) =>
-              location.jsLocation.line == line &&
-              location.jsLocation.column >= column,
-          orElse: () => null);
+  Future<Location> locationForJs(String url, int line, int column) async {
+    var locations = await locationsForUrl(url);
+    return _bestJsLocation(locations, line, column);
+  }
+
+  /// Find closest existing Dart location for the line and column.
+  ///
+  /// Dart columns for breakpoints are either exact or start at the
+  /// beginning of the line - return the first existing location
+  /// that comes after the given column.
+  Location _bestDartLocation(
+      Iterable<Location> locations, int line, int column) {
+    Location bestLocation;
+    for (var location in locations) {
+      if (location.dartLocation.line == line &&
+          location.dartLocation.column >= column) {
+        bestLocation ??= location;
+        if (location.dartLocation.column < bestLocation.dartLocation.column) {
+          bestLocation = location;
+        }
+      }
+    }
+    return bestLocation;
+  }
+
+  /// Find closest existing JavaScript location for the line and column.
+  ///
+  /// Some JS locations are not stored in the source maps, so we find the
+  /// closest existing location, preferring the one coming after the given
+  /// column, if the current break is at an expression statement, or the
+  /// one coming before if the current break is at a function call.
+  ///
+  /// This is a known problem that other code bases solve using by finding
+  /// the closest location to the current one:
+  ///
+  /// https://github.com/microsoft/vscode-js-debug/blob/536f96bae61a3d87546b61bc7916097904c81429/src/common/sourceUtils.ts#L286
+  ///
+  /// Unfortunately, this approach fails for Flutter code too often, as it
+  /// frequently contains multi-line statements with nested objects.
+  ///
+  /// For example:
+  ///
+  ///   - `t33 = main.doSomething()` in top frame:
+  ///      Current column is at `t33`. Return existing location starting
+  ///      at `main`.
+  ///   - `main.doSomething()` in top frame:
+  ///      Current column is at `main`. Return existing location starting
+  ///      at `main`.
+  ///   - `main.doSomething()` in a frame down the stack:
+  ///      Current column is at `doSomething`. Source map does not have a
+  ///      location stored that starts at `doSomething()`. Return existing
+  ///      location starting at `main`.
+  Location _bestJsLocation(Iterable<Location> locations, int line, int column) {
+    Location bestLocationBefore;
+    Location bestLocationAfter;
+
+    var locationsAfter = locations.where((location) =>
+        location.jsLocation.line == line &&
+        location.jsLocation.column >= column);
+    var locationsBefore = locations.where((location) =>
+        location.jsLocation.line == line &&
+        location.jsLocation.column < column);
+
+    for (var location in locationsAfter) {
+      bestLocationAfter ??= location;
+      if (location.jsLocation.column < bestLocationAfter.jsLocation.column) {
+        bestLocationAfter = location;
+      }
+    }
+    for (var location in locationsBefore) {
+      bestLocationBefore ??= location;
+      if (location.jsLocation.column > bestLocationBefore.jsLocation.column) {
+        bestLocationBefore = location;
+      }
+    }
+    if (bestLocationAfter == null) return bestLocationBefore;
+    if (bestLocationBefore == null) return bestLocationAfter;
+
+    if (bestLocationAfter.jsLocation.line == line &&
+        bestLocationAfter.jsLocation.column == column) {
+      // Prefer exact match.
+      return bestLocationAfter;
+    }
+
+    // Return the closest location after the current if the current location
+    // is at the beginning of the line (i.e. on expression statement).
+    // Return the closest location before the current if the current location
+    // is in the middle of the line (i.e. on function call).
+    if (locationsBefore.length == 1 &&
+        locationsBefore.first.jsLocation.column == 0) {
+      // Best guess on whether the the current location is at the beginning of
+      // the line (i.e. expression statement):
+      // The only location on the left has column 0, so only spaces are on the
+      // left.
+      return bestLocationAfter;
+    }
+    // Current column is in the middle of the line (i.e .function call).
+    return bestLocationBefore;
+  }
 
   /// Returns the tokenPosTable for the provided Dart script path as defined
   /// in:

dwds/lib/src/debugging/metadata/provider.dart

@@ -216,7 +216,7 @@ class MetadataProvider {
           _soundNullSafety = hasSoundNullSafety;
         }
         _logger.info('Loaded debug metadata '
-            '(${_soundNullSafety ? "" : "no "}sound null safety)');
+            '(${_soundNullSafety ? "sound" : "weak"} null safety)');
       }
     });
   }

dwds/lib/src/services/expression_evaluator.dart

@@ -155,7 +155,9 @@ class ExpressionEvaluator {
     var jsFrame = _inspector.debugger.jsFrameForIndex(frameIndex);
     if (jsFrame == null) {
       return _createError(
-          ErrorKind.internal, 'No frame with index $frameIndex');
+          ErrorKind.internal,
+          'Expression evaluation in async frames '
+          'is not supported. No frame with index $frameIndex.');
     }
 
     var functionName = jsFrame.functionName;
@@ -165,12 +167,6 @@ class ExpressionEvaluator {
     var jsScope = await _collectLocalJsScope(jsFrame);
 
     // Find corresponding dart location and scope.
-    //
-    // TODO(annagrin): handle unknown dart locations
-    // Debugger does not map every js location to a dart location,
-    // so this will result in expressions not evaluated in some
-    // cases. Invent location matching strategy for those cases.
-    // [issue 890](https://github.com/dart-lang/webdev/issues/890)
     var url = _urlForScriptId(jsScriptId);
     var locationMap = await _locations.locationForJs(url, jsLine, jsColumn);
     if (locationMap == null) {

dwds/test/build_daemon_breakpoint_test.dart

@@ -74,28 +74,6 @@ void main() {
         await service.removeBreakpoint(isolate.id, bp.id);
       });
 
-      test('set breakpoint inside a JavaScript line succeeds on', () async {
-        var line = await context.findBreakpointLine(
-            'printNestedObjectMultiLine', isolate.id, mainScript);
-        var column = 0;
-        var bp = await service.addBreakpointWithScriptUri(
-            isolate.id, mainScript.uri, line,
-            column: column);
-        await stream.firstWhere(
-            (Event event) => event.kind == EventKind.kPauseBreakpoint);
-
-        expect(bp, isNotNull);
-        expect(
-            bp.location,
-            isA<SourceLocation>()
-                .having((loc) => loc.script, 'script', equals(mainScript))
-                .having((loc) => loc.line, 'line', equals(line))
-                .having((loc) => loc.column, 'column', greaterThan(column)));
-
-        // Remove breakpoint so it doesn't impact other tests.
-        await service.removeBreakpoint(isolate.id, bp.id);
-      });
-
       test('set breakpoint again', () async {
         var line = await context.findBreakpointLine(
             'printLocal', isolate.id, mainScript);