Unifying compiler context

Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

    - renamed CompilationError to CompilationMessage

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs



Ideas for follow up after this CL:
 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream

R=ahe@google.com

Committed: https://github.com/dart-lang/sdk/commit/1aa139bc9407735b772b27949a5308931a521104

[Siggi Cherem (dart-lang), 2017-07-17 20:44:41]

Description was changed from

==========
Unifying compiler context (step 1):
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"

Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class
==========

to

==========
Unifying compiler context (step 1):
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"
 - all error reporting goes through ProcessedOptions, which reports
   errors using the onError handler (if provided) or the
   default command_line_reporting

Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class
==========

[Siggi Cherem (dart-lang), 2017-07-17 20:46:21]

Description was changed from

==========
Unifying compiler context (step 1):
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"
 - all error reporting goes through ProcessedOptions, which reports
   errors using the onError handler (if provided) or the
   default command_line_reporting

Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class
==========

to

==========
Unifying compiler context (step 1):
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"
 - ProcessedOptions.reportMessage is unified with CompilerContext report. It now
lives in ProcessedOptions (for now)
 - all error reporting goes through ProcessedOptions, which reports
   errors using the onError handler (if provided) or the
   default command_line_reporting


Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class
==========

[Siggi Cherem (dart-lang), 2017-07-17 20:54:58]

Description was changed from

==========
Unifying compiler context (step 1):
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"
 - ProcessedOptions.reportMessage is unified with CompilerContext report. It now
lives in ProcessedOptions (for now)
 - all error reporting goes through ProcessedOptions, which reports
   errors using the onError handler (if provided) or the
   default command_line_reporting


Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class
==========

to

==========
Unifying compiler context (step 1)

Changes in this CL:
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"
 - ProcessedOptions.reportMessage is unified with CompilerContext report
 - all error reporting goes through ProcessedOptions, which reports
   errors using the onError handler (if provided) or the
   default command_line_reporting
 - setting exit code is done on report, not on format
 - default error handler prints and doesn't throw (it used to just throw)
 - fixed corner cases not covered in previous CL (error reporting with
missing-main, error-reporting for inferring packages when input Uris are not
file:* URis)

Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class?
==========

[Siggi Cherem (dart-lang), 2017-07-17 23:29:01]

Description was changed from

==========
Unifying compiler context (step 1)

Changes in this CL:
 - CompilerContext now contains a ProcessedOptions object
 - ContextContext no longer depends on CommandLine
 - use "withContext" term instead of "withGlobalOptions"
 - ProcessedOptions.reportMessage is unified with CompilerContext report
 - all error reporting goes through ProcessedOptions, which reports
   errors using the onError handler (if provided) or the
   default command_line_reporting
 - setting exit code is done on report, not on format
 - default error handler prints and doesn't throw (it used to just throw)
 - fixed corner cases not covered in previous CL (error reporting with
missing-main, error-reporting for inferring packages when input Uris are not
file:* URis)

Not addressed in this CL:
 - Combine ProcessedOptions and CompilerContext into a single class?
==========

to

==========
Unifying compiler context (step 1)

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be more clear
about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line fasta tools

    - default onError is different:
        - we now use the command_line_reporting report, which prints and throws
          on fatal messages, but doesn't throw eagerly on all messages as
before.

    - introduced "printMessages" option:
        - make it easy to have both onError + command_line_reporting
          (see kernel-service is the main use case at this time, other ideas
welcome!)

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with a context
        - missing error cases when inferring .packages and input Uris are not
file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class (or extend
one from the other)
 - switch onError to a stream
==========

[Siggi Cherem (dart-lang), 2017-07-17 23:30:29]

Description was changed from

==========
Unifying compiler context (step 1)

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be more clear
about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line fasta tools

    - default onError is different:
        - we now use the command_line_reporting report, which prints and throws
          on fatal messages, but doesn't throw eagerly on all messages as
before.

    - introduced "printMessages" option:
        - make it easy to have both onError + command_line_reporting
          (see kernel-service is the main use case at this time, other ideas
welcome!)

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with a context
        - missing error cases when inferring .packages and input Uris are not
file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class (or extend
one from the other)
 - switch onError to a stream
==========

to

==========
Unifying compiler context (step 1)

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

[Siggi Cherem (dart-lang), 2017-07-17 23:30:49]

Description was changed from

==========
Unifying compiler context (step 1)

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

to

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

[Siggi Cherem (dart-lang), 2017-07-17 23:35:25]

Description was changed from

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

to

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

    - renamed CompilationError to CompilationMessage

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

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

Description was changed from

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

    - renamed CompilationError to CompilationMessage

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs

Ideas for follow up after this CL:

 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

to

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

    - renamed CompilationError to CompilationMessage

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs



Ideas for follow up after this CL:
 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

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

Patchset #1 (id:1) has been deleted

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

Patchset #1 (id:20001) has been deleted

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

Patchset #1 (id:40001) has been deleted

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

sigmund@google.com changed reviewers:
+ ahe@google.com, paulberry@google.com

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

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:54: bool reportMessages;
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

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:214: bool errorsAreFatal = false;
should this default to true?

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:312: // When compiling the SDK
the input files are normaly `dart:` URIs.
*FYI this is new logic (handling more error cases)

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/command_line_reporting.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:146: if
(CompilerContext.current.options.setExitCodeOnProblem) {
moved here because I believe we'd like to use `format` even when we are not
using `dart:io` (e.g. using the public API in memory, or pub, or in the
browser).

I moved this assuming this was equivalent. If not, I can revert it back and we
can follow up on this later.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_context.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:68: return
command_line_reporting.format(message, severity);
Peter - the default format has the notion of colors, but other than that it
didn't seem very command-line specific. Should we have it in a separate file
than `report`?

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:126: Future<KernelTarget>
buildOutline([Uri output]) async {
Peter - this task now is practically doing the same as kernelForProgram, you OK
with deleting it here and simply calling the public API intead?

[ahe, 2017-07-18 10:40:51]

I haven't found any really blocking issues, but I think it would make sense to
go through this in our 1:1 later today.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compile_platform.dart (left):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compile_platform.dart:49: Ticker ticker = new
Ticker();
When is this initialized?

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:143: dynamic
f(CompilerContext context, List<String> restArguments)) {
This method needs to create a Ticker as the first thing it does. Ideally, it
should be passed a Ticker.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_context.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:32: // TODO(sigmund): move
here methods in ProcessedOptions that don't seem
"move" -> "Move".

Consider: "Move methods in ProcessedOptions here that don't ...

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:39: /// This is populated as
the compiler loads files, and it is used for error
loads -> reads

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:57: // TODO: default to >
command_line_reporting.report;
No name on TODO.
"default" -> "Default"
";" -> "."

Write out what ">" means

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:57: // TODO: default to >
command_line_reporting.report;
I'm not sure we should submit this CL with this TODO.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:63: // TODO: default to >
command_line_reporting.reportWithoutLocation;
Ditto.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:68: return
command_line_reporting.format(message, severity);
On 2017/07/18 00:11:08, Siggi Cherem (dart-lang) wrote:
> Peter - the default format has the notion of colors, but other than that it
> didn't seem very command-line specific. Should we have it in a separate file
> than `report`?

I think command_line_reporting uses a format that's suitable for command-line
applications.

It's not suitable for an IDE as it includes a source snippet.

It's not suitable for output in a log, or in a web page.

Have I overlooked some non-command-line use cases where this format is useful?

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:103: var pOptions = new
ProcessedOptions(options);
Consider:

var options = new ProcessedOptions(new CompilerOptions());

Just to avoid having a variable named pOptions. Otherwise, rename it to
processedOptions.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:126: Future<KernelTarget>
buildOutline([Uri output]) async {
On 2017/07/18 00:11:08, Siggi Cherem (dart-lang) wrote:
> Peter - this task now is practically doing the same as kernelForProgram, you
OK
> with deleting it here and simply calling the public API intead?

Yes. We just need to make sure that the Ticker still works and is created when
the program starts, not after an unknown amount of time buried in the API.

To avoid exposing the Ticker in the public API, we can pass in a Stopwatch and
initialize the Ticker with that.

[ahe, 2017-07-18 10:42:26]

On 2017/07/18 10:40:51, ahe wrote:
> I haven't found any really blocking issues, but I think it would make sense to
> go through this in our 1:1 later today.

And my overall impression is very positive :-)

[ahe, 2017-07-18 16:54:37]

lgtm

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compilation_message.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compilation_message.dart:19: /// A text description of the
compile error.
of the problem.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compilation_message.dart:22: /// A suggestion for the user to
hint them on how to fix the error. May be
A suggestion for how to fix the problem. May be null.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:54: bool reportMessages;
On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> 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

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.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:214: bool errorsAreFatal = false;
On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> should this default to true?

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.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:220: bool nitsAreFatal = false;
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.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:462: String debugString() {
Nice.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:540: class _CompilationMessage
implements CompilationMessage {
You may be able to merge this class with CompilationMessage.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/command_line_reporting.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:146: if
(CompilerContext.current.options.setExitCodeOnProblem) {
On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> moved here because I believe we'd like to use `format` even when we are not
> using `dart:io` (e.g. using the public API in memory, or pub, or in the
> browser).
> 
> I moved this assuming this was equivalent. If not, I can revert it back and we
> can follow up on this later.

I think this makes sense.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:153: "Compilation
aborted due to fatal ${severityName(severity)}.");
FYI: When this InputError is handled, we end up back here. This isn't working as
we intend. Consider adding a TODO.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compile_platform.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compile_platform.dart:73:
c.options.ticker.logMs("Wrote program to ${fullOutput.toFilePath()}");
Note: I'm currently seeing these messages printed when running "fasta
compile-platform" without --verbose.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:13: import
'package:front_end/src/base/processed_options.dart';
Use relative URIs.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:86: usage, "Ignored
'--compile-sdk' option.");
This option isn't really ignored as it's reporting an error here.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:143: dynamic
f(CompilerContext context, List<String> restArguments)) {
On 2017/07/18 10:40:50, ahe wrote:
> This method needs to create a Ticker as the first thing it does. Ideally, it
> should be passed a Ticker.

We discussed this at our 1:1, handle as you see fit.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_loader.dart:84:
CompilerContext.current.options.excludeSourceInformation;
This option literally means "do not include source code in the generated dill
file".

This is not just about line number tables or source mapping. Perhaps you want to
rename the option?

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/kernel_driver.dart:155: ..reportMessages =
false; // ignore errors.
Seems like we need a TODO or an explanation why we should ignore errors.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/messages....
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/messages....
pkg/front_end/messages.yaml:878: template: "Cannot infer a .packages file from
an input `package:*` URI."
Suggestion:

Can't infer a '.packages' file from '#uri'.

If you don't like my suggestion, you should change "cannot" to "can't" and use
single quotes, not Markdown.

https://codereview.chromium.org/2982093003/diff/60001/utils/kernel-service/ke...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/utils/kernel-service/ke...
utils/kernel-service/kernel-service.dart:91: if (hasErrors) {
Something has regressed here, and I think we need to restore the old behavior
(soon, but not in this CL).

Previously, the list of errors would be extracted from KernelTarget, and it
would only be errors that couldn't be handled by generating an inline "throw
CompileTimeError".

This meant that we would be exercising the compile-time error handling much
better by actually running the code.

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

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:54: bool reportMessages;
On 2017/07/18 16:54:36, ahe wrote:
> On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> > 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
> 
> 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.

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.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:214: bool errorsAreFatal = false;
On 2017/07/18 16:54:36, ahe wrote:
> On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> > should this default to true?
> 
> 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.

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]

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:54: bool reportMessages;
On 2017/07/18 18:36:21, Paul Berry wrote:
> On 2017/07/18 16:54:36, ahe wrote:
> > On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> > > 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
> > 
> > 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.
> 
> 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.

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

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:214: bool errorsAreFatal = false;
On 2017/07/18 18:36:21, Paul Berry wrote:
> On 2017/07/18 16:54:36, ahe wrote:
> > On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> > > should this default to true?
> > 
> > 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.
> 
> 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.

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:44]

Thank Peter and Paul for all the comments!

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compilation_message.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compilation_message.dart:19: /// A text description of the
compile error.
On 2017/07/18 16:54:36, ahe wrote:
> of the problem.

Done.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compilation_message.dart:22: /// A suggestion for the user to
hint them on how to fix the error. May be
On 2017/07/18 16:54:36, ahe wrote:
> A suggestion for how to fix the problem. May be null.

Nice, concise. thx.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:54: bool reportMessages;
On 2017/07/18 18:36:21, Paul Berry wrote:
> On 2017/07/18 16:54:36, ahe wrote:
> > On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> > > 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
> > 
> > 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.
> 
> 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.

Sounds good, thanks both, added TODO.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:214: bool errorsAreFatal = false;
On 2017/07/18 21:07:26, ahe wrote:
> On 2017/07/18 18:36:21, Paul Berry wrote:
> > On 2017/07/18 16:54:36, ahe wrote:
> > > On 2017/07/18 00:11:07, Siggi Cherem (dart-lang) wrote:
> > > > should this default to true?
> > > 
> > > 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.
> > 
> > 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.
> 
> 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.

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.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/compi...
pkg/front_end/lib/compiler_options.dart:220: bool nitsAreFatal = false;
On 2017/07/18 16:54:36, ahe wrote:
> 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.

Done. Renamed to `throwOnX`

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:540: class _CompilationMessage
implements CompilationMessage {
On 2017/07/18 16:54:37, ahe wrote:
> You may be able to merge this class with CompilationMessage.

I have a slight preference to keep them separate because I like documenting the
public API without including noise/internal implementation details in the same
file, but happy to chat more about it if you disagree

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/command_line_reporting.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:153: "Compilation
aborted due to fatal ${severityName(severity)}.");
On 2017/07/18 16:54:37, ahe wrote:
> FYI: When this InputError is handled, we end up back here. This isn't working
as
> we intend. Consider adding a TODO.

Done. Added TODO

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compile_platform.dart (left):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compile_platform.dart:49: Ticker ticker = new
Ticker();
On 2017/07/18 10:40:50, ahe wrote:
> When is this initialized?

When we allocate the context, which happens on this same line.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compile_platform.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compile_platform.dart:73:
c.options.ticker.logMs("Wrote program to ${fullOutput.toFilePath()}");
On 2017/07/18 16:54:37, ahe wrote:
> Note: I'm currently seeing these messages printed when running "fasta
> compile-platform" without --verbose.

Is that an invariant you would like to preserve?

With my change the new ticker is not verbose by default, so they would not be
shown.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:13: import
'package:front_end/src/base/processed_options.dart';
On 2017/07/18 16:54:37, ahe wrote:
> Use relative URIs.

Done.

BTW, the reason I used package:* here is that my general style preference is to
use relative for going deeper and non-relative for going up (../../* is a bit
harder to follow for me).

Happy to keep the existing style, but let me know if you like my general rule
for future code.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:86: usage, "Ignored
'--compile-sdk' option.");
On 2017/07/18 16:54:37, ahe wrote:
> This option isn't really ignored as it's reporting an error here.

Good point, rephrased

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:143: dynamic
f(CompilerContext context, List<String> restArguments)) {
On 2017/07/18 16:54:37, ahe wrote:
> On 2017/07/18 10:40:50, ahe wrote:
> > This method needs to create a Ticker as the first thing it does. Ideally, it
> > should be passed a Ticker.
> 
> We discussed this at our 1:1, handle as you see fit.

Thanks. The ticker is created in line 172 below eagerly, but I understand what
you mean that this could be too late.

I added a todo next to the creation of the ticker in ProcessedOptions to follow
up on this.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/compiler_context.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:32: // TODO(sigmund): move
here methods in ProcessedOptions that don't seem
On 2017/07/18 10:40:51, ahe wrote:
> "move" -> "Move".
> 
> Consider: "Move methods in ProcessedOptions here that don't ...

Done.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:39: /// This is populated as
the compiler loads files, and it is used for error
On 2017/07/18 10:40:51, ahe wrote:
> loads -> reads

Done.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:57: // TODO: default to >
command_line_reporting.report;
On 2017/07/18 10:40:51, ahe wrote:
> No name on TODO.
> "default" -> "Default"
> ";" -> "."
> 
> Write out what ">" means

Sorry, I forgot to delete this line. It was a note-to-self to come back to it
before finishing the CL. I implemented the logic but forgot to remove this note.

We now default correctly to command_line_reporting.report in ProcessedOptions.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:63: // TODO: default to >
command_line_reporting.reportWithoutLocation;
On 2017/07/18 10:40:50, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:68: return
command_line_reporting.format(message, severity);
On 2017/07/18 10:40:51, ahe wrote:
> On 2017/07/18 00:11:08, Siggi Cherem (dart-lang) wrote:
> > Peter - the default format has the notion of colors, but other than that it
> > didn't seem very command-line specific. Should we have it in a separate file
> > than `report`?
> 
> I think command_line_reporting uses a format that's suitable for command-line
> applications.
> 
> It's not suitable for an IDE as it includes a source snippet.
> 
> It's not suitable for output in a log, or in a web page.
> 
> Have I overlooked some non-command-line use cases where this format is useful?

I see, I don't have other use cases in mind at the moment. I overlooked at the
fact that format was also used in messages generated in the output program.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/compiler_context.dart:103: var pOptions = new
ProcessedOptions(options);
On 2017/07/18 10:40:51, ahe wrote:
> Consider:
> 
> var options = new ProcessedOptions(new CompilerOptions());
> 
> Just to avoid having a variable named pOptions. Otherwise, rename it to
> processedOptions.

Done.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/fasta.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/fasta.dart:126: Future<KernelTarget>
buildOutline([Uri output]) async {
On 2017/07/18 10:40:51, ahe wrote:
> On 2017/07/18 00:11:08, Siggi Cherem (dart-lang) wrote:
> > Peter - this task now is practically doing the same as kernelForProgram, you
> OK
> > with deleting it here and simply calling the public API intead?
> 
> Yes. We just need to make sure that the Ticker still works and is created when
> the program starts, not after an unknown amount of time buried in the API.
> 
> To avoid exposing the Ticker in the public API, we can pass in a Stopwatch and
> initialize the Ticker with that.

Sounds good, I like this plan.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_loader.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_loader.dart:84:
CompilerContext.current.options.excludeSourceInformation;
On 2017/07/18 16:54:37, ahe wrote:
> This option literally means "do not include source code in the generated dill
> file".
> 
> This is not just about line number tables or source mapping. Perhaps you want
to
> rename the option?

Good point. Renamed to embedSourceText

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/i...
File pkg/front_end/lib/src/incremental/kernel_driver.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/lib/src/i...
pkg/front_end/lib/src/incremental/kernel_driver.dart:155: ..reportMessages =
false; // ignore errors.
On 2017/07/18 16:54:37, ahe wrote:
> Seems like we need a TODO or an explanation why we should ignore errors.

Done.

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/messages....
File pkg/front_end/messages.yaml (right):

https://codereview.chromium.org/2982093003/diff/60001/pkg/front_end/messages....
pkg/front_end/messages.yaml:878: template: "Cannot infer a .packages file from
an input `package:*` URI."
On 2017/07/18 16:54:37, ahe wrote:
> Suggestion:
> 
> Can't infer a '.packages' file from '#uri'.
> 

I decided to keep the original phrasing because I wanted to highlight the actual
issue (the actual #uri might be a bit distracting in this case)

> If you don't like my suggestion, you should change "cannot" to "can't" and use
> single quotes, not Markdown.

Done.

https://codereview.chromium.org/2982093003/diff/60001/utils/kernel-service/ke...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2982093003/diff/60001/utils/kernel-service/ke...
utils/kernel-service/kernel-service.dart:91: if (hasErrors) {
On 2017/07/18 16:54:37, ahe wrote:
> Something has regressed here, and I think we need to restore the old behavior
> (soon, but not in this CL).
> 
> Previously, the list of errors would be extracted from KernelTarget, and it
> would only be errors that couldn't be handled by generating an inline "throw
> CompileTimeError".
> 
> This meant that we would be exercising the compile-time error handling much
> better by actually running the code.

I see, let's talk on our next sync up about this. I want to make sure we are on
the same page on where this fits in our planning and the overall move to 2.0.
Added a TODO here for now.

In terms of implementation:
 - since now all errors are conflated together, we need some way to tell them
apart. One option here could be to model it as yet another category/severity:
unhandledError vs handledError?
 - we should have a flag "emitWithErrors" to enable this behavior. My
understanding is that with the move to 2.0, the default behavior will be to have
this flag off? maybe some clients (e.g. hot-reload) might be interested in
turning it on? let's chat more.

[Siggi Cherem (dart-lang), 2017-07-19 00:01:01]

FYI - I undid a change in kernel-service because some dartk/vm tests expected
the old behavior (i tried to remove duplicate error reporting, we can do that
separate from the other changes)

[Siggi Cherem (dart-lang), 2017-07-19 00:03:06]

Description was changed from

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

    - renamed CompilationError to CompilationMessage

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs



Ideas for follow up after this CL:
 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream
==========

to

==========
Unifying compiler context

Changes in this CL:
 - Updated CompilerContext:
    - it now contains a ProcessedOptions object

    - it no longer depends on CompilerCommandLine/CommandLine

    - it delegates to ProcessedOptions.report so all error reporting
      goes to one single place.

    - use "withContext" term instead of "withGlobalOptions" to be
      more clear about the intent


 - Changes in public API
    - added more options that correspond to flags in command-line
      fasta tools

    - default onError is different: we now use the
      command_line_reporting report, which prints and throws
      on fatal messages, but doesn't throw eagerly on all messages
      as before.

    - introduced "printMessages" option: make it easy to have
      both onError + command_line_reporting  (kernel-service.dart
      is the main use case at this time, other ideas welcome!)

    - renamed CompilationError to CompilationMessage

 - Other changes

    - set exit code is done on report, not on format
    - fixed corner cases not covered in previous CL
        - error reporting with missing-main needs to happen with
          a context
        - missing error cases when inferring .packages and input
          URIs are not file:* URIs



Ideas for follow up after this CL:
 - combine ProcessedOptions and CompilerContext into a single class
   (or extend one from the other)
 - switch onError to a stream

R=ahe@google.com

Committed:
https://github.com/dart-lang/sdk/commit/1aa139bc9407735b772b27949a5308931a521104
==========

[Siggi Cherem (dart-lang), 2017-07-19 00:03:07]

Committed patchset #3 (id:100001) manually as
1aa139bc9407735b772b27949a5308931a521104 (presubmit successful).

[ahe, 2017-08-17 13:18:21]

https://codereview.chromium.org/2982093003/diff/100001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/compiler_command_line.dart (right):

https://codereview.chromium.org/2982093003/diff/100001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/compiler_command_line.dart:163:
..setExitCodeOnProblem = true
Why is this set to true?