More fixes for GenericFunctionType.

1. Read GenericFunctionType fully, but in two steps to avoid recursion.
   We need to read it fully, because it might be a part of a const
   initializer, so the client might request the initializer expression,
   and there is no way to him to finish reading. Amnd in general, we
   don't read expressions lazily.

2. For consistency we need to set identifiers for all GenericFunctionType
   nodes that we want to store. So, DeclarationResolver should be updated
   to move the identifier number even though we don't use the same
   mechanism for building elements ininitializers, as we use for type
   annotations outside expressions.

R=brianwilkerson@google.com

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

pkg/analyzer/lib/src/generated/declaration_resolver.dart

@@ -32,6 +32,9 @@ class DeclarationResolver extends RecursiveAstVisitor<void> {
   /// element model.
   ElementWalker _walker;
 
+  /// Is `true` if the current [ClassDeclaration] has a const constructor.
+  bool _hasConstConstructor = false;
+
   /// The number of [GenericFunctionType] nodes that we encountered so far.
   /// We use it to request the corresponding resolved node.
   int _nextGenericFunctionTypeId = 0;
@@ -83,6 +86,14 @@ class DeclarationResolver extends RecursiveAstVisitor<void> {
 
   @override
   void visitClassDeclaration(ClassDeclaration node) {
+    _hasConstConstructor = false;
+    for (var member in node.members) {
+      if (member is ConstructorDeclaration && member.constKeyword != null) {
+        _hasConstConstructor = true;
+        break;
+      }
+    }
+
     ClassElement element = _match(node.name, _walker.getClass());
     _walk(new ElementWalker.forClass(element), () {
       super.visitClassDeclaration(node);
@@ -192,6 +203,10 @@ class DeclarationResolver extends RecursiveAstVisitor<void> {
     super.visitFieldDeclaration(node);
     FieldElement firstFieldElement = node.fields.variables[0].declaredElement;
     resolveMetadata(node, node.metadata, firstFieldElement);
+    if (node.fields.isConst ||
+        !node.isStatic && node.fields.isFinal && _hasConstConstructor) {
+      _consumeGenericFunctionTypeIds(node.fields);
+    }
   }
 
   @override
@@ -433,6 +448,9 @@ class DeclarationResolver extends RecursiveAstVisitor<void> {
     super.visitTopLevelVariableDeclaration(node);
     VariableElement firstElement = node.variables.variables[0].declaredElement;
     resolveMetadata(node, node.metadata, firstElement);
+    if (node.variables.isConst) {
+      _consumeGenericFunctionTypeIds(node.variables);
+    }
   }
 
   @override
@@ -484,6 +502,14 @@ class DeclarationResolver extends RecursiveAstVisitor<void> {
     }
   }
 
+  /// See [_ConsumeGenericFunctionTypeIdsVisitor].
+  void _consumeGenericFunctionTypeIds(VariableDeclarationList node) {
+    if (AnalysisDriver.useSummary2) {
+      var visitor = _ConsumeGenericFunctionTypeIdsVisitor(this);
+      node.variables.accept(visitor);
+    }
+  }
+
   /// Updates [identifier] to point to [element], after ensuring that the
   /// element has the expected name.
   ///
@@ -959,6 +985,24 @@ class ElementWalker {
   static bool _isNotSynthetic(Element e) => !e.isSynthetic;
 }
 
+/// For consistency we set identifiers for [GenericFunctionType]s in constant
+/// variable initializers, and instance final fields of classes with constant
+/// constructors. However [DeclarationResolver] does not visit these
+/// initializers, in builds separate local elements. We still need to consume
+/// them to ensure that identifiers expected by the element model, and by
+/// [DeclarationResolver] match.
+class _ConsumeGenericFunctionTypeIdsVisitor extends RecursiveAstVisitor<void> {
+  final DeclarationResolver resolver;
+
+  _ConsumeGenericFunctionTypeIdsVisitor(this.resolver);
+
+  @override
+  void visitGenericFunctionType(GenericFunctionType node) {
+    resolver._nextGenericFunctionTypeId++;
+    super.visitGenericFunctionType(node);
+  }
+}
+
 class _ElementMismatchException extends AnalysisException {
   /// Creates an exception to refer to the given [compilationUnit], [element],
   /// and [cause].

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

@@ -45,18 +45,64 @@ class AstBinaryReader {
 
   InterfaceType get _stringType => _unitContext.typeProvider.stringType;
 
-  /// This method is invoked by [LinkedUnitContext] to perform actual reading.
-  GenericFunctionType readGenericFunctionType(LinkedNode data) {
+  /// This method is invoked by [LinkedUnitContext] to finish reading.
+  void readGenericFunctionTypeFinish(
+    LinkedNode data,
+    GenericFunctionType node,
+  ) {
+    var typeParameterListData = data.genericFunctionType_typeParameters;
+    if (typeParameterListData != null) {
+      var dataList = typeParameterListData.typeParameterList_typeParameters;
+      var typeParameters = node.typeParameters.typeParameters;
+      for (var i = 0; i < dataList.length; ++i) {
+        var data = dataList[i];
+        var node = typeParameters[i];
+        node.bound = _readNode(data.typeParameter_bound);
+      }
+    }
+    node.returnType = readNode(data.genericFunctionType_returnType);
+    node.parameters = _readNode(data.genericFunctionType_formalParameters);
+  }
+
+  /// This method is invoked by [LinkedUnitContext] to perform shallow reading.
+  ///
+  /// It reads [TypeParameter] names, and creates [GenericFunctionType] node,
+  /// so that [LinkedUnitContext] can create elements for these nodes.
+  ///
+  /// But we cannot read the return type and formal parameters yet, until the
+  /// corresponding elements are created.
+  GenericFunctionType readGenericFunctionTypeShallow(LinkedNode data) {
+    TypeParameterList typeParameterList;
+    var typeParameterListData = data.genericFunctionType_typeParameters;
+    if (typeParameterListData != null) {
+      var dataList = typeParameterListData.typeParameterList_typeParameters;
+      var typeParameters = List<TypeParameter>(dataList.length);
+      for (var i = 0; i < dataList.length; ++i) {
+        var data = dataList[i];
+        typeParameters[i] = astFactory.typeParameter(
+          _readNode(data.annotatedNode_comment),
+          _readNodeList(data.annotatedNode_metadata),
+          _declaredIdentifier(data),
+          data.typeParameter_bound != null ? _Tokens.EXTENDS : null,
+          null,
+        );
+      }
+      typeParameterList = astFactory.typeParameterList(
+        _Tokens.LT,
+        typeParameters,
+        _Tokens.GT,
+      );
+    }
+
     GenericFunctionTypeImpl node = astFactory.genericFunctionType(
-      _readNodeLazy(data.genericFunctionType_returnType),
+      null,
       _Tokens.FUNCTION,
-      _readNode(data.genericFunctionType_typeParameters),
-      _readNodeLazy(data.genericFunctionType_formalParameters),
+      typeParameterList,
+      null,
       question:
           AstBinaryFlags.hasQuestion(data.flags) ? _Tokens.QUESTION : null,
     );
     node.type = _readType(data.genericFunctionType_type);
-    LazyGenericFunctionType.setData(node, data);
     return node;
   }
 

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

@@ -36,6 +36,10 @@ class AstBinaryWriter extends ThrowingAstVisitor<LinkedNodeBuilder> {
   /// in these declarations.
   LinkedNodeVariablesDeclarationBuilder _variablesDeclaration;
 
+  /// Is `true` if the current [ClassDeclaration] has a const constructor,
+  /// so initializers of final fields should be written.
+  bool _hasConstConstructor = false;
+
   AstBinaryWriter(this._linkingContext);
 
   @override
@@ -193,6 +197,15 @@ class AstBinaryWriter extends ThrowingAstVisitor<LinkedNodeBuilder> {
   LinkedNodeBuilder visitClassDeclaration(ClassDeclaration node) {
     try {
       timerAstBinaryWriterClass.start();
+
+      _hasConstConstructor = false;
+      for (var member in node.members) {
+        if (member is ConstructorDeclaration && member.constKeyword != null) {
+          _hasConstConstructor = true;
+          break;
+        }
+      }
+
       var builder = LinkedNodeBuilder.classDeclaration(
         classDeclaration_extendsClause: node.extendsClause?.accept(this),
         classDeclaration_nativeClause: node.nativeClause?.accept(this),
@@ -593,9 +606,9 @@ class AstBinaryWriter extends ThrowingAstVisitor<LinkedNodeBuilder> {
   @override
   LinkedNodeBuilder visitFunctionDeclaration(FunctionDeclaration node) {
     var builder = LinkedNodeBuilder.functionDeclaration(
+      functionDeclaration_returnType: node.returnType?.accept(this),
       functionDeclaration_functionExpression:
           node.functionExpression?.accept(this),
-      functionDeclaration_returnType: node.returnType?.accept(this),
     );
     builder.flags = AstBinaryFlags.encode(
       isExternal: node.externalKeyword != null,
@@ -681,6 +694,7 @@ class AstBinaryWriter extends ThrowingAstVisitor<LinkedNodeBuilder> {
   @override
   LinkedNodeBuilder visitGenericFunctionType(GenericFunctionType node) {
     var id = LazyAst.getGenericFunctionTypeId(node);
+    assert(id != null);
     assert(genericFunctionTypes.length == id);
     genericFunctionTypes.add(null);
 
@@ -1336,11 +1350,19 @@ class AstBinaryWriter extends ThrowingAstVisitor<LinkedNodeBuilder> {
   @override
   LinkedNodeBuilder visitVariableDeclaration(VariableDeclaration node) {
     var initializer = node.initializer;
-    VariableDeclarationList declarationList = node.parent;
-    if (declarationList.parent is TopLevelVariableDeclaration) {
+    var declarationList = node.parent as VariableDeclarationList;
+    var declaration = declarationList.parent;
+    if (declaration is TopLevelVariableDeclaration) {
       if (!declarationList.isConst) {
         initializer = null;
       }
+    } else if (declaration is FieldDeclaration) {
+      if (!(declarationList.isConst ||
+          !declaration.isStatic &&
+              declarationList.isFinal &&
+              _hasConstConstructor)) {
+        initializer = null;
+      }
     }
 
     var builder = LinkedNodeBuilder.variableDeclaration(

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

@@ -513,10 +513,6 @@ class LazyDirective {
     return node.getProperty(_key);
   }
 
-  static String getSelectedUri(UriBasedDirective node) {
-    return node.getProperty(_uriKey);
-  }
-
   static int getNameOffset(Directive node) {
     var lazy = get(node);
     if (lazy != null) {
@@ -526,6 +522,10 @@ class LazyDirective {
     }
   }
 
+  static String getSelectedUri(UriBasedDirective node) {
+    return node.getProperty(_uriKey);
+  }
+
   static void readMetadata(AstBinaryReader reader, Directive node) {
     var lazy = get(node);
     if (lazy != null && !lazy._hasMetadata) {
@@ -1172,67 +1172,6 @@ class LazyFunctionTypeAlias {
   }
 }
 
-class LazyGenericFunctionType {
-  static const _key = 'lazyAst';
-
-  final LinkedNode data;
-
-  bool _hasFormalParameters = false;
-  bool _hasReturnType = false;
-  bool _hasReturnTypeNode = false;
-
-  LazyGenericFunctionType(this.data);
-
-  static LazyGenericFunctionType get(GenericFunctionType node) {
-    return node.getProperty(_key);
-  }
-
-  static DartType getReturnType(
-    AstBinaryReader reader,
-    GenericFunctionType node,
-  ) {
-    if (reader.isLazy) {
-      var lazy = get(node);
-      if (!lazy._hasReturnType) {
-        var type = reader.readType(lazy.data.actualReturnType);
-        LazyAst.setReturnType(node, type);
-        lazy._hasReturnType = true;
-      }
-    }
-    return LazyAst.getReturnType(node);
-  }
-
-  static void readFormalParameters(
-    AstBinaryReader reader,
-    GenericFunctionType node,
-  ) {
-    var lazy = get(node);
-    if (lazy != null && !lazy._hasFormalParameters) {
-      node.parameters = reader.readNode(
-        lazy.data.genericFunctionType_formalParameters,
-      );
-      lazy._hasFormalParameters = true;
-    }
-  }
-
-  static void readReturnTypeNode(
-    AstBinaryReader reader,
-    GenericFunctionType node,
-  ) {
-    var lazy = get(node);
-    if (lazy != null && !lazy._hasReturnTypeNode) {
-      node.returnType = reader.readNode(
-        lazy.data.genericFunctionType_returnType,
-      );
-      lazy._hasReturnTypeNode = true;
-    }
-  }
-
-  static void setData(GenericFunctionType node, LinkedNode data) {
-    node.setProperty(_key, LazyGenericFunctionType(data));
-  }
-}
-
 class LazyGenericTypeAlias {
   static const _key = 'lazyAst';
   static const _hasSelfReferenceKey = 'lazyAst_hasSelfReferenceKey';

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

@@ -323,7 +323,6 @@ class LinkedUnitContext {
       LazyFunctionTypeAlias.readFormalParameters(_astReader, node);
       return node.parameters.parameters;
     } else if (node is GenericFunctionType) {
-      LazyGenericFunctionType.readFormalParameters(_astReader, node);
       return node.parameters.parameters;
     } else if (node is MethodDeclaration) {
       LazyMethodDeclaration.readFormalParameters(_astReader, node);
@@ -337,7 +336,7 @@ class LinkedUnitContext {
     GenericFunctionTypeImpl node = _genericFunctionTypeNodeList[id];
     if (node == null) {
       var data = this.data.genericFunctionTypes[id];
-      node = _astReader.readGenericFunctionType(data);
+      node = _astReader.readGenericFunctionTypeShallow(data);
       LazyAst.setGenericFunctionTypeId(node, id);
       _genericFunctionTypeNodeList[id] = node;
 
@@ -349,6 +348,8 @@ class LinkedUnitContext {
         node,
       );
       node.declaredElement = element;
+
+      _astReader.readGenericFunctionTypeFinish(data, node);
     }
     return node;
   }
@@ -538,7 +539,7 @@ class LinkedUnitContext {
     } else if (node is FunctionTypeAlias) {
       return LazyFunctionTypeAlias.getReturnType(_astReader, node);
     } else if (node is GenericFunctionType) {
-      return LazyGenericFunctionType.getReturnType(_astReader, node);
+      return node.returnType?.type ?? DynamicTypeImpl.instance;
     } else if (node is MethodDeclaration) {
       return LazyMethodDeclaration.getReturnType(_astReader, node);
     } else {
@@ -551,7 +552,6 @@ class LinkedUnitContext {
       LazyFunctionTypeAlias.readReturnTypeNode(_astReader, node);
       return node.returnType;
     } else if (node is GenericFunctionType) {
-      LazyGenericFunctionType.readReturnTypeNode(_astReader, node);
       return node.returnType;
     } else if (node is FunctionDeclaration) {
       LazyFunctionDeclaration.readReturnTypeNode(_astReader, node);

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

@@ -41,6 +41,9 @@ class ReferenceResolver extends ThrowingAstVisitor<void> {
   Reference reference;
   Scope scope;
 
+  /// Is `true` if the current [ClassDeclaration] has a const constructor.
+  bool _hasConstConstructor = false;
+
   ReferenceResolver(
     this.nodesToBuildType,
     this.elementFactory,
@@ -68,6 +71,14 @@ class ReferenceResolver extends ThrowingAstVisitor<void> {
     scope = new ClassScope(scope, element);
     LinkingNodeContext(node, scope);
 
+    _hasConstConstructor = false;
+    for (var member in node.members) {
+      if (member is ConstructorDeclaration && member.constKeyword != null) {
+        _hasConstConstructor = true;
+        break;
+      }
+    }
+
     node.typeParameters?.accept(this);
     node.extendsClause?.accept(this);
     node.implementsClause?.accept(this);
@@ -153,6 +164,12 @@ class ReferenceResolver extends ThrowingAstVisitor<void> {
   @override
   void visitFieldDeclaration(FieldDeclaration node) {
     node.fields.accept(this);
+
+    if (node.fields.isConst ||
+        !node.isStatic && node.fields.isFinal && _hasConstConstructor) {
+      var visitor = _SetGenericFunctionTypeIdVisitor(this);
+      node.fields.variables.accept(visitor);
+    }
   }
 
   @override
@@ -389,6 +406,10 @@ class ReferenceResolver extends ThrowingAstVisitor<void> {
   @override
   void visitTopLevelVariableDeclaration(TopLevelVariableDeclaration node) {
     node.variables.accept(this);
+    if (node.variables.isConst) {
+      var visitor = _SetGenericFunctionTypeIdVisitor(this);
+      node.variables.variables.accept(visitor);
+    }
   }
 
   @override
@@ -487,3 +508,20 @@ class ReferenceResolver extends ThrowingAstVisitor<void> {
     }
   }
 }
+
+/// For consistency we set identifiers for [GenericFunctionType]s in constant
+/// variable initializers, and instance final fields of classes with constant
+/// constructors.
+class _SetGenericFunctionTypeIdVisitor extends RecursiveAstVisitor<void> {
+  final ReferenceResolver resolver;
+
+  _SetGenericFunctionTypeIdVisitor(this.resolver);
+
+  @override
+  void visitGenericFunctionType(GenericFunctionType node) {
+    var id = resolver._nextGenericFunctionTypeId++;
+    LazyAst.setGenericFunctionTypeId(node, id);
+
+    super.visitGenericFunctionType(node);
+  }
+}

pkg/analyzer/test/src/dart/resolution/generic_function_type_test.dart

@@ -0,0 +1,49 @@
+// Copyright (c) 2019, 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.
+
+import 'package:analyzer/dart/ast/ast.dart';
+import 'package:test_reflective_loader/test_reflective_loader.dart';
+
+import 'driver_resolution.dart';
+
+main() {
+  defineReflectiveSuite(() {
+    defineReflectiveTests(GenericFunctionTypeResolutionTest);
+  });
+}
+
+@reflectiveTest
+class GenericFunctionTypeResolutionTest extends DriverResolutionTest {
+  /// Test that when [GenericFunctionType] is used in a constant variable
+  /// initializer, analysis does not throw an exception; and that the next
+  /// [GenericFunctionType] is also handled correctly.
+  test_constInitializer_field_static_const() async {
+    await assertNoErrorsInCode('''
+class A<T> {
+  const A();
+}
+
+class B {
+  static const x = const A<bool Function()>();
+}
+
+int Function(int a) y;
+''');
+  }
+
+  /// Test that when [GenericFunctionType] is used in a constant variable
+  /// initializer, analysis does not throw an exception; and that the next
+  /// [GenericFunctionType] is also handled correctly.
+  test_constInitializer_topLevel() async {
+    await assertNoErrorsInCode('''
+class A<T> {
+  const A();
+}
+
+const x = const A<bool Function()>();
+
+int Function(int a) y;
+''');
+  }
+}

pkg/analyzer/test/src/dart/resolution/test_all.dart

@@ -17,6 +17,7 @@ import 'for_element_test.dart' as for_element;
 import 'for_in_test.dart' as for_in;
 import 'function_expression_invocation_test.dart'
     as function_expression_invocation;
+import 'generic_function_type_test.dart' as generic_function_type;
 import 'generic_type_alias_test.dart' as generic_type_alias;
 import 'import_prefix_test.dart' as import_prefix;
 import 'instance_creation_test.dart' as instance_creation;
@@ -47,6 +48,7 @@ main() {
     for_element.main();
     for_in.main();
     function_expression_invocation.main();
+    generic_function_type.main();
     generic_type_alias.main();
     import_prefix.main();
     instance_creation.main();

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

@@ -572,6 +572,17 @@ class _ElementWriter {
       }
     } else if (e is DoubleLiteral) {
       buffer.write(e.value);
+    } else if (e is GenericFunctionType) {
+      if (e.returnType != null) {
+        writeNode(e.returnType);
+        buffer.write(' ');
+      }
+      buffer.write('Function');
+      if (e.typeParameters != null) {
+        writeList('<', '>', e.typeParameters.typeParameters, ', ', writeNode);
+      }
+      writeList('(', ')', e.parameters.parameters, ', ', writeNode,
+          includeEmpty: true);
     } else if (e is InstanceCreationExpression) {
       if (e.keyword != null) {
         buffer.write(e.keyword.lexeme);
@@ -661,6 +672,12 @@ class _ElementWriter {
       }
       writeList('(', ')', e.argumentList.arguments, ', ', writeNode,
           includeEmpty: true);
+    } else if (e is SimpleFormalParameter) {
+      writeNode(e.type);
+      if (e.identifier != null) {
+        buffer.write(' ');
+        buffer.write(e.identifier.name);
+      }
     } else if (e is SimpleIdentifier) {
       if (withConstElements) {
         buffer.writeln();

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

@@ -5756,6 +5756,60 @@ class C {
 ''');
   }
 
+  test_field_final_hasInitializer_hasConstConstructor() async {
+    var library = await checkLibrary('''
+class C {
+  final x = 42;
+  const C();
+}
+''');
+    checkElementText(library, r'''
+class C {
+  final int x = 42;
+  const C();
+}
+''');
+  }
+
+  test_field_final_hasInitializer_hasConstConstructor_genericFunctionType() async {
+    var library = await checkLibrary('''
+class A<T> {
+  const A();
+}
+class B {
+  final f = const A<int Function(double a)>();
+  const B();
+}
+''');
+    if (isAstBasedSummary) {
+      checkElementText(library, r'''
+class A<T> {
+  const A();
+}
+class B {
+  final A<int Function(double)> f = const
+        A/*location: test.dart;A*/<
+        int/*location: dart:core;int*/ Function(
+        double/*location: dart:core;double*/ a)>();
+  const B();
+}
+''');
+    }
+  }
+
+  test_field_final_hasInitializer_noConstConstructor() async {
+    var library = await checkLibrary('''
+class C {
+  final x = 42;
+}
+''');
+    checkElementText(library, r'''
+class C {
+  final int x;
+}
+''');
+  }
+
   test_field_formal_param_inferred_type_implicit() async {
     var library = await checkLibrary('class C extends D { var v; C(this.v); }'
         ' abstract class D { int get v; }');