Unifying compiler context

pkg/front_end/lib/compiler_options.dart

Index: pkg/front_end/lib/compiler_options.dart
diff --git a/pkg/front_end/lib/compiler_options.dart b/pkg/front_end/lib/compiler_options.dart
index 3b46067f333a7355b349a2bf7de5461a50939fd3..f67d9f0c3ca35f02d659b80ff40658e2ec148eee 100644
--- a/pkg/front_end/lib/compiler_options.dart
+++ b/pkg/front_end/lib/compiler_options.dart
@@ -8,15 +8,12 @@ import 'package:front_end/src/base/performace_logger.dart';
 import 'package:front_end/src/incremental/byte_store.dart';
 import 'package:kernel/target/targets.dart' show Target;
 
-import 'compilation_error.dart';
+import 'compilation_message.dart';
 import 'file_system.dart';
 import 'physical_file_system.dart';
 
-/// Default error handler used by [CompilerOptions.onError].
-void defaultErrorHandler(CompilationError error) => throw error;
-
 /// Callback used to report errors encountered during compilation.
-typedef void ErrorHandler(CompilationError error);
+typedef void ErrorHandler(CompilationMessage error);
 
 /// Front-end options relevant to compiler back ends.
 ///
@@ -43,9 +40,18 @@ class CompilerOptions {
 
   /// Callback to which compilation errors should be delivered.
   ///
-  /// By default, the first error will be reported by throwing an exception of
-  /// type [CompilationError].
-  ErrorHandler onError = defaultErrorHandler;
+  /// By default, when no callback is provided, the compiler will report
+  /// messages on the console and will throw when fatal errors are discovered.
+  ErrorHandler onError;
+
+  /// Whether messages should be reported using the compiler's internal
+  /// reporting mechanism.
+  ///
+  /// If no [onError] handler is provided, the default is true. If an [onError]
+  /// handler is provided, the default is false. Setting this to true will
+  /// ensure that error messages are printed in the console and that fatal
+  /// errors cause an exception.
+  bool reportMessages;

[Siggi Cherem (dart-lang), 2017/07/18 00:11:07]

Paul & Peter - there are many ways to address what I'm trying to do. Let me know
if you prefer going down a different route.

Some other options:
 - make command_line_report.report easier to reuse externally, for example,
change it to be written in terms of the public CompilationMessage
 - unify LocatedMessage and ComplilationMessage

[ahe, 2017/07/18 16:54:36]

As we discussed in our 1:1, I'd like to make formatting part of this object.
This is because I want to allow the user of this API to control how errors are
formatted in generated code.

[Paul Berry, 2017/07/18 18:36:21]

I'm fine with the code as is for now.  IMHO we can address Peter's formatting
request in a follow up CL if/when it is needed.

[ahe, 2017/07/18 21:07:26]

Agreed. If it wasn't clear when we discussed it, all of my comments can be
addressed in future CLs.

[Siggi Cherem (dart-lang), 2017/07/18 22:50:43]

Sounds good, thanks both, added TODO.

 
   /// URI of the ".packages" file (typically a "file:" URI).
   ///
@@ -197,4 +203,19 @@ class CompilerOptions {
   /// Whether to set the exit code to non-zero if any problem (including
   /// warning, etc.) is encountered during compilation.
   bool setExitCodeOnProblem = false;
+
+  /// Whether to generate kernel that excludes source informaton.
+  ///
+  /// When false, APIs that return a kernel `Program` include a
+  /// `uriToSource` map.
+  bool excludeSourceInformation = false;
+
+  /// Whether errors are considered fatal and should stop the compiler.
+  bool errorsAreFatal = false;

[Siggi Cherem (dart-lang), 2017/07/18 00:11:07]

should this default to true?

[ahe, 2017/07/18 16:54:36]

I don't think so. For both hot reloading and the analyzer (IDE), errors
shouldn't be fatal. I think those are the most important use cases, I think it
makes sense to use them to guide the default. Unless it turns out to be annoying
in a bunch of tests.

[Paul Berry, 2017/07/18 18:36:21]

Normally I agree with the notion of having the defaults guided by the important
use cases.  But I think this situation is an exception because a default value
of false for errorsAreFatal is dangerous: it means that a client that forgets to
change this default will receive an error-recovered kernel representation of the
user's code even if that code is erroneous.

I know Fasta works hard to try to produce valid kernel code even in its error
recovery paths.  But my experience from analyzer is that getting this right 100%
of the time is hard; when we don't get it right, we place a burden on clients to
handle invalid output, and clients don't tend to do a good job of that unless
they're thinking hard about it.

This has cropped up a lot in analyzer because it has the same sort of default
behavior, and IIRC it was specifically identified by the DDC folks as a concern
when we were first talking about the design of the front end API.  Even
analyzer's own unit tests (written by analyzer folks who ought to know better)
often forget to check for errors--this has led us to accidentally write
completely bogus tests that don't test what they're intended to.  It was a
specific goal of mine when I designed the front end API to try to avoid this
problem.

[ahe, 2017/07/18 21:07:26]

As long as we make sure to set this to false for all the language tests, etc.,
the default probably doesn't matter much. At a later time, I want to talk more
about those problems from the analyzer, because it is also something Kevin
brought up in relation to transformers. I have some ideas, and I wanna make sure
those weren't already tried and rejected in the analyzer.

[Siggi Cherem (dart-lang), 2017/07/18 22:50:43]

OK - a few updates.

I changed the default as follows:
 - if no onError handler is given, it throws on errors (default is true)
temporarily **1

 - if an onError handler is given, it's up to the handler to throw or handle
errors differently. DDC will eventually use this pattern.

 - command-line tools like `fasta-compile` set this to `false`.

 - some unit tests also set it to false because they rely on the old behavior,
we can eventually change the tests to use the onError handler instead.

 - kernel-service has an onError handler, so it works as before: no crash and
continues to report errors as it did before for the language tests etc.  **2


*1 - I had a couple chats with Paul about this and we agreed that we want to
switch it back to default to "false" soon :). We agreed that the main use case
is that if there are errors (even recoverable errors) that the frontend doesn't
give a program to a client like DDC who doesn't expect it. One way to do that is
by throwing on errors (default to true here), but a different approach is to
returning null in `kernelForProgram` (or a result object containing all errors).

We have partial support for this in kernelForProgram (we returns null when there
are errors in kernelTarget), but we need to fix it up a bit before we remove
swap the flag here: we need to return null even when there are other errors that
we could have recovered from.

I filed https://github.com/dart-lang/sdk/issues/30194 to track.

*2 - Peter, I wrote more details on the kernel-service.dart file, let's chat on
VC about this, I want to make sure we are aligned with what we want to do for
this and the 2.0 plans from the rest of the team.

+
+  /// Whether warnings are considered fatal and should stop the compiler.
+  bool warningsAreFatal = false;
+
+  /// Whether errors are considered fatal and should stop the compiler.
+  bool nitsAreFatal = false;

[ahe, 2017/07/18 16:54:36]

Note: all these "AreFatal" options are mostly used for debugging the compiler.
So as we discussed at our 1:1, it's possible that we should rename them to
something more internal-development-sounding.

[Siggi Cherem (dart-lang), 2017/07/18 22:50:43]

Done. Renamed to `throwOnX`

 }