Add columns to breakpoints (#1560)

* Add columns to breakpoints

* Added tests and updated version

* Remove debug printing

* Removed adjusting JS locations from 0-based to 1-based and back

* Add debug information on failure to set breakpoints

* Update tests

* Update skiplist tests

Author: Anna Gringauze

dwds/CHANGELOG.md

@@ -1,3 +1,6 @@
+## 13.1.1-dev
+- Add column information to breakpoints to allow precise breakpoint placement.
+
 ## 13.1.0
 - Update _fe_analyzer_shared to version ^38.0.0.
 

dwds/lib/src/debugging/debugger.dart

@@ -228,8 +228,9 @@ class Debugger extends Domain {
     int line, {
     int column,
   }) async {
+    column ??= 0;
     checkIsolate('addBreakpoint', isolateId);
-    final breakpoint = await _breakpoints.add(scriptId, line);
+    final breakpoint = await _breakpoints.add(scriptId, line, column);
     _notifyBreakpoint(breakpoint);
     return breakpoint;
   }
@@ -249,7 +250,9 @@ class Debugger extends Domain {
     for (var breakpoint in previousBreakpoints) {
       var scriptRef = await _updatedScriptRefFor(breakpoint);
       var updatedLocation = await _locations.locationForDart(
-          DartUri(scriptRef.uri, _root), _lineNumberFor(breakpoint));
+          DartUri(scriptRef.uri, _root),
+          _lineNumberFor(breakpoint),
+          _columnNumberFor(breakpoint));
       var updatedBreakpoint = _breakpoints._dartBreakpoint(
           scriptRef, updatedLocation, breakpoint.id);
       _breakpoints._note(
@@ -263,7 +266,8 @@ class Debugger extends Domain {
       await addBreakpoint(
           inspector.isolate.id,
           (await _updatedScriptRefFor(breakpoint)).id,
-          _lineNumberFor(breakpoint));
+          _lineNumberFor(breakpoint),
+          column: _columnNumberFor(breakpoint));
     }
   }
 
@@ -324,10 +328,11 @@ class Debugger extends Domain {
     var frame = e.params['callFrames'][0];
     var location = frame['location'];
     var scriptId = location['scriptId'] as String;
-    var lineNumber = location['lineNumber'] as int;
+    var line = location['lineNumber'] as int;
+    var column = location['columnNumber'] as int;
 
     var url = _urlForScriptId(scriptId);
-    return _locations.locationForJs(url, lineNumber + 1);
+    return _locations.locationForJs(url, line, column);
   }
 
   /// The variables visible in a frame in Dart protocol [BoundVariable] form.
@@ -459,9 +464,8 @@ class Debugger extends Domain {
     bool populateVariables = true,
   }) async {
     var location = frame.location;
-    // Chrome is 0 based. Account for this.
-    var line = location.lineNumber + 1;
-    var column = location.columnNumber + 1;
+    var line = location.lineNumber;
+    var column = location.columnNumber;
 
     var url = _urlForScriptId(location.scriptId);
     if (url == null) {
@@ -723,14 +727,20 @@ bool isNativeJsObject(InstanceRef instanceRef) {
 
 /// Returns the Dart line number for the provided breakpoint.
 int _lineNumberFor(Breakpoint breakpoint) =>
-    int.parse(breakpoint.id.split('#').last);
+    int.parse(breakpoint.id.split('#').last.split(':').first);
+
+/// Returns the Dart column number for the provided breakpoint.
+int _columnNumberFor(Breakpoint breakpoint) =>
+    int.parse(breakpoint.id.split('#').last.split(':').last);
 
 /// Returns the breakpoint ID for the provided Dart script ID and Dart line
 /// number.
-String breakpointIdFor(String scriptId, int line) => 'bp/$scriptId#$line';
+String breakpointIdFor(String scriptId, int line, int column) =>
+    'bp/$scriptId#$line:$column';
 
 /// Keeps track of the Dart and JS breakpoint Ids that correspond.
 class _Breakpoints extends Domain {
+  final logger = Logger('Breakpoints');
   final _dartIdByJsId = <String, String>{};
   final _jsIdByDartId = <String, String>{};
 
@@ -752,13 +762,16 @@ class _Breakpoints extends Domain {
   }) : super(provider);
 
   Future<Breakpoint> _createBreakpoint(
-      String id, String scriptId, int line) async {
+      String id, String scriptId, int line, int column) async {
     var dartScript = inspector.scriptWithId(scriptId);
     var dartUri = DartUri(dartScript.uri, root);
-    var location = await locations.locationForDart(dartUri, line);
-
+    var location = await locations.locationForDart(dartUri, line, column);
     // TODO: Handle cases where a breakpoint can't be set exactly at that line.
     if (location == null) {
+      logger
+          .fine('Failed to set breakpoint at ${dartScript.uri}:$line:$column: '
+              'Dart location not found for scriptId: $scriptId, '
+              'server path: ${dartUri.serverPath}, root:$root');
       throw RPCError(
           'addBreakpoint',
           102,
@@ -779,10 +792,10 @@ class _Breakpoints extends Domain {
 
   /// Adds a breakpoint at [scriptId] and [line] or returns an existing one if
   /// present.
-  Future<Breakpoint> add(String scriptId, int line) async {
-    final id = breakpointIdFor(scriptId, line);
+  Future<Breakpoint> add(String scriptId, int line, int column) async {
+    final id = breakpointIdFor(scriptId, line, column);
     return _bpByDartId.putIfAbsent(
-        id, () => _createBreakpoint(id, scriptId, line));
+        id, () => _createBreakpoint(id, scriptId, line, column));
   }
 
   /// Create a Dart breakpoint at [location] in [dartScript] with [id].
@@ -792,17 +805,19 @@ class _Breakpoints extends Domain {
       id: id,
       breakpointNumber: int.parse(createId()),
       resolved: true,
-      location: SourceLocation(script: dartScript, tokenPos: location.tokenPos),
+      location: SourceLocation(
+        script: dartScript,
+        tokenPos: location.tokenPos,
+        line: location.dartLocation.line,
+        column: location.dartLocation.column,
+      ),
       enabled: true,
     )..id = id;
     return breakpoint;
   }
 
   /// Calls the Chrome protocol setBreakpoint and returns the remote ID.
   Future<String> _setJsBreakpoint(Location location) async {
-    // Location is 0 based according to:
-    // https://chromedevtools.github.io/devtools-protocol/tot/Debugger#type-Location
-
     // The module can be loaded from a nested path and contain an ETAG suffix.
     var urlRegex = '.*${location.jsLocation.module}.*';
     // Prevent `Aww, snap!` errors when setting multiple breakpoints
@@ -811,7 +826,8 @@ class _Breakpoints extends Domain {
       var response = await remoteDebugger
           .sendCommand('Debugger.setBreakpointByUrl', params: {
         'urlRegex': urlRegex,
-        'lineNumber': location.jsLocation.line - 1,
+        'lineNumber': location.jsLocation.line,
+        'columnNumber': location.jsLocation.column,
       });
       return response.result['breakpointId'] as String;
     });

dwds/lib/src/debugging/location.dart

@@ -40,6 +40,7 @@ class Location {
     var dartColumn = entry.sourceColumn;
     var jsLine = lineEntry.line;
     var jsColumn = entry.column;
+
     // lineEntry data is 0 based according to:
     // https://docs.google.com/document/d/1U1RGAehQwRypUTovF1KRlpiOFze0b-_2gc6fAH0KY0k
     return Location._(
@@ -71,21 +72,18 @@ class DartLocation {
   @override
   String toString() => '[${uri.serverPath}:$line:$column]';
 
-  static DartLocation fromZeroBased(DartUri uri, int line, int column) =>
+  factory DartLocation.fromZeroBased(DartUri uri, int line, int column) =>
       DartLocation._(uri, line + 1, column + 1);
-
-  static DartLocation fromOneBased(DartUri uri, int line, int column) =>
-      DartLocation._(uri, line, column);
 }
 
 /// Location information for a JS source.
 class JsLocation {
   final String module;
 
-  /// 1 based row offset within the JS source code.
+  /// 0 based row offset within the JS source code.
   final int line;
 
-  /// 1 based column offset within the JS source code.
+  /// 0 based column offset within the JS source code.
   final int column;
 
   JsLocation._(
@@ -97,10 +95,9 @@ class JsLocation {
   @override
   String toString() => '[$module:$line:$column]';
 
-  static JsLocation fromZeroBased(String module, int line, int column) =>
-      JsLocation._(module, line + 1, column + 1);
-
-  static JsLocation fromOneBased(String module, int line, int column) =>
+  // JS Location is 0 based according to:
+  // https://chromedevtools.github.io/devtools-protocol/tot/Debugger#type-Location
+  factory JsLocation.fromZeroBased(String module, int line, int column) =>
       JsLocation._(module, line, column);
 }
 
@@ -155,17 +152,21 @@ class Locations {
   /// Find the [Location] for the given Dart source position.
   ///
   /// The [line] number is 1-based.
-  Future<Location> locationForDart(DartUri uri, int line) async =>
+  Future<Location> locationForDart(DartUri uri, int line, int column) async =>
       (await locationsForDart(uri.serverPath)).firstWhere(
-          (location) => location.dartLocation.line == line,
+          (location) =>
+              location.dartLocation.line == line &&
+              location.dartLocation.column >= column,
           orElse: () => null);
 
   /// Find the [Location] for the given JS source position.
   ///
-  /// The [line] number is 1-based.
-  Future<Location> locationForJs(String url, int line) async =>
+  /// 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) =>
+              location.jsLocation.line == line &&
+              location.jsLocation.column >= column,
           orElse: () => null);
 
   /// Returns the tokenPosTable for the provided Dart script path as defined

dwds/lib/src/debugging/skip_list.dart

@@ -31,8 +31,8 @@ class SkipLists {
     var startColumn = 0;
     for (var location in locations) {
       // Account for 1 based.
-      var endLine = location.jsLocation.line - 1;
-      var endColumn = location.jsLocation.column - 1;
+      var endLine = location.jsLocation.line;
+      var endColumn = location.jsLocation.column;
       // Stop before the known location.
       endColumn -= 1;
       if (endColumn < 0) {
@@ -42,8 +42,8 @@ class SkipLists {
       if (endLine > startLine || endColumn > startColumn) {
         ranges.add(
             _rangeFor(scriptId, startLine, startColumn, endLine, endColumn));
-        startLine = location.jsLocation.line - 1;
-        startColumn = location.jsLocation.column;
+        startLine = location.jsLocation.line;
+        startColumn = location.jsLocation.column + 1;
       }
     }
     ranges.add(_rangeFor(scriptId, startLine, startColumn, maxValue, maxValue));

dwds/lib/src/services/expression_evaluator.dart

@@ -159,8 +159,9 @@ class ExpressionEvaluator {
     }
 
     var functionName = jsFrame.functionName;
-    var jsLine = jsFrame.location.lineNumber + 1;
+    var jsLine = jsFrame.location.lineNumber;
     var jsScriptId = jsFrame.location.scriptId;
+    var jsColumn = jsFrame.location.columnNumber;
     var jsScope = await _collectLocalJsScope(jsFrame);
 
     // Find corresponding dart location and scope.
@@ -171,7 +172,7 @@ class ExpressionEvaluator {
     // 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);
+    var locationMap = await _locations.locationForJs(url, jsLine, jsColumn);
     if (locationMap == null) {
       return _createError(
           ErrorKind.internal,

dwds/lib/src/version.dart

@@ -1,6 +1,6 @@
 name: dwds
 # Every time this changes you need to run `dart run build_runner build`.
-version: 13.1.0
+version: 13.1.1-dev
 homepage: https://github.com/dart-lang/webdev/tree/master/dwds
 description: >-
   A service that proxies between the Chrome debug protocol and the Dart VM

dwds/pubspec.yaml

@@ -74,6 +74,28 @@ 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);

dwds/test/build_daemon_breakpoint_test.dart

@@ -104,6 +104,29 @@ void main() {
         // Remove breakpoint so it doesn't impact other tests.
         await service.removeBreakpoint(isolate.id, bp.id);
       });
+
+      test('set breakpoint inside a JavaScript line succeeds', () 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);
+      });
     });
   });
 }

dwds/test/frontend_server_breakpoint_test.dart

@@ -40,6 +40,7 @@ void main() {
     printFromTestPackage();
     printCallExtension();
     printLoopVariable();
+    printNestedObjectsMultiLine();
   });
 
   document.body.appendText(concatenate('Program', ' is running!'));
@@ -99,10 +100,25 @@ Future<void> printDeferred() async {
   d.deferredPrintLocal();
 }
 
+void printNestedObjectsMultiLine() {
+  print(// Breakpoint: printEnclosingFunctionMultiLine
+      EnclosingMainClass(// Breakpoint: printEnclosingObjectMultiLine
+          MainClass(0) // Breakpoint: printNestedObjectMultiLine
+          ));
+}
+
 class MainClass {
   final int _field;
   MainClass(this._field);
 
   @override
   String toString() => '$_field';
 }
+
+class EnclosingMainClass {
+  final MainClass _field;
+  EnclosingMainClass(this._field);
+
+  @override
+  String toString() => '$_field';
+}