pkg/kernel/lib/binary/ast_from_binary.dart - Issue 2987553002: Add import dependencies to Kernel libraries and use them to resynthesize ImportElement(s) in Analyz…

Unified Diff: pkg/kernel/lib/binary/ast_from_binary.dart

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

Patch Set: Created 3 years, 5 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« no previous file with comments | « pkg/front_end/lib/src/fasta/kernel/kernel_library_builder.dart ('k') | pkg/kernel/lib/text/ast_to_text.dart » ('j') | pkg/kernel/lib/text/ast_to_text.dart » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: pkg/kernel/lib/binary/ast_from_binary.dart

diff --git a/pkg/kernel/lib/binary/ast_from_binary.dart b/pkg/kernel/lib/binary/ast_from_binary.dart

index 3d556eb82bc75aead025532c4a3c49a00492c0ff..ca70d671e20bc3ea9c4dfd385413b183bc67b85c 100644

--- a/pkg/kernel/lib/binary/ast_from_binary.dart

+++ b/pkg/kernel/lib/binary/ast_from_binary.dart

@@ -399,10 +399,6 @@ class BinaryBuilder {

void _readLibraryDependencies(Library library) {

int length = readUInt();

- if (library.isExternal) {

- assert(length == 0);

Siggi Cherem (dart-lang) 2017/07/20 18:29:00 do we need the library dependencies for external l

scheglov 2017/07/20 19:33:53 Analyzer's LibraryElement has "imports" that shoul

Siggi Cherem (dart-lang) 2017/07/20 22:50:36 I see - I understand now. Thanks for the explanati

scheglov 2017/07/21 16:21:33 I agree about external libraries. And we don't nee

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.

- return;

- }

library.dependencies.length = length;

for (int i = 0; i < length; ++i) {

var flags = readByte();