Add interfaces for a new compiler API.

Add interfaces for a new compiler API.

Main changes are in the added files, compiler/compiler.dart, src/compiler.dart and src/apiimpl.dart.

Start by looking at compiler/compiler_new.dart and compiler/compiler.dart.

BUG=
R=floitsch@google.com

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

[Johnni Winther, 2015-07-13 15:34:24]

johnniwinther@google.com changed reviewers:
+ floitsch@google.com

[Siggi Cherem (dart-lang), 2015-07-13 17:28:36]

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

[Siggi Cherem (dart-lang), 2015-07-13 17:28:37]

nice!

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
File pkg/compiler/lib/compiler_new.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:57: EventSink<String> createEventSink(String
name, String extension);
EventSink seems like an implementation detail of how we produce the contents of
the output, rather than the thing we intend to create.

So a few comments/ideas here:
- let's change this to something that describes that we are creating a compiler
output. For example, `createOutputStream` or `createOutput`?
- do we need the `addError` API in EventSink? If not, could the return type be
simply Sink?
- (this is a bit more radical): It's seems a bit reverse that the compiler asks
an external interface for how to create the output stream. What if we invert the
relation. For example, have something like:

  Future emitOutput(String name, String extension, data);

(where data could be a string buffer or a Stream that we create ourselves)

Thoughts?

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:64: /// [begin] and [end]. No other arguments
may be `null`. If [uri] is not
given that `uri, begin, end` may be null, should we put them at the end as
optional args?

void reportDiagnostic(String message, Diagnostic kind,
  [Uri uri, int begin, int end]);

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:70: void reportDiagnostic(Uri uri, int begin,
int end,
Consider renaming to just `report`?

(given that the class is already called *Diagnostics, most likely the field
we'll use to store this will be named something like that too, so I expect we'll
have context whenever we see this)

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:84: final compiler;
I know this is currently used in tests, but besides that, are there other
reasons to keep this? It would be nice to eventually get rid it here.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:90: class CompilerOptions {
Yay!!!!!

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:96: final List<String> options;
Maybe add a TODO that this list of options should go away - all options should
be explicitly modeled here.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
File pkg/compiler/lib/src/source_file_provider.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
pkg/compiler/lib/src/source_file_provider.dart:116: Future<List<int>> call(Uri
resourceUri) => readFromUri(resourceUri);
After we are done with the refactor, will we need the `call` API anymore? add a
TODO?

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
pkg/compiler/lib/src/source_file_provider.dart:233: void call(Uri uri, int
begin, int end, String message, api.Diagnostic kind) {
same here

[Johnni Winther, 2015-07-13 17:45:19]

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
File pkg/compiler/lib/compiler_new.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:57: EventSink<String> createEventSink(String
name, String extension);
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> EventSink seems like an implementation detail of how we produce the contents
of
> the output, rather than the thing we intend to create.
> 
> So a few comments/ideas here:
> - let's change this to something that describes that we are creating a
compiler
> output. For example, `createOutputStream` or `createOutput`?
> - do we need the `addError` API in EventSink? If not, could the return type be
> simply Sink?
> - (this is a bit more radical): It's seems a bit reverse that the compiler
asks
> an external interface for how to create the output stream. What if we invert
the
> relation. For example, have something like:
> 
>   Future emitOutput(String name, String extension, data);
> 
> (where data could be a string buffer or a Stream that we create ourselves)
> 
> Thoughts?

EventSink _is_ going away. My plan was to add a Output interface of our own with
the functionality we want.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:64: /// [begin] and [end]. No other arguments
may be `null`. If [uri] is not
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> given that `uri, begin, end` may be null, should we put them at the end as
> optional args?
> 
> void reportDiagnostic(String message, Diagnostic kind,
>   [Uri uri, int begin, int end]);

My plan is to use more structured objects instead of adding more arguments. More
to come in a follow-up.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:70: void reportDiagnostic(Uri uri, int begin,
int end,
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> Consider renaming to just `report`?
> 
> (given that the class is already called *Diagnostics, most likely the field
> we'll use to store this will be named something like that too, so I expect
we'll
> have context whenever we see this)

Good idea.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:84: final compiler;
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> I know this is currently used in tests, but besides that, are there other
> reasons to keep this? It would be nice to eventually get rid it here.

Do you have any ideas for an alternative way of supporting tests?

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:96: final List<String> options;
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> Maybe add a TODO that this list of options should go away - all options should
> be explicitly modeled here.

Not all options. We need a backdoor for experimental/temporary options. You'll
see in a follow-up :)

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
File pkg/compiler/lib/src/source_file_provider.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
pkg/compiler/lib/src/source_file_provider.dart:116: Future<List<int>> call(Uri
resourceUri) => readFromUri(resourceUri);
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> After we are done with the refactor, will we need the `call` API anymore? add
a
> TODO?

These _are_ going away. Adding TODOs.

[Siggi Cherem (dart-lang), 2015-07-13 18:08:06]

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
File pkg/compiler/lib/compiler_new.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:84: final compiler;
On 2015/07/13 17:45:19, Johnni Winther wrote:
> On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> > I know this is currently used in tests, but besides that, are there other
> > reasons to keep this? It would be nice to eventually get rid it here.
> 
> Do you have any ideas for an alternative way of supporting tests?

Nothing concrete at this time. I have to look closer at how and where we use it
in tests. One option might be to include the compiler, but only on the test side
of things (either by returning it when we create it, or by wrapping the
CompilerResult in a TestCompilerResult that includes the compiler). I'm also
curious about what compiler APIs we access from tests and whether to expose only
the subset that is needed.

[floitsch, 2015-07-13 18:16:49]

LGTM.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
File pkg/compiler/lib/compiler_new.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:144: /// If the compilation fails, the
future's `CompilationResult.isSuccess` will
is `false` ... is invoked

[Johnni Winther, 2015-07-13 19:02:22]

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
File pkg/compiler/lib/compiler_new.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/compiler_n...
pkg/compiler/lib/compiler_new.dart:144: /// If the compilation fails, the
future's `CompilationResult.isSuccess` will
On 2015/07/13 18:16:49, floitsch wrote:
> is `false` ... is invoked

Done.

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
File pkg/compiler/lib/src/source_file_provider.dart (right):

https://codereview.chromium.org/1235563003/diff/1/pkg/compiler/lib/src/source...
pkg/compiler/lib/src/source_file_provider.dart:233: void call(Uri uri, int
begin, int end, String message, api.Diagnostic kind) {
On 2015/07/13 17:28:37, Siggi Cherem (dart-lang) wrote:
> same here

Done.

[Johnni Winther, 2015-07-13 19:15:34]

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

[Siggi Cherem (dart-lang), 2015-07-13 19:18:35]

lgtm too