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

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

 // Copyright (c) 2016, the Dart project authors.  Please see the AUTHORS file
 // for details. All rights reserved. Use of this source code is governed by a
 // BSD-style license that can be found in the LICENSE file.
 
 library fasta.kernel_target;
 
 import 'dart:async' show Future;
 
 import 'dart:io' show File;
 
@@ skipping 22 matching lines @@
         Program,
         Source,
         StringLiteral,
         SuperInitializer,
         Throw,
         TypeParameter,
         VariableDeclaration,
         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;
 
 import 'package:kernel/transformations/mixin_full_resolution.dart'
     show MixinFullResolution;
 
 import 'package:kernel/type_algebra.dart' show substitute;
 
 import '../source/source_loader.dart' show SourceLoader;
@@ skipping 193 matching lines @@
       loader.resolveConstructors();
       loader.finishTypeVariables(objectClassBuilder);
       _program =
           link(new List<Library>.from(loader.libraries), nameRoot: nameRoot);
       loader.computeHierarchy(_program);
       loader.checkOverrides(sourceClasses);
       loader.prepareInitializerInference();
       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);
     }
     return _program;
   }
 
   /// Build the kernel representation of the program loaded by this target. The
   /// program will contain full bodies for the code loaded from sources, and
   /// only references to the code loaded by the [DillTarget], which may or may
   /// not include method bodies (depending on what was loaded into that target,
@@ skipping 388 matching lines @@
   }
 
   /// Run all transformations that are needed when building a program for the
   /// first time.
   void runBuildTransformations() {
     transformMixinApplications();
     otherTransformations();
   }
 
   void transformMixinApplications() {
-    new MixinFullResolution(backendTarget).transform(_program);
+    new MixinFullResolution(backendTarget, loader.coreTypes)
+        .transform(_program);
     ticker.logMs("Transformed mixin applications");
   }
 
   void otherTransformations() {
     // TODO(ahe): Don't generate type variables in the first place.
     if (!strongMode) {
       _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");
   }
 
   void verify() {
     var verifyErrors = verifyProgram(_program);
     errors.addAll(verifyErrors.map((error) => '$error'));
     ticker.logMs("Verified program");
   }
 
   /// Tree-shakes most code from the [dillTarget] by visiting all other
   /// libraries in [_program] and marking the APIs from the [dillTarget]
   /// libraries that are in use.
   ///
   /// Note: while it's likely we'll do some trimming of programs for modular
   /// compilation, it is unclear at this time when and how that trimming should
   /// happen. We are likely going to remove the extra visitor my either marking
   /// things while code is built, or by handling tree-shaking after the fact
   /// (e.g. during serialization).
   trimDependenciesInProgram() {
     var toShake =
         dillTarget.loader.libraries.map((lib) => lib.importUri).toSet();
     var isIncluded = (Uri uri) => !toShake.contains(uri);
     var data = new RetainedDataBuilder();
     // 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);
   }
 
   /// Return `true` if the given [library] was built by this [KernelTarget]
   /// from sources, and not loaded from a [DillTarget].
   bool isSourceLibrary(Library library) {
     return loader.libraries.contains(library);
   }
 }
 
 /// Looks for a constructor call that matches `super()` from a constructor in
 /// [cls]. Such a constructor may have optional arguments, but no required
 /// arguments.
 Constructor defaultSuperConstructor(Class cls) {
   Class superclass = cls.superclass;
   while (superclass != null && superclass.isMixinApplication) {
     superclass = superclass.superclass;
   }
   for (Constructor constructor in superclass.constructors) {
     if (constructor.name.name.isEmpty) {
       return constructor.function.requiredParameterCount == 0
           ? constructor
           : null;
     }
   }
   return null;
 }