Add a way to use shared CanonicalName root to deserialize Program.

Add a way to use shared CanonicalName root to deserialize Program.

This allows for example to add SDK into a Program, then load the
"incomplete" Program A that has only the library A, and name sequences
that references SDK classes. Because we look into the nameRoot which
aleady has SDK CanonicalName(s), we can find these names while filling
the link table and use references which point to the existing SDK
AST nodes.  Then we can load another set of library cycles, etc.

At the end we have a set of self-consistent libraries that we can
feed into DillTarget/DillLoader and resolve anothersource target
against it.

This CL is based on https://codereview.chromium.org/2872903005/
which has not been reviewed yet.


R=kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[Siggi Cherem (dart-lang), 2017-05-10 18:00:38]

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4219: final CanonicalName root;
Question for Kevin on API design here:
 - should we have a notion of a "Bundle" or something other than Program here
for our use-case?
 - if we do, we might still need a mechanism to create a program from a bundle
and avoid recomputing the canonical names, for that purpose alone it might be
valuable to allow passing in an existing root canonical name.

[Kevin Millikin (Google), 2017-05-11 08:22:55]

I don't completely understand your use case.  Is it possible to link all the
binaries using a shared canonical name tree, unbind all the canonical names,
resolve against the program, and then compute the canonical name table for the
linked program before writing it out?

That would allow us to get rid of the line in Program.computeCanonicalNames that
skips libraries, which seems problematic.

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4219: final CanonicalName root;
On 2017/05/10 18:00:38, Siggi Cherem (dart-lang) wrote:
> Question for Kevin on API design here:
>  - should we have a notion of a "Bundle" or something other than Program here
> for our use-case?
>  - if we do, we might still need a mechanism to create a program from a bundle
> and avoid recomputing the canonical names, for that purpose alone it might be
> valuable to allow passing in an existing root canonical name.

Even without these changes, Program is already not quite the right name.  The
name is a left-over from when we had a distinction between the binary format for
a (linked) program and (unlinked) bundle of libraries.

Let's just keep it for now.

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4243: if (library.canonicalName != null) continue;
Keep in mind that this approach of skipping libraries only works if a library
having a canonical name implies that every (named) thing in that library also
has a canonical name.

I don't know enough about your use case to know if that's true, but I'd rather
not assume it in package Kernel if we can think of a way around it.

(I'm thinking that you might want to read an external library from a binary, in
which case the library will have a canonical name.  Then later you might read
some named member of the library from some other source than a Kernel binary, in
which case it will not have a canonical name.  Skipping the library will never
build the name for that member.)

I'm also concerned about the converse problem.  You could imagine a Kernel
transformation that introduced fresh libraries (without a canonical name
computed) built out of members that were deserialized and so had a canonical
name.  For instance, maybe you wanted a transformation that combined distinct
libraries into new ones.  In that case, you will start naming the library and
then name the children and still hit the assert that you don't name something
that already has a name.

[scheglov, 2017-05-11 18:31:25]

PTAL

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4243: if (library.canonicalName != null) continue;
On 2017/05/11 08:22:55, Kevin Millikin (Google) wrote:
> Keep in mind that this approach of skipping libraries only works if a library
> having a canonical name implies that every (named) thing in that library also
> has a canonical name.
> 
> I don't know enough about your use case to know if that's true, but I'd rather
> not assume it in package Kernel if we can think of a way around it.
> 
> (I'm thinking that you might want to read an external library from a binary,
in
> which case the library will have a canonical name.  Then later you might read
> some named member of the library from some other source than a Kernel binary,
in
> which case it will not have a canonical name.  Skipping the library will never
> build the name for that member.)
> 
> I'm also concerned about the converse problem.  You could imagine a Kernel
> transformation that introduced fresh libraries (without a canonical name
> computed) built out of members that were deserialized and so had a canonical
> name.  For instance, maybe you wanted a transformation that combined distinct
> libraries into new ones.  In that case, you will start naming the library and
> then name the children and still hit the assert that you don't name something
> that already has a name.

OK, I rolled this back.

[scheglov, 2017-05-12 15:39:01]

Ping!

On 2017/05/11 18:31:25, scheglov wrote:
> PTAL
> 
> https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart
> File pkg/kernel/lib/ast.dart (right):
> 
>
https://codereview.chromium.org/2874723002/diff/20001/pkg/kernel/lib/ast.dart...
> pkg/kernel/lib/ast.dart:4243: if (library.canonicalName != null) continue;
> On 2017/05/11 08:22:55, Kevin Millikin (Google) wrote:
> > Keep in mind that this approach of skipping libraries only works if a
library
> > having a canonical name implies that every (named) thing in that library
also
> > has a canonical name.
> > 
> > I don't know enough about your use case to know if that's true, but I'd
rather
> > not assume it in package Kernel if we can think of a way around it.
> > 
> > (I'm thinking that you might want to read an external library from a binary,
> in
> > which case the library will have a canonical name.  Then later you might
read
> > some named member of the library from some other source than a Kernel
binary,
> in
> > which case it will not have a canonical name.  Skipping the library will
never
> > build the name for that member.)
> > 
> > I'm also concerned about the converse problem.  You could imagine a Kernel
> > transformation that introduced fresh libraries (without a canonical name
> > computed) built out of members that were deserialized and so had a canonical
> > name.  For instance, maybe you wanted a transformation that combined
distinct
> > libraries into new ones.  In that case, you will start naming the library
and
> > then name the children and still hit the assert that you don't name
something
> > that already has a name.
> 
> OK, I rolled this back.

[Siggi Cherem (dart-lang), 2017-05-12 16:27:57]

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

https://codereview.chromium.org/2874723002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:382: nameRoot:
dillTarget.nameRoot,
I think I don't have the full picture yet.

I'm currently wondering whether we want to put nameRoot in dillTarget or
somewhere else. Maybe it will help if you could tell me how do you intend to use
the nameRoot afterwards?

In the current CL it is used here while linking. If it's just going to be used
here, I'd be inclined to move the declaration right here in KernelTarget or
consider passing it as an optional argument to the linking function.

https://codereview.chromium.org/2874723002/diff/40001/pkg/kernel/lib/ast.dart
File pkg/kernel/lib/ast.dart (right):

https://codereview.chromium.org/2874723002/diff/40001/pkg/kernel/lib/ast.dart...
pkg/kernel/lib/ast.dart:4253: {CanonicalName nameRoot,
just a thought - Kevin if you prefer a different API here, let us know. For
example, we could make the root non-final and keep the original API for creating
the Program as it was before.

[scheglov, 2017-05-12 17:15:16]

PTAL

Please ignore the similar separate CL that was sent a minute ago. I accidentally
sent it as a new CL.

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

https://codereview.chromium.org/2874723002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:382: nameRoot:
dillTarget.nameRoot,
On 2017/05/12 16:27:57, Siggi Cherem (dart-lang) wrote:
> I think I don't have the full picture yet.
> 
> I'm currently wondering whether we want to put nameRoot in dillTarget or
> somewhere else. Maybe it will help if you could tell me how do you intend to
use
> the nameRoot afterwards?
> 
> In the current CL it is used here while linking. If it's just going to be used
> here, I'd be inclined to move the declaration right here in KernelTarget or
> consider passing it as an optional argument to the linking function.

Done.

[Siggi Cherem (dart-lang), 2017-05-12 17:29:25]

lgtm

https://codereview.chromium.org/2874723002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (left):

https://codereview.chromium.org/2874723002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:378:
uriToSource.addAll(binary.uriToSource);
do we need to preserve this somehow? (when linking a full program, we will need
to include the uriToSource for all the other code to get appropriate debug
information in the resulting program, I believe.

I'm OK marking this with a TODO for this CL and do this separately. Possibly
we'd track uriToSource in loader as well?

[Siggi Cherem (dart-lang), 2017-05-12 17:31:17]

lgtm

(Kevin - we are assuming that the API change Program was OK by you, but please
do let us know if we assumed incorrectly - if so we'll be happy to fix it right
away)

[scheglov, 2017-05-12 17:42:14]

Description was changed from

==========
Add a way to use shared CanonicalName root to deserialize Program.

This allows for example to add SDK into a Program, then load the
"incomplete" Program A that has only the library A, and name sequences
that references SDK classes. Because we look into the nameRoot which
aleady has SDK CanonicalName(s), we can find these names while filling
the link table and use references which point to the existing SDK
AST nodes.  Then we can load another set of library cycles, etc.

At the end we have a set of self-consistent libraries that we can
feed into DillTarget/DillLoader and resolve anothersource target
against it.

This CL is based on https://codereview.chromium.org/2872903005/
which has not been reviewed yet.


R=kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=
==========

to

==========
Add a way to use shared CanonicalName root to deserialize Program.

This allows for example to add SDK into a Program, then load the
"incomplete" Program A that has only the library A, and name sequences
that references SDK classes. Because we look into the nameRoot which
aleady has SDK CanonicalName(s), we can find these names while filling
the link table and use references which point to the existing SDK
AST nodes.  Then we can load another set of library cycles, etc.

At the end we have a set of self-consistent libraries that we can
feed into DillTarget/DillLoader and resolve anothersource target
against it.

This CL is based on https://codereview.chromium.org/2872903005/
which has not been reviewed yet.


R=kmillikin@google.com, paulberry@google.com, sigmund@google.com
BUG=

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

[scheglov, 2017-05-12 17:42:15]

Committed patchset #5 (id:80001) manually as
bdedd6768b4f594a3034151ada169b058515542f (presubmit successful).

[scheglov, 2017-05-12 17:44:21]

https://codereview.chromium.org/2874723002/diff/60001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/kernel/kernel_target.dart (left):

https://codereview.chromium.org/2874723002/diff/60001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:378:
uriToSource.addAll(binary.uriToSource);
On 2017/05/12 17:29:25, Siggi Cherem (dart-lang) wrote:
> do we need to preserve this somehow? (when linking a full program, we will
need
> to include the uriToSource for all the other code to get appropriate debug
> information in the resulting program, I believe.
> 
> I'm OK marking this with a TODO for this CL and do this separately. Possibly
> we'd track uriToSource in loader as well?

I will add TODO for now.

[sra1, 2017-05-12 23:07:44]

sra@google.com changed reviewers:
+ sra@google.com

[sra1, 2017-05-12 23:07:45]

https://codereview.chromium.org/2874723002/diff/80001/pkg/compiler/lib/src/ke...
File pkg/compiler/lib/src/kernel/task.dart (right):

https://codereview.chromium.org/2874723002/diff/80001/pkg/compiler/lib/src/ke...
pkg/compiler/lib/src/kernel/task.dart:45: libraries:
kernel.libraryDependencies(library.canonicalUri))
This turned one of the dart2js bots red.

NoSuchMethodError: No constructor 'Program' with matching arguments declared in
class 'Program'.
Receiver: Type: class 'Program'
Tried calling: new Program(Instance(length:18) of '_GrowableList')
Found: new Program({CanonicalName nameRoot, List<Library> libraries, Map<String,
Source> uriToSource})
#0      NoSuchMethodError._throwNew (dart:core-patch/errors_patch.dart:196)
#1      compileMultiple.<anonymous closure>
(file:///b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/kernel/compiler_helper.dart:71:42)
<asynchronous suspension>
#2      main.<anonymous closure>
(file:///b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/no_such_method_enabled_test.dart:261:47)
<asynchronous suspension>
#3      asyncTest (package:async_helper/async_helper.dart:93:4)
#4      main
(file:///b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/no_such_method_enabled_test.dart:241:3)
#5      _startIsolate.<anonymous closure>
(dart:isolate-patch/isolate_patch.dart:265)
#6      _RawReceivePortImpl._handleMessage
(dart:isolate-patch/isolate_patch.dart:151)


Command[vm]: DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dart
--enable_asserts --enable_type_checks --ignore-unrecognized-flags
--packages=/b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/.packages
/b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/no_such_method_enabled_test.dart
Took 0:00:27.899555

Short reproduction command (experimental):
    python tools/test.py -mrelease --checked -t60 --use-sdk
--reset-browser-configuration dart2js/no_such_method_enabled_test

[sra1, 2017-05-12 23:14:19]

On 2017/05/12 23:07:45, sra1 wrote:
>
https://codereview.chromium.org/2874723002/diff/80001/pkg/compiler/lib/src/ke...
> File pkg/compiler/lib/src/kernel/task.dart (right):
> 
>
https://codereview.chromium.org/2874723002/diff/80001/pkg/compiler/lib/src/ke...
> pkg/compiler/lib/src/kernel/task.dart:45: libraries:
> kernel.libraryDependencies(library.canonicalUri))
> This turned one of the dart2js bots red.
> 
> NoSuchMethodError: No constructor 'Program' with matching arguments declared
in
> class 'Program'.
> Receiver: Type: class 'Program'
> Tried calling: new Program(Instance(length:18) of '_GrowableList')
> Found: new Program({CanonicalName nameRoot, List<Library> libraries,
Map<String,
> Source> uriToSource})
> #0      NoSuchMethodError._throwNew (dart:core-patch/errors_patch.dart:196)
> #1      compileMultiple.<anonymous closure>
>
(file:///b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/kernel/compiler_helper.dart:71:42)
> <asynchronous suspension>
> #2      main.<anonymous closure>
>
(file:///b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/no_such_method_enabled_test.dart:261:47)
> <asynchronous suspension>
> #3      asyncTest (package:async_helper/async_helper.dart:93:4)
> #4      main
>
(file:///b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/no_such_method_enabled_test.dart:241:3)
> #5      _startIsolate.<anonymous closure>
> (dart:isolate-patch/isolate_patch.dart:265)
> #6      _RawReceivePortImpl._handleMessage
> (dart:isolate-patch/isolate_patch.dart:151)
> 
> 
> Command[vm]: DART_CONFIGURATION=ReleaseX64 out/ReleaseX64/dart-sdk/bin/dart
> --enable_asserts --enable_type_checks --ignore-unrecognized-flags
> --packages=/b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/.packages
>
/b/build/slave/dart2js-linux-jsshell-3-4-be/build/sdk/tests/compiler/dart2js/no_such_method_enabled_test.dart
> Took 0:00:27.899555
> 
> Short reproduction command (experimental):
>     python tools/test.py -mrelease --checked -t60 --use-sdk
> --reset-browser-configuration dart2js/no_such_method_enabled_test

I will fix it

[Kevin Millikin (Google), 2017-05-15 08:47:56]

On 2017/05/12 17:31:17, Siggi Cherem (dart-lang) wrote:
> lgtm
> 
> (Kevin - we are assuming that the API change Program was OK by you, but please
> do let us know if we assumed incorrectly - if so we'll be happy to fix it
right
> away)

It looks good to me.  (My objection was only to skipping libraries when
assigning canonical names.)

[ahe, 2017-05-15 09:08:44]

Why wasn't I CC'ed on this review?

https://codereview.chromium.org/2874723002/diff/80001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_loader.dart (right):

https://codereview.chromium.org/2874723002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_loader.dart:17: final libraries =
<Library>[];
Please add a type.

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

https://codereview.chromium.org/2874723002/diff/80001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/kernel/kernel_target.dart:379: //   
uriToSource.addAll(binary.uriToSource);
Yes. You need to update uriToSource. Otherwise the created dill file lacks
source information.