Add option --compile-sdk to fasta compile and run.

Add option --compile-sdk to fasta compile and run.

R=sigmund@google.com

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

[ahe, 2017-02-03 14:37:12]

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

[ahe, 2017-02-03 14:37:12]

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:272: if (isFullProgram) {
If we're generated an outline, we shouldn't add an executable main method.

[ahe, 2017-02-03 15:41:33]

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2246: push(new
Block(<Statement>[new InvalidStatement()]));
This is the next thing I want to implement. It should be good enough for
measuring performance, so I sent it out now.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:138: if
(part.fileUri != null) {
This should be == or part.fileUri ??= newFileUri.

[Siggi Cherem (dart-lang), 2017-02-03 18:16:58]

lgtm

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:28: import
'../parser/dart_vm_native.dart' show
for consistency with the imports above, maybe change into
'package:front_end/src/fasta/parser/dart_vm_native.dart'?

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:2239: //  
currentMember.addAnnotation(new ast.ConstructorInvocation(
I see! and I just saw your response on the other CL too. Thanks!

I agree, it would be great to switch to annotations once they don't read from
source. Given this, +100 on not including the native changes in the main parser
- that part of the logic can be deleted once we can switch over to annotations.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:272: if (isFullProgram) {
On 2017/02/03 14:37:12, ahe wrote:
> If we're generated an outline, we shouldn't add an executable main method.

makes sense - same thing in the future when generating a single library or a set
of libraries as a separate module.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/diet_listener.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/diet_listener.dart:472: if (isDartLibrary &&
kind == ErrorKind.ExpectedBlockToSkip) {
I am guessing the recovery below will be null in the case we are reading
dart2js's patch files, but would it make sense to have a check here to ensure we
attempt this error recovery in the context of the vm patch files only?

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:542: Token
handleUnrecoverableError(Token token, ErrorKind kind, Map arguments) {
to avoid the duplication, maybe move these 2 methods to a mixin?

For example adding a NativeClauseRecovery class to dart_vm_native.dart?

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:138: if
(part.fileUri != null) {
On 2017/02/03 15:41:33, ahe wrote:
> This should be == or part.fileUri ??= newFileUri.

Not sure I follow here - what is the underlying issue?

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

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/translate_uri.dart:41: // This list is generated
with [bin/generate_dart_libraries.dart] below.
I think we need to do a few changes regarding how we generate this. I'm OK
landing the changes you have here and making this later, but I don't want to
delay on this too long.

In particular, at some point Sigurd changed dart2js to no longer use the
libraries in sdk_library_metadata and consume the dart_*.platform files. I
recall his change was motivated by dartino. As a result currently no one reads
in the library medatadata from the sdk_library_metadata package anymore.

I honestly like the change Sigurd started: I like using a data text file and not
using .dart for this. I personally don't mind the the .ini style format. I
recall you had concerns about it. In case it's relevant here is the code that
parses it, the code is pretty small
https://github.com/dart-lang/sdk/blob/master/pkg/compiler/lib/src/platform_co....

If we were to change the format, I prefer something that has comment syntax on
it. Plain json is not my favorite, we could do json + comments (preprocess a
json text to strip the comments out) or yaml.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/translate_uri.dart:45: Map<String, Uri>
dartLibraries = {
You probably need to add a couple VM libraries here that are not declared in
either the *.platform files nor the sdk_library_metadata code.  Note that two of
these have their code in runtime/bin (these are not patch files, but the actual
library definition):


'dart:_builtin'     -> 'runtime/bin/builtin.dart'
'dart:vmservice_io' -> 'runtime/bin/vmservice/vmservice_io.dart'
'dart:_vmservice'   -> 'sdk/lib/vmservice/vmservice.dart'
'dart:profiler'     -> 'sdk/lib/profiler/profiler.dart'


dart:profiler is deprecated and empty, but I still see logic to keep imports to
it working in random places.

We need to address the location of these libraries under 'runtime/...' (and of
the vm patches too), since we need to be able to ship them in the future. Kevin
has more context, so ask him about it too. My initial inclination was to move
them to the sdk/lib/ folder, but Kevin pointed out that flutter also has more
`dart:*` libraries which wont ship in the sdk folder either. I believe he is
suggesting that we have another location that is configurable so we can easily
apply one kind of patches or another.

[ahe, 2017-02-03 19:34:14]

Thank you, Siggi!

I haven't uploaded a new patch set yet.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/body_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/body_builder.dart:28: import
'../parser/dart_vm_native.dart' show
On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> for consistency with the imports above, maybe change into
> 'package:front_end/src/fasta/parser/dart_vm_native.dart'?

I was actually planning on sending out a CL where I remove package:front_end
imports from all fasta files. WDYT?

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:272: if (isFullProgram) {
On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> On 2017/02/03 14:37:12, ahe wrote:
> > If we're generated an outline, we shouldn't add an executable main method.
> 
> makes sense - same thing in the future when generating a single library or a
set
> of libraries as a separate module.

Good point. I should turn this into a comment. Also, another note would be to
say that there should probably be a way to record a compile time error in dill
file.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/outline_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/outline_builder.dart:542: Token
handleUnrecoverableError(Token token, ErrorKind kind, Map arguments) {
On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> to avoid the duplication, maybe move these 2 methods to a mixin?
> 
> For example adding a NativeClauseRecovery class to dart_vm_native.dart?

I'm avoiding mixins for critical code based on something Stephen told me about
problems in dart2js. I've been thinking about moving this to StackListener
instead.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:138: if
(part.fileUri != null) {
On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> On 2017/02/03 15:41:33, ahe wrote:
> > This should be == or part.fileUri ??= newFileUri.
> 
> Not sure I follow here - what is the underlying issue?

I should set the fileUri if it *is* null. Not when it already has a value. If
you want to use this for benchmarking, you need to change this.

I'm actually cleaning this up right now.

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

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/translate_uri.dart:41: // This list is generated
with [bin/generate_dart_libraries.dart] below.
On 2017/02/03 18:16:58, Siggi Cherem (dart-lang) wrote:
> I think we need to do a few changes regarding how we generate this. I'm OK
> landing the changes you have here and making this later, but I don't want to
> delay on this too long.
> 
> In particular, at some point Sigurd changed dart2js to no longer use the
> libraries in sdk_library_metadata and consume the dart_*.platform files. I
> recall his change was motivated by dartino. As a result currently no one reads
> in the library medatadata from the sdk_library_metadata package anymore.
> 
> I honestly like the change Sigurd started: I like using a data text file and
not
> using .dart for this. I personally don't mind the the .ini style format. I
> recall you had concerns about it. In case it's relevant here is the code that
> parses it, the code is pretty small
>
https://github.com/dart-lang/sdk/blob/master/pkg/compiler/lib/src/platform_co....

I started using the .platform files Sigurd started. However, I couldn't find any
for the VM, only dart2js.

I also recalled discussing this with someone recently (most likely you), and I
thought the conclusion was that nobody is actually using the .platform files
right now.

It's getting late here, so I'd appreciate if you could look into which file to
actually use here. I'll change bin/generate_dart_libraries.dart accordingly. I
don't mind using platfom_configuration.dart, that's the entire point of
generating this list instead of reading it at start up.

> If we were to change the format, I prefer something that has comment syntax on
> it. Plain json is not my favorite, we could do json + comments (preprocess a
> json text to strip the comments out) or yaml.

I agree. I think YAML is our best bet. JSON + comments is horrible for reporing
errors (even if JSON.decode provided offsets, we'd then have to do some kind of
source mapping). I think we can more easily add source offset for error
reporting to package:yaml (if it isn't there already).

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/translate_uri.dart:45: Map<String, Uri>
dartLibraries = {
On 2017/02/03 18:16:58, Siggi Cherem (dart-lang) wrote:
> You probably need to add a couple VM libraries here that are not declared in
> either the *.platform files nor the sdk_library_metadata code.  Note that two
of
> these have their code in runtime/bin (these are not patch files, but the
actual
> library definition):
> 
> 
> 'dart:_builtin'     -> 'runtime/bin/builtin.dart'
> 'dart:vmservice_io' -> 'runtime/bin/vmservice/vmservice_io.dart'
> 'dart:_vmservice'   -> 'sdk/lib/vmservice/vmservice.dart'
> 'dart:profiler'     -> 'sdk/lib/profiler/profiler.dart'
> 
> 
> dart:profiler is deprecated and empty, but I still see logic to keep imports
to
> it working in random places.
> 
> We need to address the location of these libraries under 'runtime/...' (and of
> the vm patches too), since we need to be able to ship them in the future.
Kevin
> has more context, so ask him about it too. My initial inclination was to move
> them to the sdk/lib/ folder, but Kevin pointed out that flutter also has more
> `dart:*` libraries which wont ship in the sdk folder either. I believe he is
> suggesting that we have another location that is configurable so we can easily
> apply one kind of patches or another.

I agree.

All this will be easier to clean up once package:front_end is always in charge
of reading sources, and that's when I'd prefer to clean it up. This way we don't
need to worry about changing the corresponding C++ code.

One caveat, though: I'm not even sure this option should be supported in a
shipped SDK. In that situation, I imagine that will have platform.dill built
into the executable as a binary resource file that fasta can read using some TBD
API.

[ahe, 2017-02-03 20:03:36]

I'll follow up with these issues later:

* move duplicated code to StackListener.

* package:front_end consistency.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:272: if (isFullProgram) {
On 2017/02/03 19:34:14, ahe wrote:
> On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> > On 2017/02/03 14:37:12, ahe wrote:
> > > If we're generated an outline, we shouldn't add an executable main method.
> > 
> > makes sense - same thing in the future when generating a single library or a
> set
> > of libraries as a separate module.
> 
> Good point. I should turn this into a comment. Also, another note would be to
> say that there should probably be a way to record a compile time error in dill
> file.

Done.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/diet_listener.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/diet_listener.dart:472: if (isDartLibrary &&
kind == ErrorKind.ExpectedBlockToSkip) {
On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> I am guessing the recovery below will be null in the case we are reading
> dart2js's patch files, but would it make sense to have a check here to ensure
we
> attempt this error recovery in the context of the vm patch files only?

Done.

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:138: if
(part.fileUri != null) {
On 2017/02/03 19:34:14, ahe wrote:
> On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> > On 2017/02/03 15:41:33, ahe wrote:
> > > This should be == or part.fileUri ??= newFileUri.
> > 
> > Not sure I follow here - what is the underlying issue?
> 
> I should set the fileUri if it *is* null. Not when it already has a value. If
> you want to use this for benchmarking, you need to change this.
> 
> I'm actually cleaning this up right now.

Done.

[Siggi Cherem (dart-lang), 2017-02-03 20:25:26]

thanks! still lgtm

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:138: if
(part.fileUri != null) {
On 2017/02/03 20:03:36, ahe wrote:
> On 2017/02/03 19:34:14, ahe wrote:
> > On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> > > On 2017/02/03 15:41:33, ahe wrote:
> > > > This should be == or part.fileUri ??= newFileUri.
> > > 
> > > Not sure I follow here - what is the underlying issue?
> > 
> > I should set the fileUri if it *is* null. Not when it already has a value.
If
> > you want to use this for benchmarking, you need to change this.
> > 
> > I'm actually cleaning this up right now.
> 
> Done.

ah - I get it now - your comment was basically a TODO to come back to the code
:)

I never used the comments to remind myself of things to do on my CLs, it's a
good idea. Maybe prefix them with "note to self:" or "TODO:" just so that we
know ;-)

[ahe, 2017-02-03 20:35:44]

Thanks

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/2676823002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:138: if
(part.fileUri != null) {
On 2017/02/03 20:25:26, Siggi Cherem (dart-lang) wrote:
> On 2017/02/03 20:03:36, ahe wrote:
> > On 2017/02/03 19:34:14, ahe wrote:
> > > On 2017/02/03 18:16:57, Siggi Cherem (dart-lang) wrote:
> > > > On 2017/02/03 15:41:33, ahe wrote:
> > > > > This should be == or part.fileUri ??= newFileUri.
> > > > 
> > > > Not sure I follow here - what is the underlying issue?
> > > 
> > > I should set the fileUri if it *is* null. Not when it already has a value.
> If
> > > you want to use this for benchmarking, you need to change this.
> > > 
> > > I'm actually cleaning this up right now.
> > 
> > Done.
> 
> ah - I get it now - your comment was basically a TODO to come back to the code
> :)
> 
> I never used the comments to remind myself of things to do on my CLs, it's a
> good idea. Maybe prefix them with "note to self:" or "TODO:" just so that we
> know ;-)

Yes. As soon as I had sent the comment, I realized I should have said "note to
self". Next time I'll try to react on that thought as well :-)

[ahe, 2017-02-06 10:20:42]

Description was changed from

==========
Add option --compile-sdk to fasta compile and run.
==========

to

==========
Add option --compile-sdk to fasta compile and run.

R=sigmund@google.com

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

[ahe, 2017-02-06 10:20:43]

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