[dds] Support debugging "just my code" in DAP using 'debugExternalPackageLibraries=false'

Change-Id: I82556df94722b72240383f57524a5692d176ca11
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/208646
Reviewed-by: Ben Konyi <bkonyi@google.com>
Commit-Queue: Ben Konyi <bkonyi@google.com>

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

@@ -7,6 +7,8 @@ import 'dart:io';
 
 import 'package:collection/collection.dart';
 import 'package:meta/meta.dart';
+import 'package:package_config/package_config.dart';
+import 'package:path/path.dart' as path;
 import 'package:vm_service/vm_service.dart' as vm;
 
 import '../../../dds.dart';
@@ -122,6 +124,17 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
   /// have been called.
   late final bool isAttach;
 
+  /// A list of all possible project paths that should be considered the users
+  /// own code.
+  ///
+  /// This is made up of the folder containing the 'program' being executed, the
+  /// 'cwd' and any 'additionalProjectPaths' from the launch arguments.
+  late final List<String> projectPaths = [
+    args.cwd,
+    path.dirname(args.program),
+    ...?args.additionalProjectPaths,
+  ].whereNotNull().toList();
+
   DartDebugAdapter(
     ByteStreamServerChannel channel, {
     this.ipv6 = false,
@@ -533,6 +546,12 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
     sendResponse();
   }
 
+  /// Resolves a `package: URI` to the real underlying source path.
+  ///
+  /// Returns `null` if no mapping was possible, for example if the package is
+  /// not in the package mapping file.
+  String? resolvePackageUri(Uri uri) => _converter.resolvePackageUri(uri);
+
   /// [scopesRequest] is called by the client to request all of the variables
   /// scopes available for a given stack frame.
   @override
@@ -715,7 +734,10 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
       // The VM doesn't support fetching an arbitrary slice of frames, only a
       // maximum limit, so if the client asks for frames 20-30 we must send a
       // request for the first 30 and trim them ourselves.
-      final limit = startFrame + numFrames;
+
+      // DAP says if numFrames is 0 or missing (which we swap to 0 above) we
+      // should return all.
+      final limit = numFrames == 0 ? null : startFrame + numFrames;
       final stack = await vmService?.getStack(thread.isolate.id!, limit: limit);
       final frames = stack?.frames;
 
@@ -820,6 +842,18 @@ abstract class DartDebugAdapter<T extends DartLaunchRequestArguments>
     sendResponse(ThreadsResponseBody(threads: threads));
   }
 
+  /// Sets the package config file to use for `package: URI` resolution.
+  ///
+  /// TODO(dantup): Remove this once
+  ///   https://github.com/dart-lang/sdk/issues/45530 is done as it will not be
+  ///   necessary.
+  void usePackageConfigFile(File packageConfig) {
+    _converter.packageConfig = PackageConfig.parseString(
+      packageConfig.readAsStringSync(),
+      Uri.file(packageConfig.path),
+    );
+  }
+
   /// [variablesRequest] is called by the client to request child variables for
   /// a given variables variablesReference.
   ///
@@ -1104,6 +1138,14 @@ class DartLaunchRequestArguments extends LaunchRequestArguments {
   final List<String>? vmAdditionalArgs;
   final bool? enableAsserts;
 
+  /// Paths that should be considered the users local code.
+  ///
+  /// These paths will generally be all of the open folders in the users editor
+  /// and are used to determine whether a library is "external" or not to
+  /// support debugging "just my code" where SDK/Pub package code will be marked
+  /// as not-debuggable.
+  final List<String>? additionalProjectPaths;
+
   /// Whether SDK libraries should be marked as debuggable.
   ///
   /// Treated as `false` if null, which means "step in" will not step into SDK
@@ -1152,6 +1194,7 @@ class DartLaunchRequestArguments extends LaunchRequestArguments {
     this.vmServicePort,
     this.vmAdditionalArgs,
     this.enableAsserts,
+    this.additionalProjectPaths,
     this.debugSdkLibraries,
     this.debugExternalPackageLibraries,
     this.evaluateGettersInDebugViews,
@@ -1167,6 +1210,8 @@ class DartLaunchRequestArguments extends LaunchRequestArguments {
         vmServicePort = obj['vmServicePort'] as int?,
         vmAdditionalArgs = (obj['vmAdditionalArgs'] as List?)?.cast<String>(),
         enableAsserts = obj['enableAsserts'] as bool?,
+        additionalProjectPaths =
+            (obj['additionalProjectPaths'] as List?)?.cast<String>(),
         debugSdkLibraries = obj['debugSdkLibraries'] as bool?,
         debugExternalPackageLibraries =
             obj['debugExternalPackageLibraries'] as bool?,
@@ -1187,6 +1232,8 @@ class DartLaunchRequestArguments extends LaunchRequestArguments {
         if (vmServicePort != null) 'vmServicePort': vmServicePort,
         if (vmAdditionalArgs != null) 'vmAdditionalArgs': vmAdditionalArgs,
         if (enableAsserts != null) 'enableAsserts': enableAsserts,
+        if (additionalProjectPaths != null)
+          'additionalProjectPaths': additionalProjectPaths,
         if (debugSdkLibraries != null) 'debugSdkLibraries': debugSdkLibraries,
         if (debugExternalPackageLibraries != null)
           'debugExternalPackageLibraries': debugExternalPackageLibraries,

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

@@ -132,6 +132,15 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments> {
       ...?args.args,
     ];
 
+    // Find the package_config file for this script.
+    // TODO(dantup): Remove this once
+    //   https://github.com/dart-lang/sdk/issues/45530 is done as it will not be
+    //   necessary.
+    final packageConfig = _findPackageConfigFile();
+    if (packageConfig != null) {
+      this.usePackageConfigFile(packageConfig);
+    }
+
     logger?.call('Spawning $vmPath with $processArgs in ${args.cwd}');
     final process = await Process.start(
       vmPath,
@@ -146,6 +155,40 @@ class DartCliDebugAdapter extends DartDebugAdapter<DartLaunchRequestArguments> {
     unawaited(process.exitCode.then(_handleExitCode));
   }
 
+  /// Find the `package_config.json` file for the program being launched.
+  ///
+  /// TODO(dantup): Remove this once
+  ///   https://github.com/dart-lang/sdk/issues/45530 is done as it will not be
+  ///   necessary.
+  File? _findPackageConfigFile() {
+    var possibleRoot = path.isAbsolute(args.program)
+        ? path.dirname(args.program)
+        : path.dirname(path.normalize(path.join(args.cwd ?? '', args.program)));
+
+    File? packageConfig;
+    while (true) {
+      packageConfig =
+          File(path.join(possibleRoot, '.dart_tool', 'package_config.json'));
+
+      // If this packageconfig exists, use it.
+      if (packageConfig.existsSync()) {
+        break;
+      }
+
+      final parent = path.dirname(possibleRoot);
+
+      // If we can't go up anymore, the search failed.
+      if (parent == possibleRoot) {
+        packageConfig = null;
+        break;
+      }
+
+      possibleRoot = parent;
+    }
+
+    return packageConfig;
+  }
+
   /// Called by [terminateRequest] to request that we gracefully shut down the
   /// app being run (or in the case of an attach, disconnect).
   Future<void> terminateImpl() async {

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

@@ -4,6 +4,7 @@
 
 import 'dart:async';
 
+import 'package:path/path.dart' as path;
 import 'package:vm_service/vm_service.dart' as vm;
 
 import 'adapters/dart.dart';
@@ -371,17 +372,37 @@ class IsolateManager {
     }
   }
 
-  bool _isExternalPackageLibrary(vm.LibraryRef library) =>
-      // TODO(dantup): This needs to check if it's _external_, eg.
-      //
-      // - is from the flutter SDK (flutter, flutter_test, ...)
-      // - is from pub/pubcache
-      //
-      // This is intended to match the users idea of "my code". For example
-      // they may wish to debug the current app being run, as well as any other
-      // projects that are references with path: dependencies (which are likely
-      // their own supporting projects).
-      false /*library.uri?.startsWith('package:') ?? false*/;
+  /// Checks whether this library is from an external package.
+  ///
+  /// This is used to support debugging "Just My Code" so Pub packages can be
+  /// marked as not-debuggable.
+  ///
+  /// A library is considered local if the path is within the 'cwd' or
+  /// 'additionalProjectPaths' in the launch arguments. An editor should include
+  /// the paths of all open workspace folders in 'additionalProjectPaths' to
+  /// support this feature correctly.
+  bool _isExternalPackageLibrary(vm.LibraryRef library) {
+    final libraryUri = library.uri;
+    if (libraryUri == null) {
+      return false;
+    }
+    final uri = Uri.parse(libraryUri);
+    if (!uri.isScheme('package')) {
+      return false;
+    }
+    final libraryPath = _adapter.resolvePackageUri(uri);
+    if (libraryPath == null) {
+      return false;
+    }
+
+    // Always compare paths case-insensitively to avoid any issues where APIs
+    // may have returned different casing (eg. Windows drive letters). It's
+    // almost certain a user wouldn't have a "local" package and an "external"
+    // package with paths differing only be case.
+    final libraryPathLower = libraryPath.toLowerCase();
+    return !_adapter.projectPaths.any((projectPath) =>
+        path.isWithin(projectPath.toLowerCase(), libraryPathLower));
+  }
 
   bool _isSdkLibrary(vm.LibraryRef library) =>
       library.uri?.startsWith('dart:') ?? false;

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

@@ -6,6 +6,7 @@ import 'dart:async';
 import 'dart:io';
 
 import 'package:collection/collection.dart';
+import 'package:package_config/package_config_types.dart';
 import 'package:path/path.dart' as path;
 import 'package:vm_service/vm_service.dart' as vm;
 
@@ -25,6 +26,11 @@ class ProtocolConverter {
   /// the debug session.
   final DartDebugAdapter _adapter;
 
+  /// Temporary PackageConfig used for resolving package: URIs.
+  /// TODO(dantup): Replace this implementation with one that calls the VM
+  ///   Service once https://github.com/dart-lang/sdk/issues/45530 is done.
+  PackageConfig packageConfig = PackageConfig.empty;
+
   ProtocolConverter(this._adapter);
 
   /// Converts an absolute path to one relative to the cwd used to launch the
@@ -325,14 +331,16 @@ class ProtocolConverter {
 
     final scriptRef = location.script;
     final tokenPos = location.tokenPos;
-    final uri = scriptRef?.uri;
+    final scriptRefUri = scriptRef?.uri;
+    final uri = scriptRefUri != null ? Uri.parse(scriptRefUri) : null;
+    final uriIsPackage = uri?.isScheme('package') ?? false;
     final sourcePath = uri != null ? await convertVmUriToSourcePath(uri) : null;
     var canShowSource = sourcePath != null && File(sourcePath).existsSync();
 
     // Download the source if from a "dart:" uri.
     int? sourceReference;
     if (uri != null &&
-        (uri.startsWith('dart:') || uri.startsWith('org-dartlang-app:')) &&
+        (uri.isScheme('dart') || uri.isScheme('org-dartlang-app')) &&
         scriptRef != null) {
       sourceReference = thread.storeData(scriptRef);
       canShowSource = true;
@@ -351,7 +359,11 @@ class ProtocolConverter {
 
     final source = canShowSource
         ? dap.Source(
-            name: sourcePath != null ? convertToRelativePath(sourcePath) : uri,
+            name: uriIsPackage
+                ? uri!.toString()
+                : sourcePath != null
+                    ? convertToRelativePath(sourcePath)
+                    : uri?.toString() ?? '<unknown source>',
             path: sourcePath,
             sourceReference: sourceReference,
             origin: null,
@@ -374,18 +386,17 @@ class ProtocolConverter {
     );
   }
 
-  /// Converts the source path from the VM to a file path.
+  /// Converts the source URI from the VM to a file path.
   ///
   /// This is required so that when the user stops (or navigates via a stack
   /// frame) we open the same file on their local disk. If we downloaded the
   /// source from the VM, they would end up seeing two copies of files (and they
   /// would each have their own breakpoints) which can be confusing.
-  Future<String?> convertVmUriToSourcePath(String uri) async {
-    if (uri.startsWith('file://')) {
-      return Uri.parse(uri).toFilePath();
-    } else if (uri.startsWith('package:')) {
-      // TODO(dantup): Handle mapping package: uris ?
-      return null;
+  Future<String?> convertVmUriToSourcePath(Uri uri) async {
+    if (uri.isScheme('file')) {
+      return uri.toFilePath();
+    } else if (uri.isScheme('package')) {
+      return resolvePackageUri(uri);
     } else {
       return null;
     }
@@ -402,6 +413,19 @@ class ProtocolConverter {
         kind == 'Closure';
   }
 
+  /// Resolves a `package: URI` to the real underlying source path.
+  ///
+  /// Returns `null` if no mapping was possible, for example if the package is
+  /// not in the package mapping file.
+  String? resolvePackageUri(Uri uri) {
+    // TODO(dantup): Replace this implementation with one that calls the VM
+    //   Service once https://github.com/dart-lang/sdk/issues/45530 is done.
+    // This implementation makes assumptions about the package file being used
+    // that might not be correct (for example if the user uses the --packages
+    // flag).
+    return packageConfig.resolve(uri)?.toFilePath();
+  }
+
   /// Invokes the toString() method on a [vm.InstanceRef] and converts the
   /// response to a user-friendly display string.
   ///

pkg/dds/pubspec.yaml

@@ -16,6 +16,7 @@ dependencies:
   devtools_shared: ^2.3.0
   json_rpc_2: ^3.0.0
   meta: ^1.1.8
+  package_config: ^2.0.0
   path: ^1.8.0
   pedantic: ^1.7.0
   shelf: ^1.0.0

pkg/dds/test/dap/integration/debug_breakpoints_test.dart

@@ -200,15 +200,103 @@ void main(List<String> args) async {
 
     test(
         'does not step into external package code with debugExternalPackageLibraries=false',
-        () {
-      // TODO(dantup): Support for debugExternalPackageLibraries
-    }, skip: true);
+        () async {
+      final client = dap.client;
+      final otherPackageUri = await dap.createFooPackage();
+      final testFile = dap.createTestFile('''
+import '$otherPackageUri';
+
+void main(List<String> args) async {
+  foo(); // BREAKPOINT
+  foo(); // STEP
+}
+    ''');
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+      final stepLine = lineWith(testFile, '// STEP');
+
+      // Hit the initial breakpoint.
+      final stop = await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        launch: () => client.launch(
+          testFile.path,
+          debugExternalPackageLibraries: false,
+        ),
+      );
+
+      // Step in and expect stopping on the next line (don't go into the package).
+      await Future.wait([
+        client.expectStop('step', file: testFile, line: stepLine),
+        client.stepIn(stop.threadId!),
+      ], eagerError: true);
+    });
 
     test(
         'steps into external package code with debugExternalPackageLibraries=true',
-        () {
-      // TODO(dantup): Support for debugExternalPackageLibraries
-    }, skip: true);
+        () async {
+      final client = dap.client;
+      final otherPackageUri = await dap.createFooPackage();
+      final testFile = dap.createTestFile('''
+import '$otherPackageUri';
+
+void main(List<String> args) async {
+  foo(); // BREAKPOINT
+  foo();
+}
+    ''');
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      // Hit the initial breakpoint.
+      final stop = await dap.client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        launch: () => client.launch(
+          testFile.path,
+          debugExternalPackageLibraries: true,
+        ),
+      );
+
+      // Step in and expect to go into the package.
+      await Future.wait([
+        client.expectStop('step', sourceName: '$otherPackageUri'),
+        client.stepIn(stop.threadId!),
+      ], eagerError: true);
+    });
+
+    test(
+        'steps into other-project package code with debugExternalPackageLibraries=false',
+        () async {
+      final client = dap.client;
+      final otherPackageUri = await dap.createFooPackage();
+      final testFile = dap.createTestFile('''
+import '$otherPackageUri';
+
+void main(List<String> args) async {
+  foo(); // BREAKPOINT
+  foo();
+}
+    ''');
+      final breakpointLine = lineWith(testFile, '// BREAKPOINT');
+
+      // Hit the initial breakpoint.
+      final stop = await client.hitBreakpoint(
+        testFile,
+        breakpointLine,
+        launch: () => client.launch(
+          testFile.path,
+          debugExternalPackageLibraries: false,
+          // Include the packages folder as an additional project path so that
+          // it will be treated as local code.
+          additionalProjectPaths: [dap.testPackageDir.path],
+        ),
+      );
+
+      // Step in and expect stopping in the the other package.
+      await Future.wait([
+        client.expectStop('step', sourceName: '$otherPackageUri'),
+        client.stepIn(stop.threadId!),
+      ], eagerError: true);
+    });
 
     test('allows changing debug settings during session', () async {
       final client = dap.client;

pkg/dds/test/dap/integration/test_client.dart

@@ -121,6 +121,7 @@ class DapTestClient {
     List<String>? args,
     String? cwd,
     bool? noDebug,
+    List<String>? additionalProjectPaths,
     bool? debugSdkLibraries,
     bool? debugExternalPackageLibraries,
     bool? evaluateGettersInDebugViews,
@@ -132,6 +133,7 @@ class DapTestClient {
         program: program,
         cwd: cwd,
         args: args,
+        additionalProjectPaths: additionalProjectPaths,
         debugSdkLibraries: debugSdkLibraries,
         debugExternalPackageLibraries: debugExternalPackageLibraries,
         evaluateGettersInDebugViews: evaluateGettersInDebugViews,
@@ -388,8 +390,12 @@ extension DapTestClientExtension on DapTestClient {
   ///
   /// If [file] or [line] are provided, they will be checked against the stop
   /// location for the top stack frame.
-  Future<StoppedEventBody> expectStop(String reason,
-      {File? file, int? line, String? sourceName}) async {
+  Future<StoppedEventBody> expectStop(
+    String reason, {
+    File? file,
+    int? line,
+    String? sourceName,
+  }) async {
     final e = await event('stopped');
     final stop = StoppedEventBody.fromJson(e.body as Map<String, Object?>);
     expect(stop.reason, equals(reason));
@@ -400,10 +406,10 @@ extension DapTestClientExtension on DapTestClient {
     final frame = result.stackFrames[0];
 
     if (file != null) {
-      expect(frame.source!.path, equals(file.path));
+      expect(frame.source?.path, equals(file.path));
     }
     if (sourceName != null) {
-      expect(frame.source!.name, equals(sourceName));
+      expect(frame.source?.name, equals(sourceName));
     }
     if (line != null) {
       expect(frame.line, equals(line));

pkg/dds/test/dap/integration/test_server.dart

@@ -31,8 +31,15 @@ class InProcessDapTestServer extends DapTestServer {
   StreamSink<List<int>> get sink => stdinController.sink;
   Stream<List<int>> get stream => stdoutController.stream;
 
-  InProcessDapTestServer._() {
-    _server = DapServer(stdinController.stream, stdoutController.sink);
+  InProcessDapTestServer._(List<String> args) {
+    _server = DapServer(
+      stdinController.stream,
+      stdoutController.sink,
+      // Simulate flags based on the args to aid testing.
+      enableDds: !args.contains('--no-dds'),
+      ipv6: args.contains('--ipv6'),
+      enableAuthCodes: !args.contains('--no-auth-codes'),
+    );
   }
 
   @override
@@ -44,7 +51,10 @@ class InProcessDapTestServer extends DapTestServer {
     Logger? logger,
     List<String>? additionalArgs,
   }) async {
-    return InProcessDapTestServer._();
+    return InProcessDapTestServer._([
+      ...?additionalArgs,
+      if (logger != null) '--verbose',
+    ]);
   }
 }
 

pkg/dds/test/dap/integration/test_support.dart

@@ -6,6 +6,7 @@ import 'dart:async';
 import 'dart:io';
 
 import 'package:dds/src/dap/logging.dart';
+import 'package:package_config/package_config.dart';
 import 'package:path/path.dart' as path;
 import 'package:test/test.dart';
 
@@ -43,35 +44,81 @@ int lineWith(File file, String searchText) =>
 class DapTestSession {
   DapTestServer server;
   DapTestClient client;
-  final _testFolders = <Directory>[];
+  final Directory _testDir =
+      Directory.systemTemp.createTempSync('dart-sdk-dap-test');
+  late final Directory testAppDir;
+  late final Directory testPackageDir;
+  var _packageConfig = PackageConfig.empty;
 
-  DapTestSession._(this.server, this.client);
+  DapTestSession._(this.server, this.client) {
+    testAppDir = _testDir.createTempSync('app');
+    testPackageDir = _testDir.createTempSync('packages');
+  }
+
+  /// Create a simple package named `foo` that has an empty `foo` function.
+  Future<Uri> createFooPackage() {
+    return createSimplePackage(
+      'foo',
+      '''
+foo() {
+  // Does nothing.
+}
+      ''',
+    );
+  }
+
+  /// Creates a simple package script and adds the package to
+  /// .dart_tool/package_config.json
+  Future<Uri> createSimplePackage(
+    String name,
+    String content,
+  ) async {
+    final dartToolDirectory =
+        Directory(path.join(testAppDir.path, '.dart_tool'))..createSync();
+    final packageConfigJsonFile =
+        File(path.join(dartToolDirectory.path, 'package_config.json'));
+    final packageConfigJsonUri = Uri.file(packageConfigJsonFile.path);
+
+    // Write the packages Dart implementation file.
+    final testPackageDirectory = Directory(path.join(testPackageDir.path, name))
+      ..createSync(recursive: true);
+    final testFile = File(path.join(testPackageDirectory.path, '$name.dart'));
+    testFile.writeAsStringSync(content);
+
+    // Add this new package to the PackageConfig.
+    final newPackage = Package(name, Uri.file('${testPackageDirectory.path}/'));
+    _packageConfig = PackageConfig([..._packageConfig.packages, newPackage]);
+
+    // Write the PackageConfig to disk.
+    final sink = packageConfigJsonFile.openWrite();
+    PackageConfig.writeString(_packageConfig, sink, packageConfigJsonUri);
+    await sink.close();
+
+    return Uri.parse('package:$name/$name.dart');
+  }
 
   /// Creates a file in a temporary folder to be used as an application for testing.
   ///
   /// The file will be deleted at the end of the test run.
   File createTestFile(String content) {
-    final testAppDir = Directory.systemTemp.createTempSync('dart-sdk-dap-test');
-    _testFolders.add(testAppDir);
     final testFile = File(path.join(testAppDir.path, 'test_file.dart'));
     testFile.writeAsStringSync(content);
     return testFile;
   }
 
-  static Future<DapTestSession> setUp({List<String>? additionalArgs}) async {
-    final server = await _startServer(additionalArgs: additionalArgs);
-    final client = await DapTestClient.connect(server);
-    return DapTestSession._(server, client);
-  }
-
   Future<void> tearDown() async {
     await client.stop();
     await server.stop();
 
     // Clean up any temp folders created during the test runs.
-    _testFolders
-      ..forEach((dir) => dir.deleteSync(recursive: true))
-      ..clear();
+    _testDir.deleteSync(recursive: true);
+  }
+
+  static Future<DapTestSession> setUp({List<String>? additionalArgs}) async {
+    final server = await _startServer(additionalArgs: additionalArgs);
+    final client =
+        await DapTestClient.connect(server, captureVmServiceTraffic: true);
+    return DapTestSession._(server, client);
   }
 
   /// Starts a DAP server that can be shared across tests.

pkg/dds/tool/dap/run_server.dart

@@ -41,6 +41,7 @@ class DapCommand extends Command {
     argParser
       ..addFlag(
         argIpv6,
+        defaultsTo: false,
         help: 'Whether to bind DAP/VM Service/DDS to IPv6 addresses',
       )
       ..addFlag(