Report messages instead of just printing.

Report messages instead of just printing.

R=sigmund@google.com

Committed: https://github.com/dart-lang/sdk/commit/18a41eba089cbbad7339ff76713a1acb3a8eb974

[ahe, 2017-07-13 10:08:21]

Patchset #1 (id:1) has been deleted

[ahe, 2017-07-13 10:21:15]

Patchset #1 (id:20001) has been deleted

[ahe, 2017-07-13 10:43:01]

Patchset #1 (id:40001) has been deleted

[ahe, 2017-07-13 10:45:51]

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

[Siggi Cherem (dart-lang), 2017-07-13 21:05:56]

lgtm

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

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:77: "${' ' *
(location.column - 1)}^";
BTW this is logic for which I'd like to use package:source_span. Consider adding
a TODO to revisit this afterwards?

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:161: if
(isFatal(severity)) {
considering adding a common shared function to avoid the duplication here and
above. 

Maybe do the printing and isFatal check together, like:

void printAndThowIfFatal(String message, Severity severity) {
}

?

that way there is only one place with a `print`?

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/deprecated_problems.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/deprecated_problems.dart:42:
print(StackTrace.current);
do we still need this here? 

I expect this will be printed by however handles the exception

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/severity.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/severity.dart:9: internalProblem,
interesting, I had not considered treating internalProblem as a severity, but as
a kind of message.

I'm not opposed to it though, just hadn't thought of it this way.

Next steps: expose this in the public API as well.

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/k...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/k...
pkg/front_end/lib/src/kernel_generator_impl.dart:152: for (LocatedMessage
message in kernelTarget.errors) {
irrelevant style question: why not
`kernelTarget.errors.forEach(options.reportMessage)`?

Agree though, this should go away once we integrate context.report with
options.reportMessage

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/tool/_fas...
File pkg/front_end/tool/_fasta/analyzer_compile.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/tool/_fas...
pkg/front_end/tool/_fasta/analyzer_compile.dart:47: exitCode = 1;
do we still need `exitCode = 1` here as well?

[ahe, 2017-07-14 09:59:50]

Patchset #2 (id:70001) has been deleted

[ahe, 2017-07-14 10:01:07]

Thank you, Siggi!

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

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:77: "${' ' *
(location.column - 1)}^";
On 2017/07/13 21:05:56, Siggi Cherem (dart-lang) wrote:
> BTW this is logic for which I'd like to use package:source_span. Consider
adding
> a TODO to revisit this afterwards?

Done.

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:161: if
(isFatal(severity)) {
On 2017/07/13 21:05:56, Siggi Cherem (dart-lang) wrote:
> considering adding a common shared function to avoid the duplication here and
> above. 
> 
> Maybe do the printing and isFatal check together, like:
> 
> void printAndThowIfFatal(String message, Severity severity) {
> }
> 
> ?
> 
> that way there is only one place with a `print`?

Done.

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/deprecated_problems.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/deprecated_problems.dart:42:
print(StackTrace.current);
On 2017/07/13 21:05:56, Siggi Cherem (dart-lang) wrote:
> do we still need this here? 
> 
> I expect this will be printed by however handles the exception

Using --fatal=errors will stop compilation immediately after the first error.

If you also use --verbose, the current stack trace will be printed. This is a
useful technique for fixing incorrect errors.

Unfortunately, I've frequently found bugs in how the VM preserves stack traces
across throws and rethrows, as well as when there's an asynchronous gap. Since
the point at line 41 is as close as it gets to the actual location I want to
find when I use --fatal=errors and --verbose, I like to keep it here.

In addition, by printing the stack trace here, I don't have to worry about it
getting printed multiple times as it sometimes did in dart2js. Also, it doesn't
confuse crash reporting as it also sometimes does in dart2js.

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/severity.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/severity.dart:9: internalProblem,
On 2017/07/13 21:05:56, Siggi Cherem (dart-lang) wrote:
> interesting, I had not considered treating internalProblem as a severity, but
as
> a kind of message.
> 
> I'm not opposed to it though, just hadn't thought of it this way.
> 
> Next steps: expose this in the public API as well.

Acknowledged.

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/k...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/lib/src/k...
pkg/front_end/lib/src/kernel_generator_impl.dart:152: for (LocatedMessage
message in kernelTarget.errors) {
On 2017/07/13 21:05:56, Siggi Cherem (dart-lang) wrote:
> irrelevant style question: why not
> `kernelTarget.errors.forEach(options.reportMessage)`?
> 
> Agree though, this should go away once we integrate context.report with
> options.reportMessage

This is a mistake because of how this code evolved in my local branch. At some
point in this code's history, I didn't have a function to conveniently pass to
forEach.

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/tool/_fas...
File pkg/front_end/tool/_fasta/analyzer_compile.dart (right):

https://codereview.chromium.org/2977013002/diff/30019/pkg/front_end/tool/_fas...
pkg/front_end/tool/_fasta/analyzer_compile.dart:47: exitCode = 1;
On 2017/07/13 21:05:56, Siggi Cherem (dart-lang) wrote:
> do we still need `exitCode = 1` here as well?

Yes. This code is called directly from main below and not otherwise going the
public API. The default implementation of report doesn't set the exit code.

You may not have noticed, but I've tried to make fasta always create a program.
And when run, that program will report any compile-time errors.

The reason for that is that I want to make sure that hot reloading is the
primary use case, and regular command-line compiler is secondary. This is why I
had to add the option --set-exit-code-on-problem.

This exit code covers the case where something went so wrong that task.compile
had to give up before it got to the try/catches in kernel_target.

[ahe, 2017-07-14 11:33:33]

Patchset #3 (id:110001) has been deleted

[ahe, 2017-07-14 11:38:34]

Patchset #4 (id:150001) has been deleted

[ahe, 2017-07-14 11:38:41]

Patchset #3 (id:130001) has been deleted

[ahe, 2017-07-14 11:40:36]

Patchset #3 (id:170001) has been deleted

[ahe, 2017-07-14 11:40:44]

Patchset #3 (id:190001) has been deleted

[ahe, 2017-07-14 11:42:12]

Patchset #3 (id:210001) has been deleted

[ahe, 2017-07-14 11:45:08]

Siggi, I made a few changes regarding test expectations before submitting. Could
you take a look at those "after the fact"?

https://codereview.chromium.org/2977013002/diff/230001/runtime/vm/isolate_rel...
File runtime/vm/isolate_reload_test.cc (left):

https://codereview.chromium.org/2977013002/diff/230001/runtime/vm/isolate_rel...
runtime/vm/isolate_reload_test.cc:3312: EXPECT_ERROR(lib, "......");
This relied on having a source snippet. For legacy reasons, the scanner
recognizes "..." as a single token, so that is included in the error message.

However, I think we should restore the source snippets, but we can discuss that
later.

[ahe, 2017-07-14 12:07:49]

Description was changed from

==========
Report messages instead of just printing.
==========

to

==========
Report messages instead of just printing.

R=sigmund@google.com

Committed:
https://github.com/dart-lang/sdk/commit/18a41eba089cbbad7339ff76713a1acb3a8eb974
==========

[ahe, 2017-07-14 12:07:50]

Committed patchset #3 (id:230001) manually as
18a41eba089cbbad7339ff76713a1acb3a8eb974 (presubmit successful).

[Siggi Cherem (dart-lang), 2017-07-14 15:09:43]

lgtm