[kernel] Fix round trip serialization of metadata when nodes are lazy loaded.

Metadata repository might be populated lazily as we are traversing the
component, so it is incorrect to skip empty metadata repositories early
in the serialization process.

Instead we filter empty repositories at the very end - after all nodes
were written out.

Change-Id: I159e0c0213a034388855944af03e72786d9e951b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/148065
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Jens Johansen <jensj@google.com>

pkg/kernel/lib/binary/ast_to_binary.dart

@@ -579,16 +579,18 @@ class BinaryPrinter implements Visitor<void>, BinarySink {
     }
   }
 
-  /// Collect non-empty metadata repositories associated with the component.
+  /// Collect metadata repositories associated with the component.
   void _collectMetadata(Component component) {
-    component.metadata.forEach((tag, repository) {
-      if (repository.mapping.isEmpty) {
-        return;
-      }
-
-      _metadataSubsections ??= <_MetadataSubsection>[];
-      _metadataSubsections.add(new _MetadataSubsection(repository));
-    });
+    if (component.metadata.isNotEmpty) {
+      // Component might be loaded lazily - meaning that we can't
+      // just skip empty repositories here, they might be populated by
+      // the serialization process. Instead we will filter empty repositories
+      // later before writing the section out.
+      _metadataSubsections = component.metadata.values
+          .map((MetadataRepository repository) =>
+              new _MetadataSubsection(repository))
+          .toList();
+    }
   }
 
   /// Writes metadata associated with the given [Node].
@@ -668,8 +670,10 @@ class BinaryPrinter implements Visitor<void>, BinarySink {
     }
 
     _binaryOffsetForMetadataPayloads = getBufferOffset();
+    _metadataSubsections
+        ?.removeWhere((_MetadataSubsection s) => s.metadataMapping.isEmpty);
 
-    if (_metadataSubsections == null) {
+    if (_metadataSubsections == null || _metadataSubsections.isEmpty) {
       _binaryOffsetForMetadataMappings = getBufferOffset();
       writeUInt32(0); // Empty section.
       return;

pkg/kernel/test/metadata_test.dart

@@ -87,57 +87,55 @@ class BytesBuilderSink implements Sink<List<int>> {
   void close() {}
 }
 
-/// Visitor that assigns [Metadata] object created with [Metadata.forNode] to
-/// each supported node in the component.
-class Annotator extends RecursiveVisitor<Null> {
-  final TestMetadataRepository repository;
+typedef NodePredicate = bool Function(TreeNode node);
 
-  Annotator(Component component)
-      : repository = component.metadata[TestMetadataRepository.kTag];
+/// Visitor calling [handle] function on every node which can have metadata
+/// associated with it and also satisfies the given [predicate].
+class Visitor extends RecursiveVisitor<Null> {
+  final NodePredicate predicate;
+  final void Function(TreeNode) handle;
+
+  Visitor(this.predicate, this.handle);
 
   defaultTreeNode(TreeNode node) {
     super.defaultTreeNode(node);
-    if (MetadataRepository.isSupported(node)) {
-      repository.mapping[node] = new Metadata.forNode(node);
+    if (MetadataRepository.isSupported(node) && predicate(node)) {
+      handle(node);
     }
   }
-
-  static void annotate(Component p) {
-    globalDebuggingNames = new NameSystem();
-    p.accept(new Annotator(p));
-  }
 }
 
-/// Visitor that checks that each supported node in the component has correct
-/// metadata.
-class Validator extends RecursiveVisitor<Null> {
-  final TestMetadataRepository repository;
+/// Visit the given component assigning [Metadata] object created with
+/// [Metadata.forNode] to each supported node in the component which matches
+/// [shouldAnnotate] predicate.
+void annotate(Component p, NodePredicate shouldAnnotate) {
+  globalDebuggingNames = new NameSystem();
+  final repository = p.metadata[TestMetadataRepository.kTag];
+  p.accept(new Visitor(shouldAnnotate, (node) {
+    repository.mapping[node] = new Metadata.forNode(node);
+  }));
+}
 
-  Validator(Component component)
-      : repository = component.metadata[TestMetadataRepository.kTag];
+/// Visit the given component and checks that each supported node in the
+/// component matching [shouldAnnotate] predicate has correct metadata.
+void validate(Component p, NodePredicate shouldAnnotate) {
+  globalDebuggingNames = new NameSystem();
+  final repository = p.metadata[TestMetadataRepository.kTag];
+  p.accept(new Visitor(shouldAnnotate, (node) {
+    final m = repository.mapping[node];
+    final expected = new Metadata.forNode(node);
 
-  defaultTreeNode(TreeNode node) {
-    super.defaultTreeNode(node);
-    if (MetadataRepository.isSupported(node)) {
-      final m = repository.mapping[node];
-      final expected = new Metadata.forNode(node);
-
-      expect(m.string, equals(expected.string));
-      expect(m.member, equals(expected.member));
-      expect(m.type, equals(expected.type));
-    }
-  }
-
-  static void validate(Component p) {
-    globalDebuggingNames = new NameSystem();
-    p.accept(new Validator(p));
-  }
+    expect(m, isNotNull);
+    expect(m.string, equals(expected.string));
+    expect(m.member, equals(expected.member));
+    expect(m.type, equals(expected.type));
+  }));
 }
 
 Component fromBinary(List<int> bytes) {
   var component = new Component();
   component.addMetadataRepository(new TestMetadataRepository());
-  new BinaryBuilderWithMetadata(bytes).readSingleFileComponent(component);
+  new BinaryBuilderWithMetadata(bytes).readComponent(component);
   return component;
 }
 
@@ -147,19 +145,53 @@ List<int> toBinary(Component p) {
   return sink.builder.takeBytes();
 }
 
-main() {
-  test('annotate-serialize-deserialize-validate', () async {
-    final Uri platform = computePlatformBinariesLocation(forceBuildDir: true)
-        .resolve("vm_platform_strong.dill");
-    final List<int> platformBinary =
-        await new File(platform.toFilePath()).readAsBytes();
+main() async {
+  bool anyNode(TreeNode node) => true;
+  bool onlyMethods(TreeNode node) =>
+      node is Procedure &&
+      node.kind == ProcedureKind.Method &&
+      node.enclosingClass != null;
 
+  final Uri platform = computePlatformBinariesLocation(forceBuildDir: true)
+      .resolve("vm_platform_strong.dill");
+  final List<int> platformBinary =
+      await new File(platform.toFilePath()).readAsBytes();
+
+  Future<void> testRoundTrip(List<int> Function(List<int>) binaryTransformer,
+      NodePredicate shouldAnnotate) async {
     final component = fromBinary(platformBinary);
-    Annotator.annotate(component);
-    Validator.validate(component);
+    annotate(component, shouldAnnotate);
+    validate(component, shouldAnnotate);
+    expect(component.metadata[TestMetadataRepository.kTag].mapping.length,
+        greaterThan(0));
 
-    final annotatedComponentBinary = toBinary(component);
+    final annotatedComponentBinary = binaryTransformer(toBinary(component));
     final annotatedComponentFromBinary = fromBinary(annotatedComponentBinary);
-    Validator.validate(annotatedComponentFromBinary);
+    validate(annotatedComponentFromBinary, shouldAnnotate);
+    expect(
+        annotatedComponentFromBinary
+            .metadata[TestMetadataRepository.kTag].mapping.length,
+        greaterThan(0));
+  }
+
+  test('annotate-serialize-deserialize-validate', () async {
+    await testRoundTrip((binary) => binary, anyNode);
+  });
+
+  test('annotate-serialize-deserialize-validate-only-methods', () async {
+    await testRoundTrip((binary) => binary, onlyMethods);
+  });
+
+  test('annotate-serialize-deserialize-twice-then-validate', () async {
+    // This test validates that serializing a component that was just
+    // deserialized (without visiting anything) works.
+    await testRoundTrip((binary) => toBinary(fromBinary(binary)), anyNode);
+  });
+
+  test('annotate-serialize-deserialize-twice-then-validate-only-methods',
+      () async {
+    // This test validates that serializing a component that was just
+    // deserialized (without visiting anything) works.
+    await testRoundTrip((binary) => toBinary(fromBinary(binary)), onlyMethods);
   });
 }