Add import dependencies to Kernel libraries and use them to resynthesize ImportElement(s) in Analyz…

Add import dependencies to Kernel libraries and use them to resynthesize ImportElement(s) in Analyzer.

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

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

[Paul Berry, 2017-07-20 18:13:49]

analyzer and front_end changes lgtm.

I'm not sure I understand the motivation for the kernel changes; perhaps they
are best reviewed by Siggi, Peter, or Kevin.

[Siggi Cherem (dart-lang), 2017-07-20 18:29:00]

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (left):

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:403: assert(length == 0);
do we need the library dependencies for external libraries? If not, I feel the
fix should be elsewhere:

 - we could we fix it in the serialization, or
 - in how we produce external libraries in fasta

I have a preference for the latter until we figure out what is the right
approach. In particular, right now fasta doesn't seem to have a clear way to
generate external libraries, and we do some adhoc things for them (e.g. in the
public API we load .dill into summaries, and then mark them as external, but we
don't mutate them in any other way - if they happen to have method bodies or
library dependencies, we keep them around).

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

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:286: for (var import in
library.dependencies) {
I'm not sure I understand this part of the change. This means we no longer print
out library dependencies? Is this to avoid changes to the expectation files in
our unit tests?

If so, I would prefer not to make this change here, but update the expectations
or make a change that doesn't affect regular printing but affects printing of
external libraries only?

[scheglov, 2017-07-20 19:33:53]

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (left):

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:403: assert(length == 0);
On 2017/07/20 18:29:00, Siggi Cherem (dart-lang) wrote:
> do we need the library dependencies for external libraries? If not, I feel the
> fix should be elsewhere:
> 
>  - we could we fix it in the serialization, or
>  - in how we produce external libraries in fasta
> 
> I have a preference for the latter until we figure out what is the right
> approach. In particular, right now fasta doesn't seem to have a clear way to
> generate external libraries, and we do some adhoc things for them (e.g. in the
> public API we load .dill into summaries, and then mark them as external, but
we
> don't mutate them in any other way - if they happen to have method bodies or
> library dependencies, we keep them around).

Analyzer's LibraryElement has "imports" that should return all imports of the
library, regardless whether this LibraryElement comes from a "local" or
"external" summary / kernel. So, yes, we need it. And it is not clear to me
which problem this check was solving. It could be to check the status quo, but
my change effectively changes this expectation.

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

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:286: for (var import in
library.dependencies) {
On 2017/07/20 18:29:00, Siggi Cherem (dart-lang) wrote:
> I'm not sure I understand this part of the change. This means we no longer
print
> out library dependencies? Is this to avoid changes to the expectation files in
> our unit tests?
> 
> If so, I would prefer not to make this change here, but update the
expectations
> or make a change that doesn't affect regular printing but affects printing of
> external libraries only?

First of all, this code has not been used, because we used not to add
dependencies to libraries when building them. Instead, the code about uses
LibraryImportTable to gather "actually used" imports and generate synthetic
prefixes assigned to them, and then we print only these "actually used" imports
into text presentation.

I agree that if we want to have our tests validate that we add required
import/export dependencies into kernels as we build them, we need to update all
the expectations. One nasty consequence of this would be that we will have
duplicate sets of imports - one for "actually used" and another for "specified".
If that's what you think is OK, then I can update all expectations.

[Siggi Cherem (dart-lang), 2017-07-20 22:50:36]

lgtm

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (left):

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:403: assert(length == 0);
On 2017/07/20 19:33:53, scheglov wrote:
> On 2017/07/20 18:29:00, Siggi Cherem (dart-lang) wrote:
> > do we need the library dependencies for external libraries? If not, I feel
the
> > fix should be elsewhere:
> > 
> >  - we could we fix it in the serialization, or
> >  - in how we produce external libraries in fasta
> > 
> > I have a preference for the latter until we figure out what is the right
> > approach. In particular, right now fasta doesn't seem to have a clear way to
> > generate external libraries, and we do some adhoc things for them (e.g. in
the
> > public API we load .dill into summaries, and then mark them as external, but
> we
> > don't mutate them in any other way - if they happen to have method bodies or
> > library dependencies, we keep them around).
> 
> Analyzer's LibraryElement has "imports" that should return all imports of the
> library, regardless whether this LibraryElement comes from a "local" or
> "external" summary / kernel. So, yes, we need it. And it is not clear to me
> which problem this check was solving. It could be to check the status quo, but
> my change effectively changes this expectation.

I see - I understand now. Thanks for the explanation.

My understanding is that an "external library" is a library where some
information may be missing - so far I don't believe there is a requirement that
some information must be missing (like lib dependencies). So, I now agree that
your change seems good here.

Regarding analyzer and input summaries, just want to make sure we are on the
same page:
  - I believe summaries are not external when they are serialized or when we
read them.
  - we will use external libraries when we read a summary, compile code, and
emit a program with some nodes that were derived from the summary - these
derived nodes will be marked as external (and if we do tree-shaking, they might
only be a subset of what was initially read from the summary)

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

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:286: for (var import in
library.dependencies) {
On 2017/07/20 19:33:53, scheglov wrote:
> On 2017/07/20 18:29:00, Siggi Cherem (dart-lang) wrote:
> > I'm not sure I understand this part of the change. This means we no longer
> print
> > out library dependencies? Is this to avoid changes to the expectation files
in
> > our unit tests?
> > 
> > If so, I would prefer not to make this change here, but update the
> expectations
> > or make a change that doesn't affect regular printing but affects printing
of
> > external libraries only?
> 
> First of all, this code has not been used, because we used not to add
> dependencies to libraries when building them. Instead, the code about uses
> LibraryImportTable to gather "actually used" imports and generate synthetic
> prefixes assigned to them, and then we print only these "actually used"
imports
> into text presentation.
> 
> I agree that if we want to have our tests validate that we add required
> import/export dependencies into kernels as we build them, we need to update
all
> the expectations. One nasty consequence of this would be that we will have
> duplicate sets of imports - one for "actually used" and another for
"specified".
> If that's what you think is OK, then I can update all expectations.

Thanks so much for the clarification. Given your explanation, I'm OK submitting
this change as it is. Let's write a TODO and open a bug to follow up with the
kernel-team on this issue. It seems low priority, but they can let us know if
they have any concerns.

[scheglov, 2017-07-21 16:21:33]

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
File pkg/kernel/lib/binary/ast_from_binary.dart (left):

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/binary/ast_f...
pkg/kernel/lib/binary/ast_from_binary.dart:403: assert(length == 0);
On 2017/07/20 22:50:36, Siggi Cherem (dart-lang) wrote:
> On 2017/07/20 19:33:53, scheglov wrote:
> > On 2017/07/20 18:29:00, Siggi Cherem (dart-lang) wrote:
> > > do we need the library dependencies for external libraries? If not, I feel
> the
> > > fix should be elsewhere:
> > > 
> > >  - we could we fix it in the serialization, or
> > >  - in how we produce external libraries in fasta
> > > 
> > > I have a preference for the latter until we figure out what is the right
> > > approach. In particular, right now fasta doesn't seem to have a clear way
to
> > > generate external libraries, and we do some adhoc things for them (e.g. in
> the
> > > public API we load .dill into summaries, and then mark them as external,
but
> > we
> > > don't mutate them in any other way - if they happen to have method bodies
or
> > > library dependencies, we keep them around).
> > 
> > Analyzer's LibraryElement has "imports" that should return all imports of
the
> > library, regardless whether this LibraryElement comes from a "local" or
> > "external" summary / kernel. So, yes, we need it. And it is not clear to me
> > which problem this check was solving. It could be to check the status quo,
but
> > my change effectively changes this expectation.
> 
> I see - I understand now. Thanks for the explanation.
> 
> My understanding is that an "external library" is a library where some
> information may be missing - so far I don't believe there is a requirement
that
> some information must be missing (like lib dependencies). So, I now agree that
> your change seems good here.
> 
> Regarding analyzer and input summaries, just want to make sure we are on the
> same page:
>   - I believe summaries are not external when they are serialized or when we
> read them.
>   - we will use external libraries when we read a summary, compile code, and
> emit a program with some nodes that were derived from the summary - these
> derived nodes will be marked as external (and if we do tree-shaking, they
might
> only be a subset of what was initially read from the summary)

I agree about external libraries.
And we don't need dependencies for them.
I made the change to kernel_generator_impl.dart and restored the check here.

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

https://codereview.chromium.org/2987553002/diff/1/pkg/kernel/lib/text/ast_to_...
pkg/kernel/lib/text/ast_to_text.dart:286: for (var import in
library.dependencies) {
On 2017/07/20 22:50:36, Siggi Cherem (dart-lang) wrote:
> On 2017/07/20 19:33:53, scheglov wrote:
> > On 2017/07/20 18:29:00, Siggi Cherem (dart-lang) wrote:
> > > I'm not sure I understand this part of the change. This means we no longer
> > print
> > > out library dependencies? Is this to avoid changes to the expectation
files
> in
> > > our unit tests?
> > > 
> > > If so, I would prefer not to make this change here, but update the
> > expectations
> > > or make a change that doesn't affect regular printing but affects printing
> of
> > > external libraries only?
> > 
> > First of all, this code has not been used, because we used not to add
> > dependencies to libraries when building them. Instead, the code about uses
> > LibraryImportTable to gather "actually used" imports and generate synthetic
> > prefixes assigned to them, and then we print only these "actually used"
> imports
> > into text presentation.
> > 
> > I agree that if we want to have our tests validate that we add required
> > import/export dependencies into kernels as we build them, we need to update
> all
> > the expectations. One nasty consequence of this would be that we will have
> > duplicate sets of imports - one for "actually used" and another for
> "specified".
> > If that's what you think is OK, then I can update all expectations.
> 
> Thanks so much for the clarification. Given your explanation, I'm OK
submitting
> this change as it is. Let's write a TODO and open a bug to follow up with the
> kernel-team on this issue. It seems low priority, but they can let us know if
> they have any concerns.

I will add TODO, https://github.com/dart-lang/sdk/issues/30224

[scheglov, 2017-07-21 16:21:56]

Description was changed from

==========
Add import dependencies to Kernel libraries and use them to resynthesize
ImportElement(s) in Analyzer.

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

to

==========
Add import dependencies to Kernel libraries and use them to resynthesize
ImportElement(s) in Analyzer.

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

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

[scheglov, 2017-07-21 16:21:57]

Committed patchset #3 (id:40001) manually as
2289ce12aeb96384637ce8333be7c4a73b26adbd (presubmit successful).