Use .callMethod instead of LocalFunctionElement as key in inference

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

 // Copyright (c) 2013, 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 simple_types_inferrer;
 
 import '../closure.dart' show ClosureRepresentationInfo;
 import '../common.dart';
 import '../common/names.dart' show Identifiers, Selectors;
 import '../compiler.dart' show Compiler;
@@ skipping 38 matching lines @@
         SemanticSendResolvedMixin<TypeInformation, dynamic>,
         CompoundBulkMixin<TypeInformation, dynamic>,
         SetIfNullBulkMixin<TypeInformation, dynamic>,
         PrefixBulkMixin<TypeInformation, dynamic>,
         PostfixBulkMixin<TypeInformation, dynamic>,
         ErrorBulkMixin<TypeInformation, dynamic>,
         NewBulkMixin<TypeInformation, dynamic>,
         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 =
       new Map<JumpTarget, List<LocalsHandler>>();
   final Map<JumpTarget, List<LocalsHandler>> continuesFor =
       new Map<JumpTarget, List<LocalsHandler>>();
   LocalsHandler locals;
   final List<TypeInformation> cascadeReceiverStack =
       new List<TypeInformation>();
 
   TypeInformation returnType;
   bool visitingInitializers = false;
   bool isConstructorRedirect = false;
   bool seenSuperConstructorCall = false;
   SideEffects sideEffects = new SideEffects.empty();
   final MemberElement outermostElement;
   final InferrerEngine inferrer;
   final Setlet<Entity> capturedVariables = new Setlet<Entity>();
   final GlobalTypeInferenceElementData memberData;
 
   ElementGraphBuilder.internal(
-      ExecutableElement analyzedElement,
+      MemberElement analyzedElement,
       this.resolvedAst,
       this.outermostElement,
       InferrerEngine inferrer,
       this.compiler,
       this.locals)
       : this.analyzedElement = analyzedElement,
         this.inferrer = inferrer,
         this.types = inferrer.types,
         this.memberData = inferrer.dataOfMember(analyzedElement.memberContext) {
     assert(outermostElement != null);
     if (locals != null) return;
     ast.Node node;
     if (resolvedAst.kind == ResolvedAstKind.PARSED) {
       node = resolvedAst.node;
     }
     FieldInitializationScope fieldScope =
         analyzedElement.isGenerativeConstructor
             ? new FieldInitializationScope(types)
             : null;
     locals =
         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);
 
   TreeElements get elements => resolvedAst.elements;
 
   bool accumulateIsChecks = false;
   bool conditionIsSimple = false;
   List<ast.Send> isChecks;
   int loopLevel = 0;
@@ skipping 800 matching lines @@
     closureData.forEachCapturedVariable((variable, field) {
       locals.setCaptured(variable, field);
     });
     closureData.forEachBoxedVariable((variable, field) {
       locals.setCapturedAndBoxed(variable, field);
     });
     if (analyzedElement.isField) {
       return visit(initializer);
     }
 
-    FunctionElement function = analyzedElement;
+    MethodElement function = analyzedElement;
     FunctionSignature signature = function.functionSignature;
     signature.forEachOptionalParameter((FormalElement _element) {
       ParameterElement element = _element;
       ast.Expression defaultValue = element.initializer;
       // TODO(25566): The default value of a parameter of a redirecting factory
       // constructor comes from the corresponding parameter of the target.
 
       // If this is a default value from a different context (because
       // the current function is synthetic, e.g., a constructor from
       // a mixin application), we have to start a new inferrer visitor
       // with the correct context.
       // 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);
       }
       TypeInformation type =
           (defaultValue == null) ? types.nullType : visitor.visit(defaultValue);
       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;
     }
 
     if (analyzedElement.isGenerativeConstructor) {
       isThisExposed = false;
       signature.forEachParameter((FormalElement _element) {
         ParameterElement element = _element;
         TypeInformation parameterType = inferrer.typeOfParameter(element);
@@ skipping 98 matching lines @@
         case AsyncMarker.ASYNC:
           recordReturnType(types.asyncFutureType);
           break;
 
         case AsyncMarker.ASYNC_STAR:
           recordReturnType(types.asyncStarStreamType);
           break;
       }
     }
 
-    inferrer.closedWorldRefiner
-        .registerSideEffects(analyzedElement.declaration, sideEffects);
+    MethodElement declaration = analyzedElement.declaration;
+    inferrer.closedWorldRefiner.registerSideEffects(declaration, sideEffects);
     assert(breaksFor.isEmpty);
     assert(continuesFor.isEmpty);
     return returnType;
   }
 
   TypeInformation visitFunctionExpression(ast.FunctionExpression node) {
     // We loose track of [this] in closures (see issue 20840). To be on
     // the safe side, we mark [this] as exposed here. We could do better by
     // analyzing the closure.
     // TODO(herhut): Analyze whether closure exposes this.
     isThisExposed = true;
     LocalFunctionElement element = elements.getFunctionDefinition(node);
     // We don'TypeInformation put the closure in the work queue of the
     // inferrer, because it will share information with its enclosing
     // 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
     // a previous closure.
     ClosureRepresentationInfo nestedClosureData = compiler
         .backendStrategy.closureDataLookup
         .getClosureRepresentationInfo(element);
     nestedClosureData.forEachCapturedVariable((variable, field) {
       if (!nestedClosureData.isVariableBoxed(variable)) {
         if (variable == nestedClosureData.thisLocal) {
           inferrer.recordTypeOfField(field, thisType);
         }
         // The type is null for type parameters.
         if (locals.locals[variable] == null) return;
         inferrer.recordTypeOfField(field, locals.locals[variable]);
       }
       capturedVariables.add(variable);
     });
 
     return inferrer.concreteTypes.putIfAbsent(node, () {
-      return types.allocateClosureForLocalFunction(node, element);
+      return types.allocateClosureForMethod(node, element.callMethod);
     });
   }
 
   TypeInformation visitFunctionDeclaration(ast.FunctionDeclaration node) {
     LocalFunctionElement element =
         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);
     return type;
   }
 
   TypeInformation visitStringInterpolation(ast.StringInterpolation node) {
     // Interpolation could have any effects since it could call any toString()
     // method.
     // TODO(sra): This could be modelled by a call to toString() but with a
@@ skipping 1555 matching lines @@
       LocalFunctionElement function,
       ast.NodeList arguments,
       CallStructure callStructure,
       _) {
     ArgumentsTypes argumentTypes = analyzeArguments(node.arguments);
     Selector selector = elements.getSelector(node);
     TypeMask mask = memberData.typeOfSend(node);
     // 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
   TypeInformation visitLocalFunctionIncompatibleInvoke(
       ast.Send node,
       LocalFunctionElement function,
       ast.NodeList arguments,
       CallStructure callStructure,
       _) {
     analyzeArguments(node.arguments);
@@ skipping 80 matching lines @@
         return types.boolType;
       } else if (types.isNull(arguments.positional[0])) {
         potentiallyAddNullCheck(node, node.receiver);
         return types.boolType;
       }
     }
     return handleDynamicSend(node, selector, mask, receiverType, arguments);
   }
 
   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)) {
       return types.nonNullEmpty();
     }
 
     List<TypeInformation> unnamed = <TypeInformation>[];
     signature.forEachRequiredParameter((FormalElement _element) {
       ParameterElement element = _element;
       assert(locals.use(element) != null);
@@ skipping 110 matching lines @@
     Selector moveNextSelector = Selectors.moveNext;
     TypeMask moveNextMask = memberData.typeOfIteratorMoveNext(node);
 
     TypeInformation iteratorType = handleDynamicSend(node, iteratorSelector,
         iteratorMask, expressionType, new ArgumentsTypes.empty());
 
     return handleForInLoop(node, iteratorType, currentSelector, currentMask,
         moveNextSelector, moveNextMask);
   }
 }