Add "load from .dill" file capability and run a white-box test.

Add "load from .dill" file capability and run a white-box test.

The dill_loader_test is rather intrusive and I look forward to rewriting it once we have the Entity model spread throughout the compiler rather than Elements.

BUG=
R=johnniwinther@google.com

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

[Emily Fortuna, 2017-04-03 23:11:53]

Patchset #1 (id:1) has been deleted

[Emily Fortuna, 2017-04-03 23:12:55]

Description was changed from

==========
Add "load from .dill" file capability and run a white-box test.

BUG=
==========

to

==========
Add "load from .dill" file capability and run a white-box test.

The dill_loader_test is rather intrusive and I look forward to rewriting it once
we have the Entity model spread throughout the compiler rather than Elements.

BUG=
==========

[Emily Fortuna, 2017-04-03 23:12:55]

efortuna@google.com changed reviewers:
+ johnniwinther@google.com, sra@google.com

[Johnni Winther, 2017-04-04 07:25:20]

lgtm

https://codereview.chromium.org/2798443002/diff/20001/pkg/compiler/lib/src/li...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/2798443002/diff/20001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:878: //return
reporter.withCurrentElement(program, () {
Just remove this. It's only need if we report an error on an AST node.

https://codereview.chromium.org/2798443002/diff/20001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:887:
.lookupLibrary(program.mainMethod.enclosingLibrary.importUri),
We need to handle `program.mainMethod == null` gracefully (not in this CL).

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/dill_loader_test.dart (right):

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:22: String basePath =
path.fromUri(Platform.script);
Use `String basePath = path.fromUri(Uri.base);` and avoid the loop.

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:27:
path.normalize(path.join(basePath, 'tools/dartk_wrappers/dartk'));
This won't work on Windows. I'll fix it when you land it.
'tools/dartk_wrappers/' doesn't contain a 'dartk.bat' and we don't run unittests
on Windows, anyway.

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:49: Uri uri =
Platform.script.resolve('../../corelib/list_literal_test.dart');
Use `Uri.base.resolve('tests/corelib/list_literal_test.dart');` instead.

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:63: LibraryLoaderTask loader = new
LibraryLoaderTask(
`LibraryLoaderTask loader` -> `dynamic loader`

Then you don't need to put 'worldBuilder' in the LibraryLoaderTask interface.

[Emily Fortuna, 2017-04-04 20:03:53]

https://codereview.chromium.org/2798443002/diff/20001/pkg/compiler/lib/src/li...
File pkg/compiler/lib/src/library_loader.dart (right):

https://codereview.chromium.org/2798443002/diff/20001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:878: //return
reporter.withCurrentElement(program, () {
On 2017/04/04 07:25:20, Johnni Winther wrote:
> Just remove this. It's only need if we report an error on an AST node.

ah yes, sorry I intended to clean that up, but forgot.

https://codereview.chromium.org/2798443002/diff/20001/pkg/compiler/lib/src/li...
pkg/compiler/lib/src/library_loader.dart:887:
.lookupLibrary(program.mainMethod.enclosingLibrary.importUri),
On 2017/04/04 07:25:20, Johnni Winther wrote:
> We need to handle `program.mainMethod == null` gracefully (not in this CL).

Added TODO

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/dill_loader_test.dart (right):

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:22: String basePath =
path.fromUri(Platform.script);
On 2017/04/04 07:25:20, Johnni Winther wrote:
> Use `String basePath = path.fromUri(Uri.base);` and avoid the loop.

Done.

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:27:
path.normalize(path.join(basePath, 'tools/dartk_wrappers/dartk'));
On 2017/04/04 07:25:20, Johnni Winther wrote:
> This won't work on Windows. I'll fix it when you land it.
> 'tools/dartk_wrappers/' doesn't contain a 'dartk.bat' and we don't run
unittests
> on Windows, anyway.

I know, but I thought as per our discussion, that was an okay shortcoming?

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:49: Uri uri =
Platform.script.resolve('../../corelib/list_literal_test.dart');
On 2017/04/04 07:25:20, Johnni Winther wrote:
> Use `Uri.base.resolve('tests/corelib/list_literal_test.dart');` instead.

Done.

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:63: LibraryLoaderTask loader = new
LibraryLoaderTask(
On 2017/04/04 07:25:20, Johnni Winther wrote:
> `LibraryLoaderTask loader` -> `dynamic loader`
> 
> Then you don't need to put 'worldBuilder' in the LibraryLoaderTask interface.

Is that really an improvement, that way we would now have no checks at all,
whereas with LibraryLoaderTask we at least know it's a LibraryLoaderTask type or
subtype.

[Emily Fortuna, 2017-04-04 23:22:18]

Description was changed from

==========
Add "load from .dill" file capability and run a white-box test.

The dill_loader_test is rather intrusive and I look forward to rewriting it once
we have the Entity model spread throughout the compiler rather than Elements.

BUG=
==========

to

==========
Add "load from .dill" file capability and run a white-box test.

The dill_loader_test is rather intrusive and I look forward to rewriting it once
we have the Entity model spread throughout the compiler rather than Elements.

BUG=
R=johnniwinther@google.com

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

[Emily Fortuna, 2017-04-04 23:22:19]

Committed patchset #2 (id:40001) manually as
310532ab508ee9d54e44abe7b1b8e6d4e2cbc7b8 (presubmit successful).

[Johnni Winther, 2017-04-05 06:53:42]

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
File tests/compiler/dart2js/dill_loader_test.dart (right):

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:27:
path.normalize(path.join(basePath, 'tools/dartk_wrappers/dartk'));
On 2017/04/04 20:03:53, Emily Fortuna wrote:
> On 2017/04/04 07:25:20, Johnni Winther wrote:
> > This won't work on Windows. I'll fix it when you land it.
> > 'tools/dartk_wrappers/' doesn't contain a 'dartk.bat' and we don't run
> unittests
> > on Windows, anyway.
> 
> I know, but I thought as per our discussion, that was an okay shortcoming?

Yes. I'll fix it.

https://codereview.chromium.org/2798443002/diff/20001/tests/compiler/dart2js/...
tests/compiler/dart2js/dill_loader_test.dart:63: LibraryLoaderTask loader = new
LibraryLoaderTask(
On 2017/04/04 20:03:53, Emily Fortuna wrote:
> On 2017/04/04 07:25:20, Johnni Winther wrote:
> > `LibraryLoaderTask loader` -> `dynamic loader`
> > 
> > Then you don't need to put 'worldBuilder' in the LibraryLoaderTask
interface.
> 
> Is that really an improvement, that way we would now have no checks at all,
> whereas with LibraryLoaderTask we at least know it's a LibraryLoaderTask type
or
> subtype.

We are relying on implementation details (that the actual type has a meaningfull
[worldBuilder] property) but that is okay in tests. `dynamic` is just a way to
circumvent the type system and avoid a warning.