Don't recreate CoreTypes in transformers. Pass it in.

pkg/front_end/lib/src/fasta/kernel/kernel_target.dart

Index: pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
diff --git a/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart b/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
index 2a0f3ffa45332105a4160fb28dcc55377fbe02ec..7841e891ff17949096042281d43e4dfc2458ee12 100644
--- a/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
+++ b/pkg/front_end/lib/src/fasta/kernel/kernel_target.dart
@@ -40,7 +40,6 @@ import 'package:kernel/ast.dart'
         VariableGet,
         VoidType;
 
-import 'package:kernel/core_types.dart' show CoreTypes;
 import 'package:kernel/transformations/erasure.dart' show Erasure;
 
 import 'package:kernel/transformations/continuation.dart' as transformAsync;
@@ -254,6 +253,10 @@ class KernelTarget extends TargetImplementation {
       loader.performInitializerInference();
     } on InputError catch (e) {
       handleInputError(e, isFullProgram: false);
+      // TODO(scheglov) Normally we create CoreTypes and ClassHierarchy.
+      // But if there is an exception, they are not created, and when we later
+      // blindly continue with buildProgram(), we crash.
+      loader.computeHierarchy(_program);

[ahe, 2017/05/29 14:08:46]

If there's an InputError during buildOutlines, we shouldn't call buildProgram.

[scheglov, 2017/05/30 00:06:10]

"We shouldn't call" as "It is a known existing problem and we need to fix it
someday" or as "This is a new problem added in this CL and you need to fix it
before landing the CL"?

[ahe, 2017/05/30 09:35:14]

I didn't know about it before seeing this change. I'm not aware of any crashes
on any of our existing test input, so I'm inclined to think that this is a new
problem.

I don't think computing the hierarchy is a solution to the problem.

Do you have an example of how this problem occurs? Could you leave out these
lines and mark a test as failing or crashing? Then we can look at it together
later today in our 1:1?

[scheglov, 2017/05/30 16:10:21]

Well, it is a pre-existing problem.

My feeling is that error handling in Fasta is written to be suitable for
one-shot compilers, not for the library that parses code for other reasons than
compile and fail fast on the very first error.

Specifically this program causes InputError in "loader.buildOutlines()".

----
class A {
  int foo() native 'foo';
}
----

But fasta still calls [buildProgram], which we agreed it should not do.

I call fasta using the following command line:
/Users/scheglov/Source/Dart/sdk.git/sdk/pkg/front_end/tool/fasta compile
--platform=/Users/scheglov/Source/Dart/sdk.git/sdk/xcodebuild/ReleaseX64/patched_sdk/outline.dill
bin/test3.dart

I will send you a tiny patch that demonstrates that [buildOutlines] completes
with InputError and [buildProgram] is still called. You can patch it in without
this CL and get convinced that this CL does not change the status quo, it merely
makes test cases on which Fasta throws InputError, and which have already been
in Fail, to be still Fail instead of Crash.

[ahe, 2017/05/31 11:20:16]

I completely understand why you get that feeling when Fasta throws an
InputError. When it does so, it's because it has given up and didn't have any
good way to report an error.

Ideally, Fasta should never throw an InputError, because that solution is only
good for a one-shot compiler. We agree 100% on that, I think.

However, there are situations that are hard to recover from, so I took some
shortcuts initially, especially when it comes to missing files and unrecoverable
parser errors. We need to remove those shortcuts.
So it turns out I was wrong, buildProgram may be called after an error has
occurred, but it's supposed to stop early. Which it does because it checks
errors.isNotEmpty. However, it calls trimDependenciesInProgram which it probably
shouldn't do.

I've created a CL which removes the call to trimDependenciesInProgram on line
286 below, and I've tested that it enables you to remove these lines. See
https://codereview.chromium.org/2912353002

[scheglov, 2017/05/31 16:49:37]

Nice.
Thank you.
I removed this workaround.

     } catch (e, s) {
       return reportCrash(e, s, loader?.currentUriForCrashReporting);
     }
@@ -662,7 +665,8 @@ class KernelTarget extends TargetImplementation {
   }
 
   void transformMixinApplications() {
-    new MixinFullResolution(backendTarget).transform(_program);
+    new MixinFullResolution(backendTarget, loader.coreTypes)
+        .transform(_program);
     ticker.logMs("Transformed mixin applications");
   }
 
@@ -672,9 +676,8 @@ class KernelTarget extends TargetImplementation {
       _program.accept(new Erasure());
       ticker.logMs("Erased type variables in generic methods");
     }
-    var coreTypes = new CoreTypes(_program);
     // TODO(kmillikin): Make this run on a per-method basis.
-    transformAsync.transformLibraries(coreTypes, loader.libraries);
+    transformAsync.transformLibraries(loader.coreTypes, loader.libraries);
     ticker.logMs("Transformed async methods");
   }
 
@@ -701,7 +704,7 @@ class KernelTarget extends TargetImplementation {
     // TODO(sigmund): replace this step with data that is directly computed from
     // the builders: we should know the tree-shaking roots without having to do
     // a second visit over the tree.
-    new RootsMarker(data).run(_program, isIncluded);
+    new RootsMarker(loader.coreTypes, data).run(_program, isIncluded);
     trimProgram(_program, data, isIncluded);
   }