Macro. More ordering for declarations phase.

Change-Id: I01e528a0f077367929e9e631992644b198165d6b
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/333581
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>

pkg/analyzer/lib/src/summary2/library_builder.dart

@@ -350,66 +350,71 @@ class LibraryBuilder {
     _declaredReferences[name] = reference;
   }
 
-  Future<void> executeMacroDeclarationsPhase({
+  /// Completes with `true` if a macro application was run in this library.
+  ///
+  /// Completes with `false` if there are no macro applications to run, either
+  /// because we ran all, or those that we have not run yet have dependencies
+  /// of interfaces declared in other libraries that, and we have not run yet
+  /// declarations phase macro applications for them.
+  Future<bool> executeMacroDeclarationsPhase({
     required OperationPerformanceImpl performance,
   }) async {
     final macroApplier = linker.macroApplier;
     if (macroApplier == null) {
-      return;
+      return false;
     }
 
-    while (true) {
+    final results = await macroApplier.executeDeclarationsPhase(
-      final results = await macroApplier.executeDeclarationsPhase(
+      library: element,
-        typeSystem: element.typeSystem,
+    );
-      );
 
-      // No more applications to execute.
+    // No more applications to execute.
-      if (results == null) {
+    if (results == null) {
-        break;
+      return false;
-      }
-
-      // No results from the application.
-      if (results.isEmpty) {
-        continue;
-      }
-
-      _macroResults.add(results);
-
-      final augmentationCode = macroApplier.buildAugmentationLibraryCode(
-        results,
-      );
-      if (augmentationCode == null) {
-        continue;
-      }
-
-      final importState = kind.addMacroAugmentation(
-        augmentationCode,
-        addLibraryAugmentDirective: true,
-        partialIndex: _macroResults.length,
-      );
-
-      final augmentation = _addMacroAugmentation(importState);
-
-      final macroLinkingUnit = units.last;
-      ElementBuilder(
-        libraryBuilder: this,
-        container: macroLinkingUnit.container,
-        unitReference: macroLinkingUnit.reference,
-        unitElement: macroLinkingUnit.element,
-      ).buildDeclarationElements(macroLinkingUnit.node);
-
-      final nodesToBuildType = NodesToBuildType();
-      final resolver =
-          ReferenceResolver(linker, nodesToBuildType, augmentation);
-      macroLinkingUnit.node.accept(resolver);
-      TypesBuilder(linker).build(nodesToBuildType);
-
-      // Append applications from the partial augmentation.
-      await macroApplier.add(
-        container: augmentation,
-        unit: macroLinkingUnit.node,
-      );
     }
+
+    // No results from the application.
+    if (results.isEmpty) {
+      return true;
+    }
+
+    _macroResults.add(results);
+
+    final augmentationCode = macroApplier.buildAugmentationLibraryCode(
+      results,
+    );
+    if (augmentationCode == null) {
+      return true;
+    }
+
+    final importState = kind.addMacroAugmentation(
+      augmentationCode,
+      addLibraryAugmentDirective: true,
+      partialIndex: _macroResults.length,
+    );
+
+    final augmentation = _addMacroAugmentation(importState);
+
+    final macroLinkingUnit = units.last;
+    ElementBuilder(
+      libraryBuilder: this,
+      container: macroLinkingUnit.container,
+      unitReference: macroLinkingUnit.reference,
+      unitElement: macroLinkingUnit.element,
+    ).buildDeclarationElements(macroLinkingUnit.node);
+
+    final nodesToBuildType = NodesToBuildType();
+    final resolver = ReferenceResolver(linker, nodesToBuildType, augmentation);
+    macroLinkingUnit.node.accept(resolver);
+    TypesBuilder(linker).build(nodesToBuildType);
+
+    // Append applications from the partial augmentation.
+    await macroApplier.add(
+      libraryElement: element,
+      container: augmentation,
+      unit: macroLinkingUnit.node,
+    );
+    return true;
   }
 
   Future<void> executeMacroTypesPhase({
@@ -460,6 +465,7 @@ class LibraryBuilder {
 
       // Append applications from the partial augmentation.
       await macroApplier.add(
+        libraryElement: element,
         container: augmentation,
         unit: macroLinkingUnit.node,
       );
@@ -470,6 +476,7 @@ class LibraryBuilder {
   Future<void> fillMacroApplier(LibraryMacroApplier macroApplier) async {
     for (final linkingUnit in units) {
       await macroApplier.add(
+        libraryElement: element,
         container: element,
         unit: linkingUnit.node,
       );

pkg/analyzer/lib/src/summary2/link.dart

@@ -305,10 +305,16 @@ class Linker {
   Future<void> _executeMacroDeclarationsPhase({
     required OperationPerformanceImpl performance,
   }) async {
-    for (final library in builders.values) {
+    while (true) {
-      await library.executeMacroDeclarationsPhase(
+      var hasProgress = false;
-        performance: performance,
+      for (final library in builders.values) {
-      );
+        hasProgress |= await library.executeMacroDeclarationsPhase(
+          performance: performance,
+        );
+      }
+      if (!hasProgress) {
+        break;
+      }
     }
   }
 

pkg/analyzer/lib/src/summary2/macro_application.dart

@@ -8,6 +8,7 @@ import 'package:_fe_analyzer_shared/src/macros/executor/multi_executor.dart';
 import 'package:_fe_analyzer_shared/src/macros/executor/protocol.dart' as macro;
 import 'package:analyzer/dart/ast/ast.dart' as ast;
 import 'package:analyzer/dart/ast/token.dart';
+import 'package:analyzer/dart/element/element.dart';
 import 'package:analyzer/dart/element/nullability_suffix.dart';
 import 'package:analyzer/dart/element/type.dart';
 import 'package:analyzer/src/dart/ast/ast.dart' as ast;
@@ -18,6 +19,7 @@ import 'package:analyzer/src/summary2/macro.dart';
 import 'package:analyzer/src/summary2/macro_application_error.dart';
 import 'package:analyzer/src/summary2/macro_declarations.dart';
 import 'package:analyzer/src/utilities/extensions/object.dart';
+import 'package:collection/collection.dart';
 
 /// The full list of [macro.ArgumentKind]s for this dart type (includes the type
 /// itself as well as type arguments, in source order with
@@ -71,8 +73,25 @@ class LibraryMacroApplier {
   final MultiMacroExecutor macroExecutor;
   final bool Function(Uri) isLibraryBeingLinked;
   final DeclarationBuilder declarationBuilder;
+
+  /// The reversed queue of macro applications to apply.
+  ///
+  /// We add classes before methods, and methods in the reverse order,
+  /// classes in the reverse order, annotations in the direct order.
+  ///
+  /// We iterate from the end looking for the next application to apply.
+  /// This way we ensure two ordering rules:
+  /// 1. inner before outer
+  /// 2. right to left
+  /// 3. source order
   final List<_MacroApplication> _applications = [];
 
+  /// The map from [InstanceElement] to the applications associated with it.
+  /// This includes applications on the class itself, and on the methods of
+  /// the class.
+  final Map<InstanceElement, List<_MacroApplication>> _interfaceApplications =
+      {};
+
   late final macro.TypePhaseIntrospector _typesPhaseIntrospector =
       _TypePhaseIntrospector(elementFactory, declarationBuilder);
 
@@ -84,6 +103,7 @@ class LibraryMacroApplier {
   });
 
   Future<void> add({
+    required LibraryElementImpl libraryElement,
     required LibraryOrAugmentationElementImpl container,
     required ast.CompilationUnit unit,
   }) async {
@@ -92,23 +112,31 @@ class LibraryMacroApplier {
         case ast.ClassDeclaration():
           final element = declaration.declaredElement;
           element as ClassElementImpl;
+          final declarationElement = element.augmented?.declaration ?? element;
+          declarationElement as ClassElementImpl;
           await _addClassLike(
+            libraryElement: libraryElement,
             container: container,
-            targetElement: element.declarationElement,
+            targetElement: declarationElement,
             classNode: declaration,
             classDeclarationKind: macro.DeclarationKind.classType,
             classAnnotations: declaration.metadata,
+            declarationsPhaseInterface: declarationElement,
             members: declaration.members,
           );
         case ast.MixinDeclaration():
           final element = declaration.declaredElement;
           element as MixinElementImpl;
+          final declarationElement = element.augmented?.declaration ?? element;
+          declarationElement as MixinElementImpl;
           await _addClassLike(
+            libraryElement: libraryElement,
             container: container,
-            targetElement: element.declarationElement,
+            targetElement: declarationElement,
             classNode: declaration,
             classDeclarationKind: macro.DeclarationKind.mixinType,
             classAnnotations: declaration.metadata,
+            declarationsPhaseInterface: declarationElement,
             members: declaration.members,
           );
       }
@@ -134,9 +162,11 @@ class LibraryMacroApplier {
   }
 
   Future<List<macro.MacroExecutionResult>?> executeDeclarationsPhase({
-    required TypeSystemImpl typeSystem,
+    required LibraryElementImpl library,
   }) async {
-    final application = _nextForDeclarationsPhase();
+    final application = _nextForDeclarationsPhase(
+      library: library,
+    );
     if (application == null) {
       return null;
     }
@@ -150,7 +180,7 @@ class LibraryMacroApplier {
         final introspector = _DeclarationPhaseIntrospector(
           elementFactory,
           declarationBuilder,
-          typeSystem,
+          library.typeSystem,
         );
 
         final result = await macroExecutor.executeDeclarationsPhase(
@@ -204,7 +234,9 @@ class LibraryMacroApplier {
   }
 
   Future<void> _addAnnotations({
+    required LibraryElementImpl libraryElement,
     required LibraryOrAugmentationElementImpl container,
+    required InstanceElement? declarationsPhaseElement,
     required ast.Declaration targetNode,
     required macro.DeclarationKind targetDeclarationKind,
     required List<ast.Annotation> annotations,
@@ -251,39 +283,54 @@ class LibraryMacroApplier {
         return instance.shouldExecute(targetDeclarationKind, phase);
       }).toSet();
 
-      _applications.add(
+      final application = _MacroApplication(
-        _MacroApplication(
+        libraryElement: libraryElement,
-          targetNode: targetNode,
+        declarationsPhaseElement: declarationsPhaseElement,
-          targetElement: targetElement,
+        targetNode: targetNode,
-          targetDeclarationKind: targetDeclarationKind,
+        targetElement: targetElement,
-          instance: instance,
+        targetDeclarationKind: targetDeclarationKind,
-          phasesToExecute: phasesToExecute,
+        annotationNode: annotation,
-        ),
+        instance: instance,
+        phasesToExecute: phasesToExecute,
       );
+
+      _applications.add(application);
+
+      // Record mapping for declarations phase dependencies.
+      if (declarationsPhaseElement != null) {
+        (_interfaceApplications[declarationsPhaseElement] ??= [])
+            .add(application);
+      }
     }
   }
 
   Future<void> _addClassLike({
+    required LibraryElementImpl libraryElement,
     required LibraryOrAugmentationElementImpl container,
     required MacroTargetElement targetElement,
     required ast.Declaration classNode,
     required macro.DeclarationKind classDeclarationKind,
     required List<ast.Annotation> classAnnotations,
+    required InterfaceElement? declarationsPhaseInterface,
     required List<ast.ClassMember> members,
   }) async {
     await _addAnnotations(
+      libraryElement: libraryElement,
       container: container,
       targetNode: classNode,
       targetDeclarationKind: classDeclarationKind,
+      declarationsPhaseElement: declarationsPhaseInterface,
       annotations: classAnnotations,
     );
 
     for (final member in members.reversed) {
       await _addAnnotations(
+        libraryElement: libraryElement,
         container: container,
         targetNode: member,
         // TODO(scheglov) incomplete
         targetDeclarationKind: macro.DeclarationKind.method,
+        declarationsPhaseElement: declarationsPhaseInterface,
         annotations: member.metadata,
       );
     }
@@ -304,6 +351,28 @@ class LibraryMacroApplier {
     }
   }
 
+  bool _hasInterfaceDependenciesSatisfied(_MacroApplication application) {
+    final dependencyElements = _interfaceDependencies(
+      application.declarationsPhaseElement,
+    );
+    if (dependencyElements == null) {
+      return true;
+    }
+
+    for (final dependencyElement in dependencyElements) {
+      final applications = _interfaceApplications[dependencyElement];
+      if (applications != null) {
+        for (final dependencyApplication in applications) {
+          if (dependencyApplication.hasDeclarationsPhase) {
+            return false;
+          }
+        }
+      }
+    }
+
+    return true;
+  }
+
   /// If [annotation] references a macro, invokes the right callback.
   _AnnotationMacro? _importedMacro({
     required LibraryOrAugmentationElementImpl container,
@@ -385,13 +454,58 @@ class LibraryMacroApplier {
     throw UnimplementedError();
   }
 
-  /// TODO(scheglov) Should use dependencies.
+  Set<InstanceElement>? _interfaceDependencies(InstanceElement? element) {
-  _MacroApplication? _nextForDeclarationsPhase() {
+    // TODO(scheglov) other elements
-    for (final application in _applications.reversed) {
+    switch (element) {
-      if (application.phasesToExecute.remove(macro.Phase.declarations)) {
+      case ExtensionElement():
-        return application;
+        // TODO(scheglov) implement
-      }
+        throw UnimplementedError();
+      case MixinElement():
+        final augmented = element.augmented;
+        switch (augmented) {
+          case null:
+            return const {};
+          default:
+            return [
+              ...augmented.superclassConstraints.map((e) => e.element),
+              ...augmented.interfaces.map((e) => e.element),
+            ].whereNotNull().toSet();
+        }
+      case InterfaceElement():
+        final augmented = element.augmented;
+        switch (augmented) {
+          case null:
+            return const {};
+          default:
+            return [
+              element.supertype?.element,
+              ...augmented.mixins.map((e) => e.element),
+              ...augmented.interfaces.map((e) => e.element),
+            ].whereNotNull().toSet();
+        }
+      default:
+        return null;
     }
+  }
+
+  _MacroApplication? _nextForDeclarationsPhase({
+    required LibraryElementImpl library,
+  }) {
+    for (final application in _applications.reversed) {
+      if (!application.hasDeclarationsPhase) {
+        continue;
+      }
+      if (application.libraryElement != library) {
+        continue;
+      }
+      if (!_hasInterfaceDependenciesSatisfied(application)) {
+        continue;
+      }
+      // The application has no dependencies to run.
+      application.removeDeclarationsPhase();
+      return application;
+    }
+
     return null;
   }
 
@@ -610,14 +724,22 @@ class _DeclarationPhaseIntrospector extends _TypePhaseIntrospector
             .map(declarationBuilder.fromElement.fieldElement)
             .toList();
     }
+    // TODO(scheglov) implement
     throw UnsupportedError('Only introspection on classes is supported');
   }
 
   @override
   Future<List<macro.MethodDeclaration>> methodsOf(
-      covariant macro.IntrospectableType clazz) {
+      covariant macro.IntrospectableType type) async {
-    // TODO: implement methodsOf
+    switch (type) {
-    throw UnimplementedError();
+      case IntrospectableClassDeclarationImpl():
+        return type.element.augmented!.methods
+            .where((e) => !e.isSynthetic)
+            .map(declarationBuilder.fromElement.methodElement)
+            .toList();
+    }
+    // TODO(scheglov) implement
+    throw UnsupportedError('Only introspection on classes is supported');
   }
 
   @override
@@ -675,19 +797,33 @@ class _DeclarationPhaseIntrospector extends _TypePhaseIntrospector
 }
 
 class _MacroApplication {
+  final LibraryElementImpl libraryElement;
+  final InstanceElement? declarationsPhaseElement;
   final ast.AstNode targetNode;
   final MacroTargetElement targetElement;
   final macro.DeclarationKind targetDeclarationKind;
+  final ast.Annotation annotationNode;
   final macro.MacroInstanceIdentifier instance;
   final Set<macro.Phase> phasesToExecute;
 
   _MacroApplication({
+    required this.libraryElement,
+    required this.declarationsPhaseElement,
     required this.targetNode,
     required this.targetElement,
     required this.targetDeclarationKind,
+    required this.annotationNode,
     required this.instance,
     required this.phasesToExecute,
   });
+
+  bool get hasDeclarationsPhase {
+    return phasesToExecute.contains(macro.Phase.declarations);
+  }
+
+  void removeDeclarationsPhase() {
+    phasesToExecute.remove(macro.Phase.declarations);
+  }
 }
 
 class _StaticTypeImpl implements macro.StaticType {
@@ -736,14 +872,3 @@ extension on macro.MacroExecutionResult {
       mixinAugmentations.isNotEmpty ||
       typeAugmentations.isNotEmpty;
 }
-
-extension<T extends InstanceElementImpl> on T {
-  T get declarationElement {
-    switch (augmented) {
-      case T(:final T declaration):
-        return declaration;
-      default:
-        return this;
-    }
-  }
-}

pkg/analyzer/test/src/summary/element_text.dart

@@ -55,6 +55,7 @@ class ElementTextConfiguration {
   bool withPropertyLinking = false;
   bool withRedirectedConstructors = false;
   bool withReferences = false;
+  bool withReturnType = true;
   bool withSyntheticDartCoreImport = false;
 
   ElementTextConfiguration({
@@ -539,7 +540,7 @@ class _ElementWriter {
       _writeCodeRange(e);
       _writeTypeParameterElements(e.typeParameters);
       _writeParameterElements(e.parameters);
-      _writeType('returnType', e.returnType);
+      _writeReturnType(e.returnType);
       _writeAugmentationTarget(e);
       _writeAugmentation(e);
     });
@@ -710,10 +711,12 @@ class _ElementWriter {
 
     _writeElements('exports', e.libraryExports, _writeExportElement);
 
-    _sink.writelnWithIndent('definingUnit');
+    if (configuration.filter(e.definingCompilationUnit)) {
-    _sink.withIndent(() {
+      _sink.writelnWithIndent('definingUnit');
-      _writeUnitElement(e.definingCompilationUnit);
+      _sink.withIndent(() {
-    });
+        _writeUnitElement(e.definingCompilationUnit);
+      });
+    }
 
     if (e is LibraryElementImpl) {
       _writeLibraryAugmentations(e);
@@ -760,7 +763,7 @@ class _ElementWriter {
 
       _writeTypeParameterElements(e.typeParameters);
       _writeParameterElements(e.parameters);
-      _writeType('returnType', e.returnType);
+      _writeReturnType(e.returnType);
       _writeNonSyntheticElement(e);
 
       if (e.isAugmentation) {
@@ -967,7 +970,7 @@ class _ElementWriter {
 
       expect(e.typeParameters, isEmpty);
       _writeParameterElements(e.parameters);
-      _writeType('returnType', e.returnType);
+      _writeReturnType(e.returnType);
       _writeNonSyntheticElement(e);
       writeLinking();
       _writeAugmentationTarget(e);
@@ -1058,6 +1061,12 @@ class _ElementWriter {
     }
   }
 
+  void _writeReturnType(DartType type) {
+    if (configuration.withReturnType) {
+      _writeType('returnType', type);
+    }
+  }
+
   void _writeShouldUseTypeForInitializerInference(
     PropertyInducingElementImpl e,
   ) {
@@ -1129,7 +1138,7 @@ class _ElementWriter {
         _sink.withIndent(() {
           _writeTypeParameterElements(aliasedElement.typeParameters);
           _writeParameterElements(aliasedElement.parameters);
-          _writeType('returnType', aliasedElement.returnType);
+          _writeReturnType(aliasedElement.returnType);
         });
       }
     });

pkg/analyzer/test/src/summary/elements_base.dart

@@ -32,18 +32,14 @@ abstract class ElementsBaseTest extends PubPackageResolutionTest {
 
     final uriStr = 'package:test/test.dart';
     final libraryResult = await analysisSession.getLibraryByUri(uriStr);
-    libraryResult as LibraryElementResult;
 
     if (keepLinkingLibraries) {
-      return libraryResult.element as LibraryElementImpl;
+      return libraryResult.element;
     } else {
       analysisContext.changeFile(file.path);
       await analysisContext.applyPendingFileChanges();
       // Ask again, should be read from bytes.
-      final analysisSession = analysisContext.currentSession;
+      return testContextLibrary(uriStr);
-      final libraryResult = await analysisSession.getLibraryByUri(uriStr);
-      libraryResult as LibraryElementResult;
-      return libraryResult.element as LibraryElementImpl;
     }
   }
 
@@ -59,4 +55,17 @@ abstract class ElementsBaseTest extends PubPackageResolutionTest {
     }
     expect(actual, expected);
   }
+
+  Future<LibraryElementImpl> testContextLibrary(String uriStr) async {
+    final analysisContext = contextFor(testFile);
+    final analysisSession = analysisContext.currentSession;
+    final libraryResult = await analysisSession.getLibraryByUri(uriStr);
+    return libraryResult.element;
+  }
+}
+
+extension on SomeLibraryElementResult {
+  LibraryElementImpl get element {
+    return (this as LibraryElementResult).element as LibraryElementImpl;
+  }
 }

pkg/analyzer/test/src/summary/macro/order.dart

@@ -4,7 +4,8 @@
 
 import 'package:_fe_analyzer_shared/src/macros/api.dart';
 
-/*macro*/ class AddClass implements ClassTypesMacro, MethodTypesMacro {
+/*macro*/ class AddClass
+    implements ClassTypesMacro, MethodTypesMacro, MixinTypesMacro {
   final String name;
 
   const AddClass(this.name);
@@ -19,6 +20,11 @@ import 'package:_fe_analyzer_shared/src/macros/api.dart';
     _add(builder);
   }
 
+  @override
+  buildTypesForMixin(method, builder) {
+    _add(builder);
+  }
+
   void _add(TypeBuilder builder) {
     final code = 'class $name {}';
     builder.declareType(name, DeclarationCode.fromString(code));
@@ -26,7 +32,10 @@ import 'package:_fe_analyzer_shared/src/macros/api.dart';
 }
 
 /*macro*/ class AddFunction
-    implements ClassDeclarationsMacro, MethodDeclarationsMacro {
+    implements
+        ClassDeclarationsMacro,
+        MethodDeclarationsMacro,
+        MixinDeclarationsMacro {
   final String name;
 
   const AddFunction(this.name);
@@ -41,9 +50,42 @@ import 'package:_fe_analyzer_shared/src/macros/api.dart';
     _add(builder);
   }
 
+  @override
+  buildDeclarationsForMixin(method, builder) {
+    _add(builder);
+  }
+
   void _add(DeclarationBuilder builder) {
     final code = 'void $name() {}';
     final declaration = DeclarationCode.fromString(code);
     builder.declareInLibrary(declaration);
   }
 }
+
+/*macro*/ class AddHierarchyMethod implements ClassDeclarationsMacro {
+  final String name;
+
+  const AddHierarchyMethod(this.name);
+
+  @override
+  buildDeclarationsForClass(clazz, builder) async {
+    // builder.typeDeclarationOf(identifier);
+    final methods = (await Future.wait(
+      clazz.interfaces.map(
+        (interface) async {
+          final type = await builder.typeDeclarationOf(interface.identifier);
+          type as IntrospectableType;
+          return await builder.methodsOf(type);
+        },
+      ),
+    ))
+        .expand((element) => element)
+        .toList();
+    final methodsStr = methods.map((e) => e.identifier.name).join('_');
+
+    final compoundName = methodsStr.isEmpty ? name : '${methodsStr}_$name';
+    final code = '  void $compoundName() {}';
+    final declaration = DeclarationCode.fromString(code);
+    builder.declareInType(declaration);
+  }
+}