Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service

Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator. Code has some modifications:
     it uses a single canonical-root when loading summaries, and
     it supports generating both outlines and kernel in one go.

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed, and I stopped covering it in
     most tests (now only covered in shaker tests). The plan is to add
     tests for the public API covering this in the future.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN

 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing

R=johnniwinther@google.com, paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-06-26 23:36:06]

Description was changed from

==========
(preview only) Tweak public APIs and start using them in patch_sdk and dart2js
unit tests.
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and starts using those APIs
outside the package. Now `dill_target` is not mentioned outside pkg/front_end
and the analyzer_target.

Changes:
 - package:front_end

    1. add kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

    2. removed code that didn't belong here
         2.a. calculating deps for .GN moved to patch_sdk
         2.b. vm-specific outcomes moved to kernel-service

    3. updated how `native` is implemented, so we can more easily support
dart2js and ddc


 - patch_sdk
    1. use the public API
    2. moved here logic internal to .GN
  
 - kernel service
    1. use the public API
    2. moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
    1. use the public API in tools and unit tests
    2. simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
    1. updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-26 23:36:40]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and starts using those APIs
outside the package. Now `dill_target` is not mentioned outside pkg/front_end
and the analyzer_target.

Changes:
 - package:front_end

    1. add kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

    2. removed code that didn't belong here
         2.a. calculating deps for .GN moved to patch_sdk
         2.b. vm-specific outcomes moved to kernel-service

    3. updated how `native` is implemented, so we can more easily support
dart2js and ddc


 - patch_sdk
    1. use the public API
    2. moved here logic internal to .GN
  
 - kernel service
    1. use the public API
    2. moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
    1. use the public API in tools and unit tests
    2. simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
    1. updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and starts using those APIs
outside the package. Now `dill_target` is not mentioned outside pkg/front_end
and the analyzer_target.

Changes:
 - package:front_end

    1. add kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

    2. removed code that didn't belong here
         2.a. calculating deps for .GN moved to patch_sdk
         2.b. vm-specific outcomes moved to kernel-service

    3. updated how `native` is implemented, so we can more easily support
dart2js and ddc
    4. updated how we check where `int`, `bool`, etc can be defined.


 - patch_sdk
    1. use the public API
    2. moved here logic internal to .GN
  
 - kernel service
    1. use the public API
    2. moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
    1. use the public API in tools and unit tests
    2. simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
    1. updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-26 23:46:25]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and starts using those APIs
outside the package. Now `dill_target` is not mentioned outside pkg/front_end
and the analyzer_target.

Changes:
 - package:front_end

    1. add kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

    2. removed code that didn't belong here
         2.a. calculating deps for .GN moved to patch_sdk
         2.b. vm-specific outcomes moved to kernel-service

    3. updated how `native` is implemented, so we can more easily support
dart2js and ddc
    4. updated how we check where `int`, `bool`, etc can be defined.


 - patch_sdk
    1. use the public API
    2. moved here logic internal to .GN
  
 - kernel service
    1. use the public API
    2. moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
    1. use the public API in tools and unit tests
    2. simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
    1. updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and starts using those APIs
outside the package. Now `dill_target` is not mentioned outside pkg/front_end
and the analyzer_target.

Changes:
 - package:front_end

    1. add kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

    2. removed code that didn't belong here
         2.a. calculating deps for .GN moved to patch_sdk
         2.b. vm-specific outcomes moved to kernel-service

    3. updated how `native` is implemented, so we can more easily support
dart2js and ddc
    4. updated how we check where `int`, `bool`, etc can be defined.


 - patch_sdk
    1. use the public API to craete platform.dill and vmservice_io.dill
    2. moved here logic internal to .GN
  
 - kernel service
    1. use the public API
    2. moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
    1. use the public API in tools and unit tests
    2. simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
    1. updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-27 00:29:43]

Patchset #1 (id:1) has been deleted

[Siggi Cherem (dart-lang), 2017-06-27 00:29:48]

Patchset #1 (id:20001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-27 00:29:57]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-27 00:30:07]

Patchset #1 (id:40001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-28 00:31:42]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and starts using those APIs
outside the package. Now `dill_target` is not mentioned outside pkg/front_end
and the analyzer_target.

Changes:
 - package:front_end

    1. add kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

    2. removed code that didn't belong here
         2.a. calculating deps for .GN moved to patch_sdk
         2.b. vm-specific outcomes moved to kernel-service

    3. updated how `native` is implemented, so we can more easily support
dart2js and ddc
    4. updated how we check where `int`, `bool`, etc can be defined.


 - patch_sdk
    1. use the public API to craete platform.dill and vmservice_io.dill
    2. moved here logic internal to .GN
  
 - kernel service
    1. use the public API
    2. moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
    1. use the public API in tools and unit tests
    2. simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
    1. updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily support dart2js
and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds ('chaseDependencies =
false' option)

   * removed `uriToSource` when serializing outlines

   * added some unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now 500K insted
of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-28 00:32:35]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that used to be in
kernel_generator and let us generate both outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily support dart2js
and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds ('chaseDependencies =
false' option)

   * removed `uriToSource` when serializing outlines

   * added some unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now 500K insted
of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no need anymore to extend internals of
fasta

 - package:kernel
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator and let us generate both
     outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * removed `uriToSource` when serializing outlines

   * added some unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-29 00:15:40]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator and let us generate both
     outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * removed `uriToSource` when serializing outlines

   * added some unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator and let us generate both
     outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-29 00:15:51]

Patchset #1 (id:60001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-29 00:15:56]

Patchset #1 (id:80001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-29 00:16:03]

Patchset #1 (id:100001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-29 03:02:54]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator and let us generate both
     outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator and let us generate both
     outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed or used in unit tests.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-29 03:05:35]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator and let us generate both
     outlines and kernel in one go

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed or used in unit tests.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator. Code has some modifications:
     it uses a single canonical-root when loading summaries, and
     it supports generating both outlines and kernel in one go.

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed or used in unit tests.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-29 03:11:21]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator. Code has some modifications:
     it uses a single canonical-root when loading summaries, and
     it supports generating both outlines and kernel in one go.

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed or used in unit tests.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator. Code has some modifications:
     it uses a single canonical-root when loading summaries, and
     it supports generating both outlines and kernel in one go.

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed, and I stopped covering it in
     most tests (now only covered in shaker tests). The plan is to add
     tests for the public API covering this in the future.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

[Siggi Cherem (dart-lang), 2017-06-29 03:26:22]

Patchset #1 (id:120001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-29 03:38:40]

sigmund@google.com changed reviewers:
+ aam@google.com, ahe@google.com, asiva@google.com, johnniwinther@google.com,
kmillikin@google.com, paulberry@google.com

[Siggi Cherem (dart-lang), 2017-06-29 03:38:41]

Note: I wrote a general summary of the changes in the CL description, I also
wrote in the code comments of the form "/// CODE REVIEW COMMENTS", I will
deleting them before submitting.

Peter/Paul - I would appreciate if both of you to take a general look at the API
design and use cases.

Peter - please also take a closer look at the internal fasta changes (like
native support).

Kevin - you only need to review the changes in package:kernel

Johnni - only review changes in package:compiler

Siva/Alex - mostly FYI, up to you if you want to review changes to
kernel-service and to patch_sdk. The changes to kernel-service and patch_sdk
have no semantic effect, but there was one one small change: kernel-service used
to strip out the sdk code from scripts that it sent over the wire, now it also
strips out sdk string tables and other data that was unnecessary.

[ahe, 2017-06-29 11:09:15]

Comments so far. I'm working my way through the API, and I expect to work on
this review for several days (subject to your feedback tonight).

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:19: typedef void
ErrorHandler(CompilationError error);
Use new syntax?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:31: /// Map of `dart.xyz` libraries to
URIs in the [fileSystem].
dart:xyz.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:37: Map<String, Uri> dartLibraries;
I think the only option should be a single URI that points to the .json file.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:48: /// package_config search algorithm.
What is the "standard package_config search algorithm"? Where can one find more
information?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:51: /// will be used.
This seems brittle. I think it would be better to define a custom URI scheme
that signals "use the algorithm," and have null mean "don't search."

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:68: /// separately. Programs should be
closed an acyclic: any libraries that they
an -> and.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:71: /// CODE REVIEW COMMENT: this is
used for the dart2js use case: they want to
This comment could be rewritten to something like:

For example, dart2js wants a linked version of the SDK, but the Dart VM ...

This way we can keep the information here.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:86: /// Any Uri that resolves to
"multi-root:///$rest" will be searched for
Is this scheme registered? If not, we should use "org.dartlang.multi-root".

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:87: /// at "$root/$rest", where "$root"
is drawn from this list.
We should use Uri.resolve, not string concatenation.

Also, what happens if $rest starts with "../"?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:108: /// The byte storage to get and put
serialized data.
"get and put" -> "access"

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:129: /// not described in a summary as
if it was explicitly listed as an input.
This reminds me of the completely broken default behavior in javac. This doesn't
work because people think it's a build system (but it isn't). We should revisit
this.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:133: bool strongMode = true;
This should be a per-package option, not a global option.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:137: /// Keys on this map are expected
to be `dart:*` URIs. The values can be
This is inconsistent with dartLibraries above, which removes the leading dart:.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:140: Map<Uri, List<Uri>> targetPatches =
{};
Think this should be single URI that points to a file instead.

[Johnni Winther, 2017-06-29 12:44:26]

package:compiler changes LGTM

[Paul Berry, 2017-06-29 18:51:08]

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:51: /// will be used.
On 2017/06/29 11:09:15, ahe wrote:
> This seems brittle. I think it would be better to define a custom URI scheme
> that signals "use the algorithm," and have null mean "don't search."

Personally I'm not a fan of custom URI schemes acting as a sentinel.  If we want
to change the current behavior, I'd suggest adding an enum that specifies how
the .packages file should be found.

Regardless of what we do, I hope that we can make the default behavior be to use
the standard package_config search algorithm, because that's what I think a
client is most likely to expect.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:157: /// tracking).
Clarify whether these are shown by print()ing them, or by some other reporting
API

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:161: /// are well formed.
If verification fails, how is the error reported?  Through onError or some other
mechanism?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:166: bool debugDump = false;
Clarify whether the dump is shown by print() or through some other mechanism.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:36: options = new
CompilerOptions.from(options)..chaseDependencies = true;
This concerns me a lot.  If someone calls this API and deliberately sets
`chaseDependencies` to false, we should respect that option.

Maybe it would be better to change the default value of `chaseDependencies` to
`null`, so that `true` means "yes, chase dependencies", `false` means "no, be
hermetic", and `null` means "do whatever is the default behavior for the API
you're using.  Then we could move the behavior of providing the default into
ProcessedOptions.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:37: var program = (await
generateKernel([source], options))?.program;
I would feel a lot safer if we could create a ProcessedOptions object here and
pass it into generateKernel.  That way there is no danger of generateKernel
accidentally bypassing ProcessedOptions and trying to read data out of the raw
options object by mistake.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:81: return (await
generateKernel(sources, options))?.program;
Similar issue here.

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

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:153:
_inputSummariesPrograms.add(await loadProgram(uri, nameRoot));
Previously, the use of Future.wait() would mean that multiple I/O operations got
scheduled concurrently.  With this change, we serialize the I/O.  Our experience
with analysis server is that such a change can really slow things down if you
are reading from a network filesystem.  Is this an intentional behavior change? 
If so, what's the advantage?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:166:
_linkedDependencies.add(await loadProgram(uri, nameRoot));
Similar concern here.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:175: var result =
loadProgramFromBytes(bytes, new Program(nameRoot: nameRoot));
Nit: why not just

return loadProgramFromBytes(...);

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:50: fs = new
HermeticFileSystem(sources.toSet(), fs);
It would be cool if we moved this logic into ProcessedOptions, so that we could
be confident that *all* of the front_end APIs are properly hermetic when
chaseDependencies is `true`.

Incidentally, I think in the long run we're going to have other reasons to wrap
the filesystem as well, e.g. to implement "multi-root" functionality.  That
logic should go into ProcessedOptions too, so that all the APIs get the benefit
of it.

[Siggi Cherem (dart-lang), 2017-06-30 03:58:15]

Patchset #2 (id:160001) has been deleted

[Siggi Cherem (dart-lang), 2017-06-30 04:12:02]

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:19: typedef void
ErrorHandler(CompilationError error);
On 2017/06/29 11:09:14, ahe wrote:
> Use new syntax?

Is the syntax supported everywhere already? happy to do that in a follow up. I'm
also considering using a different API for diagnostics btw.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:31: /// Map of `dart.xyz` libraries to
URIs in the [fileSystem].
On 2017/06/29 11:09:15, ahe wrote:
> dart:xyz.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:37: Map<String, Uri> dartLibraries;
On 2017/06/29 11:09:15, ahe wrote:
> I think the only option should be a single URI that points to the .json file.

Happy to look into this in a follow-up CL and discuss further. I added a comment
in the TODO about this.

In particular:
 - I like having fewer ways to specify the sdk libraries if they are redundant
with each other, once I add the explicit .json URI option, there is potential
for redundancy here.

 - the Map is a very easy to use API in testing, but I commonly have to create a
memory-file system in those cases, adding a json file in that file system seems
as easy and not a big issue.

We can discuss more together with Paul about this - there might be other
motivations for also supporting a Map API.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:48: /// package_config search algorithm.
On 2017/06/29 11:09:15, ahe wrote:
> What is the "standard package_config search algorithm"? Where can one find
more
> information?

Basically the language team provided a default implementation for resolving
.packages in `package:package_config`. The code is here:

https://github.com/dart-lang/package_config

and the algorithm is described here:

https://github.com/lrhn/dep-pkgspec/blob/master/DEP-pkgspec.md#resolving-pack...

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:51: /// will be used.
On 2017/06/29 18:51:07, Paul Berry wrote:
> On 2017/06/29 11:09:15, ahe wrote:
> > This seems brittle. I think it would be better to define a custom URI scheme
> > that signals "use the algorithm," and have null mean "don't search."
> 
> Personally I'm not a fan of custom URI schemes acting as a sentinel.  If we
want
> to change the current behavior, I'd suggest adding an enum that specifies how
> the .packages file should be found.
> 
> Regardless of what we do, I hope that we can make the default behavior be to
use
> the standard package_config search algorithm, because that's what I think a
> client is most likely to expect.

Ha, I had not noticed the special behavior with the empty path.

Let's also put this on a separate CL. My inclination is similar to Paul's:

I'd like the default (`null`) to be the most common behavior of users of the
API, most likely that is to do the lookup and default to an empty map if nothing
is found - we wouldn't issue an immediate error if no .packages is found, but
only later if you try to load a `package:*` URI. 

Paul - do you feel we still need the other option ("pretend it is empty"?).
Because I didn't see this, I've been creating an empty file in the unit tests
already.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:68: /// separately. Programs should be
closed an acyclic: any libraries that they
On 2017/06/29 11:09:15, ahe wrote:
> an -> and.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:71: /// CODE REVIEW COMMENT: this is
used for the dart2js use case: they want to
On 2017/06/29 11:09:14, ahe wrote:
> This comment could be rewritten to something like:
> 
> For example, dart2js wants a linked version of the SDK, but the Dart VM ...
> 
> This way we can keep the information here.

thanks for the suggestion. Updated the doc.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:86: /// Any Uri that resolves to
"multi-root:///$rest" will be searched for
On 2017/06/29 11:09:15, ahe wrote:
> Is this scheme registered? If not, we should use "org.dartlang.multi-root".

No, it should be possible to change it, let's discuss in person. 

For background, this is not used yet, but we will this quarter.

We implemented something similar in dart2js with bazel last year. Instead of a
scheme, we used a prefix of the form:

  file:///bazel-root/

but I like the idea of switching to a scheme because it makes it easier to work
with the URIs afterwards (no need to manipulate the path, just drop the scheme).

Note: besides being useful to find multiple roots, this also helps work with
absolute URIs in a way that is agnostic of the underlying machine directory
layout. Like the `app:` scheme used in package:kernel. This is helpful for
distributed build systems.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:87: /// at "$root/$rest", where "$root"
is drawn from this list.
On 2017/06/29 11:09:14, ahe wrote:
> We should use Uri.resolve, not string concatenation.
> 
> Also, what happens if $rest starts with "../"?

note that $rest was meant to be an absolute path. I switched "rest" to
"absolute-path" in the comment to make this clearer. In that context, resolve or
joining the paths should be equivalent, but I can reword it if you prefer.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:108: /// The byte storage to get and put
serialized data.
On 2017/06/29 11:09:15, ahe wrote:
> "get and put" -> "access"

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:129: /// not described in a summary as
if it was explicitly listed as an input.
On 2017/06/29 11:09:14, ahe wrote:
> This reminds me of the completely broken default behavior in javac. This
doesn't
> work because people think it's a build system (but it isn't). We should
revisit
> this.

Not sure I follow. We can discuss more on our next sync up.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:133: bool strongMode = true;
On 2017/06/29 11:09:15, ahe wrote:
> This should be a per-package option, not a global option.

Not sure either - this might be a question for the language team actually. I
don't believe that's a mode that they intend to support.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:137: /// Keys on this map are expected
to be `dart:*` URIs. The values can be
On 2017/06/29 11:09:14, ahe wrote:
> This is inconsistent with dartLibraries above, which removes the leading
dart:.

Good point. Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:140: Map<Uri, List<Uri>> targetPatches =
{};
On 2017/06/29 11:09:15, ahe wrote:
> Think this should be single URI that points to a file instead.

I plan to add the .json input like with libraries.json.

As we discussed offline, I'm inclined in making both a single .json file
containing both, since most platforms have variations in both the libraries they
support and the patch-file location.

Added TODO to handle this separately.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:157: /// tracking).
On 2017/06/29 18:51:07, Paul Berry wrote:
> Clarify whether these are shown by print()ing them, or by some other reporting
> API

Done and filed a bug that we want to change this.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:161: /// are well formed.
On 2017/06/29 18:51:06, Paul Berry wrote:
> If verification fails, how is the error reported?  Through onError or some
other
> mechanism?

Well.. both via onError and stdout (I even had a bug that I passed it to onError
when verifying programs, but not when verifying summaries, I just made it
consistent)

I added documentation here that we intend to report via onError, and created
Issue #30056 to track that we want to remove the stdout reporting.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:166: bool debugDump = false;
On 2017/06/29 18:51:07, Paul Berry wrote:
> Clarify whether the dump is shown by print() or through some other mechanism.

Thanks. Done

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:36: options = new
CompilerOptions.from(options)..chaseDependencies = true;
On 2017/06/29 18:51:07, Paul Berry wrote:
> This concerns me a lot.  If someone calls this API and deliberately sets
> `chaseDependencies` to false, we should respect that option.
> 
> Maybe it would be better to change the default value of `chaseDependencies` to
> `null`, so that `true` means "yes, chase dependencies", `false` means "no, be
> hermetic", and `null` means "do whatever is the default behavior for the API
> you're using.  Then we could move the behavior of providing the default into
> ProcessedOptions.

Done. I like your suggestion, it required that I added more parameters to
ProcessedOptions though, this relates also to the discussion about the
hermetic-file-system.


While we are here - wow do you feel about renaming the option to "hermetic"?

In a way the compiler is chasing dependencies regardless, the distinction in my
mind is whether it has access to them.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:37: var program = (await
generateKernel([source], options))?.program;
On 2017/06/29 18:51:07, Paul Berry wrote:
> I would feel a lot safer if we could create a ProcessedOptions object here and
> pass it into generateKernel.  That way there is no danger of generateKernel
> accidentally bypassing ProcessedOptions and trying to read data out of the raw
> options object by mistake.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:81: return (await
generateKernel(sources, options))?.program;
On 2017/06/29 18:51:07, Paul Berry wrote:
> Similar issue here.

Done.

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

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:153:
_inputSummariesPrograms.add(await loadProgram(uri, nameRoot));
On 2017/06/29 18:51:07, Paul Berry wrote:
> Previously, the use of Future.wait() would mean that multiple I/O operations
got
> scheduled concurrently.  With this change, we serialize the I/O.  Our
experience
> with analysis server is that such a change can really slow things down if you
> are reading from a network filesystem.  Is this an intentional behavior
change? 
> If so, what's the advantage?

It was initially deliberate, but I didn't need to go as far as making IO
operations in sequence.  PTAL.

The main reason I did this in order was to guarantee that we load into
[nameRoot] each library in the order that was provided. I was afraid of loading
out of order and risk that we would get the "wrong" definition for a library,
class, or member.

It appears that this is handled by the kernel deserialization, though (e.g. if
an existing definition is available, it merges the data with it), but I'm not
100% trusting that it is designed to be commutative. I didn't see any issues in
my tests, but I still don't trust it.

I decided instead to split this step in two: load bytes in parallel and then
deserialize the programs in sequence. This might be a good middleground for your
concern and mine.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:166:
_linkedDependencies.add(await loadProgram(uri, nameRoot));
On 2017/06/29 18:51:07, Paul Berry wrote:
> Similar concern here.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:175: var result =
loadProgramFromBytes(bytes, new Program(nameRoot: nameRoot));
On 2017/06/29 18:51:07, Paul Berry wrote:
> Nit: why not just
> 
> return loadProgramFromBytes(...);

Thanks - I had an extra debugging step earlier and forgot to undo the tmp
variable.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:50: fs = new
HermeticFileSystem(sources.toSet(), fs);
On 2017/06/29 18:51:07, Paul Berry wrote:
> It would be cool if we moved this logic into ProcessedOptions, so that we
could
> be confident that *all* of the front_end APIs are properly hermetic when
> chaseDependencies is `true`.
> 
> Incidentally, I think in the long run we're going to have other reasons to
wrap
> the filesystem as well, e.g. to implement "multi-root" functionality.  That
> logic should go into ProcessedOptions too, so that all the APIs get the
benefit
> of it.

Done.

I initially had discarded this idea because I thought the intent was for
ProcessedOptions to be just a wrapper on top of CompilerOptions and that we
wanted inputs to the APIs to be given explicitly here instead. Without the
inputs, we couldn't create the hermetic file system.

Thinking more about it, though, there is no reason why ProcessedOptions couldn't
have the inputs stored in it. Your other suggestion about computing a default
for chaseDependencies pushed me already in the direction of passing more data to
processed options, to the point that I think it makes sense to include there all
data that was provided from the user (including entry-points).

Let me know what you think of the new approach.

[Siggi Cherem (dart-lang), 2017-06-30 23:03:11]

I just uploaded more patch from a couple suggestions Peter mentioned offine: I
just added crash report and the global zone around the generate-kernel API.

Peter - my long term goal with the zones + options:
 - continue using the zone as a way of sharing "global" state until we decide we
want to something different
 - stop using CompilerCommandLine as the "options" object, and switch to use
ProcessedOptions everywhere where we need any configuration
 - create the zone using the ProcessedOptions object
 - for our helper scripts, provide a way to parse command-line arguments into
ProcessedOptions. I could try to leverage CompilerCommandLine for this, or I can
use pacakge:args (and to keep deps to a minimum, we could keep this under the
tools folder or create a separate package for all the tools)

WDYT?

One (short-term) question: I believe it is safe to nest 2 of these zones
upfront, but I wanted to double check. The context is for intermediate step
where I use the public APIs for our existing scripts before I replace
CompilerCommandLine.

I haven't done this extensively yet, but I already did it in this CL in
`compile_platform.dart`: this script is not used for patch_sdk, is only used for
the command-line tool under pkg/front_end/tool/fasta. The tool creates a zone
where it command-line flags are parsed then it passes the values of those flags
to the options in the public API, and then I create yet another zone that is the
one used by the compiler elsewhere. In essence, the outer zone is ignored.

[ahe, 2017-07-03 10:05:31]

Comments so far.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:43: ErrorHandler onError =
defaultErrorHandler;
I think we should provide a stream of diagnostics instead and offer to abort on
first error or warning.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/summ...
File pkg/front_end/lib/summary_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/summ...
pkg/front_end/lib/summary_generator.dart:37: /// The return value is a list of
bytes to write to the summary file.
If we're concerned about memory usage, this API should take a Sink or an
EventSink instead of returning a List.

https://codereview.chromium.org/2953703002/diff/180001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/180001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:30: bool trimDependencies:
false}) async {
Need to enter a new zone here.

https://codereview.chromium.org/2953703002/diff/180001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:131: }
Add crash reporting here.

[Paul Berry, 2017-07-04 15:12:15]

lgtm

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:51: /// will be used.
On 2017/06/30 04:12:01, Siggi Cherem (dart-lang) wrote:
> On 2017/06/29 18:51:07, Paul Berry wrote:
> > On 2017/06/29 11:09:15, ahe wrote:
> > > This seems brittle. I think it would be better to define a custom URI
scheme
> > > that signals "use the algorithm," and have null mean "don't search."
> > 
> > Personally I'm not a fan of custom URI schemes acting as a sentinel.  If we
> want
> > to change the current behavior, I'd suggest adding an enum that specifies
how
> > the .packages file should be found.
> > 
> > Regardless of what we do, I hope that we can make the default behavior be to
> use
> > the standard package_config search algorithm, because that's what I think a
> > client is most likely to expect.
> 
> Ha, I had not noticed the special behavior with the empty path.
> 
> Let's also put this on a separate CL. My inclination is similar to Paul's:
> 
> I'd like the default (`null`) to be the most common behavior of users of the
> API, most likely that is to do the lookup and default to an empty map if
nothing
> is found - we wouldn't issue an immediate error if no .packages is found, but
> only later if you try to load a `package:*` URI. 
> 
> Paul - do you feel we still need the other option ("pretend it is empty"?).
> Because I didn't see this, I've been creating an empty file in the unit tests
> already.

IIRC, the "pretend it is empty" was added as an afterthought while writing tests
of something (can't remember what).  I'm happy removing this functionality for
now.  If we discover that creating an empty file in the tests becomes too
burdensome (which seems unlikely) we can always re-add it later.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:36: options = new
CompilerOptions.from(options)..chaseDependencies = true;
On 2017/06/30 04:12:02, Siggi Cherem (dart-lang) wrote:
> On 2017/06/29 18:51:07, Paul Berry wrote:
> > This concerns me a lot.  If someone calls this API and deliberately sets
> > `chaseDependencies` to false, we should respect that option.
> > 
> > Maybe it would be better to change the default value of `chaseDependencies`
to
> > `null`, so that `true` means "yes, chase dependencies", `false` means "no,
be
> > hermetic", and `null` means "do whatever is the default behavior for the API
> > you're using.  Then we could move the behavior of providing the default into
> > ProcessedOptions.
> 
> Done. I like your suggestion, it required that I added more parameters to
> ProcessedOptions though, this relates also to the discussion about the
> hermetic-file-system.
> 
> 
> While we are here - wow do you feel about renaming the option to "hermetic"?
> 
> In a way the compiler is chasing dependencies regardless, the distinction in
my
> mind is whether it has access to them.

I don't have strong feelings about the name--if "hermetic" makes more sense to
you, feel free to change it.  (But maybe consider changing it in a separate CL
just so that we keep issues separate; there's a lot going on in this CL
already!)

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:50: fs = new
HermeticFileSystem(sources.toSet(), fs);
On 2017/06/30 04:12:02, Siggi Cherem (dart-lang) wrote:
> On 2017/06/29 18:51:07, Paul Berry wrote:
> > It would be cool if we moved this logic into ProcessedOptions, so that we
> could
> > be confident that *all* of the front_end APIs are properly hermetic when
> > chaseDependencies is `true`.
> > 
> > Incidentally, I think in the long run we're going to have other reasons to
> wrap
> > the filesystem as well, e.g. to implement "multi-root" functionality.  That
> > logic should go into ProcessedOptions too, so that all the APIs get the
> benefit
> > of it.
> 
> Done.
> 
> I initially had discarded this idea because I thought the intent was for
> ProcessedOptions to be just a wrapper on top of CompilerOptions and that we
> wanted inputs to the APIs to be given explicitly here instead. Without the
> inputs, we couldn't create the hermetic file system.
> 
> Thinking more about it, though, there is no reason why ProcessedOptions
couldn't
> have the inputs stored in it. Your other suggestion about computing a default
> for chaseDependencies pushed me already in the direction of passing more data
to
> processed options, to the point that I think it makes sense to include there
all
> data that was provided from the user (including entry-points).
> 
> Let me know what you think of the new approach.

I like it!  Thanks :)

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:94: /// Any Uri that resolves to
"multi-root:///$absolute-path" will be searched
Nit: consider changing this to "multi-root:///$absolute_path" to minimize
confusion.  Brains which are used to Dart syntax might incorrectly interpret
"multi-root:///$absolute-path" as meaning "multi-root:///${absolute}-path" :)

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

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:90: final bool modularApi;
Nit: consider making this private.  Any reference to this from outside this file
would almost certainly be a mistake (other files should be referring to
chaseDependencies instead)

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:325: '(where chaseDependencies
is false), but it was not '
Is this going to be a user-visible error message?  Because end users won't know
about chaseDependencies.

[Siggi Cherem (dart-lang), 2017-07-05 03:39:30]

Thanks for all the comments!

Peter - it's great to see all your API suggestions. I'd like to start a thread
or doc to dive into them, discuss, and brainstorm with you and the rest of the
team. Since most of those changes will be addressed on separate CLs, please let
me know if you are OK with me submitting the CL now that Paul has approved it,
or if you would like me to wait until you finish reviewing the changes.

Thanks!

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:43: ErrorHandler onError =
defaultErrorHandler;
On 2017/07/03 10:05:31, ahe wrote:
> I think we should provide a stream of diagnostics instead and offer to abort
on
> first error or warning.

Yeah, I like the idea of using a stream. I also want a way to have good defaults
(so if no options are passed, we report errors or throw). I think we can come up
with a good API to achieve both. I'll start a thread separately to discuss
ideas.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/summ...
File pkg/front_end/lib/summary_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/summ...
pkg/front_end/lib/summary_generator.dart:37: /// The return value is a list of
bytes to write to the summary file.
On 2017/07/03 10:05:31, ahe wrote:
> If we're concerned about memory usage, this API should take a Sink or an
> EventSink instead of returning a List.

True - we also have some open discussion about whether we want the file-system
API or an input-provider/output-provider API instead.

Like with the other suggestions, I'd like to keep this separate from this CL.

https://codereview.chromium.org/2953703002/diff/180001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/180001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:30: bool trimDependencies:
false}) async {
On 2017/07/03 10:05:31, ahe wrote:
> Need to enter a new zone here.

I'm not sure if you saw these two changes on the last patch-set.

Is something else needed, or do the changes there cover it?

https://codereview.chromium.org/2953703002/diff/180001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:131: }
On 2017/07/03 10:05:31, ahe wrote:
> Add crash reporting here.

Ditto, I believe this is done.

[ahe, 2017-07-05 13:29:40]

I'm sorry, but I'm still working on this review. Comments so far.

So far I'm aware of three issues that I think needs to be addressed:

* Not using a zone somewhere (which we discussed in a meeting).

* Filter option is needed when dumping kernel text.

* Instantiating Ticker too late.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/dart2js_target.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/dart2js_target.dart:72: return new
InvalidExpression();
Someone needs to implement this for dart2js; add a TODO?

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/dart2js_target.dart:88: return new
InvalidExpression();
Ditto.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/tool/gene...
File pkg/compiler/tool/generate_kernel.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/tool/gene...
pkg/compiler/tool/generate_kernel.dart:21: var flags = _argParser.parse(args);
A type would be nice here.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/tool/gene...
pkg/compiler/tool/generate_kernel.dart:36: var entryUri =
Uri.base.resolve(flags.rest.first);
Personally, I strongly prefer:

    Uri entry

over

    var entryUri

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:52: /// The build unit by default
contains only the source files in [sources]
The term "build unit" isn't clearly defined.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:54: ///
[CompilerOptions.chaseDependencies] is true, it may include some additional
Only document the behavior of CompilerOptions.chaseDependencies once.

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

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:69: Ticker get ticker =>
_ticker ??= new Ticker(isVerbose: _raw.verbose);
The ticker should be created eagerly. Otherwise it doesn't count from the
beginning.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:137: Future<Program>
loadSdkSummary(CanonicalName nameRoot) async {
This doesn't feel like an option to me.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:147: Future<List<Program>>
loadInputSummaries(CanonicalName nameRoot) async {
Ditto.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:153:
_inputSummariesPrograms.add(await loadProgram(uri, nameRoot));
On 2017/06/30 04:12:02, Siggi Cherem (dart-lang) wrote:
> On 2017/06/29 18:51:07, Paul Berry wrote:
> > Previously, the use of Future.wait() would mean that multiple I/O operations
> got
> > scheduled concurrently.  With this change, we serialize the I/O.  Our
> experience
> > with analysis server is that such a change can really slow things down if
you
> > are reading from a network filesystem.  Is this an intentional behavior
> change? 
> > If so, what's the advantage?
> 
> It was initially deliberate, but I didn't need to go as far as making IO
> operations in sequence.  PTAL.
> 
> The main reason I did this in order was to guarantee that we load into
> [nameRoot] each library in the order that was provided. I was afraid of
loading
> out of order and risk that we would get the "wrong" definition for a library,
> class, or member.
> 
> It appears that this is handled by the kernel deserialization, though (e.g. if
> an existing definition is available, it merges the data with it), but I'm not
> 100% trusting that it is designed to be commutative. I didn't see any issues
in
> my tests, but I still don't trust it.
> 
> I decided instead to split this step in two: load bytes in parallel and then
> deserialize the programs in sequence. This might be a good middleground for
your
> concern and mine.
> 

We should throttle the number of concurrent operations. I've noticed that
starting too many asynchronous operations concurrently with Future.wait
completely locks up the system (on Mac OS X).

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:160: Future<List<Program>>
loadLinkDependencies(CanonicalName nameRoot) async {
Also doesn't seem like an option.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:208: throw new
UnimplementedError(); // TODO(paulberry): search for .packages
Could we throw a string with information about what isn't implemented?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:222: /// This is an
asynchronous getter since file system operations may be
What is an asynchronous getter?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:231: throw new
UnimplementedError();
Throw string about what isn't implemented?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/compile_platform.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/compile_platform.dart:21: import
'../../compiler_options.dart';
Add show and move to before line 11.

I've sorted imports by:

dart
package
../
relative without ..

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/compile_platform.dart:22: import
'../kernel_generator_impl.dart';
Ditto.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/utils.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/utils.dart:48: var printer = filter == null
&& !excludeUriToSource
Put type on printer.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/dart_vm_native.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/dart_vm_native.dart:5: /// Implements support
for two variants of the native syntax extension.
I'd say that we should rename this file, but as I plan to implement this in the
parser and delete this file, it doesn't matter :-)

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/dart_vm_native.dart:23: /// patch set.
This comment was 15 lines to late for my thought process ;-)

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:37: options.onError(new
SimpleError(msg));
What is a SimpleError?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:54:
pOptions.ticker.logMs("Parsed arguments");
This will always report that parsing arguments took 0ms. The Ticker needs to be
created on line 36.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:93:
dillTarget.loader.uriToSource.clear();
Why do we clear this? Have you made sure that stack traces are correct for
platform libraries?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:114: // both we'd have to clone
the program first.
To support both what?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:135: if (pOptions.debugDump)
printProgramText(program);
libraryFilter option missing here.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:181: return inputError(
Throw FileSystemException instead, then Fasta can recover as from any other
file-system exception. Eventually, when I implement that (I have an open bug on
it).

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:182: null,
uri

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/simple_error.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/simple_error.dart:5: /// An error that only contains a
message and no error location.
I would assume this should still have an error code, right?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/simple_error.dart:12: String toString() => "FrontEnd
Error: $message";
I'm not sure how the user experience is improved by adding "FrontEnd Error: " to
the message.

[Siggi Cherem (dart-lang), 2017-07-05 18:40:26]

Patchset #4 (id:220001) has been deleted

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

Thanks!

> I'm sorry, but I'm still working on this review. Comments so far.

No worries, I appreciate you taking the time to review in detail.

> So far I'm aware of three issues that I think needs to be addressed:

> * Not using a zone somewhere (which we discussed in a meeting).

I believe this is addressed in patchset 3 in `generateKernel` (which is where
all public APIs interact with fasta).

> * Filter option is needed when dumping kernel text.

Good catch, should be done now in patchset 4.

> * Instantiating Ticker too late.

Thanks, should be fixed in patched 4 as well.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/lib/src/k...
File pkg/compiler/lib/src/kernel/dart2js_target.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/dart2js_target.dart:72: return new
InvalidExpression();
On 2017/07/05 13:29:39, ahe wrote:
> Someone needs to implement this for dart2js; add a TODO?

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/lib/src/k...
pkg/compiler/lib/src/kernel/dart2js_target.dart:88: return new
InvalidExpression();
On 2017/07/05 13:29:39, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/tool/gene...
File pkg/compiler/tool/generate_kernel.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/tool/gene...
pkg/compiler/tool/generate_kernel.dart:21: var flags = _argParser.parse(args);
On 2017/07/05 13:29:39, ahe wrote:
> A type would be nice here.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/compiler/tool/gene...
pkg/compiler/tool/generate_kernel.dart:36: var entryUri =
Uri.base.resolve(flags.rest.first);
On 2017/07/05 13:29:39, ahe wrote:
> Personally, I strongly prefer:
> 
>     Uri entry
> 
> over
> 
>     var entryUri

Done, although I'm personally in favor of the latter.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:51: /// will be used.
On 2017/07/04 15:12:14, Paul Berry wrote:
> On 2017/06/30 04:12:01, Siggi Cherem (dart-lang) wrote:
> > On 2017/06/29 18:51:07, Paul Berry wrote:
> > > On 2017/06/29 11:09:15, ahe wrote:
> > > > This seems brittle. I think it would be better to define a custom URI
> scheme
> > > > that signals "use the algorithm," and have null mean "don't search."
> > > 
> > > Personally I'm not a fan of custom URI schemes acting as a sentinel.  If
we
> > want
> > > to change the current behavior, I'd suggest adding an enum that specifies
> how
> > > the .packages file should be found.
> > > 
> > > Regardless of what we do, I hope that we can make the default behavior be
to
> > use
> > > the standard package_config search algorithm, because that's what I think
a
> > > client is most likely to expect.
> > 
> > Ha, I had not noticed the special behavior with the empty path.
> > 
> > Let's also put this on a separate CL. My inclination is similar to Paul's:
> > 
> > I'd like the default (`null`) to be the most common behavior of users of the
> > API, most likely that is to do the lookup and default to an empty map if
> nothing
> > is found - we wouldn't issue an immediate error if no .packages is found,
but
> > only later if you try to load a `package:*` URI. 
> > 
> > Paul - do you feel we still need the other option ("pretend it is empty"?).
> > Because I didn't see this, I've been creating an empty file in the unit
tests
> > already.
> 
> IIRC, the "pretend it is empty" was added as an afterthought while writing
tests
> of something (can't remember what).  I'm happy removing this functionality for
> now.  If we discover that creating an empty file in the tests becomes too
> burdensome (which seems unlikely) we can always re-add it later.

Thanks - sounds good. I can remove it on a follow up CL.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
File pkg/front_end/lib/kernel_generator.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:36: options = new
CompilerOptions.from(options)..chaseDependencies = true;
On 2017/07/04 15:12:14, Paul Berry wrote:
> On 2017/06/30 04:12:02, Siggi Cherem (dart-lang) wrote:
> > On 2017/06/29 18:51:07, Paul Berry wrote:
> > > This concerns me a lot.  If someone calls this API and deliberately sets
> > > `chaseDependencies` to false, we should respect that option.
> > > 
> > > Maybe it would be better to change the default value of
`chaseDependencies`
> to
> > > `null`, so that `true` means "yes, chase dependencies", `false` means "no,
> be
> > > hermetic", and `null` means "do whatever is the default behavior for the
API
> > > you're using.  Then we could move the behavior of providing the default
into
> > > ProcessedOptions.
> > 
> > Done. I like your suggestion, it required that I added more parameters to
> > ProcessedOptions though, this relates also to the discussion about the
> > hermetic-file-system.
> > 
> > 
> > While we are here - wow do you feel about renaming the option to "hermetic"?
> > 
> > In a way the compiler is chasing dependencies regardless, the distinction in
> my
> > mind is whether it has access to them.
> 
> I don't have strong feelings about the name--if "hermetic" makes more sense to
> you, feel free to change it.  (But maybe consider changing it in a separate CL
> just so that we keep issues separate; there's a lot going on in this CL
> already!)

Sounds good - I'll track this separately. Your comment on the error message on
the other file also made me realize that it would be nice to match the same term
here as what we use in error messages for our users.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:52: /// The build unit by default
contains only the source files in [sources]
On 2017/07/05 13:29:39, ahe wrote:
> The term "build unit" isn't clearly defined.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/kern...
pkg/front_end/lib/kernel_generator.dart:54: ///
[CompilerOptions.chaseDependencies] is true, it may include some additional
On 2017/07/05 13:29:39, ahe wrote:
> Only document the behavior of CompilerOptions.chaseDependencies once.

Done.

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

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:69: Ticker get ticker =>
_ticker ??= new Ticker(isVerbose: _raw.verbose);
On 2017/07/05 13:29:39, ahe wrote:
> The ticker should be created eagerly. Otherwise it doesn't count from the
> beginning.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:137: Future<Program>
loadSdkSummary(CanonicalName nameRoot) async {
On 2017/07/05 13:29:39, ahe wrote:
> This doesn't feel like an option to me.

Agree - I don't believe we want to put this separately though, maybe it is time
to rename this class instead?

Somehow I feel "CompilerContext" is the right name for this too. Which is not
surprising because I'd like to merge this with the existing `CompilerContext`
(or at least I wanted CompilerContext to have a reference to this object).


Possible next steps:
 (a) keep the CL as is with a TODO
 (b) move these 3 functions to CompilerContext, and replace
CompilerContxt.options with processedoptions
 (c) merge CompilerContext + ProcessedOptions entirely.

If you are OK, I'd like to start with (a) and discuss offline about whether we
want to do (b), (c) or something else.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:147: Future<List<Program>>
loadInputSummaries(CanonicalName nameRoot) async {
On 2017/07/05 13:29:39, ahe wrote:
> Ditto.

Acknowledged.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:153:
_inputSummariesPrograms.add(await loadProgram(uri, nameRoot));
On 2017/07/05 13:29:39, ahe wrote:
> On 2017/06/30 04:12:02, Siggi Cherem (dart-lang) wrote:
> > On 2017/06/29 18:51:07, Paul Berry wrote:
> > > Previously, the use of Future.wait() would mean that multiple I/O
operations
> > got
> > > scheduled concurrently.  With this change, we serialize the I/O.  Our
> > experience
> > > with analysis server is that such a change can really slow things down if
> you
> > > are reading from a network filesystem.  Is this an intentional behavior
> > change? 
> > > If so, what's the advantage?
> > 
> > It was initially deliberate, but I didn't need to go as far as making IO
> > operations in sequence.  PTAL.
> > 
> > The main reason I did this in order was to guarantee that we load into
> > [nameRoot] each library in the order that was provided. I was afraid of
> loading
> > out of order and risk that we would get the "wrong" definition for a
library,
> > class, or member.
> > 
> > It appears that this is handled by the kernel deserialization, though (e.g.
if
> > an existing definition is available, it merges the data with it), but I'm
not
> > 100% trusting that it is designed to be commutative. I didn't see any issues
> in
> > my tests, but I still don't trust it.
> > 
> > I decided instead to split this step in two: load bytes in parallel and then
> > deserialize the programs in sequence. This might be a good middleground for
> your
> > concern and mine.
> > 
> 
> We should throttle the number of concurrent operations. I've noticed that
> starting too many asynchronous operations concurrently with Future.wait
> completely locks up the system (on Mac OS X).

Good to know. Added TODO for now.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:160: Future<List<Program>>
loadLinkDependencies(CanonicalName nameRoot) async {
On 2017/07/05 13:29:39, ahe wrote:
> Also doesn't seem like an option.

Acknowledged.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:208: throw new
UnimplementedError(); // TODO(paulberry): search for .packages
On 2017/07/05 13:29:39, ahe wrote:
> Could we throw a string with information about what isn't implemented?

Done, practically moved the TODO into the error message.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:222: /// This is an
asynchronous getter since file system operations may be
On 2017/07/05 13:29:39, ahe wrote:
> What is an asynchronous getter?

Deleted comment, I believe long ago this used to be a getter that returned a
future:

Future<Uri> get normalizedSdkRoot async => ...;

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:231: throw new
UnimplementedError();
On 2017/07/05 13:29:39, ahe wrote:
> Throw string about what isn't implemented?

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/compile_platform.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/compile_platform.dart:21: import
'../../compiler_options.dart';
On 2017/07/05 13:29:39, ahe wrote:
> Add show and move to before line 11.
> 
> I've sorted imports by:
> 
> dart
> package
> ../
> relative without ..

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/compile_platform.dart:22: import
'../kernel_generator_impl.dart';
On 2017/07/05 13:29:39, ahe wrote:
> Ditto.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/kernel/utils.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/kernel/utils.dart:48: var printer = filter == null
&& !excludeUriToSource
On 2017/07/05 13:29:39, ahe wrote:
> Put type on printer.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/fasta/parser/dart_vm_native.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/dart_vm_native.dart:5: /// Implements support
for two variants of the native syntax extension.
On 2017/07/05 13:29:40, ahe wrote:
> I'd say that we should rename this file, but as I plan to implement this in
the
> parser and delete this file, it doesn't matter :-)

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/fasta/parser/dart_vm_native.dart:23: /// patch set.
On 2017/07/05 13:29:39, ahe wrote:
> This comment was 15 lines to late for my thought process ;-)

:)

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/kernel_generator_impl.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:37: options.onError(new
SimpleError(msg));
On 2017/07/05 13:29:40, ahe wrote:
> What is a SimpleError?

An error object that needs a better name? ;-)

The external API provides an interface for an error class, SimpleError was a
default implementation that only had a constructor with a string input (no error
location etc). 

For now, I renamed it to something more generic ("compilation-error") and all
the error reporting logic is handled behind the processed-options object, so
this class it now only used there.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:54:
pOptions.ticker.logMs("Parsed arguments");
On 2017/07/05 13:29:40, ahe wrote:
> This will always report that parsing arguments took 0ms. The Ticker needs to
be
> created on line 36.

Done - now ticker is created eagerly when allocating ProcessedOptions, which
happens before generateKernel gets called.

The "parsed arguments" event doesn't quite make sense anymore, so I got rid of
it.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:93:
dillTarget.loader.uriToSource.clear();
On 2017/07/05 13:29:40, ahe wrote:
> Why do we clear this? Have you made sure that stack traces are correct for
> platform libraries?

Deleted this, but for a different reason :)

The background:

I was clearing this because we don't use it: none of the target platforms uses
source-info from external libraries. For instance, the VM loads the
platform.dill directly (which does contain the uriToSource data in it). The
script itself may contain references to platform APIs, but we don't even send
those external libraries to the VM. In contrast, Dart2js uses
`linkedDependencies` for the sdk, those are added further below and their
uriToSource remains intact.

A couple design questions that affect what we should do:
(a)- should summaries/outlines have uriToSource when we generate them?

I don't think they should. When I first wrote this `clear` line, the
`buildSummary` code-path below was not doing so. Now that it is, we shouldn't
need to clear it here if the input summary was built correctly.

(b.1)- should we support loading any .dill file and treat it as a summary, even
if it has bodies and uriToSource?

I don't know. This might depend on how the summary format evolves, maybe we
can't?

(b.2)- If yes, should this behave the same as if the input was an outline?

I don't know either. Clearing the uriToSource here was part of hiding the
differences, but there might be more. I think in scenarios where we really care
about how things are composed, we will use the API to generate summaries and
will not try to use full .dill files.


Given all of that, I think for now it is sufficient to clear the uriToSource
when we generate the summaries, so I've dropped it here.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:114: // both we'd have to clone
the program first.
On 2017/07/05 13:29:40, ahe wrote:
> To support both what?

buildSummary & buildProgram at once + trimming deps

Trimming is destructive, so if you do buildSummary + buildProgram, by the time
we run kernelTarget.buildProgram we have deleted stuff in the outline that we
might need later.

reworded.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:135: if (pOptions.debugDump)
printProgramText(program);
On 2017/07/05 13:29:40, ahe wrote:
> libraryFilter option missing here.

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:181: return inputError(
On 2017/07/05 13:29:40, ahe wrote:
> Throw FileSystemException instead, then Fasta can recover as from any other
> file-system exception. Eventually, when I implement that (I have an open bug
on
> it).

Done. I like it.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/kernel_generator_impl.dart:182: null,
On 2017/07/05 13:29:40, ahe wrote:
> uri

n/a now that I use FileSystemException.

However, if I didn't change the exception, I thought I had to use `null` here.
Could you add a comment to InputError/inputError about what Uri is supposed to
be?

My understanding was that this uri would be used for the location where the
error is reported. For example, if we have `package:a/a.dart` with these
contents:

  import 'b.dart';
  ...

but `b.dart` doesn't. I expect `b.dart` to be mentioned in the message body, but
the Uri in input error would be `package:a/a.dart` and the offset would
highlight the import line above.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/simple_error.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/simple_error.dart:5: /// An error that only contains a
message and no error location.
On 2017/07/05 13:29:40, ahe wrote:
> I would assume this should still have an error code, right?

Good point - for now added a TODO and will follow up with a new CL. I should add
all the errors that are currently strings in ProcessedOptions and generateKernel
to messages.yaml and autogenerate their error object instead.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/simple_error.dart:12: String toString() => "FrontEnd
Error: $message";
On 2017/07/05 13:29:40, ahe wrote:
> I'm not sure how the user experience is improved by adding "FrontEnd Error: "
to
> the message.

Done.

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/comp...
File pkg/front_end/lib/compiler_options.dart (right):

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/comp...
pkg/front_end/lib/compiler_options.dart:94: /// Any Uri that resolves to
"multi-root:///$absolute-path" will be searched
On 2017/07/04 15:12:14, Paul Berry wrote:
> Nit: consider changing this to "multi-root:///$absolute_path" to minimize
> confusion.  Brains which are used to Dart syntax might incorrectly interpret
> "multi-root:///$absolute-path" as meaning "multi-root:///${absolute}-path" :)

Done.

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

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:90: final bool modularApi;
On 2017/07/04 15:12:14, Paul Berry wrote:
> Nit: consider making this private.  Any reference to this from outside this
file
> would almost certainly be a mistake (other files should be referring to
> chaseDependencies instead)

Done.

https://codereview.chromium.org/2953703002/diff/200001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/base/processed_options.dart:325: '(where chaseDependencies
is false), but it was not '
On 2017/07/04 15:12:14, Paul Berry wrote:
> Is this going to be a user-visible error message?  Because end users won't
know
> about chaseDependencies.

Good point. removed.

[ahe, 2017-07-06 13:12:22]

Still working...

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/testing/compiler_common.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:6: library
front_end.test.compiler_options_common;
front_end.test -> fasta.testing

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:68: ///   contain either
source files (values as strings) or .dill files (values
(value is [String])

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:69: ///   are bytes).
(value is [List<int>]).

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:103: var _defaultDir =
Uri.parse('file:///a/b/c/');
I don't understand this.

If you want to create a fake root directory, use a custom URI, for example,
"org.dartlang.front-end.test:///".

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:108: var invalidCoreLibs = {
Map<String, Uri>

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/fas...
File pkg/front_end/test/fasta/testing/suite.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/fas...
pkg/front_end/test/fasta/testing/suite.dart:142: if (outline == null) {
This is rather brittle. There's a potential for race conditions here.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:15: import
'package:test_reflective_loader/test_reflective_loader.dart';
Not a big fan of this.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:30: new
Program(libraries: [new Library(Uri.parse('file:///a/b.dart'))]);
As far as I can tell, this file relies heavily on the assumption that its being
run on a machine that doesn't have a the following directories:

/a
/sdk
etc.

This is super brittle and easily avoided by using custom URI schemes.

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_from_binary.dart:160: void
_fillTreeNodeList(List<TreeNode> list, TreeNode buildObject(),
If it isn't just me that find this method confusing to begin with, I suggest
that you remove the parameter shouldWriteData and add this method instead:

void _skipNodeList(void buildObject()) {
  int length = readUInt();
  for (int i = 0; i < length; i++) {
    buildObject();
  }
}

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/limited_ast_to_binary.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/limited_ast_to_binary.dart:16: /// Excludes also all
uriToSource information.
"also" implies a context that's kinda missing here.

I'd try something like this:

/// Exclues all uriToSource information in addition to ... (what?).

https://codereview.chromium.org/2953703002/diff/140001/utils/kernel-service/k...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/utils/kernel-service/k...
utils/kernel-service/kernel-service.dart:43: /// specific to the kernel-service
and nothing else.
I'm not at all excited about moving stuff from pkg/front_end to here.

[Paul Berry, 2017-07-06 14:20:05]

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:30: new
Program(libraries: [new Library(Uri.parse('file:///a/b.dart'))]);
On 2017/07/06 13:12:22, ahe wrote:
> As far as I can tell, this file relies heavily on the assumption that its
being
> run on a machine that doesn't have a the following directories:
> 
> /a
> /sdk
> etc.
> 
> This is super brittle and easily avoided by using custom URI schemes.

Actually it doesn't matter what directories exist on the machine, since we're
using a MemoryFileSystem.  The physical file system is never examined.

[Siggi Cherem (dart-lang), 2017-07-06 19:05:28]

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
File pkg/front_end/lib/src/testing/compiler_common.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:6: library
front_end.test.compiler_options_common;
On 2017/07/06 13:12:22, ahe wrote:
> front_end.test -> fasta.testing

Partially done: sicne this is for APIs outside fasta, I used front_end.testing

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:68: ///   contain either
source files (values as strings) or .dill files (values
On 2017/07/06 13:12:22, ahe wrote:
> (value is [String])

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:69: ///   are bytes).
On 2017/07/06 13:12:22, ahe wrote:
> (value is [List<int>]).

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:103: var _defaultDir =
Uri.parse('file:///a/b/c/');
On 2017/07/06 13:12:22, ahe wrote:
> I don't understand this.
> 
> If you want to create a fake root directory, use a custom URI, for example,
> "org.dartlang.front-end.test:///".

Added comment to clarify that this is only used in a MemoryFileSystem, so it is
OK to use a fake directory.

Using a custom-scheme should be possible in the future. Today, however, we need
it to be file:*, otherwise fasta will complain (in source_loader). This also
came up with the multi-root CL. The plan is to remove those checks from
source_loader and put all scheme-specific logic inside the file-system. At that
point, it should be trivial to change this to any other scheme.

I don't feel like we need to use a fully-qualified scheme for our unit tests
though, so I'd be inclined to use something else. In dart2js unit-tests we use
`memory:`, for example.

  -

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/lib/src/...
pkg/front_end/lib/src/testing/compiler_common.dart:108: var invalidCoreLibs = {
On 2017/07/06 13:12:22, ahe wrote:
> Map<String, Uri>

Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/fas...
File pkg/front_end/test/fasta/testing/suite.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/fas...
pkg/front_end/test/fasta/testing/suite.dart:142: if (outline == null) {
On 2017/07/06 13:12:22, ahe wrote:
> This is rather brittle. There's a potential for race conditions here.

Is the change making it worse, or are you saying this was brittle from before as
well?

I recall we made the change in https://codereview.chromium.org/2914773003 to
ensure we didn't have a race condition before we invoke the `run` step. If
that's not enough, we could move this and load the outline upfront in
`FastaContext.create`

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:30: new
Program(libraries: [new Library(Uri.parse('file:///a/b.dart'))]);
On 2017/07/06 14:20:05, Paul Berry wrote:
> On 2017/07/06 13:12:22, ahe wrote:
> > As far as I can tell, this file relies heavily on the assumption that its
> being
> > run on a machine that doesn't have a the following directories:
> > 
> > /a
> > /sdk
> > etc.
> > 
> > This is super brittle and easily avoided by using custom URI schemes.
> 
> Actually it doesn't matter what directories exist on the machine, since we're
> using a MemoryFileSystem.  The physical file system is never examined.

exactly.

Note however that new test files I added (for summary_generator and
kernel_generator) do use a hybrid system to read sources from memory, but sdk
and outline.dill files from disk. I still think it is safe due to how ew overlay
them.

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/ast_from_binary.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/ast_from_binary.dart:160: void
_fillTreeNodeList(List<TreeNode> list, TreeNode buildObject(),
On 2017/07/06 13:12:22, ahe wrote:
> If it isn't just me that find this method confusing to begin with, I suggest
> that you remove the parameter shouldWriteData and add this method instead:
> 
> void _skipNodeList(void buildObject()) {
>   int length = readUInt();
>   for (int i = 0; i < length; i++) {
>     buildObject();
>   }
> }

I like it. Done.

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
File pkg/kernel/lib/binary/limited_ast_to_binary.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/kernel/lib/binary/...
pkg/kernel/lib/binary/limited_ast_to_binary.dart:16: /// Excludes also all
uriToSource information.
On 2017/07/06 13:12:22, ahe wrote:
> "also" implies a context that's kinda missing here.
> 
> I'd try something like this:
> 
> /// Exclues all uriToSource information in addition to ... (what?).

Done.

The "what" was is basically everything this class does :) - excluding code and
canonical names for filtered libraries.

https://codereview.chromium.org/2953703002/diff/140001/utils/kernel-service/k...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/utils/kernel-service/k...
utils/kernel-service/kernel-service.dart:43: /// specific to the kernel-service
and nothing else.
On 2017/07/06 13:12:22, ahe wrote:
> I'm not at all excited about moving stuff from pkg/front_end to here.

I hear you. Given that the code below is modified to use the public API (and not
internal fasta APIs), I feel the code is much smaller and simpler than it used
to be. I still would like to simplify it further. For example, I think it will
once we add the logic for discovering .packages in front_end directly.

[Siggi Cherem (dart-lang), 2017-07-07 22:03:51]

Peter said offline that he will continue reviewing, but that I can go ahead and
submit, so I plan to do so momentarily.

[Siggi Cherem (dart-lang), 2017-07-07 22:14:27]

Description was changed from

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator. Code has some modifications:
     it uses a single canonical-root when loading summaries, and
     it supports generating both outlines and kernel in one go.

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed, and I stopped covering it in
     most tests (now only covered in shaker tests). The plan is to add
     tests for the public API covering this in the future.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN
  
 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing
==========

to

==========
Tweak public APIs and use them in patch_sdk, dart2js, and kernel-service.

This CL tweaks the public APIs in package:front_end, and
starts using those APIs outside the package. For example, this
removes 9 uses of DillTarget, so it is not longer mentioned
outside pkg/front_end and the analyzer_target.

Actual changes:
 - in package:front_end

   * added kernel_generator_impl: new file contains code that
     used to be in kernel_generator. Code has some modifications:
     it uses a single canonical-root when loading summaries, and
     it supports generating both outlines and kernel in one go.

   * removed code that didn't belong here:
     a. most of calculating deps for .GN moved to patch_sdk
     b. vm-specific outcomes moved to kernel-service

   * updated how `native` is implemented, so we can more easily
     support dart2js and ddc

   * updated how we check where `int`, `bool`, etc can be implemented.

   * added support "hermetic mode" in modular builds
     ('chaseDependencies = false' option)

   * moved `trim` step out of fasta, and for now call it only within
     the public API. This is not yet exposed, and I stopped covering it in
     most tests (now only covered in shaker tests). The plan is to add
     tests for the public API covering this in the future.

   * removed `uriToSource` when serializing outlines

   * added unit tests for public APIs

 - patch_sdk
   * use the public API to craete platform.dill, outline.dill (now
     500K insted of 3Mb because it excludes sources), and vmservice_io.dill
   * moved here logic internal to .GN

 - kernel service
   * use the public API
   * moved here logic that depends on VM internals (e.g. status enum,
     compilation results)

 - package:compiler
   * use the public API in tools and unit tests
   * simplified patched-sdk generation: no more extending fasta's internals

 - package:kernel
   * fix bug in deserialization: initializers and other lists were
     overwritten accidentally with external definitions.
   * updated unit tests, moved shared logic to frontend/src/fasta/testing

R=johnniwinther@google.com, paulberry@google.com

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

[Siggi Cherem (dart-lang), 2017-07-07 22:14:29]

Committed patchset #5 (id:260001) manually as
610d0819477c5bb0ede1d9f219c50e91e5994afa (presubmit successful).

[ahe, 2017-07-11 09:34:27]

This completes my review of patch set 1.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
File pkg/front_end/test/kernel_generator_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:23: await compileScript('main() =>
print("hi");', options: options);
Make this source snippet a shared constant and test that it can actually be
compiled.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:31: ..sdkSummary =
Uri.parse('file:///not_existing_summary_file')
Use custom URI.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:79:
expect(program.uriToSource['file:///a/b/c/a.dart'], isNotNull);
Use custom URI.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:83: var program = await
compileScript('a() => print("hi"); main() {}',
Add TODO about requiring main here?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:102: .firstWhere((lib) =>
lib.importUri.path == '/a/b/c/a.dart');
Custom URI.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:107: var program = await
compileScript('a() => print("hi"); main() {}',
TODO about main?

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:125: .firstWhere((lib) =>
lib.importUri.path == '/a/b/c/a.dart');
Custom URI.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:30: new
Program(libraries: [new Library(Uri.parse('file:///a/b.dart'))]);
On 2017/07/06 14:20:05, Paul Berry wrote:
> On 2017/07/06 13:12:22, ahe wrote:
> > As far as I can tell, this file relies heavily on the assumption that its
> being
> > run on a machine that doesn't have a the following directories:
> > 
> > /a
> > /sdk
> > etc.
> > 
> > This is super brittle and easily avoided by using custom URI schemes.
> 
> Actually it doesn't matter what directories exist on the machine, since we're
> using a MemoryFileSystem.  The physical file system is never examined.

I think it matters for the same reason as this is problematic:

    const bool isTrue = false;

Using file URIs as fake URIs is brittle, especially since we also have a hybrid
file system.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/tool/fas...
File pkg/front_end/tool/fasta_perf.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/tool/fas...
pkg/front_end/tool/fasta_perf.dart:74:
Uri.base.resolve(Platform.resolvedExecutable).resolve('patched_sdk/');
Uri.base.resolveUri(new
Uri.file(Platform.resolvedExecutable)).resolve('patched_sdk/');

https://codereview.chromium.org/2953703002/diff/140001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/tools/patch_sdk.dart#n...
tools/patch_sdk.dart:129: var target = forVm
This seems brittle compared to:

switch (mode) {
  case "vm": ...
  case "flutter": ...
  case "dart2js" ...
  default:
    throw "Unhandled mode: $mode";
}

https://codereview.chromium.org/2953703002/diff/140001/utils/kernel-service/k...
File utils/kernel-service/kernel-service.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/utils/kernel-service/k...
utils/kernel-service/kernel-service.dart:43: /// specific to the kernel-service
and nothing else.
On 2017/07/06 19:05:28, Siggi Cherem (dart-lang) wrote:
> On 2017/07/06 13:12:22, ahe wrote:
> > I'm not at all excited about moving stuff from pkg/front_end to here.
> 
> I hear you. Given that the code below is modified to use the public API (and
not
> internal fasta APIs), I feel the code is much smaller and simpler than it used
> to be. I still would like to simplify it further. For example, I think it will
> once we add the logic for discovering .packages in front_end directly.

We need to do something about this. This is way too much logic to have living
outside clear ownership.

I had the same complaint when you deleted code from fasta/testing, and it led to
problems, and you undid in this CL. I think this will lead to problems as well.

[Siggi Cherem (dart-lang), 2017-07-11 23:16:32]

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
File pkg/front_end/test/kernel_generator_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:23: await compileScript('main() =>
print("hi");', options: options);
On 2017/07/11 09:34:26, ahe wrote:
> Make this source snippet a shared constant and test that it can actually be
> compiled.

will do

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:31: ..sdkSummary =
Uri.parse('file:///not_existing_summary_file')
On 2017/07/11 09:34:26, ahe wrote:
> Use custom URI.

Ack (I replied in more detail on the other file)

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:79:
expect(program.uriToSource['file:///a/b/c/a.dart'], isNotNull);
On 2017/07/11 09:34:26, ahe wrote:
> Use custom URI.

Acknowledged.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:83: var program = await
compileScript('a() => print("hi"); main() {}',
On 2017/07/11 09:34:26, ahe wrote:
> Add TODO about requiring main here?

I am not sure I follow - you want to not require it?

Note that to keep this unrelated to the summary generator tests I decided to use
here the compileScript + serialize it as a summary (hence why it needs a
`main`). I could just use the summary generator here instead though, then the
main would not be necessary.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:102: .firstWhere((lib) =>
lib.importUri.path == '/a/b/c/a.dart');
On 2017/07/11 09:34:26, ahe wrote:
> Custom URI.

Acknowledged.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:107: var program = await
compileScript('a() => print("hi"); main() {}',
On 2017/07/11 09:34:26, ahe wrote:
> TODO about main?

Acknowledged.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/ker...
pkg/front_end/test/kernel_generator_test.dart:125: .firstWhere((lib) =>
lib.importUri.path == '/a/b/c/a.dart');
On 2017/07/11 09:34:26, ahe wrote:
> Custom URI.

Acknowledged.

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
File pkg/front_end/test/src/base/processed_options_test.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/test/src...
pkg/front_end/test/src/base/processed_options_test.dart:30: new
Program(libraries: [new Library(Uri.parse('file:///a/b.dart'))]);
On 2017/07/11 09:34:26, ahe wrote:
> On 2017/07/06 14:20:05, Paul Berry wrote:
> > On 2017/07/06 13:12:22, ahe wrote:
> > > As far as I can tell, this file relies heavily on the assumption that its
> > being
> > > run on a machine that doesn't have a the following directories:
> > > 
> > > /a
> > > /sdk
> > > etc.
> > > 
> > > This is super brittle and easily avoided by using custom URI schemes.
> > 
> > Actually it doesn't matter what directories exist on the machine, since
we're
> > using a MemoryFileSystem.  The physical file system is never examined.
> 
> I think it matters for the same reason as this is problematic:
> 
>     const bool isTrue = false;
> 
> Using file URIs as fake URIs is brittle, especially since we also have a
hybrid
> file system.

Next time we see each other we can discuss more about this. For now I filed
https://github.com/dart-lang/sdk/issues/30141

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/tool/fas...
File pkg/front_end/tool/fasta_perf.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/pkg/front_end/tool/fas...
pkg/front_end/tool/fasta_perf.dart:74:
Uri.base.resolve(Platform.resolvedExecutable).resolve('patched_sdk/');
On 2017/07/11 09:34:26, ahe wrote:
> Uri.base.resolveUri(new
> Uri.file(Platform.resolvedExecutable)).resolve('patched_sdk/');

When do these two options behave differently? If you have a different scheme?

https://codereview.chromium.org/2953703002/diff/140001/tools/patch_sdk.dart
File tools/patch_sdk.dart (right):

https://codereview.chromium.org/2953703002/diff/140001/tools/patch_sdk.dart#n...
tools/patch_sdk.dart:129: var target = forVm
On 2017/07/11 09:34:26, ahe wrote:
> This seems brittle compared to:
> 
> switch (mode) {
>   case "vm": ...
>   case "flutter": ...
>   case "dart2js" ...
>   default:
>     throw "Unhandled mode: $mode";
> }

will do