Move tools to tool folder.

Move tools to tool folder.

R=sigmund@google.com

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

[ahe, 2017-08-23 14:53:27]

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

[ahe, 2017-08-23 15:07:18]

Added some background information.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_options.dart:220: bool throwOnErrors = false;
This causes all tools to exit with an unhandled exception if a
deprecated_inputError is raised.

The reason is that all fasta subtools are written this way:

    try {
      ...
    } on deprecated_InputError catch (e) {
      exitCode = 1;
      CompilerContext.runWithDefaultOptions(
          (c) => c.report(deprecated_InputError.toMessage(e), Severity.error));
      return null;
    }
  }

And c.report will eventually end up calling _printAndThrowIfDebugging (nee
_printAndThrowIfFatal).

So if you run "pkg/front_end/tool/fasta compile" without arguments, it prints a
usage message and then exits with an uncaught exception (compiler aborted due to
fatal errors).

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
File pkg/front_end/test/kernel_generator_test.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
pkg/front_end/test/kernel_generator_test.dart:69: // use case we have.
Please let me know if I can delete this test?

If you want to preserve it, please implement this behavior by way of a default
ErrorHandler that doesn't throw deprecated_InputError.

[Siggi Cherem (dart-lang), 2017-08-23 20:17:35]

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_options.dart:220: bool throwOnErrors = false;
On 2017/08/23 15:07:18, ahe wrote:
> This causes all tools to exit with an unhandled exception if a
> deprecated_inputError is raised.
> 
> The reason is that all fasta subtools are written this way:
> 
>     try {
>       ...
>     } on deprecated_InputError catch (e) {
>       exitCode = 1;
>       CompilerContext.runWithDefaultOptions(
>           (c) => c.report(deprecated_InputError.toMessage(e),
Severity.error));
>       return null;
>     }
>   }
> 
> And c.report will eventually end up calling _printAndThrowIfDebugging (nee
> _printAndThrowIfFatal).
> 
> So if you run "pkg/front_end/tool/fasta compile" without arguments, it prints
a
> usage message and then exits with an uncaught exception (compiler aborted due
to
> fatal errors).

I commented on  issue #30194, I think we should instead keep this default as
true for now until we have implemented a full alternative for the public APIs.
Instead, I recommend we change the tools to provide a false value to this
option. One idea to make that kind of change simpler could be to add another
kind of static method in CompilerContext or CompilerCommandLine that defines the
default set of options for command-line tools. For instance:

try {
} on deprecated_InputError catch (e) {
  ...
  CompilerContext.runWithCommandLineOptions((c) => ...);
}

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/processed_options.dart:566:
sb.writeln('throwOnWarningsAreFatal: ${throwOnWarnings}');
while you are here, let's remove AreFatal on this one too

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/command_line_reporting.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:138: void
_printAndThrowIfDebugging(
I do like these renames - just want to make sure we agree on the plan for how to
fail on errors by default in the public APIs. If we intend to fix as I suggested
on the bug, then this change lgtm, otherwise, we may want to discuss more what
to do here.

Also, maybe we should also rename the options to indicate they are only meant to
be used internally. For example:

throwOnErrors => throwOnErrorsForDebugging (or IfDebugging)

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/get_dependencies.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/get_dependencies.dart:28: Future<List<Uri>>
getDependencies(Uri script, FileSystem fileSystem,
let's remove this argument, just use `c.options.fileSystem` below instead.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/kernel_chain.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/kernel_chain.dart:73: var options = new
ProcessedOptions(new CompilerOptions());
I think we should undo this for now, but later when we can set it to default to
false, we can change this to use runWithDefaultOptions below

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
File pkg/front_end/test/fasta/tool_test.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:5: /// Tests the tool
`pkg/front_end/tool/fasta`.
yay!

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:37: const List<String> unsafeTools =
const <String>[
Please document what does it mean for a tool to be unsafe. I'm guessing that it
can modify the source-tree?

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:47: // The tool is a shell script and
only works on Mac and Linux.
should we replace it with a Dart script?

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:57: Process.runSync("/bin/bash",
<String>[toolPath, subtool]);
should we also run all tools with a good example set of arguments (e.g. a .dart
file for most things that expect one?)

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
File pkg/front_end/test/kernel_generator_test.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
pkg/front_end/test/kernel_generator_test.dart:69: // use case we have.
On 2017/08/23 15:07:18, ahe wrote:
> Please let me know if I can delete this test?
> 
> If you want to preserve it, please implement this behavior by way of a default
> ErrorHandler that doesn't throw deprecated_InputError.

I'd like to keep something to test a default-error behavior. I agree we don't
need to leak deprecated_InputError, I just want to make sure we have good
defaults for the public API. Rather than disabling it, I'd prefer to keep the
current broken behavior in the public APIs until we fix #30194 (and fix the
issue on the command-line-tools).

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
pkg/front_end/test/kernel_generator_test.dart:71: if (false) {
Might not be relevant depending on how we address things above, but instead of
if (false) {...}, you can pass `skip: true` at the end of the test declaration:

test('...', () {
   ...
  }, skip: true /* Issue XYZ */);

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/_fasta/d...
File pkg/front_end/tool/_fasta/dump_partial.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/_fasta/d...
pkg/front_end/tool/_fasta/dump_partial.dart:16: return;
thanks

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/fasta
File pkg/front_end/tool/fasta (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/fasta#ne...
pkg/front_end/tool/fasta:26: stop "'$1' isn't supported anymore, please use
'compile' instead."
we could also just delete this - not sure anyone is invoking this these days.

[Siggi Cherem (dart-lang), 2017-08-23 20:18:48]

ha it got marked "green" even though I said l-g-t-m only for a subpart below.

Big picture comment: the move makes sense and I like it, lets discuss/decide
what to do about the default-error behavior before we land it.

[ahe, 2017-08-24 09:48:59]

On 2017/08/23 20:18:48, Siggi Cherem (dart-lang) wrote:
> Big picture comment: the move makes sense and I like it, lets discuss/decide
> what to do about the default-error behavior before we land it.

Absolutely.

[ahe, 2017-08-24 11:52:02]

I've addressed most comments, except for the one thing that we need to discuss
further.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/compiler_...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/compiler_...
pkg/front_end/lib/compiler_options.dart:220: bool throwOnErrors = false;
On 2017/08/23 20:17:34, Siggi Cherem (dart-lang) wrote:
> I commented on  issue #30194, I think we should instead keep this default as
> true for now until we have implemented a full alternative for the public APIs.

I'd rather go ahead with this and let the resolution of issue 30194 deal with
the default behavior in the public API. I don't think having a stable public API
is on the critical path right now, but breaking all the command line tools is a
problem for making progress on stuff that is on the critical path.

I think this solution is preferable over reverting
b48584d3d0eb480943d72958104baf8e0e366074 that caused the breakage.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/processed_options.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/processed_options.dart:566:
sb.writeln('throwOnWarningsAreFatal: ${throwOnWarnings}');
On 2017/08/23 20:17:34, Siggi Cherem (dart-lang) wrote:
> while you are here, let's remove AreFatal on this one too

Done.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/command_line_reporting.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/command_line_reporting.dart:138: void
_printAndThrowIfDebugging(
On 2017/08/23 20:17:34, Siggi Cherem (dart-lang) wrote:
> I do like these renames - just want to make sure we agree on the plan for how
to
> fail on errors by default in the public APIs. If we intend to fix as I
suggested
> on the bug, then this change lgtm, otherwise, we may want to discuss more what
> to do here.

I don't think we agree on what the default should be in the API, but I think we
agree that it shouldn't be implemented using these hooks.

> Also, maybe we should also rename the options to indicate they are only meant
to
> be used internally. For example:
> 
> throwOnErrors => throwOnErrorsForDebugging (or IfDebugging)

That's a great idea.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/get_dependencies.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/get_dependencies.dart:28: Future<List<Uri>>
getDependencies(Uri script, FileSystem fileSystem,
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> let's remove this argument, just use `c.options.fileSystem` below instead.

Done. Great catch!

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/testing/kernel_chain.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/testing/kernel_chain.dart:73: var options = new
ProcessedOptions(new CompilerOptions());
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> I think we should undo this for now, but later when we can set it to default
to
> false, we can change this to use runWithDefaultOptions below

Acknowledged.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
File pkg/front_end/test/fasta/tool_test.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:5: /// Tests the tool
`pkg/front_end/tool/fasta`.
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> yay!

Acknowledged.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:37: const List<String> unsafeTools =
const <String>[
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> Please document what does it mean for a tool to be unsafe. I'm guessing that
it
> can modify the source-tree?

Done.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:47: // The tool is a shell script and
only works on Mac and Linux.
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> should we replace it with a Dart script?

Yes. I've added a TODO to the script.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/fasta/to...
pkg/front_end/test/fasta/tool_test.dart:57: Process.runSync("/bin/bash",
<String>[toolPath, subtool]);
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> should we also run all tools with a good example set of arguments (e.g. a
.dart
> file for most things that expect one?)

For now, I assume that happens as part of our regular development. I'm not sure
that's entirely correct, but at least this test is covering what is being
addressed in the CL.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
File pkg/front_end/test/kernel_generator_test.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
pkg/front_end/test/kernel_generator_test.dart:69: // use case we have.
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> On 2017/08/23 15:07:18, ahe wrote:
> > Please let me know if I can delete this test?
> > 
> > If you want to preserve it, please implement this behavior by way of a
default
> > ErrorHandler that doesn't throw deprecated_InputError.
> 
> I'd like to keep something to test a default-error behavior. I agree we don't
> need to leak deprecated_InputError, I just want to make sure we have good
> defaults for the public API. Rather than disabling it, I'd prefer to keep the
> current broken behavior in the public APIs until we fix #30194 (and fix the
> issue on the command-line-tools).

I'd rather wait until you have time to implement throwOnErrors correctly.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/test/kernel_g...
pkg/front_end/test/kernel_generator_test.dart:71: if (false) {
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> Might not be relevant depending on how we address things above, but instead of
> if (false) {...}, you can pass `skip: true` at the end of the test
declaration:
> 
> test('...', () {
>    ...
>   }, skip: true /* Issue XYZ */);

Done.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/_fasta/d...
File pkg/front_end/tool/_fasta/dump_partial.dart (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/_fasta/d...
pkg/front_end/tool/_fasta/dump_partial.dart:16: return;
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> thanks

Acknowledged.

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/fasta
File pkg/front_end/tool/fasta (right):

https://codereview.chromium.org/3003743002/diff/1/pkg/front_end/tool/fasta#ne...
pkg/front_end/tool/fasta:26: stop "'$1' isn't supported anymore, please use
'compile' instead."
On 2017/08/23 20:17:35, Siggi Cherem (dart-lang) wrote:
> we could also just delete this - not sure anyone is invoking this these days.

That's very likely. I think I'll remove this and "run" when I implement a Dart
version of this script.

[Siggi Cherem (dart-lang), 2017-08-24 18:30:22]

lgtm - as discussed offline, we'll defer this until we fix the issue by adding
some mechanism to allow users to configure whether to enable or not error
recovery, and include a result object with all the information they need. We can
decide at that point what the default value should be.

[Siggi Cherem (dart-lang), 2017-08-24 18:34:36]

in case it was unclear: lgtm to land as it is.

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

https://codereview.chromium.org/3003743002/diff/20001/pkg/front_end/lib/src/b...
pkg/front_end/lib/src/base/processed_options.dart:552: '${_raw.chaseDependencies
== null ? null : !_raw.chaseDependencies})');
thanks - I also have this in a local branch and forgot to send this out.

[ahe, 2017-08-29 12:30:58]

Description was changed from

==========
Move tools to tool folder.
==========

to

==========
Move tools to tool folder.

R=sigmund@google.com

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

[ahe, 2017-08-29 12:30:59]

Committed patchset #2 (id:20001) manually as
a5bcf58b064bd032af98b32ca37a15d0cb57d9d6 (presubmit successful).