Use .callMethod instead of LocalFunctionElement as key in inference

pkg/compiler/lib/src/inferrer/builder.dart

Index: pkg/compiler/lib/src/inferrer/builder.dart
diff --git a/pkg/compiler/lib/src/inferrer/builder.dart b/pkg/compiler/lib/src/inferrer/builder.dart
index 711b190856b63bc873630bf643b401ce93b74da7..c85914a910845c93f9a1b5180684ece04f710c80 100644
--- a/pkg/compiler/lib/src/inferrer/builder.dart
+++ b/pkg/compiler/lib/src/inferrer/builder.dart
@@ -56,7 +56,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
         SetBulkMixin<TypeInformation, dynamic>
     implements SemanticSendVisitor<TypeInformation, dynamic> {
   final Compiler compiler;
-  final ExecutableElement analyzedElement;
+  final MemberElement analyzedElement;
   final ResolvedAst resolvedAst;
   final TypeSystem types;
   final Map<JumpTarget, List<LocalsHandler>> breaksFor =
@@ -78,7 +78,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
   final GlobalTypeInferenceElementData memberData;
 
   ElementGraphBuilder.internal(
-      ExecutableElement analyzedElement,
+      MemberElement analyzedElement,
       this.resolvedAst,
       this.outermostElement,
       InferrerEngine inferrer,
@@ -102,7 +102,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
         new LocalsHandler(inferrer, types, compiler.options, node, fieldScope);
   }
 
-  ElementGraphBuilder(ExecutableElement element, ResolvedAst resolvedAst,
+  ElementGraphBuilder(MemberElement element, ResolvedAst resolvedAst,
       Compiler compiler, InferrerEngine inferrer, [LocalsHandler handler])
       : this.internal(element, resolvedAst, element.memberContext, inferrer,
             compiler, handler);
@@ -923,7 +923,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
       return visit(initializer);
     }
 
-    FunctionElement function = analyzedElement;
+    MethodElement function = analyzedElement;
     FunctionSignature signature = function.functionSignature;
     signature.forEachOptionalParameter((FormalElement _element) {
       ParameterElement element = _element;
@@ -938,7 +938,15 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
       // TODO(johnniwinther): Remove once function signatures are fixed.
       ElementGraphBuilder visitor = this;
       if (inferrer.hasAlreadyComputedTypeOfParameterDefault(element)) return;
-      if (element.functionDeclaration != analyzedElement) {
+      if (element.functionDeclaration.isLocal) {

[Siggi Cherem (dart-lang), 2017/06/30 18:13:54]

I was a bit confused by this extra check, but I think it is because some of the
invariants here are presented "out of order". The way it currently reads is like
"Because isLocal it can't be a new context, let me check that it is true with
the implicit invariant about how declarations are represented for local
functions."

An idea that might help here, would be to do it in the opposite order with some
helper variables that give name to the concepts ("here is the declaration for
local functions, let me make sure it can't be a new context in that case"). For
example:

FunctionElement declaration = element.functionDeclaration;
MethodElement declarationMethod = declaration is LocalFunctionElement
    ? declaration.callMethod
    : declaration;
bool needNewContext = declarationMethod != analyzedElement;

if (needNewContext) {
  assert(declarationMethod is ConstructorElement
      failedAt(element, 'unexpected ... '));
  visitor = new ElementGraphBuilder(declarationMethod, ...)
}

thoughts?

[Johnni Winther, 2017/06/30 19:02:02]

Copied in :)

+        LocalFunctionElement localFunction = element.functionDeclaration;
+        assert(
+            localFunction.callMethod == analyzedElement,
+            failedAt(
+                element,
+                "Unexpected function declaration "
+                "${localFunction.callMethod}, expected ${analyzedElement}."));
+      } else if (element.functionDeclaration != analyzedElement) {
         ConstructorElement constructor = element.functionDeclaration;
         visitor = new ElementGraphBuilder(constructor,
             element.functionDeclaration.resolvedAst, compiler, inferrer);
@@ -948,7 +956,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
       inferrer.setDefaultTypeOfParameter(element, type);
     });
 
-    if (inferrer.isNativeMember(analyzedElement)) {
+    if (closedWorld.nativeData.isNativeMember(analyzedElement)) {
       // Native methods do not have a body, and we currently just say
       // they return dynamic.
       return types.dynamicType;
@@ -1067,8 +1075,8 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
       }
     }
 
-    inferrer.closedWorldRefiner
-        .registerSideEffects(analyzedElement.declaration, sideEffects);
+    MethodElement declaration = analyzedElement.declaration;
+    inferrer.closedWorldRefiner.registerSideEffects(declaration, sideEffects);
     assert(breaksFor.isEmpty);
     assert(continuesFor.isEmpty);
     return returnType;
@@ -1086,10 +1094,10 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
     // method, like for example the types of local variables.
     LocalsHandler closureLocals =
         new LocalsHandler.from(locals, node, useOtherTryBlock: false);
-    ElementGraphBuilder visitor = new ElementGraphBuilder(
-        element, element.resolvedAst, compiler, inferrer, closureLocals);
+    ElementGraphBuilder visitor = new ElementGraphBuilder(element.callMethod,
+        element.resolvedAst, compiler, inferrer, closureLocals);
     visitor.run();
-    inferrer.recordReturnTypeOfLocalFunction(element, visitor.returnType);
+    inferrer.recordReturnType(element.callMethod, visitor.returnType);
 
     // Record the types of captured non-boxed variables. Types of
     // these variables may already be there, because of an analysis of
@@ -1110,7 +1118,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
     });
 
     return inferrer.concreteTypes.putIfAbsent(node, () {
-      return types.allocateClosureForLocalFunction(node, element);
+      return types.allocateClosureForMethod(node, element.callMethod);
     });
   }
 
@@ -1119,7 +1127,7 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
         elements.getFunctionDefinition(node.function);
     TypeInformation type =
         inferrer.concreteTypes.putIfAbsent(node.function, () {
-      return types.allocateClosureForLocalFunction(node.function, element);
+      return types.allocateClosureForMethod(node.function, element.callMethod);
     });
     locals.update(element, type, node);
     visit(node.function);
@@ -2695,8 +2703,8 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
     // This only works for function statements. We need a
     // more sophisticated type system with function types to support
     // more.
-    return inferrer.registerCalledLocalFunction(node, selector, mask,
-        outermostElement, function, argumentTypes, sideEffects, inLoop);
+    return inferrer.registerCalledMember(node, selector, mask, outermostElement,
+        function.callMethod, argumentTypes, sideEffects, inLoop);
   }
 
   @override
@@ -2797,19 +2805,14 @@ class ElementGraphBuilder extends ast.Visitor<TypeInformation>
   }
 
   void recordReturnType(TypeInformation type) {
-    if (analyzedElement.isLocal) {
-      returnType = inferrer.addReturnTypeForLocalFunction(
-          analyzedElement, returnType, type);
-    } else {
-      returnType =
-          inferrer.addReturnTypeForMethod(analyzedElement, returnType, type);
-    }
+    returnType =
+        inferrer.addReturnTypeForMethod(analyzedElement, returnType, type);
   }
 
   TypeInformation synthesizeForwardingCall(
       Spannable node, ConstructorElement element) {
     element = element.implementation;
-    FunctionElement function = analyzedElement;
+    MethodElement function = analyzedElement;
     FunctionSignature signature = function.functionSignature;
     FunctionSignature calleeSignature = element.functionSignature;
     if (!calleeSignature.isCompatibleWith(signature)) {