[dds/dap] Parse all output events for stack frames instead of online …

…stderr

The legacy debug adapters would scan all output for stack frames but when building the new adapters I only parsed stderr (because the stack frame parsing captures quite a lot and it seemed unnecessary). It turned out many users appreciated the IDE linking up stack traces in stdout too (because their exception logging framework may handle exceptions and print them this way, or they might have helpers to print the current stack trace to aid debugging).

This marks `parseStackFrames` as deprecated from `sendOutput` and scans all output for frames, while filtering out things that don't look like Dart URIs before calling `lookupResolvedUris` to try and avoid making general output async/slow.

If Flutter considers deprecations a bot failure, this will need to be rolled in manually.

Fixes Dart-Code/Dart-Code#5072

Change-Id: I8857281fb79ca69328074d6ad6a0c0a505e5c193
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/362804
Reviewed-by: Helin Shiah <helinx@google.com>
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Helin Shiah <helinx@google.com>

pkg/dds/CHANGELOG.md

@@ -1,3 +1,6 @@
+# 4.2.0
+- [DAP] All `OutputEvent`s are now scanned for stack frames to attach `source` metadata to. The [parseStackFrames] parameter for `sendOutput` is ignored and deprecated.
+
 # 4.1.0
 - Internal change: removed static method `DevToolsUtils.initializeAnalytics`
 and prepared DDS for using `unified_analytics` through the Dart Tooling Daemon.

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

@@ -1414,10 +1414,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
   /// Sends an OutputEvent (without a newline, since calls to this method
   /// may be using buffered data that is not split cleanly on newlines).
   ///
-  /// If [parseStackFrames] is set, it controls whether to look for stack traces
-  /// and extract file/line information to add to the metadata of the event. If
-  /// it is `null` then parsing will occur only if [category] is `"stderr"`.
-  ///
   /// To ensure output is sent to the client in the correct order even if
   /// processing stack frames requires async calls, this function will insert
   /// output events into a queue and only send them when previous calls have
@@ -1426,6 +1422,8 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     String category,
     String message, {
     int? variablesReference,
+    @Deprecated(
+        'parseStackFrames has no effect, stack frames are always parsed')
     bool? parseStackFrames,
   }) async {
     // Reserve our place in the queue be inserting a future that we can complete
@@ -1439,7 +1437,6 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
         category,
         message,
         variablesReference: variablesReference,
-        parseStackFrames: parseStackFrames,
       );
 
       // Chain our sends onto the end of the previous one, and complete our Future
@@ -2149,41 +2146,39 @@ abstract class DartDebugAdapter<TL extends LaunchRequestArguments,
     String category,
     String message, {
     int? variablesReference,
-    bool? parseStackFrames,
   }) async {
-    parseStackFrames ??= category == 'stderr';
-    try {
-      if (parseStackFrames) {
-        return await _buildErrorOutputEvents(category, message);
-      } else {
-        return [
-          OutputEventBody(
-            category: category,
-            output: message,
-            variablesReference: variablesReference,
-          )
-        ];
+    if (variablesReference != null) {
+      return [
+        OutputEventBody(
+          category: category,
+          output: message,
+          variablesReference: variablesReference,
+        )
+      ];
+    } else {
+      try {
+        return await _buildOutputEventsWithSourceReferences(category, message);
+      } catch (e, s) {
+        // Since callers of [sendOutput] may not await it, don't allow unhandled
+        // errors (for example if the VM Service quits while we were trying to
+        // map URIs), just log and return the event without metadata.
+        logger?.call('Failed to build OutputEvent: $e, $s');
+        return [OutputEventBody(category: category, output: message)];
       }
-    } catch (e, s) {
-      // Since callers of [sendOutput] may not await it, don't allow unhandled
-      // errors (for example if the VM Service quits while we were trying to
-      // map URIs), just log and return the event without metadata.
-      logger?.call('Failed to build OutputEvent: $e, $s');
-      return [OutputEventBody(category: category, output: message)];
     }
   }
 
-  /// Builds OutputEvents for errors.
+  /// Builds OutputEvents with source references if they contain stack frames.
   ///
   /// If a stack trace can be parsed from [message], file/line information will
   /// be included in the metadata of the event.
-  Future<List<OutputEventBody>> _buildErrorOutputEvents(
+  Future<List<OutputEventBody>> _buildOutputEventsWithSourceReferences(
       String category, String message) async {
     final events = <OutputEventBody>[];
 
     // Extract all the URIs so we can send a batch request for resolving them.
     final lines = message.split('\n');
-    final frames = lines.map(parseStackFrame).toList();
+    final frames = lines.map(parseDartStackFrame).toList();
     final uris = frames.nonNulls.map((f) => f.uri).toList();
 
     // We need an Isolate to resolve package URIs. Since we don't know what

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

@@ -69,6 +69,8 @@ mixin TestAdapter {
     String category,
     String message, {
     int? variablesReference,
+    @Deprecated(
+        'parseStackFrames has no effect, stack frames are always parsed')
     bool? parseStackFrames,
   });
 

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

@@ -22,13 +22,59 @@ bool isResolvableUri(Uri uri) {
       !uri.hasEmptyPath;
 }
 
+/// Attempts to parse a line as a stack frame in order to read path/line/col
+/// information.
+///
+/// Frames that do not look like real Dart stack frames (such as including path
+/// or URIs that look like real Dart libraries) will be filtered out but it
+/// should not be assumed that if a [stack.Frame] is returned that the input
+/// was necessarily a stack frame or that calling `toString` will return the
+/// original input text.
+stack.Frame? parseDartStackFrame(String line) {
+  final frame = _parseStackFrame(line);
+  final uri = frame?.uri;
+  return uri != null && _isDartUri(uri) ? frame : null;
+}
+
+/// Checks whether [uri] is a possible Dart URI that should be mapped to try
+/// and attach location metadata to an output event.
+///
+/// This is a performance optimization to avoid calling the VM's
+/// `lookupResolvedUris` method for output events that are probably not
+/// stack frames.
+bool _isDartUri(Uri uri) {
+  // Stack frame parsing captures a lot of things that aren't real URIs, often
+  // with no scheme or empty paths.
+  if (!uri.hasScheme || uri.hasEmptyPath) {
+    return false;
+  }
+
+  // Anything starting with dart: is potential
+  // - dart:io
+  if (uri.isScheme('dart')) {
+    return true;
+  }
+
+  // Only accept package: and file: URIs if they end with .dart.
+  // - package:foo/foo.dart
+  // - file:///c:/foo/bar.dart
+  if (uri.isScheme('package') ||
+      uri.isScheme('file') ||
+      uri.scheme.endsWith('+file')) {
+    return uri.path.endsWith('.dart');
+  }
+
+  // Some other scheme we didn't recognize and likely cannot parse.
+  return false;
+}
+
 /// Attempts to parse a line as a stack frame in order to read path/line/col
 /// information.
 ///
 /// It should not be assumed that if a [stack.Frame] is returned that the input
 /// was necessarily a stack frame or that calling `toString` will return the
 /// original input text.
-stack.Frame? parseStackFrame(String line) {
+stack.Frame? _parseStackFrame(String line) {
   // Because we split on \n, on Windows there may be trailing \r which prevents
   // package:stack_trace from parsing correctly.
   line = line.trim();

pkg/dds/pubspec.yaml

@@ -1,5 +1,5 @@
 name: dds
-version: 4.1.0
+version: 4.2.0
 description: >-
   A library used to spawn the Dart Developer Service, used to communicate with
   a Dart VM Service instance.