Add front end code for reading the SDK's "libraries.dart" file.

Add front end code for reading the SDK's "libraries.dart" file.

R=danrubel@google.com, sigmund@google.com

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

[Paul Berry, 2016-12-14 20:09:47]

paulberry@google.com changed reviewers:
+ danrubel@google.com, sigmund@google.com

[Siggi Cherem (dart-lang), 2016-12-14 20:48:44]

lgtm, just some minor comments below and an open discussion for the future.

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/library_info.dart (right):

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/library_info.dart:1: /// A bit flag used by
[LibraryInfo] indicating that a library is used by
+dartdoc for the library.

I wonder if we can avoid duplicating this by importing libraries.dart in some
way.

I assume you are extracting the contents of libraries.dart so that the 
front_end package can work on multiple versions of the sdk, but maybe we can
split libraries.dart in two:

sdk_library_metadata/lib/library_info.dart # basically what we have here
sdk_library_metadata/lib/libraries.dart # the actual list of libraries exported
in libraries.dart today.

Then you can import package:sdk_library_metadata/library_info.dart in front_end,
but parse sdk/lib/_internal/sdk_library_metadata/libraries.dart ?

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
File pkg/front_end/lib/src/libraries_reader.dart (right):

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
pkg/front_end/lib/src/libraries_reader.dart:14: Map<String, LibraryInfo>
readLibraries(UnlinkedUnit librariesUnit) {
cool to see that we can use summaries for this - but I hope we can move to a
textual format and avoid this in the long run. Maybe we should add a TODO?

I believe libraries.dart is only used by analyzer, and maybe ddc? We are no
longer using it in dart2js because we are consuming the .platform files. We can
move back to libraries.dart, but I rather move to simple format that is easy to
load.

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
pkg/front_end/lib/src/libraries_reader.dart:70: bool isMaterialized = false;
minor nit: do we expect to have `null` values? If not, change it to a computed
getter:

  bool get isMaterialized => _value != null;

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
pkg/front_end/lib/src/libraries_reader.dart:75: dynamic get value {
similarly, if `null` is not a valid value, then:

  get value => _value ??= _materialize();

(changing _materialize to return the result)

[danrubel, 2016-12-14 21:02:31]

LGTM

[Paul Berry, 2016-12-14 23:17:25]

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/base/...
File pkg/front_end/lib/src/base/library_info.dart (right):

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/base/...
pkg/front_end/lib/src/base/library_info.dart:1: /// A bit flag used by
[LibraryInfo] indicating that a library is used by
On 2016/12/14 20:48:43, Siggi Cherem (dart-lang) wrote:
> +dartdoc for the library.
> 
> I wonder if we can avoid duplicating this by importing libraries.dart in some
> way.
> 
> I assume you are extracting the contents of libraries.dart so that the 
> front_end package can work on multiple versions of the sdk, but maybe we can
> split libraries.dart in two:
> 
> sdk_library_metadata/lib/library_info.dart # basically what we have here
> sdk_library_metadata/lib/libraries.dart # the actual list of libraries
exported
> in libraries.dart today.
> 
> Then you can import package:sdk_library_metadata/library_info.dart in
front_end,
> but parse sdk/lib/_internal/sdk_library_metadata/libraries.dart ?

Hmm, let me think about that.  Assuming we keep libraries.dart around in the
long term, I can definitely see the benefit of that.  Note that we would have to
start publishing sdk_library_metadata on pub, since front end is used by
analyzer, and analyzer gets published on pub.  Alternatively, we could move
library_info to a public part of the SDK so that front_end could just import
"dart:library_info".

But I actually would like to challenge the idea that we want to keep
libraries.dart around at all.  The same information could just as easily be
stored in some other format (perhaps a JSON file).  Then it would be trivial to
parse--we wouldn't have to have all this weird hacky logic that tries to build
the values of the constants from an unresolved AST.  We could still expose the
LibraryInfo interface to people (assuming it's useful) but instead of defining
it inside the SDK, we would just define it inside the front end, and provide a
convenience method in the front end that reads the JSON, translates it into
LibraryInfo.

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
File pkg/front_end/lib/src/libraries_reader.dart (right):

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
pkg/front_end/lib/src/libraries_reader.dart:14: Map<String, LibraryInfo>
readLibraries(UnlinkedUnit librariesUnit) {
On 2016/12/14 20:48:43, Siggi Cherem (dart-lang) wrote:
> cool to see that we can use summaries for this - but I hope we can move to a
> textual format and avoid this in the long run. Maybe we should add a TODO?
> 
> I believe libraries.dart is only used by analyzer, and maybe ddc? We are no
> longer using it in dart2js because we are consuming the .platform files. We
can
> move back to libraries.dart, but I rather move to simple format that is easy
to
> load.

That's wonderful to hear (see my other comment)!  Yeah, let's try to get to a
point sometime (maybe perhaps post-Dart 1.5) where libraries.dart is completely
gone and all tools get this information from a textual format.

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
pkg/front_end/lib/src/libraries_reader.dart:70: bool isMaterialized = false;
On 2016/12/14 20:48:43, Siggi Cherem (dart-lang) wrote:
> minor nit: do we expect to have `null` values? If not, change it to a computed
> getter:
> 
>   bool get isMaterialized => _value != null;
> 
> 

Fair enough.  Following your suggestion below gets rid of the only use of
`isMaterialized` so I just dropped it entirely.

https://codereview.chromium.org/2578693002/diff/1/pkg/front_end/lib/src/libra...
pkg/front_end/lib/src/libraries_reader.dart:75: dynamic get value {
On 2016/12/14 20:48:43, Siggi Cherem (dart-lang) wrote:
> similarly, if `null` is not a valid value, then:
> 
>   get value => _value ??= _materialize();
> 
> (changing _materialize to return the result)

Done.

[Paul Berry, 2016-12-14 23:23:35]

Description was changed from

==========
Add front end code for reading the SDK's "libraries.dart" file.
==========

to

==========
Add front end code for reading the SDK's "libraries.dart" file.

R=danrubel@google.com, sigmund@google.com

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

[Paul Berry, 2016-12-14 23:23:36]

Committed patchset #1 (id:1) manually as
4e33530fbfdba2ec9e8f54c77f706db0507a4fb0 (presubmit successful).