Fixes pkg/analysis_server/test/analysis/get_hover_test

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

 // Copyright (c) 2014, 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.
 
 library engine.resolver;
 
 import 'dart:collection';
 
 import 'ast.dart';
 import 'constant.dart';
@@ skipping 2535 matching lines @@
    * A flag indicating whether a variable declaration is within the body of a method or function.
    */
   bool _inFunction = false;
 
   /**
    * A flag indicating whether the class currently being visited can be used as a mixin.
    */
   bool _isValidMixin = false;
 
   /**
+   * A collection holding the elements defined in a class that need to have
+   * their function type fixed to take into account type parameters of the
+   * enclosing class, or `null` if we are not currently processing nodes within
+   * a class.
+   */
+  List<ExecutableElementImpl> _functionTypesToFix = null;
+
+  /**
    * A table mapping field names to field elements for the fields defined in the current class, or
    * `null` if we are not in the scope of a class.
    */
   HashMap<String, FieldElement> _fieldMap;
 
   /**
    * Initialize a newly created element builder to build the elements for a compilation unit.
    *
    * @param initialHolder the element holder associated with the compilation unit being built
    */
@@ skipping 34 matching lines @@
         stackTraceParameter.staticElement = stackTrace;
       }
     }
     return super.visitCatchClause(node);
   }
 
   @override
   Object visitClassDeclaration(ClassDeclaration node) {
     ElementHolder holder = new ElementHolder();
     _isValidMixin = true;
+    _functionTypesToFix = new List<ExecutableElementImpl>();
     //
     // Process field declarations before constructors and methods so that field
     // formal parameters can be correctly resolved to their fields.
     //
     ElementHolder previousHolder = _currentHolder;
     _currentHolder = holder;
     try {
       List<ClassMember> nonFields = new List<ClassMember>();
       node.visitChildren(
           new _ElementBuilder_visitClassDeclaration(this, nonFields));
       _buildFieldMap(holder.fieldsWithoutFlushing);
       int count = nonFields.length;
       for (int i = 0; i < count; i++) {
         nonFields[i].accept(this);
       }
     } finally {
       _currentHolder = previousHolder;
     }
     SimpleIdentifier className = node.name;
     ClassElementImpl element = new ClassElementImpl.forNode(className);
     List<TypeParameterElement> typeParameters = holder.typeParameters;
     List<DartType> typeArguments = _createTypeParameterTypes(typeParameters);
     InterfaceTypeImpl interfaceType = new InterfaceTypeImpl(element);
     interfaceType.typeArguments = typeArguments;
     element.type = interfaceType;
-    List<ConstructorElement> constructors = holder.constructors;
-    if (constructors.length == 0) {
-      //
-      // Create the default constructor.
-      //
-      constructors = _createDefaultConstructors(interfaceType);
-    }
+    element.typeParameters = typeParameters;
     _setDocRange(element, node);
     element.abstract = node.isAbstract;
     element.accessors = holder.accessors;
+    List<ConstructorElement> constructors = holder.constructors;
+    if (constructors.isEmpty) {
+      constructors = _createDefaultConstructors(element);
+    }
     element.constructors = constructors;
     element.fields = holder.fields;
     element.methods = holder.methods;
-    element.typeParameters = typeParameters;
     element.validMixin = _isValidMixin;
+    // Function types must be initialized after the enclosing element has been
+    // set, for them to pick up the type parameters.
+    for (ExecutableElementImpl e in _functionTypesToFix) {
+      e.type = new FunctionTypeImpl(e);
+    }
+    _functionTypesToFix = null;

[Brian Wilkerson, 2015/11/16 20:53:04]

Consider moving these six lines into a separate method that can be re-used in
visitClassTypeAlias (unless it's not needed there).

[Jennifer Messerly, 2015/11/16 21:45:44]

Done. It does not appear to be needed in visitClassTypeAlias.

     _currentHolder.addType(element);
     className.staticElement = element;
     _fieldMap = null;
     holder.validate();
     return null;
   }
 
   /**
    * Implementation of this method should be synchronized with
    * [visitClassDeclaration].
    */
   void visitClassDeclarationIncrementally(ClassDeclaration node) {
     //
     // Process field declarations before constructors and methods so that field
     // formal parameters can be correctly resolved to their fields.
     //
     ClassElement classElement = node.element;
     _buildFieldMap(classElement.fields);
   }
 
   @override
   Object visitClassTypeAlias(ClassTypeAlias node) {
     ElementHolder holder = new ElementHolder();
+    _functionTypesToFix = new List<ExecutableElementImpl>();

[Brian Wilkerson, 2015/11/16 20:53:04]

Can a ClassTypeAlias actually contain executable elements? If not, then I don't
think we need this here.

[Jennifer Messerly, 2015/11/16 21:45:44]

Restored it mainly because the original code had it. But yeah, I was wondering
the same thing--it doesn't seem like the list will ever get populated. Anyway I
added an assert(isEmpty) and it wasn't hit by tests, so I'll try removing this
for now. Hopefully it will stay gone this time. :)

     _visitChildren(holder, node);
     SimpleIdentifier className = node.name;
     ClassElementImpl element = new ClassElementImpl.forNode(className);
     element.abstract = node.abstractKeyword != null;
     element.mixinApplication = true;
     List<TypeParameterElement> typeParameters = holder.typeParameters;
     element.typeParameters = typeParameters;
     List<DartType> typeArguments = _createTypeParameterTypes(typeParameters);
     InterfaceTypeImpl interfaceType = new InterfaceTypeImpl(element);
     interfaceType.typeArguments = typeArguments;
     element.type = interfaceType;
+    // set default constructor's function type
+    for (ExecutableElementImpl e in _functionTypesToFix) {
+      e.type = new FunctionTypeImpl(e);
+    }
+    _functionTypesToFix = null;
     _currentHolder.addType(element);
     className.staticElement = element;
     holder.validate();
     return null;
   }
 
   @override
   Object visitConstructorDeclaration(ConstructorDeclaration node) {
     _isValidMixin = false;
     ElementHolder holder = new ElementHolder();
@@ skipping 318 matching lines @@
       element.generator = true;
     }
     if (_inFunction) {
       Block enclosingBlock = node.getAncestor((node) => node is Block);
       if (enclosingBlock != null) {
         int functionEnd = node.offset + node.length;
         int blockEnd = enclosingBlock.offset + enclosingBlock.length;
         element.setVisibleRange(functionEnd, blockEnd - functionEnd - 1);
       }
     }
-    element.type = new FunctionTypeImpl(element);
+    if (_functionTypesToFix != null) {
+      _functionTypesToFix.add(element);
+    } else {
+      // TODO(jmesserly): for local functions inside of top-level generic
+      // functions, this is probably not right. The function type should be set
+      // after the enclosingElement is set, otherwise we won't be able to
+      // substitute those type parameters later.

[Brian Wilkerson, 2015/11/16 20:53:04]

I assume the longer term fix is to either (a) create the collection in
visitFunctionDeclaration and to create the function elements on the way out
(like we do elsewhere) or (b) to move the collection creation and handling to
visitCompilationUnit and just defer the creation of function types until after
the element structure is complete.

[Jennifer Messerly, 2015/11/16 21:45:44]

nice, yeah, I think either of those would work great. Do we always enter via
visitCompilationUnit? If so I might take a stab at that change right after this
(adding a test to show the generic nested function case that is fixed by it).

[Brian Wilkerson, 2015/11/16 21:55:30]

I fairly sure that's the case, but I'd have to double check. We've talked before
about not creating elements for declarations within method / function bodies
until they're needed, but I don't believe that we've done that.

+      element.type = new FunctionTypeImpl(element);
+    }
     element.hasImplicitReturnType = true;
     _currentHolder.addFunction(element);
     node.element = element;
     holder.validate();
     return null;
   }
 
   @override
   Object visitFunctionTypeAlias(FunctionTypeAlias node) {
     ElementHolder holder = new ElementHolder();
@@ skipping 382 matching lines @@
     }
   }
 
   /**
    * Creates the [ConstructorElement]s array with the single default constructor element.
    *
    * @param interfaceType the interface type for which to create a default constructor
    * @return the [ConstructorElement]s array with the single default constructor element
    */
   List<ConstructorElement> _createDefaultConstructors(
-      InterfaceTypeImpl interfaceType) {
+      ClassElementImpl definingClass) {
     ConstructorElementImpl constructor =
         new ConstructorElementImpl.forNode(null);
     constructor.synthetic = true;
-    constructor.returnType = interfaceType;
+    constructor.returnType = definingClass.type;
+    constructor.enclosingElement = definingClass;
     constructor.type = new FunctionTypeImpl(constructor);
     return <ConstructorElement>[constructor];
   }
 
   /**
    * Create the types associated with the given type parameters, setting the type of each type
    * parameter, and return an array of types corresponding to the given parameters.
    *
    * @param typeParameters the type parameters for which types are to be created
    * @return an array of types corresponding to the given parameters
@@ skipping 12109 matching lines @@
     nonFields.add(node);
     return null;
   }
 
   @override
   Object visitNode(AstNode node) => node.accept(TypeResolverVisitor_this);
 
   @override
   Object visitWithClause(WithClause node) => null;
 }