[ddc] Remove lib/ directories from source maps

Given the existing assumptions that original source files will be
served in a directory structure similar to layout defined by the
package_config.json but without `lib/` directories this change
attempts to detect when a source map contains sources from multiple
lib directories and clean them up from the relative paths.

The strategy used relies on passing a package_config.json as a signal
to detect when sources come from different packages.

Fixes: dart-lang/webdev#1692
Issue: #40251

Change-Id: I6b82b33bae485d74fc61ef118dbe9ffddafab9a7
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/456500
Reviewed-by: Sigmund Cherem <[email protected]>
Commit-Queue: Nicholas Shahan <[email protected]>

Author: nshahan

pkg/dev_compiler/lib/src/command/command.dart

@@ -19,6 +19,7 @@ import 'package:kernel/kernel.dart';
 import 'package:kernel/target/targets.dart';
 import 'package:kernel/text/ast_to_text.dart' as kernel show Printer;
 import 'package:kernel/text/debug_printer.dart';
+import 'package:package_config/package_config.dart';
 import 'package:path/path.dart' as p;
 import 'package:source_maps/source_maps.dart' show SourceMapBuilder;
 
@@ -786,7 +787,7 @@ class JSCode {
 /// Converts [moduleTree] to [JSCode], using [format].
 ///
 /// See [placeSourceMap] for a description of [sourceMapBase], [customScheme],
-/// and [multiRootOutputPath] arguments.
+/// [multiRootOutputPath] and [packageConfig] arguments.
 JSCode jsProgramToCode(
   js_ast.Program moduleTree,
   ModuleFormat format, {
@@ -802,6 +803,7 @@ JSCode jsProgramToCode(
   String? multiRootOutputPath,
   Compiler? compiler,
   Component? component,
+  PackageConfig? packageConfig,
 }) {
   var opts = js_ast.JavaScriptPrintingOptions(
     allowKeywordsInProperties: true,
@@ -835,6 +837,7 @@ JSCode jsProgramToCode(
       customScheme,
       multiRootOutputPath: multiRootOutputPath,
       sourceMapBase: sourceMapBase,
+      packageConfig: packageConfig,
     );
     var jsDir = p.dirname(p.fromUri(jsUrl));
     var relative = p.relative(p.fromUri(mapUrl), from: jsDir);
@@ -1102,6 +1105,17 @@ Uri sourcePathToRelativeUri(String source, {bool? windows}) {
   return uri;
 }
 
+/// Pattern to match when a relative path appears to leave the `lib/` directory
+/// of one package to enter the `lib/` directory of another package.
+final _crossPackageLib = RegExp(
+  // Two or more '../'.
+  r'\.\.\/(?:\.\.\/)+'
+  // The package name (captured).
+  r'([^\/]*)'
+  // The `/lib/` directory.
+  r'\/lib\/',
+);
+
 /// Adjusts the source uris in [sourceMap] to be relative uris, and returns
 /// the new map.
 ///
@@ -1117,6 +1131,11 @@ Uri sourcePathToRelativeUri(String source, {bool? windows}) {
 ///   to the [multiRootOutputPath], and assert that [multiRootOutputPath]
 ///   starts with `/packages` (more explanation inline).
 ///
+/// Passing a [packageConfig] will enable the rewriting of original source paths
+/// when the sourcemap contains files from `lib/` directories across multiple
+/// packages. The rewriting assumes that the sources will be served in a package
+/// directory structure without `lib/` directories.
+///
 // TODO(#40251): Remove this logic from dev_compiler itself, push it to the
 // invokers of dev_compiler which have more knowledge about how they want
 // source paths to look.
@@ -1126,12 +1145,14 @@ Map<String, Object?> placeSourceMap(
   String? multiRootScheme, {
   String? multiRootOutputPath,
   String? sourceMapBase,
+  PackageConfig? packageConfig,
 }) {
   var map = Map.of(sourceMap);
   // Convert to a local file path if it's not.
   sourceMapPath = sourcePathToUri(p.absolute(p.fromUri(sourceMapPath))).path;
   var sourceMapDir = p.url.dirname(sourceMapPath);
   sourceMapBase ??= sourceMapDir;
+  var basePackageName = packageConfig?.packageOf(p.toUri(sourceMapBase))?.name;
   var list = (map['sources'] as List).toList();
 
   String makeRelative(String sourcePath) {
@@ -1161,7 +1182,31 @@ Map<String, Object?> placeSourceMap(
     sourcePath = p.url.relative(sourcePath, from: sourceMapBase);
 
     // Convert from relative local path to relative URI.
-    return p.toUri(sourcePath).path;
+    var relativeUriPath = p.toUri(sourcePath).path;
+    // Handle source paths when the original sources come from the `lib/`
+    // directories from different packages.
+    //
+    // This assumes that the original source files and source map will be hosted
+    // in a packages directory structure with all of the `lib/` directories
+    // removed.
+    // For reference:
+    //  - https://github.com/dart-lang/sdk/issues/40251
+    //  - https://github.com/dart-lang/webdev/issues/1692
+    if (basePackageName != null && uri.isScheme('file')) {
+      var sourcePackage = packageConfig?.packageOf(uri);
+      if (sourcePackage != null && basePackageName != sourcePackage.name) {
+        // The source location is part of a different package.
+        var match = _crossPackageLib.matchAsPrefix(relativeUriPath);
+        if (match != null) {
+          var crossPackageName = match.group(1);
+          return relativeUriPath.replaceFirst(
+            '../../$crossPackageName/lib/',
+            '../$crossPackageName/',
+          );
+        }
+      }
+    }
+    return relativeUriPath;
   }
 
   for (var i = 0; i < list.length; i++) {

pkg/dev_compiler/pubspec.yaml

@@ -19,6 +19,7 @@ dependencies:
   front_end: any
   js_shared: any
   kernel: any
+  package_config: any
   path: any
   shell_arg_splitter: any
   source_maps: any

pkg/frontend_server/lib/src/javascript_bundle.dart

@@ -351,6 +351,7 @@ class IncrementalJavaScriptBundler {
         customScheme: _fileSystemScheme,
         compiler: compiler,
         component: summaryComponent,
+        packageConfig: packageConfig,
       );
       final Uint8List codeBytes = utf8.encode(code.code);
       final Uint8List sourceMapBytes = utf8.encode(json.encode(code.sourceMap));

pkg/frontend_server/test/frontend_server_test.dart

@@ -2684,6 +2684,104 @@ e() {
         await runTests(moduleFormat: 'ddc', canary: true);
       });
     });
+    group(
+        'sourcemaps with cross package sources do not reference `lib/` '
+        'directories', () {
+      Future<void> runTests(
+          {required String moduleFormat, bool canary = false}) async {
+        // Create two packages in a dependency cycle.
+        final String packagesDir = '${tempDir.path}/packages';
+        final File packageConfig =
+            new File('${tempDir.path}/.dart_tool/package_config.json')
+              ..createSync(recursive: true)
+              ..writeAsStringSync('''
+    {
+      "configVersion": 2,
+      "packages": [
+        {
+          "name": "a",
+          "rootUri": "$packagesDir/a",
+          "packageUri": "lib/"
+        },
+        {
+          "name": "b",
+          "rootUri": "$packagesDir/b",
+          "packageUri": "lib/"
+        }
+      ]
+    }
+    ''');
+        new File('$packagesDir/a/lib/a.dart')
+          ..createSync(recursive: true)
+          ..writeAsStringSync("import 'package:b/b.dart';\n"
+              "String a1() => 'a1';\n"
+              "String a2() => 'a2' + b2();\n");
+        new File('$packagesDir/b/lib/b.dart')
+          ..createSync(recursive: true)
+          ..writeAsStringSync("import 'package:a/a.dart';\n"
+              "String b1() => 'b1' + a1();\n"
+              "String b2() => 'b2';\n");
+        // Compile the cycle as a single output.
+        final String library = 'package:a/a.dart';
+        final File dillFile = new File('${tempDir.path}/a.dill');
+        final File manifestFile = new File('${dillFile.path}.json');
+        final File sourceMapsFile = new File('${dillFile.path}.map');
+        final List<String> args = <String>[
+          '--sdk-root=${sdkRoot.toFilePath()}',
+          '--incremental',
+          '--platform=${ddcPlatformKernel.path}',
+          '--output-dill=${dillFile.path}',
+          '--target=dartdevc',
+          '--dartdevc-module-format=$moduleFormat',
+          if (canary) '--dartdevc-canary',
+          '--packages=${packageConfig.path}',
+        ];
+        final FrontendServer frontendServer = new FrontendServer();
+        final Future<int> result = frontendServer.open(args);
+        frontendServer.compile(library);
+        final Completer<bool> expectationCompleter = new Completer<bool>();
+        frontendServer.listen((Result compiledResult) {
+          final CompilationResult result =
+              new CompilationResult.parse(compiledResult.status);
+          expect(result.errorsCount, 0);
+          expect(result.filename, dillFile.path);
+          expect(manifestFile.existsSync(), true);
+          // Extract the sourcemap.
+          final Map<String, dynamic> manifest =
+              json.decode(utf8.decode(manifestFile.readAsBytesSync()));
+          final List<dynamic> offsets =
+              manifest['/packages/a/a.dart.lib.js']['sourcemap'];
+          final Map<String, dynamic> sourcemap = json.decode(utf8.decode(
+              sourceMapsFile
+                  .readAsBytesSync()
+                  .sublist(offsets.first, offsets.last)));
+          // Verify source maps are pointing at correct source files.
+          //
+          // There is a built in assumption that the original sources for all
+          // packages are served in a structure similar to the definition from
+          // the package_config.json file but without the `lib/` directories.
+          final List<dynamic> sources = sourcemap['sources'];
+          expect(sources.length, 2);
+          expect(sources, containsAll(['a.dart', '../b/b.dart']));
+
+          frontendServer.accept();
+          frontendServer.quit();
+          expectationCompleter.complete(true);
+        });
+
+        await expectationCompleter.future;
+        expect(await result, 0);
+        frontendServer.close();
+      }
+
+      test('AMD module format', () async {
+        await runTests(moduleFormat: 'amd');
+      });
+
+      test('DDC module format and canary', () async {
+        await runTests(moduleFormat: 'ddc', canary: true);
+      });
+    });
 
     group('compile expression to JavaScript', () {
       Future<void> runTests(