[dds][dap] Fix global evaluation inside package: files

The initial global evaluation support only worked when the open script was loaded into the VM as a `file:///` URI (eg. things in a `bin/` folder, and not a `lib/` folder). This is because we short-cut resolving file paths to `package:` URIs for most VM functionality (for example setting breakpoints) because it's unnecessary (the VM supports setting breakpoints with file:/// URIs even for `lib/`).

This change forces us to resolve paths to their resolved URIs (eg. `package:` where applicable) in the case where we're looking up a script for global evaluation.

We could enable this lookup for all cases (to remove the additional `force` flag here), but since it's called much more from the other path (breakpoints) we should add caching (which was more than I wanted to change for this fix).

Fixes https://github.com/Dart-Code/Dart-Code/issues/4932

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

pkg/dds/CHANGELOG.md

@@ -1,3 +1,6 @@
+# 3.1.3-unreleased
+- [DAP] Fixed "Unable to find library" errors when using global evaluation when the context file resolves to a `package:` URI.
+
 # 3.1.2
 - Improved error handling for serving static DevTools assets.
 - Updated `devtools_shared` constraint to ^6.0.3.

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

@@ -1088,22 +1088,46 @@ class ThreadInfo with FileUtils {
   /// This is required so that when the user sets a breakpoint in an SDK source
   /// (which they may have navigated to via the Analysis Server) we generate a
   /// valid URI that the VM would create a breakpoint for.
-  Future<Uri?> resolvePathToUri(String filePath) async {
-    var google3Path = _convertPathToGoogle3Uri(filePath);
-    if (google3Path != null) {
-      var result = await _manager._adapter.vmService
-          ?.lookupPackageUris(isolate.id!, [google3Path.toString()]);
-      var uriStr = result?.uris?.first;
-      return uriStr != null ? Uri.parse(uriStr) : null;
+  ///
+  /// Because the VM supports using `file:` URIs in many places, we usually do
+  /// not need to convert file paths into `package:` URIs, however this will
+  /// be done if [forceResolveFileUris] is `true`.
+  Future<Uri?> resolvePathToUri(
+    String filePath, {
+    bool forceResolveFileUris = false,
+  }) async {
+    final sdkUri = _manager._adapter.convertPathToOrgDartlangSdk(filePath);
+    if (sdkUri != null) {
+      return sdkUri;
     }
 
-    // We don't need to call lookupPackageUris in non-google3 because the VM can
-    // handle incoming file:/// URIs for packages, and also the org-dartlang-sdk
-    // URIs directly for SDK sources (we do not need to convert to 'dart:'),
-    // however this method is Future-returning in case this changes in future
-    // and we need to include a call to lookupPackageUris here.
-    return _manager._adapter.convertPathToOrgDartlangSdk(filePath) ??
-        Uri.file(filePath);
+    final google3Uri = _convertPathToGoogle3Uri(filePath);
+    final uri = google3Uri ?? Uri.file(filePath);
+
+    // As an optimisation, we don't resolve file -> package URIs in many cases
+    // because the VM can set breakpoints for file: URIs anyway. However for
+    // G3 or if [forceResolveFileUris] is set, we will.
+    final performResolve = google3Uri != null || forceResolveFileUris;
+
+    // TODO(dantup): Consider caching results for this like we do for
+    //  resolveUriToPath (and then forceResolveFileUris can be removed and just
+    //  always used).
+    final packageUriList = performResolve
+        ? await _manager._adapter.vmService
+            ?.lookupPackageUris(isolate.id!, [uri.toString()])
+        : null;
+    final packageUriString = packageUriList?.uris?.firstOrNull;
+
+    if (packageUriString != null) {
+      // Use package URI if we resolved something
+      return Uri.parse(packageUriString);
+    } else if (google3Uri != null) {
+      // If we failed to resolve and was a Google3 URI, return null
+      return null;
+    } else {
+      // Otherwise, use the original (file) URI
+      return uri;
+    }
   }
 
   /// Batch resolves source URIs from the VM to a file path for the package lib
@@ -1359,8 +1383,10 @@ class ThreadInfo with FileUtils {
     //
     // We need to handle msimatched drive letters, and also file vs package
     // URIs.
-    final scriptResolvedUri =
-        await resolvePathToUri(scriptFileUri.toFilePath());
+    final scriptResolvedUri = await resolvePathToUri(
+      scriptFileUri.toFilePath(),
+      forceResolveFileUris: true,
+    );
     final candidateUris = {
       scriptFileUri.toString(),
       normalizeUri(scriptFileUri).toString(),

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

@@ -458,7 +458,7 @@ void main(List<String> args) async {
         'does not step into external package code with debugExternalPackageLibraries=false',
         () async {
       final client = dap.client;
-      final otherPackageUri = await dap.createFooPackage();
+      final (otherPackageUri, _) = await dap.createFooPackage();
       final testFile = dap.createTestFile('''
 import '$otherPackageUri';
 
@@ -491,7 +491,7 @@ void main(List<String> args) async {
         'steps into external package code with debugExternalPackageLibraries=true',
         () async {
       final client = dap.client;
-      final otherPackageUri = await dap.createFooPackage();
+      final (otherPackageUri, _) = await dap.createFooPackage();
       final testFile = dap.createTestFile('''
 import '$otherPackageUri';
 
@@ -523,7 +523,7 @@ void main(List<String> args) async {
         'steps into other-project package code with debugExternalPackageLibraries=false',
         () async {
       final client = dap.client;
-      final otherPackageUri = await dap.createFooPackage();
+      final (otherPackageUri, _) = await dap.createFooPackage();
       final testFile = dap.createTestFile('''
 import '$otherPackageUri';
 

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

@@ -285,8 +285,10 @@ void foo() {
     });
 
     group('global evaluation', () {
-      test('can evaluate when not paused given a script URI', () async {
+      test('can evaluate in a bin/ file when not paused given a bin/ URI',
+          () async {
         final client = dap.client;
+        await dap.createFooPackage();
         final testFile = dap.createTestFile(globalEvaluationProgram);
 
         await Future.wait([
@@ -305,12 +307,34 @@ void foo() {
         );
       });
 
+      test('can evaluate when not paused given a lib/ URI', () async {
+        final client = dap.client;
+        final (_, libFile) = await dap.createFooPackage();
+        final binFile = dap.createTestFile(globalEvaluationProgram);
+
+        await Future.wait([
+          client.initialize(),
+          client.launch(binFile.path),
+        ], eagerError: true);
+
+        // Wait for a '.' to be printed to know the script is up and running.
+        await dap.client.outputEvents
+            .firstWhere((event) => event.output.trim() == '.');
+
+        await client.expectGlobalEvalResult(
+          'fooGlobal',
+          '"Hello, foo!"',
+          context: Uri.file(libFile.path).toString(),
+        );
+      });
+
       test('returns a suitable error with no context', () async {
         const expectedErrorMessage = 'Evaluation is only supported when the '
             'debugger is paused unless you have a Dart file active in the '
             'editor';
 
         final client = dap.client;
+        await dap.createFooPackage();
         final testFile = dap.createTestFile(globalEvaluationProgram);
 
         await Future.wait([
@@ -343,6 +367,7 @@ void foo() {
 
       test('returns a suitable error with an unknown script context', () async {
         final client = dap.client;
+        await dap.createFooPackage();
         final testFile = dap.createTestFile(globalEvaluationProgram);
 
         await Future.wait([

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

@@ -99,7 +99,7 @@ main() {
     });
 
     test('runs a simple script with commas in the filename', () async {
-      final packageUri = await dap.createFooPackage('foo,foo.dart');
+      final (packageUri, _) = await dap.createFooPackage('foo,foo.dart');
       final testFile = dap.createTestFile(
         '''
           import '$packageUri';
@@ -167,7 +167,7 @@ main() {
       // - stack frames with package URIs (that need asynchronously resolving)
       // - stack frames with dart URIs (that need asynchronously resolving)
       final fileUri = Uri.file(dap.createTestFile('').path);
-      final packageUri = await dap.createFooPackage();
+      final (packageUri, _) = await dap.createFooPackage();
       final dartUri = Uri.parse('dart:isolate-patch/isolate_patch.dart');
       final testFile = dap.createTestFile(
         stderrPrintingProgram(fileUri, packageUri, dartUri),
@@ -212,7 +212,7 @@ main() {
       // - stack frames with package URIs (that need asynchronously resolving)
       // - stack frames with dart URIs (that need asynchronously resolving)
       final fileUri = Uri.file(dap.createTestFile('').path);
-      final packageUri = await dap.createFooPackage();
+      final (packageUri, _) = await dap.createFooPackage();
       final dartUri = Uri.parse('dart:isolate-patch/isolate_patch.dart');
       final testFile = dap.createTestFile(
         stderrPrintingProgram(fileUri, packageUri, dartUri),

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

@@ -111,10 +111,15 @@ const infiniteRunningProgram = '''
 ///
 /// A top-level String variable `myGlobal` is available with the value
 /// `"Hello, world!"`.
+///
+/// Requires the 'foo' package.
 const globalEvaluationProgram = '''
+  import 'package:foo/foo.dart';
+
   var myGlobal = 'Hello, world!';
   void main(List<String> args) async {
     while (true) {
+      foo();
       print('.');
       await Future.delayed(const Duration(seconds: 1));
     }

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

@@ -174,11 +174,14 @@ class DapTestSession {
     );
   }
 
-  /// Create a simple package named `foo` that has an empty `foo` function.
-  Future<Uri> createFooPackage([String? filename]) {
+  /// Create a simple package named `foo` that has an empty `foo` function and
+  /// a top-level variable `fooGlobal`.
+  Future<(Uri, File)> createFooPackage([String? filename]) {
     return createSimplePackage(
       'foo',
       '''
+var fooGlobal = 'Hello, foo!';
+
 foo() {
   // Does nothing.
 }
@@ -202,7 +205,7 @@ environment:
 
   /// Creates a simple package script and adds the package to
   /// .dart_tool/package_config.json
-  Future<Uri> createSimplePackage(
+  Future<(Uri, File)> createSimplePackage(
     String name,
     String content, [
     String? filename,
@@ -222,7 +225,7 @@ environment:
     final fileUri = Uri.file('${packageDir.path}/');
     await addPackageDependency(testAppDir, name, fileUri);
 
-    return Uri.parse('package:$name/$filename');
+    return (Uri.parse('package:$name/$filename'), testFile);
   }
 
   /// Creates a file in a temporary folder to be used as an application for testing.