Store actual Reference(s) for additional exports.

Store actual Reference(s) for additional exports.

We need this to be able to tree shake outline.
https://github.com/dart-lang/sdk/issues/30448#issuecomment-325434406

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

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

[jensj, 2017-08-29 06:58:32]

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

[jensj, 2017-08-29 06:58:32]

DBC: Please remember to update binary.md.

[ahe, 2017-08-29 11:04:23]

Please coordinate this with Kevin and me by setting up a GVC.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:49: /// The third
element (if present) is an error message.
This documentation should be updated.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:135: node.name, new
DillClassBuilder(node, this));
This code has two problems:

1. It creates new builders instead of looking up the existing ones.

2. It doesn't deal with errors.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:143: throw new
UnimplementedError('${node.runtimeType}');
You should use one of the methods from ../problems.dart instead of throwing.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:102: /// format
used.
This documentation should be updated.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/test/fasta/am...
File pkg/front_end/test/fasta/ambiguous_export_test.dart (left):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/test/fasta/am...
pkg/front_end/test/fasta/ambiguous_export_test.dart:23: await asyncTest(() async
{
What are you replacing this test with?

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/testcases/amb...
File pkg/front_end/testcases/ambiguous_exports.dart.direct.expect (left):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/testcases/amb...
pkg/front_end/testcases/ambiguous_exports.dart.direct.expect:4: static const
field dynamic _exports# = "[[null,\"main\",\"'main' is exported from both
'pkg/front_end/testcases/hello.dart' and
'pkg/front_end/testcases/map.dart'.\"]]" /* from null */;
Exports are missing from the textual format.

https://codereview.chromium.org/3008763002/diff/1/pkg/kernel/lib/text/ast_to_...
File pkg/kernel/lib/text/ast_to_text.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:291: write('too many items');
I don't think we should truncate the output.

[scheglov, 2017-08-29 15:16:26]

What time (Pacific TZ) is OK to GVC for you?
My guess is that it would be impossible to talk to Kevin.
It is more Fasta parts, Kernel parts are straightforward.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:49: /// The third
element (if present) is an error message.
On 2017/08/29 11:04:23, ahe wrote:
> This documentation should be updated.

Done.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:135: node.name, new
DillClassBuilder(node, this));
On 2017/08/29 11:04:23, ahe wrote:
> This code has two problems:
> 
> 1. It creates new builders instead of looking up the existing ones.

  How to get existing builders, when we already have nodes?

> 
> 2. It doesn't deal with errors.

  Do you mean export conflicts?
  Any advice how to do this?

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:143: throw new
UnimplementedError('${node.runtimeType}');
On 2017/08/29 11:04:23, ahe wrote:
> You should use one of the methods from ../problems.dart instead of throwing.

Done.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:102: /// format
used.
On 2017/08/29 11:04:23, ahe wrote:
> This documentation should be updated.

Done.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/test/fasta/am...
File pkg/front_end/test/fasta/ambiguous_export_test.dart (left):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/test/fasta/am...
pkg/front_end/test/fasta/ambiguous_export_test.dart:23: await asyncTest(() async
{
On 2017/08/29 11:04:23, ahe wrote:
> What are you replacing this test with?

I don't.
But when I understand how to handle errors, I might be able to se how to do
this.

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/testcases/amb...
File pkg/front_end/testcases/ambiguous_exports.dart.direct.expect (left):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/testcases/amb...
pkg/front_end/testcases/ambiguous_exports.dart.direct.expect:4: static const
field dynamic _exports# = "[[null,\"main\",\"'main' is exported from both
'pkg/front_end/testcases/hello.dart' and
'pkg/front_end/testcases/map.dart'.\"]]" /* from null */;
On 2017/08/29 11:04:23, ahe wrote:
> Exports are missing from the textual format.

Well, for purposes of other libraries, this library does not have additional
exports.
There is an export error (TBD how to represent), but no exports.

https://codereview.chromium.org/3008763002/diff/1/pkg/kernel/lib/text/ast_to_...
File pkg/kernel/lib/text/ast_to_text.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:291: write('too many items');
On 2017/08/29 11:04:23, ahe wrote:
> I don't think we should truncate the output.

Done.

[scheglov, 2017-08-29 16:56:57]

PTAL

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
File pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/1/pkg/front_end/lib/src/fasta...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:135: node.name, new
DillClassBuilder(node, this));
On 2017/08/29 15:16:26, scheglov wrote:
> On 2017/08/29 11:04:23, ahe wrote:
> > This code has two problems:
> > 
> > 1. It creates new builders instead of looking up the existing ones.
> 
>   How to get existing builders, when we already have nodes?
> 
> > 
> > 2. It doesn't deal with errors.
> 
>   Do you mean export conflicts?
>   Any advice how to do this?

I fixed (1), and do nothing for (2) because (if I understand correctly) we
decided that the specification does not require errors in clients of this
library.

https://codereview.chromium.org/3008763002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:524:
additionalExports.add(member.target.reference);
The specification says that it is an error to re-export two different nodes with
the same name.

18.2 Exports
It is a compile-time error if a name N is re-exported by a library L and N is
introduced into the export namespace of L by more than one export, unless all
exports refer to same declaration for the name N.

I would say that with presence of a compile-time error we are free to do
whatever reasonable thing we decide to do. In this case we should report an
error in L, and skip N from its export namespace.

[ahe, 2017-08-30 14:24:52]

If we agree on the general principle I mention below, I think you can move
forward with this CL.

https://codereview.chromium.org/3008763002/diff/20001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/source/source_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/20001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/source/source_library_builder.dart:524:
additionalExports.add(member.target.reference);
On 2017/08/29 16:56:57, scheglov wrote:
> The specification says that it is an error to re-export two different nodes
with
> the same name.
> 
> 18.2 Exports
> It is a compile-time error if a name N is re-exported by a library L and N is
> introduced into the export namespace of L by more than one export, unless all
> exports refer to same declaration for the name N.
> 
> I would say that with presence of a compile-time error we are free to do
> whatever reasonable thing we decide to do. In this case we should report an
> error in L, and skip N from its export namespace.

I had more time to think about this. I think it is important for error recovery
that we can serialize errors regarding ambiguous imports and exports. Yes, the
specification doesn't require the behavior of the Dart VM here, but I do think
it is important that we can represent errors at all positions in the kernel AST.

If we agree on the general principle, I don't mind deferring the implementation
of that (possibly leaving it to the kernel team to work out precisely how to
handle these kinds of errors).

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:9: import
'package:kernel/ast.dart';
Remove the second import of kernel/ast.dart.

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:39: /// References to
nodes exported by the `export` directives, don't conflict
Consider this:

/// References to nodes exported by `export` declarations that:
/// - aren't ambiguous, or
/// - aren't hidden by local declarations.

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:139: Builder builder
= library.exportScopeBuilder[name];
assert(node == builder.target);

[ahe, 2017-08-30 16:18:53]

lgtm

[scheglov, 2017-08-30 16:56:27]

Description was changed from

==========
Store actual Reference(s) for additional exports.

We need this to be able to tree shake outline.
https://github.com/dart-lang/sdk/issues/30448#issuecomment-325434406

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

to

==========
Store actual Reference(s) for additional exports.

We need this to be able to tree shake outline.
https://github.com/dart-lang/sdk/issues/30448#issuecomment-325434406

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

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

[scheglov, 2017-08-30 16:56:28]

Committed patchset #4 (id:60001) manually as
234c4a7f493e1fc3dafc393472e39163ac2ad783 (presubmit successful).

[scheglov, 2017-08-30 16:56:50]

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
File pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart (right):

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:9: import
'package:kernel/ast.dart';
On 2017/08/30 14:24:52, ahe wrote:
> Remove the second import of kernel/ast.dart.

Done.

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:39: /// References to
nodes exported by the `export` directives, don't conflict
On 2017/08/30 14:24:52, ahe wrote:
> Consider this:
> 
> /// References to nodes exported by `export` declarations that:
> /// - aren't ambiguous, or
> /// - aren't hidden by local declarations.

Done.

https://codereview.chromium.org/3008763002/diff/40001/pkg/front_end/lib/src/f...
pkg/front_end/lib/src/fasta/dill/dill_library_builder.dart:139: Builder builder
= library.exportScopeBuilder[name];
On 2017/08/30 14:24:52, ahe wrote:
> assert(node == builder.target);

Done.