Migrate DDC to the new analysis driver.

Migrate DDC to the new analysis driver.

This version is not ready to be landed as is yet.

Known problems:

1. The changes to AnalysisDriver and file system state are not tested,
   and some are just incorrect are is (e.g. _cachedResults).

2. There are still 16 test failures while running DDC presubmit.


But, I'd like to get review from DDC people, whether they think this
approach is OK, or there are some fundamental problems.


R=vsm@google.com, jmesserly@google.com, brianwilkerson@google.com
BUG=

[vsm, 2017-03-17 15:50:32]

On 2017/03/16 20:33:49, scheglov wrote:

Generally ok with this approach.  Some q's:

do you see any perf issues with being async?

are there race conditions we need to worry about?

[scheglov, 2017-03-17 16:22:17]

On 2017/03/17 15:50:32, vsm wrote:
> On 2017/03/16 20:33:49, scheglov wrote:
> 
> Generally ok with this approach.  Some q's:
> 
> do you see any perf issues with being async?
> 
> are there race conditions we need to worry about?

It seems to make SDK compilation to JavaScript and running tests about 10%
slower.

I don't think there are race conditions, we don't run anything in parallel, we
wait for every asynchronous invocation.

[Jennifer Messerly, 2017-03-17 18:33:38]

seems very promising!!!

high level comment ... it's best to keep async as coarse grained as possible. I
would prefer if we isolate async stuff to the highest level of API/call stack,
and most classes in the compiler don't need to worry about it because we'll
already have the results we need.

DDC is essentially a stateless pure function, it takes SDK+module
dependencies+your source code and turns it into a compiled JS module. Thus it's
desirable for the architecture remains that way to the maximal extent possible.

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:119: {
i'm not sure this block is necessary

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:121: //      var logger = new
PerformanceLog(stdout);
remove?

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:239: var codeGenerator = await
CodeGenerator.create(
I would rather keep async out of CodeGenerator, and pass down the things that
are needed after we await them.

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/extension_types.dart (right):

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/extension_types.dart:108: Future<Null>
_initialize() async {
similar comment here; I would prefer to factor this such that we:

await SDK libs outside of this class

this class can remain fully sync.

[scheglov, 2017-04-25 17:07:35]

PTAL

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/compiler.dart (right):

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:119: {
On 2017/03/17 18:33:37, Jennifer Messerly wrote:
> i'm not sure this block is necessary

We defined a couple of variables that are not useful outside.
So, I use the block to scope them.

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:121: //      var logger = new
PerformanceLog(stdout);
On 2017/03/17 18:33:38, Jennifer Messerly wrote:
> remove?

Done.

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/compiler.dart:239: var codeGenerator = await
CodeGenerator.create(
On 2017/03/17 18:33:38, Jennifer Messerly wrote:
> I would rather keep async out of CodeGenerator, and pass down the things that
> are needed after we await them.

Done.

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
File pkg/dev_compiler/lib/src/compiler/extension_types.dart (right):

https://codereview.chromium.org/2757753002/diff/1/pkg/dev_compiler/lib/src/co...
pkg/dev_compiler/lib/src/compiler/extension_types.dart:108: Future<Null>
_initialize() async {
On 2017/03/17 18:33:38, Jennifer Messerly wrote:
> similar comment here; I would prefer to factor this such that we:
> 
> await SDK libs outside of this class
> 
> this class can remain fully sync.

Done.

[Brian Wilkerson, 2017-04-25 17:13:50]

lgtm

[scheglov, 2017-04-25 17:27:00]

With this change we pass presubmit tests.
But when I try to run tool/browser_test.sh, it fails :-(

25 04 2017 10:24:42.802:INFO [karma]: Karma v1.5.0 server started at
http://0.0.0.0:9876/
25 04 2017 10:24:42.804:INFO [launcher]: Launching browser Chrome with unlimited
concurrency
25 04 2017 10:24:42.811:INFO [launcher]: Starting browser Chrome
25 04 2017 10:24:43.576:INFO [Chrome 57.0.2987 (Mac OS X 10.12.4)]: Connected on
socket 3wLeQc3DN_cbOjCwAAAA with id 96785213
Chrome 57.0.2987 (Mac OS X 10.12.4) ERROR
  Uncaught TypeError: Class extends value undefined is not a constructor or null
  at lib/js/amd/dart_sdk.js:2864


Any ideas what I could have broken?

[scheglov, 2017-04-25 20:39:02]

Some progress.
I replaced using HashSet.identity() with normal HashSet.
Now all tests run and still, all with the same error.

Chrome 57.0.2987 (Mac OS X 10.12.4)  language/abstract_getter2_test FAILED
	TypeError: Cannot read property 'asyncTestInitialize' of undefined
	    at Context.<anonymous> (test/browser/language_tests.js:841:21)


It seems that something is generated incorrectly.
Any clues?

-  async_helper._buildException = function(msg) {
+  test__codegen__async_helper._buildException = function(msg) {
     return core.Exception.new(dart.str`Fatal: ${msg}. This is most likely a bug
in your test.`);
   };
-  dart.fn(async_helper._buildException, StringToException());
-  async_helper.asyncTestInitialize = function(callback) {
-    async_helper._asyncLevel = 0;
-    async_helper._initialized = false;
-    async_helper._onAsyncEnd = callback;
+  dart.fn(test__codegen__async_helper._buildException, StringToException());
+  test__codegen__async_helper.asyncTestInitialize = function(callback) {
+    test__codegen__async_helper._asyncLevel = 0;
+    test__codegen__async_helper._initialized = false;
+    test__codegen__async_helper._onAsyncEnd = callback;
   };
-  dart.fn(async_helper.asyncTestInitialize, _Action0Tovoid());
-  dart.copyProperties(async_helper, {
+  dart.fn(test__codegen__async_helper.asyncTestInitialize, _Action0Tovoid());
+  dart.copyProperties(test__codegen__async_helper, {

[Jennifer Messerly, 2017-05-01 19:27:14]

On 2017/04/25 20:39:02, scheglov wrote:
> Some progress.
> I replaced using HashSet.identity() with normal HashSet.
> Now all tests run and still, all with the same error.
> 
> Chrome 57.0.2987 (Mac OS X 10.12.4)  language/abstract_getter2_test FAILED
> 	TypeError: Cannot read property 'asyncTestInitialize' of undefined
> 	    at Context.<anonymous> (test/browser/language_tests.js:841:21)
> 
> 
> It seems that something is generated incorrectly.
> Any clues?
> 
> -  async_helper._buildException = function(msg) {
> +  test__codegen__async_helper._buildException = function(msg) {
>      return core.Exception.new(dart.str`Fatal: ${msg}. This is most likely a
bug
> in your test.`);
>    };
> -  dart.fn(async_helper._buildException, StringToException());
> -  async_helper.asyncTestInitialize = function(callback) {
> -    async_helper._asyncLevel = 0;
> -    async_helper._initialized = false;
> -    async_helper._onAsyncEnd = callback;
> +  dart.fn(test__codegen__async_helper._buildException, StringToException());
> +  test__codegen__async_helper.asyncTestInitialize = function(callback) {
> +    test__codegen__async_helper._asyncLevel = 0;
> +    test__codegen__async_helper._initialized = false;
> +    test__codegen__async_helper._onAsyncEnd = callback;
>    };
> -  dart.fn(async_helper.asyncTestInitialize, _Action0Tovoid());
> -  dart.copyProperties(async_helper, {
> +  dart.fn(test__codegen__async_helper.asyncTestInitialize, _Action0Tovoid());
> +  dart.copyProperties(test__codegen__async_helper, {

Hmmmm I dunno for sure, if you push a branch maybe I can try it out? Or let me
know what commit this CL is based on so I can "git cl patch" ?

Some guesses, "test__codegen__async_helper" is the library name. So something
has changed there.

I would start with emitLibraryName/jsLibraryName and work backwards. It seems to
rely on:
* LibraryElement hashing
* LibraryElement.source.uri
* DDC's module "libraryRoot" path, which IIRC usually comes from a command line
option

... so changes in that could affect its behavior.

[Jennifer Messerly, 2017-05-01 19:31:35]

Once the test issue is worked out, this LGTM! Thanks for putting so much work
into DDC and doing this in such a clean way! Love it.

https://codereview.chromium.org/2757753002/diff/30001/pkg/dev_compiler/lib/sr...
File pkg/dev_compiler/lib/src/compiler/extension_types.dart (right):

https://codereview.chromium.org/2757753002/diff/30001/pkg/dev_compiler/lib/sr...
pkg/dev_compiler/lib/src/compiler/extension_types.dart:37: {@required
LibraryElement coreLibrary,
thought here: since the list of dart:* libraries tends to change and increase
over time, would it be possible to pass these in as a Map<String,
LibraryElement> ?

That would make it a lot easier to run through them. WDYT?

That said ... our old code had a hard coded list, so I don't think you're making
things any worse. I'm happy to play around with this on my own next time we
actually add a new dart:* library.

Nice use of @required BTW. I need to remember that more myself :)

[scheglov, 2017-05-03 17:12:40]

On 2017/05/01 19:27:14, Jennifer Messerly wrote:
> On 2017/04/25 20:39:02, scheglov wrote:
> > Some progress.
> > I replaced using HashSet.identity() with normal HashSet.
> > Now all tests run and still, all with the same error.
> > 
> > Chrome 57.0.2987 (Mac OS X 10.12.4)  language/abstract_getter2_test FAILED
> > 	TypeError: Cannot read property 'asyncTestInitialize' of undefined
> > 	    at Context.<anonymous> (test/browser/language_tests.js:841:21)
> > 
> > 
> > It seems that something is generated incorrectly.
> > Any clues?
> > 
> > -  async_helper._buildException = function(msg) {
> > +  test__codegen__async_helper._buildException = function(msg) {
> >      return core.Exception.new(dart.str`Fatal: ${msg}. This is most likely a
> bug
> > in your test.`);
> >    };
> > -  dart.fn(async_helper._buildException, StringToException());
> > -  async_helper.asyncTestInitialize = function(callback) {
> > -    async_helper._asyncLevel = 0;
> > -    async_helper._initialized = false;
> > -    async_helper._onAsyncEnd = callback;
> > +  dart.fn(test__codegen__async_helper._buildException,
StringToException());
> > +  test__codegen__async_helper.asyncTestInitialize = function(callback) {
> > +    test__codegen__async_helper._asyncLevel = 0;
> > +    test__codegen__async_helper._initialized = false;
> > +    test__codegen__async_helper._onAsyncEnd = callback;
> >    };
> > -  dart.fn(async_helper.asyncTestInitialize, _Action0Tovoid());
> > -  dart.copyProperties(async_helper, {
> > +  dart.fn(test__codegen__async_helper.asyncTestInitialize,
_Action0Tovoid());
> > +  dart.copyProperties(test__codegen__async_helper, {
> 
> Hmmmm I dunno for sure, if you push a branch maybe I can try it out? Or let me
> know what commit this CL is based on so I can "git cl patch" ?
> 
> Some guesses, "test__codegen__async_helper" is the library name. So something
> has changed there.
> 
> I would start with emitLibraryName/jsLibraryName and work backwards. It seems
to
> rely on:
> * LibraryElement hashing
> * LibraryElement.source.uri
> * DDC's module "libraryRoot" path, which IIRC usually comes from a command
line
> option
> 
> ... so changes in that could affect its behavior.

The CL can be cleanly patched on 4752b67c62a09141c0f6210d58664b2f6fe67f90 (the
latest at the moment).

[Jennifer Messerly, 2017-05-10 18:53:31]

Okay here's what's up with async_helper:

if I add this print statement to compiler.dart:

      UnitElementResult unitResult = await driver.getUnitElement(sourcePath);
      LibraryElement library = unitResult.element.library;
      print('!!! $sourceUri|${library.source.uri}');
      librariesToCompile.add(library);

and then run ./tool/build_pkgs.sh

... you'll see it print:

!!!
package:async_helper/async_helper.dart|file:///Users/jmesserly/src/dart/sdk/pkg/dev_compiler/test/codegen/async_helper.dart
!!! package:expect/expect.dart|package:expect/expect.dart
!!! package:expect/minitest.dart|package:expect/minitest.dart
...


so we request package:async_helper, but we get back something with a file URI.
This resolves differently. Other packages work.

So what's special about async_helper? well if you look in tools/build_pkgs.dart,
you'll see:

  if (module == 'async_helper') {
    args.add('--url-mapping=package:async_helper/async_helper.dart,'
        'test/codegen/async_helper.dart');
  }

... so my guess is the --url-mapping has changed somehow. It seems to be applied
"eagerly" leaving the LibraryElement without the correct URI.

I don't know if that's a problem or not. I forgot what that Analyzer feature is
intended for.

If you believe that is correct behavior for --url-mapping or you think it's okay
to temporarily regress it, you can fix the test in question with this patch:

diff --git a/pkg/dev_compiler/tool/build_pkgs.dart
b/pkg/dev_compiler/tool/build_pkgs.dart
index 901e9a8a4c..85ca884e66 100755
--- a/pkg/dev_compiler/tool/build_pkgs.dart
+++ b/pkg/dev_compiler/tool/build_pkgs.dart
@@ -101,6 +101,12 @@ Future<Null> compileModule(String module,
   if (module == 'async_helper') {
     args.add('--url-mapping=package:async_helper/async_helper.dart,'
         'test/codegen/async_helper.dart');
+    // The custom url-mapping option in Analyzer has changed.
+    // Now the package URI gets turned into a file URI during resolution.
+    //
+    // Because of that, we need a library-root to find the correct root
+    // directory for this package, and thus find the library's name.
+    args.add('--library-root=test/codegen/');
   }
 
   await compile(args);