First pass at asynchronous input loading in dart2js.

First pass at asynchronous input loading in dart2js.

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

Committed: https://code.google.com/p/dart/source/detail?r=27028

[Bob Nystrom, 2013-06-26 01:03:23]

I'm still working on this, but I wanted to give you a chance to look at it and
tell me what you think.

I did the minimal changes I could to dart2js to get the async test passing. That
basically meant removing the call to deprecatedFutureValue() and then
mechanically (but by hand) translating everything above that on the callstack to
be async.

Most of the changes are in tests (since they are the top of the callstack, most
of them are affected). Most of dart2js itself is unaffected, though the
deferring loading stuff will be a bit tricky.

I haven't tried to do anything clever asynchronously. No work queues or anything
like that. I just tried to get the existing semantics and preserve them will
using futures.

Let me know what you think. Thanks!

- bob

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:382: // TODO(rnystrom):
Handle futures.
This method assumes the f() is completely done by the time it returns, which
isn't the case for futures.

In the places that use this, I've manually called withCurrentElement() again
isn't the future's then() callback to restore the "current" element, but that's
a bit ugly. There may be a cleaner solution to this that's more future-aware.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:806:
isolateHelperLibrary = library;
This is a bit ugly, but it ensures that these libraries are scanned
sequentially. If you don't care about that, I can use Future.wait() here and it
will actually load them in parallel.

That might make the order of error messages non-deterministic, though.

With this patch, I've tried to keep everything sequential and in the same order,
even though it's async.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:1375: print("done
measuring async $this");
Oops, I'll delete these debug prints.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:67: //
TODO(rnystrom): Handle future.
I still have to get this working. This is a bit harder because we don't get here
until much later in the compile process and I'd like all async stuff to be done
by now.

One option would be to just load "dart:async" at the same time we load other
libraries early on and then pass it to this task. Thoughts?

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:234: return
createLibrary(currentHandler, null, resolvedUri, node,
canonicalUri).then((library) {
I'll fix these long lines.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:234: });
Well, at least one part of the codebase got cleaner!

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:143:
LinkBuilder<tree.LibraryTag> imports = new LinkBuilder<tree.LibraryTag>();
I'll fix this.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/util/link.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/util/link.dart:7: class Link<T>
extends IterableBase<T> implements Iterable<T> {
I added this because I'm passing a Link<T> to Future.forEach(), which expects an
Iterable. Is this OK?

If not, I can manually copy the Link to a list instead.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/sa...
File sdk/lib/_internal/compiler/samples/leap/leap_leg.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/sa...
sdk/lib/_internal/compiler/samples/leap/leap_leg.dart:128: // TODO(rnystrom):
Handle future.
I haven't updated this sample to the new async API yet. Is this still live code?
Do you know how I can test it?

https://codereview.chromium.org/17759007/diff/1/tests/compiler/dart2js/async_...
File tests/compiler/dart2js/async_compiler_input_provider_test.dart (right):

https://codereview.chromium.org/17759007/diff/1/tests/compiler/dart2js/async_...
tests/compiler/dart2js/async_compiler_input_provider_test.dart:3: // BSD-style
license that can be found in the LICENSE file.
This is the new test.

This fails with the current dart2js, but passes with these changes.

https://codereview.chromium.org/17759007/diff/1/tests/compiler/dart2js/async_...
tests/compiler/dart2js/async_compiler_input_provider_test.dart:36: return new
Future(() => source);
If you do new Future.value(source) here, the test will pass in the current code.
But if you actually pump the event loop like this does... fail.

[ahe, 2013-06-26 07:02:00]

Bob, this is very nice.  Thank you for doing this.  I'm not sure I could have
done this myself, it seems like you have become quite the async expert.

One thing is that all the tests are now async (so they need to tell the test
runner that). As we compiler engineers generally have a hard time dealing with
frameworks, I think we would prefer to not use the unittest framework.  However,
you can use dart/tests/async_helper.dart.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/co...
File sdk/lib/_internal/compiler/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/co...
sdk/lib/_internal/compiler/compiler.dart:95: return
compiler.run(script).then((_) {
Could you add something like:

// TODO(ahe): Use the value of the future (which signals success or failure).

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:806:
isolateHelperLibrary = library;
On 2013/06/26 01:03:24, Bob Nystrom wrote:
> This is a bit ugly, but it ensures that these libraries are scanned
> sequentially. If you don't care about that, I can use Future.wait() here and
it
> will actually load them in parallel.
> 
> That might make the order of error messages non-deterministic, though.
> 
> With this patch, I've tried to keep everything sequential and in the same
order,
> even though it's async.

I think we can use Future.wait here.  Normally these libraries will not produce
error messages.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:821: return
libraryLoader.loadLibrary(uri, null, uri).then((libraryElement) {
Please restore the explicit type of libraryElement.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:1374: return
action().whenComplete(() {
I'm not sure measureAsync actually works.

The problem I see is that when you return from this method, previousTask is
still "running", but whatever work it does before yielding to the main event
loop will be counted as belonging to this task.

As far as I can tell, we can only count how much time is being spent reading
files by having a global counter that covers the time from the beginning of the
compiler until it reaches a point when all libraries are scanned, and subtract
the time spent in other tasks.

I think you should drop measureAsync. I think we can measure the time spent
reading files in dart2js.dart (you can just add a TODO).

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:67: //
TODO(rnystrom): Handle future.
On 2013/06/26 01:03:24, Bob Nystrom wrote:
> I still have to get this working. This is a bit harder because we don't get
here
> until much later in the compile process and I'd like all async stuff to be
done
> by now.
> 
> One option would be to just load "dart:async" at the same time we load other
> libraries early on and then pass it to this task. Thoughts?

Actually, I think this is easy to fix.  I just need to slightly rewrite this
code, which should also make the compiler faster by not scanning dart:async
unless it really is needed. I'll handle this.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:230:
Future<LibraryElement> loadLibrary(Uri resolvedUri, Node node, Uri canonicalUri)
{
Long line.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:231: return
measureAsync(() {
I think this should be measure

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:234: return
createLibrary(currentHandler, null, resolvedUri, node,
canonicalUri).then((library) {
An then wrap this is in measure as well.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:234: return
createLibrary(currentHandler, null, resolvedUri, node,
canonicalUri).then((library) {
Please restore the type of "library".

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:269: return
Future.forEach(library.tags.reverse(), (tag) {
I need to figure out what this does.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:234: });
On 2013/06/26 01:03:24, Bob Nystrom wrote:
> Well, at least one part of the codebase got cleaner!

Indeed!

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/util/link.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/util/link.dart:7: class Link<T>
extends IterableBase<T> implements Iterable<T> {
On 2013/06/26 01:03:24, Bob Nystrom wrote:
> I added this because I'm passing a Link<T> to Future.forEach(), which expects
an
> Iterable. Is this OK?

Nope, we don't support length.

> If not, I can manually copy the Link to a list instead.

You can say link.toList(), but I've filed http://dartbug.com/11523.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/sa...
File sdk/lib/_internal/compiler/samples/leap/leap_leg.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/sa...
sdk/lib/_internal/compiler/samples/leap/leap_leg.dart:128: // TODO(rnystrom):
Handle future.
On 2013/06/26 01:03:24, Bob Nystrom wrote:
> I haven't updated this sample to the new async API yet. Is this still live
code?
> Do you know how I can test it?

Don't bother, it is probably broken anyways. I should delete this when I land
try.dartlang.org as a compiler sample.

[Bob Nystrom, 2013-06-27 00:38:18]

I think I caught all of your comments, but let me know if I missed any.

With this (and getting latest), most of the language tests seem to be passing. I
get a few test failures in the dart2js unit tests:

FAILED: dart2js/analyze_api_test

    sdk/lib/_internal/lib/js_mirrors.dart:91:16: Error: cannot resolve type
DeclarationMirror
        implements DeclarationMirror {
                   ^^^^^^^^^^^^^^^^^
    Uncaught Error: Expect.isFalse(true) fails.

    Expect._fail (package:expect/expect.dart:368:5)
    Expect.isFalse (package:expect/expect.dart:120:10)
    CollectingDiagnosticHandler.checkResults
(dart/tests/compiler/dart2js/analyze_helper.dart:52:19)
    analyze.<anonymous closure>
(dart/tests/compiler/dart2js/analyze_helper.dart:139:25)

    python tools/test.py -mrelease -t60 dart2js/analyze_api_test

FAILED: dart2js/analyze_dart2js_test

    sdk/lib/isolate/isolate_stream.dart:97:36: Error: cannot resolve type
EventSink
    abstract class IsolateSink extends EventSink<dynamic> {
                                       ^^^^^^^^^
    Uncaught Error: Expect.isFalse(true) fails.

    Expect._fail (package:expect/expect.dart:368:5)
    Expect.isFalse (package:expect/expect.dart:120:10)
    CollectingDiagnosticHandler.checkResults
(dart/tests/compiler/dart2js/analyze_helper.dart:52:19)
    analyze.<anonymous closure>
(dart/tests/compiler/dart2js/analyze_helper.dart:139:25)

    python tools/test.py -mrelease -t60 dart2js/analyze_dart2js_test

FAILED: dart2js/deferred_load_graph_segmentation_test

    python tools/test.py -mrelease -t60
dart2js/deferred_load_graph_segmentation_test

FAILED: dart2js/metadata_test

    python tools/test.py -mrelease -t60 dart2js/metadata_test

FAILED: dart2js/mirrors_test

    sdk/lib/_internal/lib/js_mirrors.dart:91:16: Error: cannot resolve type
DeclarationMirror
        implements DeclarationMirror {
                   ^^^^^^^^^^^^^^^^^
    Uncaught Error: Failed to create mirror system.

    python tools/test.py -mrelease -t60 dart2js/mirrors_test

I could be wrong, but I think these all boil down to the deferred library load
that isn't async. Any thoughts on how I should move forward on that? I'd like to
have a nice clean check-in that leaves everything passing like it was before, if
possible, but I'm open to anything.

Cheers!

- bob

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/co...
File sdk/lib/_internal/compiler/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/co...
sdk/lib/_internal/compiler/compiler.dart:95: return
compiler.run(script).then((_) {
On 2013/06/26 07:02:00, ahe wrote:
> Could you add something like:
> 
> // TODO(ahe): Use the value of the future (which signals success or failure).

Done.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:806:
isolateHelperLibrary = library;
On 2013/06/26 07:02:00, ahe wrote:
> On 2013/06/26 01:03:24, Bob Nystrom wrote:
> > This is a bit ugly, but it ensures that these libraries are scanned
> > sequentially. If you don't care about that, I can use Future.wait() here and
> it
> > will actually load them in parallel.
> > 
> > That might make the order of error messages non-deterministic, though.
> > 
> > With this patch, I've tried to keep everything sequential and in the same
> order,
> > even though it's async.
> 
> I think we can use Future.wait here.  Normally these libraries will not
produce
> error messages.

I just tried that but it apparently causes some other issues. I think it gets
confused and ends up loading "dart:core" multiple times because there are
multiple loads of it in flight at the same time.

I can try to clean this up to use Future.forEach() or leave it with explicit
chained .then()s like this.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:821: return
libraryLoader.loadLibrary(uri, null, uri).then((libraryElement) {
On 2013/06/26 07:02:00, ahe wrote:
> Please restore the explicit type of libraryElement.

Done.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/compiler.dart:1374: return
action().whenComplete(() {
On 2013/06/26 07:02:00, ahe wrote:
> I'm not sure measureAsync actually works.
> 
> The problem I see is that when you return from this method, previousTask is
> still "running", but whatever work it does before yielding to the main event
> loop will be counted as belonging to this task.
> 
> As far as I can tell, we can only count how much time is being spent reading
> files by having a global counter that covers the time from the beginning of
the
> compiler until it reaches a point when all libraries are scanned, and subtract
> the time spent in other tasks.

That sounds good to me.

> 
> I think you should drop measureAsync.

Done.

> I think we can measure the time spent
> reading files in dart2js.dart (you can just add a TODO).

I put this in compiler.dart above ReadingFilesTask since that seemed related.
Does that work?

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:67: //
TODO(rnystrom): Handle future.
On 2013/06/26 07:02:00, ahe wrote:
> On 2013/06/26 01:03:24, Bob Nystrom wrote:
> > I still have to get this working. This is a bit harder because we don't get
> here
> > until much later in the compile process and I'd like all async stuff to be
> done
> > by now.
> > 
> > One option would be to just load "dart:async" at the same time we load other
> > libraries early on and then pass it to this task. Thoughts?
> 
> Actually, I think this is easy to fix.  I just need to slightly rewrite this
> code, which should also make the compiler faster by not scanning dart:async
> unless it really is needed. I'll handle this.

This is breaking a couple of tests. Should I file a bug and mark them failing,
or should I try to get them passing?

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:230:
Future<LibraryElement> loadLibrary(Uri resolvedUri, Node node, Uri canonicalUri)
{
On 2013/06/26 07:02:00, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:231: return
measureAsync(() {
On 2013/06/26 07:02:00, ahe wrote:
> I think this should be measure

Done.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:234: return
createLibrary(currentHandler, null, resolvedUri, node,
canonicalUri).then((library) {
On 2013/06/26 07:02:00, ahe wrote:
> An then wrap this is in measure as well.

Done.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:234: return
createLibrary(currentHandler, null, resolvedUri, node,
canonicalUri).then((library) {
On 2013/06/26 07:02:00, ahe wrote:
> Please restore the type of "library".

Done.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/library_loader.dart:269: return
Future.forEach(library.tags.reverse(), (tag) {
On 2013/06/26 07:02:00, ahe wrote:
> I need to figure out what this does.

It should be semantically equivalent to the previous code, just async.
Future.forEach takes a collection and a callback. For each item in the
collection it invokes the callback and passes in the item. The callback is
expected to return a future. It then waits until that future completes before
advancing to the next item.

It's basically a sequential wait. Where Future.wait() waits for all futures in
parallel, .forEach() runs them in series.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
File sdk/lib/_internal/compiler/implementation/util/link.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/im...
sdk/lib/_internal/compiler/implementation/util/link.dart:7: class Link<T>
extends IterableBase<T> implements Iterable<T> {
On 2013/06/26 07:02:00, ahe wrote:
> On 2013/06/26 01:03:24, Bob Nystrom wrote:
> > I added this because I'm passing a Link<T> to Future.forEach(), which
expects
> an
> > Iterable. Is this OK?
> 
> Nope, we don't support length.

Ah, that makes sense.

> 
> > If not, I can manually copy the Link to a list instead.
> 
> You can say link.toList(), but I've filed http://dartbug.com/11523.

OK, changed it to use that and added TODOs mentioning that bug.

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/sa...
File sdk/lib/_internal/compiler/samples/leap/leap_leg.dart (right):

https://codereview.chromium.org/17759007/diff/1/sdk/lib/_internal/compiler/sa...
sdk/lib/_internal/compiler/samples/leap/leap_leg.dart:128: // TODO(rnystrom):
Handle future.
On 2013/06/26 07:02:00, ahe wrote:
> On 2013/06/26 01:03:24, Bob Nystrom wrote:
> > I haven't updated this sample to the new async API yet. Is this still live
> code?
> > Do you know how I can test it?
> 
> Don't bother, it is probably broken anyways. I should delete this when I land
> http://try.dartlang.org as a compiler sample.

That's what I figured. Updated the TODOs to explain more.

[Bob Nystrom, 2013-06-27 23:33:00]

The patch you sent definitely helps. It fixes the two fails that touch the
deferred loading code. I still get these three failures:

~/dev/dart/dart (async-dart2js) → ./tools/test.py -m release dart2js
Test configuration: none_vm_release_ia32
FAILED: none-vm release_ia32 dart2js/analyze_api_test
Expected: pass slow
Actual: fail
CommandOutput[0]:

stdout:
sdk/lib/_internal/lib/js_mirrors.dart:91:16: Error: cannot resolve type
DeclarationMirror
    implements DeclarationMirror {
               ^^^^^^^^^^^^^^^^^
Uncaught Error: Expect.isFalse(true) fails.
Stack Trace:
#0      Expect._fail (package:expect/expect.dart:368:5)
#1      Expect.isFalse (package:expect/expect.dart:120:10)
#2      CollectingDiagnosticHandler.checkResults
(file:///Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/analyze_helper.dart:52:19)
#3      analyze.<anonymous closure>
(file:///Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/analyze_helper.dart:139:25)

Command[0]: xcodebuild/ReleaseIA32/dart --ignore-unrecognized-flags
--package-root=xcodebuild/ReleaseIA32/packages/
/Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/analyze_api_test.dart
Took 0:00:02.213000

Short reproduction command (experimental):
    python tools/test.py -mrelease -t60 dart2js/analyze_api_test

FAILED: none-vm release_ia32 dart2js/analyze_dart2js_test
Expected: pass
Actual: fail
CommandOutput[0]:

stdout:
sdk/lib/isolate/isolate_stream.dart:97:36: Error: cannot resolve type EventSink
abstract class IsolateSink extends EventSink<dynamic> {
                                   ^^^^^^^^^
Uncaught Error: Expect.isFalse(true) fails.
Stack Trace:
#0      Expect._fail (package:expect/expect.dart:368:5)
#1      Expect.isFalse (package:expect/expect.dart:120:10)
#2      CollectingDiagnosticHandler.checkResults
(file:///Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/analyze_helper.dart:52:19)
#3      analyze.<anonymous closure>
(file:///Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/analyze_helper.dart:139:25)

Command[0]: xcodebuild/ReleaseIA32/dart --ignore-unrecognized-flags
--package-root=xcodebuild/ReleaseIA32/packages/
/Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/analyze_dart2js_test.dart
Took 0:00:02.774000

Short reproduction command (experimental):
    python tools/test.py -mrelease -t60 dart2js/analyze_dart2js_test

FAILED: none-vm release_ia32 dart2js/mirrors_test
Expected: pass
Actual: fail
CommandOutput[0]:

stdout:
sdk/lib/_internal/lib/js_mirrors.dart:91:16: Error: cannot resolve type
DeclarationMirror
    implements DeclarationMirror {
               ^^^^^^^^^^^^^^^^^
Uncaught Error: Failed to create mirror system.


stderr:
Unhandled exception:
Failed to create mirror system.


Command[0]: xcodebuild/ReleaseIA32/dart --ignore-unrecognized-flags
--package-root=xcodebuild/ReleaseIA32/packages/
/Users/rnystrom/dev/dart/dart/tests/compiler/dart2js/mirrors_test.dart
Took 0:00:01.430000

Short reproduction command (experimental):
    python tools/test.py -mrelease -t60 dart2js/mirrors_test


I don't quite understand what's going on here or why my change would have caused
these. Any ideas?

[ahe, 2013-07-30 13:13:39]

Johnni, could you take a look?

[Johnni Winther, 2013-07-30 13:48:14]

On 2013/07/30 13:13:39, ahe wrote:
> Johnni, could you take a look?

Do you know which revision this CL is based upon so I can patch and test it
locally?

[Johnni Winther, 2013-07-31 07:34:08]

LGTM when applying the patch fix I've mailed you.

[Bob Nystrom, 2013-08-01 00:33:44]

Took your fix and then updated to latest code.

./tools/test.py -m release dart2js

passes all tests.

./tools/test.py -m release -r d8 -c dart2js

got about halfway before I had to leave for the evening. I saw two failures:
both stack overflows on tests that were marked "pass crash". I think because
they are async now, instead of a hard crash it fails? I'm not exactly sure.

PTAL and let me know what you think. Thanks!

[Johnni Winther, 2013-08-01 15:01:31]

On 2013/08/01 00:33:44, Bob Nystrom wrote:
> Took your fix and then updated to latest code.
> 
> ./tools/test.py -m release dart2js
> 
> passes all tests.
> 
> ./tools/test.py -m release -r d8 -c dart2js
> 
> got about halfway before I had to leave for the evening. I saw two failures:
> both stack overflows on tests that were marked "pass crash". I think because
> they are async now, instead of a hard crash it fails? I'm not exactly sure.
> 
> PTAL and let me know what you think. Thanks!

I have a fix for all but one test. I expect to have it ready for you tomorrow
(friday).

[Bob Nystrom, 2013-08-01 17:31:42]

On 2013/08/01 15:01:31, Johnni Winther wrote:
> On 2013/08/01 00:33:44, Bob Nystrom wrote:
> > Took your fix and then updated to latest code.
> > 
> > ./tools/test.py -m release dart2js
> > 
> > passes all tests.
> > 
> > ./tools/test.py -m release -r d8 -c dart2js
> > 
> > got about halfway before I had to leave for the evening. I saw two failures:
> > both stack overflows on tests that were marked "pass crash". I think because
> > they are async now, instead of a hard crash it fails? I'm not exactly sure.
> > 
> > PTAL and let me know what you think. Thanks!
> 
> I have a fix for all but one test. I expect to have it ready for you tomorrow
> (friday).

That sounds awesome!

If you would like to take this patch and land it yourself, do feel free. You
certainly understand the larger dart2js context better than I do. But if you're
rather I land it deal with any breakages I cause, that's fine too.

- bob

[Johnni Winther, 2013-08-02 11:05:57]

On 2013/08/01 17:31:42, Bob Nystrom wrote:
> On 2013/08/01 15:01:31, Johnni Winther wrote:
> > On 2013/08/01 00:33:44, Bob Nystrom wrote:
> > > Took your fix and then updated to latest code.
> > > 
> > > ./tools/test.py -m release dart2js
> > > 
> > > passes all tests.
> > > 
> > > ./tools/test.py -m release -r d8 -c dart2js
> > > 
> > > got about halfway before I had to leave for the evening. I saw two
failures:
> > > both stack overflows on tests that were marked "pass crash". I think
because
> > > they are async now, instead of a hard crash it fails? I'm not exactly
sure.
> > > 
> > > PTAL and let me know what you think. Thanks!
> > 
> > I have a fix for all but one test. I expect to have it ready for you
tomorrow
> > (friday).
> 
> That sounds awesome!
> 
> If you would like to take this patch and land it yourself, do feel free. You
> certainly understand the larger dart2js context better than I do. But if
you're
> rather I land it deal with any breakages I cause, that's fine too.
> 
> - bob

I will land this CL.

[ahe, 2013-08-02 11:50:19]

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:459: return
f().catchError((ex) {
I don't think this will work. I'l talk to you in person.

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:464: // Do nothing.
Why is that?

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/source_file_provider.dart
(right):

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/source_file_provider.dart:41: return
new Future.error(
As far as I'm concerned, the compiler should be able to handle an exception
thrown by this method.

[Johnni Winther, 2013-08-02 13:47:47]

lgtm

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/compiler.dart:459: return
f().catchError((ex) {
On 2013/08/02 11:50:19, ahe wrote:
> I don't think this will work. I'l talk to you in person.

Removed.

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
File sdk/lib/_internal/compiler/implementation/source_file_provider.dart
(right):

https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
sdk/lib/_internal/compiler/implementation/source_file_provider.dart:41: return
new Future.error(
On 2013/08/02 11:50:19, ahe wrote:
> As far as I'm concerned, the compiler should be able to handle an exception
> thrown by this method.

Done.

[Johnni Winther, 2013-08-02 13:48:21]

On 2013/08/02 13:47:47, Johnni Winther wrote:
> lgtm
> 
>
https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
> File sdk/lib/_internal/compiler/implementation/compiler.dart (right):
> 
>
https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
> sdk/lib/_internal/compiler/implementation/compiler.dart:459: return
> f().catchError((ex) {
> On 2013/08/02 11:50:19, ahe wrote:
> > I don't think this will work. I'l talk to you in person.
> 
> Removed.
> 
>
https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
> File sdk/lib/_internal/compiler/implementation/source_file_provider.dart
> (right):
> 
>
https://codereview.chromium.org/17759007/diff/79001/sdk/lib/_internal/compile...
> sdk/lib/_internal/compiler/implementation/source_file_provider.dart:41: return
> new Future.error(
> On 2013/08/02 11:50:19, ahe wrote:
> > As far as I'm concerned, the compiler should be able to handle an exception
> > thrown by this method.
> 
> Done.

Well, I'm used to pushing the 'Quick LGTM' button ;-)

[Bob Nystrom, 2013-08-02 15:15:54]

On 2013/08/02 11:05:57, Johnni Winther wrote:
> On 2013/08/01 17:31:42, Bob Nystrom wrote:
> > On 2013/08/01 15:01:31, Johnni Winther wrote:
> > > On 2013/08/01 00:33:44, Bob Nystrom wrote:
> > > > Took your fix and then updated to latest code.
> > > > 
> > > > ./tools/test.py -m release dart2js
> > > > 
> > > > passes all tests.
> > > > 
> > > > ./tools/test.py -m release -r d8 -c dart2js
> > > > 
> > > > got about halfway before I had to leave for the evening. I saw two
> failures:
> > > > both stack overflows on tests that were marked "pass crash". I think
> because
> > > > they are async now, instead of a hard crash it fails? I'm not exactly
> sure.
> > > > 
> > > > PTAL and let me know what you think. Thanks!
> > > 
> > > I have a fix for all but one test. I expect to have it ready for you
> tomorrow
> > > (friday).
> > 
> > That sounds awesome!
> > 
> > If you would like to take this patch and land it yourself, do feel free. You
> > certainly understand the larger dart2js context better than I do. But if
> you're
> > rather I land it deal with any breakages I cause, that's fine too.
> > 
> > - bob
> 
> I will land this CL.

Thanks! Let me know if there's anything else I can do to help. :)

- bob

[Johnni Winther, 2013-08-05 08:39:59]

@ahe: PTAL at the latest changes.

[ahe, 2013-08-06 16:29:17]

I stopped reviewing the tests.  There seems to be a systematic problem.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:179: // [Future] to
ensure that we exexute an asynchronous action without setting
"we exexute" -> "we never execute"

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:95: class
ReadingFilesTask extends CompilerTask {
Can you remove this task?

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:686: return true;
Can't you eliminate this call to then?

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:709: return
!compilationFailed;
As it should be handled by this.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:916: Element main =
null;
I think it would make sense to move the rest of this method into its own method.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:7: import
'dart:async';
Remove unneeded import.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:237: return
measure(() {
Record current element?

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:306: }).then((_) {
Seems like there is a lack of calls to withCurrentElement below. Not sure.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:415:
.then((LibraryElement loadedLibrary) {
withCurrentElement?

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:453: return
compiler.readScript(readableUri, importingLibrary, node).
Period goes on next line according to style guide IIRC.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:459:
compiler.withCurrentElement(element, () {
Would it make sense to move lines 456-458 into this closure?

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:140:
handler.registerNewLibrary(patchLibrary);
Shouldn't lines 140-141 be inside withCurrentElement(patchLibrary... ?

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:153: return
Future.forEach(imports.toLink().toList(), (tag) {
Shouldn't there be a withCurrentElement here?

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/analyze_all_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
tests/compiler/dart2js/analyze_all_test.dart:30: Uri uri =
Uri.parse('test:code');
Needs to use dart/tests/async_helper.dart.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/analyze_helper.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
tests/compiler/dart2js/analyze_helper.dart:138: return
compiler.run(null).then((_) {
Needs dart/tests/async_helper.dart.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/async_compiler_input_provider_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
tests/compiler/dart2js/async_compiler_input_provider_test.dart:49: provideInput,
handleDiagnostic, []).then((code) {
Needs dart/tests/async_helper.dart.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/b...
File tests/compiler/dart2js/bad_loop_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/b...
tests/compiler/dart2js/bad_loop_test.dart:40:
compiler.run(Uri.parse('memory:main.dart')).then((_) {
Needs dart/tests/async_helper.dart.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/m...
File tests/compiler/dart2js/missing_file_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/m...
tests/compiler/dart2js/missing_file_test.dart:62: compiler.run(main).then((_) {
This test is async and must use dart/tests/async_helper.dart.

[ahe, 2013-08-06 16:30:53]

On 2013/08/06 16:29:17, ahe wrote:
> I stopped reviewing the tests.  There seems to be a systematic problem.

Besides the issues with lack of using dart/tests/async_helper.dart, I think
we're pretty close.

[Bob Nystrom, 2013-08-09 22:46:36]

On 2013/08/06 16:30:53, ahe wrote:
> On 2013/08/06 16:29:17, ahe wrote:
> > I stopped reviewing the tests.  There seems to be a systematic problem.
> 
> Besides the issues with lack of using dart/tests/async_helper.dart, I think
> we're pretty close.

Friendly ping. Any update on this?

[Johnni Winther, 2013-08-10 13:34:23]

On 2013/08/09 22:46:36, Bob Nystrom wrote:
> On 2013/08/06 16:30:53, ahe wrote:
> > On 2013/08/06 16:29:17, ahe wrote:
> > > I stopped reviewing the tests.  There seems to be a systematic problem.
> > 
> > Besides the issues with lack of using dart/tests/async_helper.dart, I think
> > we're pretty close.
> 
> Friendly ping. Any update on this?

Peter and I got busy with other stuff. Will commit the CL after my vacation next
week.

[Bob Nystrom, 2013-08-12 19:45:02]

On 2013/08/10 13:34:23, Johnni Winther wrote:
> On 2013/08/09 22:46:36, Bob Nystrom wrote:
> > On 2013/08/06 16:30:53, ahe wrote:
> > > On 2013/08/06 16:29:17, ahe wrote:
> > > > I stopped reviewing the tests.  There seems to be a systematic problem.
> > > 
> > > Besides the issues with lack of using dart/tests/async_helper.dart, I
think
> > > we're pretty close.
> > 
> > Friendly ping. Any update on this?
> 
> Peter and I got busy with other stuff. Will commit the CL after my vacation
next
> week.

Another week? This patch is a month and a half old and it's blocking a key
component of the dev server. I'd really like to have dart2js support integrated
in the dev server by M6 but the window for that happening is getting very short.

- bob

[Bob Nystrom, 2013-08-20 16:04:43]

On 2013/08/12 19:45:02, Bob Nystrom wrote:
> On 2013/08/10 13:34:23, Johnni Winther wrote:
> > On 2013/08/09 22:46:36, Bob Nystrom wrote:
> > > On 2013/08/06 16:30:53, ahe wrote:
> > > > On 2013/08/06 16:29:17, ahe wrote:
> > > > > I stopped reviewing the tests.  There seems to be a systematic
problem.
> > > > 
> > > > Besides the issues with lack of using dart/tests/async_helper.dart, I
> think
> > > > we're pretty close.
> > > 
> > > Friendly ping. Any update on this?
> > 
> > Peter and I got busy with other stuff. Will commit the CL after my vacation
> next
> > week.
> 
> Another week? This patch is a month and a half old and it's blocking a key
> component of the dev server. I'd really like to have dart2js support
integrated
> in the dev server by M6 but the window for that happening is getting very
short.
> 
> - bob

Friendly ping. Any update?

- bob

[Johnni Winther, 2013-08-20 16:14:06]

On 2013/08/20 16:04:43, Bob Nystrom wrote:
> On 2013/08/12 19:45:02, Bob Nystrom wrote:
> > On 2013/08/10 13:34:23, Johnni Winther wrote:
> > > On 2013/08/09 22:46:36, Bob Nystrom wrote:
> > > > On 2013/08/06 16:30:53, ahe wrote:
> > > > > On 2013/08/06 16:29:17, ahe wrote:
> > > > > > I stopped reviewing the tests.  There seems to be a systematic
> problem.
> > > > > 
> > > > > Besides the issues with lack of using dart/tests/async_helper.dart, I
> > think
> > > > > we're pretty close.
> > > > 
> > > > Friendly ping. Any update on this?
> > > 
> > > Peter and I got busy with other stuff. Will commit the CL after my
vacation
> > next
> > > week.
> > 
> > Another week? This patch is a month and a half old and it's blocking a key
> > component of the dev server. I'd really like to have dart2js support
> integrated
> > in the dev server by M6 but the window for that happening is getting very
> short.
> > 
> > - bob
> 
> Friendly ping. Any update?
> 
> - bob

Halfway through updating unittests to use async_helper.dart

[Bob Nystrom, 2013-08-26 19:53:43]

On 2013/08/20 16:14:06, Johnni Winther wrote:
> On 2013/08/20 16:04:43, Bob Nystrom wrote:
> > On 2013/08/12 19:45:02, Bob Nystrom wrote:
> > > On 2013/08/10 13:34:23, Johnni Winther wrote:
> > > > On 2013/08/09 22:46:36, Bob Nystrom wrote:
> > > > > On 2013/08/06 16:30:53, ahe wrote:
> > > > > > On 2013/08/06 16:29:17, ahe wrote:
> > > > > > > I stopped reviewing the tests.  There seems to be a systematic
> > problem.
> > > > > > 
> > > > > > Besides the issues with lack of using dart/tests/async_helper.dart,
I
> > > think
> > > > > > we're pretty close.
> > > > > 
> > > > > Friendly ping. Any update on this?
> > > > 
> > > > Peter and I got busy with other stuff. Will commit the CL after my
> vacation
> > > next
> > > > week.
> > > 
> > > Another week? This patch is a month and a half old and it's blocking a key
> > > component of the dev server. I'd really like to have dart2js support
> > integrated
> > > in the dev server by M6 but the window for that happening is getting very
> > short.
> > > 
> > > - bob
> > 
> > Friendly ping. Any update?
> > 
> > - bob
> 
> Halfway through updating unittests to use async_helper.dart

Ping! What's the word?

- bob

[Johnni Winther, 2013-08-27 11:04:59]

Tests updated. PTAL

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:179: // [Future] to
ensure that we exexute an asynchronous action without setting
On 2013/08/06 16:29:17, ahe wrote:
> "we exexute" -> "we never execute"

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:95: class
ReadingFilesTask extends CompilerTask {
On 2013/08/06 16:29:17, ahe wrote:
> Can you remove this task?

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:686: return true;
On 2013/08/06 16:29:17, ahe wrote:
> Can't you eliminate this call to then?

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:916: Element main =
null;
On 2013/08/06 16:29:17, ahe wrote:
> I think it would make sense to move the rest of this method into its own
method.

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/deferred_load.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/deferred_load.dart:7: import
'dart:async';
On 2013/08/06 16:29:17, ahe wrote:
> Remove unneeded import.

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:237: return
measure(() {
On 2013/08/06 16:29:17, ahe wrote:
> Record current element?

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:306: }).then((_) {
On 2013/08/06 16:29:17, ahe wrote:
> Seems like there is a lack of calls to withCurrentElement below. Not sure.

Don't think they are needed but better safe than sorry.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:415:
.then((LibraryElement loadedLibrary) {
On 2013/08/06 16:29:17, ahe wrote:
> withCurrentElement?

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:453: return
compiler.readScript(readableUri, importingLibrary, node).
On 2013/08/06 16:29:17, ahe wrote:
> Period goes on next line according to style guide IIRC.

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:459:
compiler.withCurrentElement(element, () {
On 2013/08/06 16:29:17, ahe wrote:
> Would it make sense to move lines 456-458 into this closure?

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/patch_parser.dart (right):

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:140:
handler.registerNewLibrary(patchLibrary);
On 2013/08/06 16:29:17, ahe wrote:
> Shouldn't lines 140-141 be inside withCurrentElement(patchLibrary... ?

Done.

https://codereview.chromium.org/17759007/diff/101001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/patch_parser.dart:153: return
Future.forEach(imports.toLink().toList(), (tag) {
On 2013/08/06 16:29:17, ahe wrote:
> Shouldn't there be a withCurrentElement here?

Done.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/analyze_all_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
tests/compiler/dart2js/analyze_all_test.dart:30: Uri uri =
Uri.parse('test:code');
On 2013/08/06 16:29:17, ahe wrote:
> Needs to use dart/tests/async_helper.dart.

Done.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/analyze_helper.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
tests/compiler/dart2js/analyze_helper.dart:138: return
compiler.run(null).then((_) {
On 2013/08/06 16:29:17, ahe wrote:
> Needs dart/tests/async_helper.dart.

Added to all callers.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/async_compiler_input_provider_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/a...
tests/compiler/dart2js/async_compiler_input_provider_test.dart:49: provideInput,
handleDiagnostic, []).then((code) {
On 2013/08/06 16:29:17, ahe wrote:
> Needs dart/tests/async_helper.dart.

Done.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/b...
File tests/compiler/dart2js/bad_loop_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/b...
tests/compiler/dart2js/bad_loop_test.dart:40:
compiler.run(Uri.parse('memory:main.dart')).then((_) {
On 2013/08/06 16:29:17, ahe wrote:
> Needs dart/tests/async_helper.dart.

Done.

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/m...
File tests/compiler/dart2js/missing_file_test.dart (right):

https://codereview.chromium.org/17759007/diff/101001/tests/compiler/dart2js/m...
tests/compiler/dart2js/missing_file_test.dart:62: compiler.run(main).then((_) {
On 2013/08/06 16:29:17, ahe wrote:
> This test is async and must use dart/tests/async_helper.dart.

Done.

[Bob Nystrom, 2013-08-29 17:04:15]

Ping!

My understanding is that our burning customer issue is no longer burning. Is
there anything else preventing you from landing this?

[Johnni Winther, 2013-08-31 15:11:27]

On 2013/08/29 17:04:15, Bob Nystrom wrote:
> Ping!
> 
> My understanding is that our burning customer issue is no longer burning. Is
> there anything else preventing you from landing this?

I'm awaiting Peter's review of the latest changes.

[ahe, 2013-09-02 13:43:24]

Compiler changes, LGTM. I suggest you submit this CL now, while I review the
tests.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:165: void
reportReadError(String exception) {
Perhaps this method would be nice as a general method on compiler:

reportError(..., {Element inElement})

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:175: // [Future] to
ensure that we never execute an asynchronous action without setting
Long line.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:178: return
provider(resourceUri).then((String text) {
I think this code would be clearer if it was:

Future future;
try { // Guard against errors in user code.
  future = provider(resourceUri);
} catch (error, trace) {
  reportReadError(error);
  return new Future<leg.Script>.error(error, trace);
}
return future.then(...).catchError(...);

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:721: return
runCompiler(uri).catchError((error) {
I think runCompiler needs to be wrapped in try-catch. I see several things that
can happen outside the future action.

Perhaps you can use Future.sync instead.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:565:
compilerMain(options).catchError((exception) {
Wrap the call to compilerMain in try-catch.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:235:
currentHandler = new LibraryDependencyHandler(compiler);
This is nasty. I suggest you document that currentHandler is rather brittle now.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:234: throw
'Failed to create mirror system.';
throw new StateError or something.

[ahe, 2013-09-02 15:24:41]

LGTM

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/async_compiler_input_provider_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/a...
tests/compiler/dart2js/async_compiler_input_provider_test.dart:59: // Do
nothing.
Throw on non-verbose messages?

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/c...
File tests/compiler/dart2js/cpa_inference_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/c...
tests/compiler/dart2js/cpa_inference_test.dart:10: import
'../../../sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart';
There is a bunch of then blocks in this test that may not execute.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/c...
tests/compiler/dart2js/cpa_inference_test.dart:1493:
Expect.equals(convert(singleton(a).union(singleton(b)).union(nullSingleton)),
Long line.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/p...
File tests/compiler/dart2js/part_of_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/p...
tests/compiler/dart2js/part_of_test.dart:28:
compiler.libraryLoader.loadLibrary(libraryUri, null, libraryUri).then((_) {
Need async guard.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/t...
File tests/compiler/dart2js/type_guard_unuser_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/t...
tests/compiler/dart2js/type_guard_unuser_test.dart:1: // Copyright (c) 2011, the
Dart project authors.  Please see the AUTHORS file
Bad edit.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/t...
tests/compiler/dart2js/type_guard_unuser_test.dart:52: generated =
compile(TEST_TWO, entry: 'foo');
I think you need asyncTest here.

https://codereview.chromium.org/17759007/diff/149001/tests/utils/recursive_im...
File tests/utils/recursive_import_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/utils/recursive_im...
tests/utils/recursive_import_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
This test need to use async_helper.dart.

[Johnni Winther, 2013-09-03 07:51:39]

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/apiimpl.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:165: void
reportReadError(String exception) {
On 2013/09/02 13:43:24, ahe wrote:
> Perhaps this method would be nice as a general method on compiler:
> 
> reportError(..., {Element inElement})
> 

Added a TODO.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:175: // [Future] to
ensure that we never execute an asynchronous action without setting
On 2013/09/02 13:43:24, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/apiimpl.dart:178: return
provider(resourceUri).then((String text) {
On 2013/09/02 13:43:24, ahe wrote:
> I think this code would be clearer if it was:
> 
> Future future;
> try { // Guard against errors in user code.
>   future = provider(resourceUri);
> } catch (error, trace) {
>   reportReadError(error);
>   return new Future<leg.Script>.error(error, trace);
> }
> return future.then(...).catchError(...);

Done.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/compiler.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/compiler.dart:721: return
runCompiler(uri).catchError((error) {
On 2013/09/02 13:43:24, ahe wrote:
> I think runCompiler needs to be wrapped in try-catch. I see several things
that
> can happen outside the future action.
> 
> Perhaps you can use Future.sync instead.

Done.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/dart2js.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/dart2js.dart:565:
compilerMain(options).catchError((exception) {
On 2013/09/02 13:43:24, ahe wrote:
> Wrap the call to compilerMain in try-catch.

Done.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/library_loader.dart (right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/library_loader.dart:235:
currentHandler = new LibraryDependencyHandler(compiler);
On 2013/09/02 13:43:24, ahe wrote:
> This is nasty. I suggest you document that currentHandler is rather brittle
now.

Done.

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
File sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart
(right):

https://codereview.chromium.org/17759007/diff/149001/sdk/lib/_internal/compil...
sdk/lib/_internal/compiler/implementation/mirrors/dart2js_mirror.dart:234: throw
'Failed to create mirror system.';
On 2013/09/02 13:43:24, ahe wrote:
> throw new StateError or something.

Done.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/a...
File tests/compiler/dart2js/async_compiler_input_provider_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/a...
tests/compiler/dart2js/async_compiler_input_provider_test.dart:59: // Do
nothing.
On 2013/09/02 15:24:41, ahe wrote:
> Throw on non-verbose messages?

Done.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/c...
File tests/compiler/dart2js/cpa_inference_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/c...
tests/compiler/dart2js/cpa_inference_test.dart:10: import
'../../../sdk/lib/_internal/compiler/implementation/types/concrete_types_inferrer.dart';
On 2013/09/02 15:24:41, ahe wrote:
> There is a bunch of then blocks in this test that may not execute.

Done.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/c...
tests/compiler/dart2js/cpa_inference_test.dart:1493:
Expect.equals(convert(singleton(a).union(singleton(b)).union(nullSingleton)),
On 2013/09/02 15:24:41, ahe wrote:
> Long line.

Done.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/p...
File tests/compiler/dart2js/part_of_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/p...
tests/compiler/dart2js/part_of_test.dart:28:
compiler.libraryLoader.loadLibrary(libraryUri, null, libraryUri).then((_) {
On 2013/09/02 15:24:41, ahe wrote:
> Need async guard.

Done.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/t...
File tests/compiler/dart2js/type_guard_unuser_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/t...
tests/compiler/dart2js/type_guard_unuser_test.dart:1: // Copyright (c) 2011, the
Dart project authors.  Please see the AUTHORS file
On 2013/09/02 15:24:41, ahe wrote:
> Bad edit.

Done.

https://codereview.chromium.org/17759007/diff/149001/tests/compiler/dart2js/t...
tests/compiler/dart2js/type_guard_unuser_test.dart:52: generated =
compile(TEST_TWO, entry: 'foo');
On 2013/09/02 15:24:41, ahe wrote:
> I think you need asyncTest here.

No. [compile] returns a String synchronously (using the MockCompiler).

https://codereview.chromium.org/17759007/diff/149001/tests/utils/recursive_im...
File tests/utils/recursive_import_test.dart (right):

https://codereview.chromium.org/17759007/diff/149001/tests/utils/recursive_im...
tests/utils/recursive_import_test.dart:1: // Copyright (c) 2012, the Dart
project authors.  Please see the AUTHORS file
On 2013/09/02 15:24:41, ahe wrote:
> This test need to use async_helper.dart.

Done.

[Johnni Winther, 2013-09-03 08:09:47]

Committed patchset #13 manually as r27028 (presubmit successful).

[Bob Nystrom, 2013-09-03 15:22:17]

On 2013/09/03 08:09:47, Johnni Winther wrote:
> Committed patchset #13 manually as r27028 (presubmit successful).

Yay!

- bob

[karlklose, 2013-09-06 09:04:47]

With this change, we no longer print stack traces for uncaught exceptions from
the compiler.

[ahe, 2013-09-06 09:51:32]

On 2013/09/06 09:04:47, karlklose wrote:
> With this change, we no longer print stack traces for uncaught exceptions from
> the compiler.

Karl and I tracked it down. The root cause is that asynchronous exception
handling is broken. The concrete problem is that Karl was throwing a string,
which means that getAttachedStackTrace doesn't work.