[dartj2s] (tech-debt) better error locations for sources from dill files.

This change addresses a technical debt issue in dart2js, where
errors sometimes get reported as a binary offset location, instead
of a line/column position with contextual data from the source file.

The reason for this behavior is that dartj2s reports binary offsets
when it can't recover line/column data and show the contents of the
file. This typically happens if the file cannot be read directly
from disk.

The fix consits of adding an API to record the contents of files
that are not read directly by the compiler. We then use this API to
provide all sources collected by the CFE when parsing components
from .dill files.

Note: this CL also deletes the `autoread` feature in
source-file-provider. That feature was added for the same purpose.
The way it worked was that instead of registering source content, it
tried to read the sources from disk on-demand as errors got
reported. This doesn't always work for three reasons:

* First, we often use custom schemes (like the multi-root scheme) in
  dill files to make the .dill deterministic in distributed build
  systems, the autoload feature had no understanding of how to
  translate those custom URIs to file URIs (that translation is only
  define at the time the .dill is being built).

* Second, sometimes the files are simply not available, for example,
  in hermetic build systems like bazel we have no access to those
  files.

* Third, when files were found, there was no guarantee that the
  contents were consistent. That is, the current version of the file
  on disk could have been modified and have different contents than
  those used when the .dill file was built.

I tested this manually by adding a crash in SSA and observing the
location in multiple scenarios, including running from source,
running from a .dill with file URIs, running from a .dill with
multi-root URIs, crashing in SDK locations in regular files and
patch files.

Change-Id: Ief09be577f4c9c4b345b4e2641918cafbe93c3fc
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/251700
Reviewed-by: Mayank Patke <[email protected]>
Commit-Queue: Sigmund Cherem <[email protected]>

Author: sigmundch

pkg/compiler/lib/compiler_api.dart

@@ -108,6 +108,20 @@ abstract class CompilerInput {
   /// is expected to hold a zero element at the last position. If this is not
   /// the case, the entire data structure is copied before scanning.
   Future<Input> readFromUri(Uri uri, {InputKind inputKind = InputKind.UTF8});
+
+  /// Register that [uri] should be an `InputKind.UTF8` input with the
+  /// given [source] as its zero-terminated list of contents.
+  ///
+  /// If [uri] was read prior to this call, this registration has no effect,
+  /// otherwise it is expected that a future [readFromUri] will return the
+  /// contents provided here.
+  ///
+  /// The main purpose of this API is to assist in error reporting when
+  /// compiling from kernel binary files. Binary files embed the contents
+  /// of source files that may not be available on disk. By using these
+  /// registered contents, dart2js will be able to provide accurate line/column
+  /// information on an error.
+  void registerUtf8ContentsForDiagnostics(Uri uri, List<int> source);
 }
 
 /// Output types used in `CompilerOutput.createOutputSink`.

pkg/compiler/lib/src/dart2js.dart

@@ -1085,7 +1085,6 @@ Future<api.CompilationResult> compile(List<String> argv,
     return result;
   }
 
-  diagnosticHandler.autoReadFileUri = true;
   CompilerOptions compilerOptions = CompilerOptions.parse(options,
       featureOptions: features,
       librariesSpecificationUri: librariesSpecificationUri,

pkg/compiler/lib/src/phase/load_kernel.dart

@@ -196,6 +196,8 @@ Future<_LoadFromKernelResult> _loadFromKernel(CompilerOptions options,
   if (options.cfeOnly) {
     _doGlobalTransforms(component);
   }
+
+  registerSources(component, compilerInput);
   return _LoadFromKernelResult(component, entryLibrary, moduleLibraries);
 }
 
@@ -287,6 +289,7 @@ Future<_LoadFromSourceResult> _loadFromSource(
   if (isModularCompile) {
     component?.computeCanonicalNames();
   }
+  registerSources(component, compilerInput);
   return _LoadFromSourceResult(
       component, initializedCompilerState, isModularCompile ? sources : []);
 }
@@ -394,3 +397,16 @@ Future<Output?> run(Input input) async {
   return _createOutput(options, reporter, entryLibrary, component,
       moduleLibraries, initializedCompilerState);
 }
+
+/// Registers with the dart2js compiler all sources embeded in a kernel
+/// component. This may include sources that were read from disk directly as
+/// files, but also sources that were embedded in binary `.dill` files (like the
+/// platform kernel file and kernel files from modular compilation pipelines).
+///
+/// This registration improves how locations are presented when errors
+/// or crashes are reported by the dart2js compiler.
+void registerSources(ir.Component? component, api.CompilerInput compilerInput) {
+  component?.uriToSource.forEach((uri, source) {
+    compilerInput.registerUtf8ContentsForDiagnostics(uri, source.source);
+  });
+}

pkg/compiler/lib/src/source_file_provider.dart

@@ -29,15 +29,7 @@ abstract class SourceFileProvider implements api.CompilerInput {
     if (!resourceUri.isAbsolute) {
       resourceUri = cwd.resolveUri(resourceUri);
     }
-    api.Input<List<int>> input;
-    switch (inputKind) {
-      case api.InputKind.UTF8:
-        input = utf8SourceFiles[resourceUri];
-        break;
-      case api.InputKind.binary:
-        input = binarySourceFiles[resourceUri];
-        break;
-    }
+    api.Input<List<int>> input = _loadInputFromCache(resourceUri, inputKind);
     if (input != null) return Future.value(input);
 
     if (resourceUri.isScheme('file')) {
@@ -49,6 +41,52 @@ abstract class SourceFileProvider implements api.CompilerInput {
     }
   }
 
+  /// Fetches any existing value of [resourceUri] in a cache.
+  ///
+  /// For `api.InputKind.UTF8` inputs, this looks up both the cache of
+  /// utf8 source files and binary source files. This is done today because of
+  /// how dart2js binds to the CFE's file system. While dart2js reads sources as
+  /// utf8, the CFE file system may read them as binary inputs. In case the CFE
+  /// needs to report errors, dart2js will only find the location data if it
+  /// checks both caches.
+  api.Input<List<int>> _loadInputFromCache(
+      Uri resourceUri, api.InputKind inputKind) {
+    switch (inputKind) {
+      case api.InputKind.UTF8:
+        var input = utf8SourceFiles[resourceUri];
+        if (input != null) return input;
+        input = binarySourceFiles[resourceUri];
+        if (input == null) return null;
+        return _storeSourceInCache(resourceUri, input.data, api.InputKind.UTF8);
+      case api.InputKind.binary:
+        return binarySourceFiles[resourceUri];
+    }
+    return null;
+  }
+
+  /// Adds [source] to the cache under the [resourceUri] key.
+  api.Input _storeSourceInCache(
+      Uri resourceUri, List<int> source, api.InputKind inputKind) {
+    switch (inputKind) {
+      case api.InputKind.UTF8:
+        return utf8SourceFiles[resourceUri] = CachingUtf8BytesSourceFile(
+            resourceUri, relativizeUri(resourceUri), source);
+      case api.InputKind.binary:
+        return binarySourceFiles[resourceUri] = Binary(resourceUri, source);
+    }
+    return null;
+  }
+
+  @override
+  void registerUtf8ContentsForDiagnostics(Uri resourceUri, List<int> source) {
+    if (!resourceUri.isAbsolute) {
+      resourceUri = cwd.resolveUri(resourceUri);
+    }
+    if (!utf8SourceFiles.containsKey(resourceUri)) {
+      _storeSourceInCache(resourceUri, source, api.InputKind.UTF8);
+    }
+  }
+
   api.Input _readFromFileSync(Uri resourceUri, api.InputKind inputKind) {
     assert(resourceUri.isScheme('file'));
     List<int> source;
@@ -61,29 +99,19 @@ abstract class SourceFileProvider implements api.CompilerInput {
       throw "Error reading '${relativizeUri(resourceUri)}' $detail";
     }
     dartCharactersRead += source.length;
-    api.Input input;
-    switch (inputKind) {
-      case api.InputKind.UTF8:
-        input = utf8SourceFiles[resourceUri] = CachingUtf8BytesSourceFile(
-            resourceUri, relativizeUri(resourceUri), source);
-        break;
-      case api.InputKind.binary:
-        input = binarySourceFiles[resourceUri] = Binary(resourceUri, source);
-        break;
-    }
-    return input;
+    return _storeSourceInCache(resourceUri, source, inputKind);
   }
 
   /// Read [resourceUri] directly as a UTF-8 file. If reading fails, `null` is
   /// returned.
-  api.Input autoReadFromFile(Uri resourceUri) {
+  api.Input readUtf8FromFileSyncForTesting(Uri resourceUri) {
     try {
       return _readFromFileSync(resourceUri, api.InputKind.UTF8);
     } catch (e) {
       // Silence the error. The [resourceUri] was not requested by the user and
       // was only needed to give better error messages.
+      return null;
     }
-    return null;
   }
 
   Future<api.Input<List<int>>> _readFromFile(
@@ -139,11 +167,14 @@ abstract class SourceFileProvider implements api.CompilerInput {
   relativizeUri(Uri uri) => fe.relativizeUri(cwd, uri, isWindows);
 
   SourceFile<List<int>> getUtf8SourceFile(Uri resourceUri) {
-    return utf8SourceFiles[resourceUri];
+    return _loadInputFromCache(resourceUri, api.InputKind.UTF8);
   }
 
   Iterable<Uri> getSourceUris() {
     Set<Uri> uris = Set<Uri>();
+    // Note: this includes also indirect sources that were used to create
+    // `.dill` inputs to the compiler. This is OK, since this API is only
+    // used to calculate DEPS for gn build systems.
     uris.addAll(utf8SourceFiles.keys);
     uris.addAll(binarySourceFiles.keys);
     return uris;
@@ -179,7 +210,6 @@ class FormattingDiagnosticHandler implements api.CompilerDiagnostics {
   bool isAborting = false;
   bool enableColors = false;
   bool throwOnError = false;
-  bool autoReadFileUri = false;
   int throwOnErrorCount = 0;
   api.Diagnostic lastKind = null;
   int fatalCount = 0;
@@ -265,17 +295,6 @@ class FormattingDiagnosticHandler implements api.CompilerDiagnostics {
       print('${color(message)}');
     } else {
       api.Input file = provider.getUtf8SourceFile(uri);
-      if (file == null &&
-          autoReadFileUri &&
-          (uri.isScheme('file') || !uri.isAbsolute) &&
-          uri.path.endsWith('.dart')) {
-        if (!uri.isAbsolute) {
-          uri = provider.cwd.resolveUri(uri);
-        }
-        // When reading from .dill files, the original source files haven't been
-        // loaded. Load the file if possible to provide a better error message.
-        file = provider.autoReadFromFile(uri);
-      }
       if (file is SourceFile) {
         print(file.getLocationMessage(color(message), begin, end,
             colorize: color));
@@ -574,21 +593,6 @@ class BazelInputProvider extends SourceFileProvider {
     }
     return result;
   }
-
-  @override
-  api.Input autoReadFromFile(Uri resourceUri) {
-    var path = resourceUri.path;
-    if (path.startsWith('/bazel-root')) {
-      path = path.substring('/bazel-root/'.length);
-      for (var dir in dirs) {
-        var file = dir.resolve(path);
-        if (File.fromUri(file).existsSync()) {
-          return super.autoReadFromFile(file);
-        }
-      }
-    }
-    return null;
-  }
 }
 
 /// Adapter to support one or more synthetic uri schemes.
@@ -635,18 +639,4 @@ class MultiRootInputProvider extends SourceFileProvider {
     }
     return result;
   }
-
-  @override
-  api.Input autoReadFromFile(Uri resourceUri) {
-    if (resourceUri.isScheme(markerScheme)) {
-      var path = resourceUri.path;
-      for (var dir in roots) {
-        var file = dir.resolve(path);
-        if (File.fromUri(file).existsSync()) {
-          return super.autoReadFromFile(file);
-        }
-      }
-    }
-    return null;
-  }
 }

pkg/compiler/test/equivalence/show_helper.dart

@@ -71,7 +71,7 @@ show<T>(ArgResults argResults, DataComputer<T> dataComputer,
       if (show != null && !show.any((f) => '$fileUri'.endsWith(f))) {
         continue;
       }
-      SourceFile sourceFile = await provider.autoReadFromFile(fileUri);
+      SourceFile sourceFile = provider.readUtf8FromFileSyncForTesting(fileUri);
       String sourceCode = sourceFile?.slowText();
       if (sourceCode == null) {
         sourceCode = new File.fromUri(fileUri).readAsStringSync();

pkg/compiler/test/helpers/memory_compiler.dart

@@ -60,13 +60,10 @@ api.CompilerDiagnostics createCompilerDiagnostics(
   api.CompilerDiagnostics handler = diagnostics;
   if (showDiagnostics) {
     if (diagnostics == null) {
-      handler = new FormattingDiagnosticHandler(provider)
-        ..verbose = verbose
-        ..autoReadFileUri = true;
+      handler = new FormattingDiagnosticHandler(provider)..verbose = verbose;
     } else {
       var formattingHandler = new FormattingDiagnosticHandler(provider)
-        ..verbose = verbose
-        ..autoReadFileUri = true;
+        ..verbose = verbose;
       handler = new MultiDiagnostics([diagnostics, formattingHandler]);
     }
   } else if (diagnostics == null) {

pkg/compiler/test/sourcemaps/helpers/sourcemap_helper.dart

@@ -105,7 +105,7 @@ class ProviderSourceFileManager implements SourceFileManager {
   @override
   SourceFile getSourceFile(uri) {
     SourceFile sourceFile = sourceFileProvider.getUtf8SourceFile(uri);
-    sourceFile ??= sourceFileProvider.autoReadFromFile(uri);
+    sourceFile ??= sourceFileProvider.readUtf8FromFileSyncForTesting(uri);
     if (sourceFile == null) {
       sourceFile = outputProvider.getSourceFile(uri);
     }