[vm/aot] Fix handling of annotations in tree shaker

Previously, annotations were not traced by global type flow analysis
(in order to avoid retaining classes which are only used in
annotations). Annotations were only traced during tree shaking
and references from such constants were treated much like
references from types. This handling of constants in annotations
conflicts with removal of fields, as tree shaker needs to know
which fields are retained upfront to be independent of the visiting
order. In a certain corner case (field was replaced with a getter
but was still used in a constant in annotation) that caused incorrect
AST and crash during serialization of AST.

In order to fix that, this change adds proper tracing through
annotations on members, classes and libraries, as if annotation
constants were used in the executable code. That also means
that annotation classes will be retained as allocated.
In order to compensate for that, a new pass is added before the global
analysis to clean all annotations except @ExternalName, @pragma
(used by the VM) and @TagNumber (used by protobuf tree shaking).

TEST=runtime/tests/vm/dart/regress_45968_test.dart
Fixes https://github.com/dart-lang/sdk/issues/45968

Change-Id: I998e4f7ec7da7b74e1738fc21b354a4ec9f0c071
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/199200
Commit-Queue: Alexander Markov <alexmarkov@google.com>
Reviewed-by: Vyacheslav Egorov <vegorov@google.com>
Reviewed-by: Martin Kustermann <kustermann@google.com>

pkg/vm/lib/transformations/type_flow/protobuf_handler.dart

@@ -68,6 +68,8 @@ class ProtobufHandler {
         _builderInfoAddMethod =
             libraryIndex.getMember(protobufLibraryUri, 'BuilderInfo', 'add');
 
+  bool usesAnnotationClass(Class cls) => cls == _tagNumberClass;
+
   /// This method is called from summary collector when analysis discovered
   /// that [member] is called and needs to construct a summary for its body.
   ///

pkg/vm/lib/transformations/type_flow/summary_collector.dart

@@ -774,6 +774,12 @@ class SummaryCollector extends RecursiveResultVisitor<TypeExpr> {
       _summary.result = _returnValue;
     }
 
+    // Visit annotations on members, classes and libraries.
+    // Other nodes currently do not have annotations used by the VM.
+    member.annotations.forEach(_visit);
+    member.enclosingClass?.annotations?.forEach(_visit);
+    member.enclosingLibrary?.annotations?.forEach(_visit);
+
     _staticTypeContext = null;
 
     debugPrint("------------ SUMMARY ------------");

pkg/vm/lib/transformations/type_flow/transformer.dart

@@ -75,6 +75,9 @@ Component transformComponent(
     typeFlowAnalysis.addRawCall(mainSelector);
   }
 
+  CleanupAnnotations(coreTypes, libraryIndex, protobufHandler)
+      .visitComponent(component);
+
   typeFlowAnalysis.process();
 
   analysisStopWatch.stop();
@@ -118,6 +121,56 @@ Component transformComponent(
   return component;
 }
 
+// Pass which removes all annotations except @ExternalName and @pragma
+// on members, classes and libraries. May also keep @TagNumber which is used
+// by protobuf handler.
+class CleanupAnnotations extends RecursiveVisitor {
+  final Class externalNameClass;
+  final Class pragmaClass;
+  final ProtobufHandler protobufHandler;
+
+  CleanupAnnotations(
+      CoreTypes coreTypes, LibraryIndex index, this.protobufHandler)
+      : externalNameClass = index.getClass('dart:_internal', 'ExternalName'),
+        pragmaClass = coreTypes.pragmaClass;
+
+  @override
+  defaultNode(Node node) {
+    if (node is Annotatable && node.annotations.isNotEmpty) {
+      _cleanupAnnotations(node, node.annotations);
+    }
+    super.defaultNode(node);
+  }
+
+  @override
+  visitTypedef(Typedef node) {
+    super.visitTypedef(node);
+    // These sub-nodes can have annotations but are not visited by
+    // Typedef.visitChildren.
+    visitList(node.positionalParameters, this);
+    visitList(node.namedParameters, this);
+  }
+
+  void _cleanupAnnotations(Node node, List<Expression> annotations) {
+    if (node is Member || node is Class || node is Library) {
+      annotations.removeWhere((a) => !_keepAnnotation(a));
+    } else {
+      annotations.clear();
+    }
+  }
+
+  bool _keepAnnotation(Expression annotation) {
+    final constant = (annotation as ConstantExpression).constant;
+    if (constant is InstanceConstant) {
+      final cls = constant.classNode;
+      return (cls == externalNameClass) ||
+          (cls == pragmaClass) ||
+          (protobufHandler != null && protobufHandler.usesAnnotationClass(cls));
+    }
+    return false;
+  }
+}
+
 /// Devirtualization based on results of type flow analysis.
 class TFADevirtualization extends Devirtualization {
   final TypeFlowAnalysis _typeFlowAnalysis;
@@ -731,6 +784,7 @@ class TreeShaker {
 
   void addUsedExtension(Extension node) {
     if (_usedExtensions.add(node)) {
+      node.annotations = const <Expression>[];
       _pass1.transformTypeParameterList(node.typeParameters, node);
       node.onType?.accept(typeVisitor);
     }
@@ -738,7 +792,7 @@ class TreeShaker {
 
   void addUsedTypedef(Typedef typedef) {
     if (_usedTypedefs.add(typedef)) {
-      _pass1.transformExpressionList(typedef.annotations, typedef);
+      typedef.annotations = const <Expression>[];
       _pass1.transformTypeParameterList(typedef.typeParameters, typedef);
       _pass1.transformTypeParameterList(
           typedef.typeParametersOfFunctionType, typedef);
@@ -1719,7 +1773,10 @@ class _TreeShakerConstantVisitor extends ConstantVisitor<Null> {
     shaker.addClassUsedInType(constant.classNode);
     visitList(constant.typeArguments, typeVisitor);
     constant.fieldValues.forEach((Reference fieldRef, Constant value) {
-      shaker.addUsedMember(fieldRef.asField);
+      if (!shaker.retainField(fieldRef.asField)) {
+        throw 'Constant $constant references field ${fieldRef.asField} '
+            'which is not retained';
+      }
       analyzeConstant(value);
     });
   }

pkg/vm/testcases/transformations/type_flow/transformer/annotation.dart.expect

@@ -2,37 +2,20 @@ library #lib;
 import self as self;
 import "dart:core" as core;
 
-@#C5
 typedef SomeType<contravariant T extends core::Object* = dynamic> = (core::List<T*>*) →* void;
-abstract class ClassAnnotation2 extends core::Object /*hasConstConstructor*/  {
-}
-abstract class MethodAnnotation extends core::Object /*hasConstConstructor*/  {
-[@vm.unreachable.metadata=] [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasNonThisUses:false,hasTearOffUses:false,getterSelectorId:1]  final field core::int* x;
-}
-abstract class TypedefAnnotation extends core::Object /*hasConstConstructor*/  {
-[@vm.unreachable.metadata=] [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasNonThisUses:false,hasTearOffUses:false,getterSelectorId:2]  final field core::List<core::int*>* list;
-}
-abstract class VarAnnotation extends core::Object /*hasConstConstructor*/  {
-}
-abstract class ParametrizedAnnotation<T extends core::Object* = dynamic> extends core::Object /*hasConstConstructor*/  {
-[@vm.unreachable.metadata=] [@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasNonThisUses:false,hasTearOffUses:false,getterSelectorId:3]  final field self::ParametrizedAnnotation::T* foo;
-}
 abstract class A extends core::Object {
   static method staticMethod() → void {}
 }
-@#C6
 class B extends core::Object {
   synthetic constructor •() → self::B*
     : super core::Object::•()
     ;
-[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:4,getterSelectorId:5]  @#C8
-  method instanceMethod() → void {}
+[@vm.procedure-attributes.metadata=methodOrSetterCalledDynamically:false,getterCalledDynamically:false,hasThisUses:false,hasTearOffUses:false,methodOrSetterSelectorId:1,getterSelectorId:2]  method instanceMethod() → void {}
 }
 [@vm.unboxing-info.metadata=()->i]static method foo() → core::int* {
-  @#C9 core::int* x = 2;
+  core::int* x = 2;
   return [@vm.direct-call.metadata=dart.core::_IntegerImplementation.+] [@vm.inferred-type.metadata=int (skip check)] x.{core::num::+}(2);
 }
-@#C11
 static method main(core::List<core::String*>* args) → dynamic {
   self::A::staticMethod();
   [@vm.direct-call.metadata=#lib::B.instanceMethod] [@vm.inferred-type.metadata=!? (skip check)] new self::B::•().{self::B::instanceMethod}();

runtime/tests/vm/dart/regress_45968_test.dart

@@ -0,0 +1,28 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Regression test for https://github.com/dart-lang/sdk/issues/45968.
+// Verifies that compiler doesn't crash if annotation references field
+// which is replaced with a getter.
+
+class Qualifier {
+  final String name;
+  const Qualifier(this.name);
+}
+
+class Foo implements Qualifier {
+  String get name => 'a';
+}
+
+class Bar {
+  @Qualifier('b')
+  void bar() {}
+}
+
+Qualifier x = Foo();
+
+main() {
+  print(x.name);
+  Bar().bar();
+}

runtime/tests/vm/dart_2/regress_45968_test.dart

@@ -0,0 +1,28 @@
+// Copyright (c) 2021, the Dart project authors.  Please see the AUTHORS file
+// for details. All rights reserved. Use of this source code is governed by a
+// BSD-style license that can be found in the LICENSE file.
+
+// Regression test for https://github.com/dart-lang/sdk/issues/45968.
+// Verifies that compiler doesn't crash if annotation references field
+// which is replaced with a getter.
+
+class Qualifier {
+  final String name;
+  const Qualifier(this.name);
+}
+
+class Foo implements Qualifier {
+  String get name => 'a';
+}
+
+class Bar {
+  @Qualifier('b')
+  void bar() {}
+}
+
+Qualifier x = Foo();
+
+main() {
+  print(x.name);
+  Bar().bar();
+}