Refactor computation of NativeBehavior.

pkg/compiler/lib/src/native/behavior.dart

Index: pkg/compiler/lib/src/native/behavior.dart
diff --git a/pkg/compiler/lib/src/native/behavior.dart b/pkg/compiler/lib/src/native/behavior.dart
index e236975ddf5b6da00f6cf5abfb059a6202db4ff8..72cb53c91fc628c3df2baff5b4feaa76da78a29d 100644
--- a/pkg/compiler/lib/src/native/behavior.dart
+++ b/pkg/compiler/lib/src/native/behavior.dart
@@ -3,20 +3,23 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import '../common.dart';
-import '../common/backend_api.dart' show ForeignResolver;
+import '../common/backend_api.dart' show BackendClasses, ForeignResolver;
 import '../common/resolution.dart' show ParsingContext, Resolution;
 import '../compiler.dart' show Compiler;
+import '../compile_time_constants.dart' show ConstantEnvironment;
 import '../constants/expressions.dart';
 import '../constants/values.dart';
 import '../core_types.dart' show CommonElements;
-import '../elements/resolution_types.dart';
 import '../elements/elements.dart';
+import '../elements/resolution_types.dart';
+import '../elements/types.dart';
 import '../js/js.dart' as js;
 import '../js_backend/js_backend.dart';
+import '../js_backend/backend_helpers.dart';
+import '../options.dart';
 import '../tree/tree.dart';
 import '../universe/side_effects.dart' show SideEffects;
 import '../util/util.dart';
-import 'enqueue.dart';
 import 'js.dart';
 
 typedef dynamic /*DartType|SpecialType*/ TypeLookup(String typeString);
@@ -762,51 +765,11 @@ class NativeBehavior {
       return cls.thisType;
     }
 
-    return ofMethod(element, type, metadata, lookup, compiler,
+    BehaviorComputer computer = new BehaviorComputerImpl(compiler);
+    return computer.ofMethod(type, metadata, lookup,
         isJsInterop: compiler.backend.isJsInterop(element));
   }
 
-  static NativeBehavior ofMethod(
-      Spannable spannable,
-      ResolutionFunctionType type,
-      List<ConstantExpression> metadata,
-      TypeLookup lookupType,
-      Compiler compiler,
-      {bool isJsInterop}) {
-    var behavior = new NativeBehavior();
-    var returnType = type.returnType;
-    // Note: For dart:html and other internal libraries we maintain, we can
-    // trust the return type and use it to limit what we enqueue. We have to
-    // be more conservative about JS interop types and assume they can return
-    // anything (unless the user provides the experimental flag to trust the
-    // type of js-interop APIs). We do restrict the allocation effects and say
-    // that interop calls create only interop types (which may be unsound if
-    // an interop call returns a DOM type and declares a dynamic return type,
-    // but otherwise we would include a lot of code by default).
-    // TODO(sigmund,sra): consider doing something better for numeric types.
-    behavior.typesReturned.add(
-        !isJsInterop || compiler.options.trustJSInteropTypeAnnotations
-            ? returnType
-            : const ResolutionDynamicType());
-    if (!type.returnType.isVoid) {
-      // Declared types are nullable.
-      behavior.typesReturned.add(compiler.commonElements.nullType);
-    }
-    behavior._capture(type, compiler.resolution,
-        isInterop: isJsInterop, compiler: compiler);
-
-    for (ResolutionDartType type in type.optionalParameterTypes) {
-      behavior._escape(type, compiler.resolution);
-    }
-    for (ResolutionDartType type in type.namedParameterTypes) {
-      behavior._escape(type, compiler.resolution);
-    }
-
-    behavior._overrideWithAnnotations(
-        spannable, metadata, lookupType, compiler);
-    return behavior;
-  }
-
   static NativeBehavior ofFieldElementLoad(
       MemberElement element, Compiler compiler) {
     Resolution resolution = compiler.resolution;
@@ -826,68 +789,72 @@ class NativeBehavior {
       return cls.thisType;
     }
 
-    return ofFieldLoad(element, type, metadata, lookup, compiler,
+    BehaviorComputer computer = new BehaviorComputerImpl(compiler);
+    return computer.ofFieldLoad(type, metadata, lookup,
         isJsInterop: compiler.backend.isJsInterop(element));
   }
 
-  static NativeBehavior ofFieldLoad(
-      Spannable spannable,
-      ResolutionDartType type,
-      List<ConstantExpression> metadata,
-      TypeLookup lookupType,
-      Compiler compiler,
-      {bool isJsInterop}) {
-    Resolution resolution = compiler.resolution;
-    var behavior = new NativeBehavior();
-    // TODO(sigmund,sra): consider doing something better for numeric types.
-    behavior.typesReturned.add(
-        !isJsInterop || compiler.options.trustJSInteropTypeAnnotations
-            ? type
-            : const ResolutionDynamicType());
-    // Declared types are nullable.
-    behavior.typesReturned.add(resolution.commonElements.nullType);
-    behavior._capture(type, resolution,
-        isInterop: isJsInterop, compiler: compiler);
-    behavior._overrideWithAnnotations(
-        spannable, metadata, lookupType, compiler);
-    return behavior;
-  }
-
   static NativeBehavior ofFieldElementStore(
-      MemberElement field, Resolution resolution) {
-    ResolutionDartType type = field.computeType(resolution);
-    return ofFieldStore(type, resolution);
+      MemberElement field, Compiler compiler) {
+    BehaviorComputer computer = new BehaviorComputerImpl(compiler);
+    ResolutionDartType type = field.computeType(compiler.resolution);
+    return computer.ofFieldStore(type);
   }
 
-  static NativeBehavior ofFieldStore(
-      ResolutionDartType type, Resolution resolution) {
-    var behavior = new NativeBehavior();
-    behavior._escape(type, resolution);
-    // We don't override the default behaviour - the annotations apply to
-    // loading the field.
-    return behavior;
+  static dynamic /*DartType|SpecialType*/ _parseType(String typeString,
+      Spannable spannable, DiagnosticReporter reporter, TypeLookup lookupType) {
+    if (typeString == '=Object') return SpecialType.JsObject;
+    if (typeString == 'dynamic') {
+      return const ResolutionDynamicType();
+    }
+    var type = lookupType(typeString);
+    if (type != null) return type;
+
+    int index = typeString.indexOf('<');
+    if (index < 1) {
+      reporter.reportErrorMessage(spannable, MessageKind.GENERIC,
+          {'text': "Type '$typeString' not found."});
+      return const ResolutionDynamicType();
+    }
+    type = lookupType(typeString.substring(0, index));
+    if (type != null) {
+      // TODO(sra): Parse type parameters.
+      return type;
+    }
+    reporter.reportErrorMessage(spannable, MessageKind.GENERIC,
+        {'text': "Type '$typeString' not found."});
+    return const ResolutionDynamicType();
   }
+}
+
+abstract class BehaviorComputer {

[Siggi Cherem (dart-lang), 2017/02/08 17:00:28]

nit: rename as BehaviorBuilder or BehaviorFactory

(I prefer builder)

[Johnni Winther, 2017/02/10 09:11:45]

Done.

+  CommonElements get commonElements;
+  BackendClasses get backendClasses;
+  BackendHelpers get helpers;
+  DiagnosticReporter get reporter;
+  ConstantEnvironment get constants;
+  bool get trustJSInteropTypeAnnotations;
+
+  Resolution get resolution => null;
+
+  NativeBehavior _behavior;
 
   void _overrideWithAnnotations(
-      Spannable spannable,
-      Iterable<ConstantExpression> metadata,
-      TypeLookup lookupType,
-      Compiler compiler) {
+      Iterable<ConstantExpression> metadata, TypeLookup lookupType) {
     if (metadata.isEmpty) return;
 
-    NativeEnqueuer enqueuer = compiler.enqueuer.resolution.nativeEnqueuer;
-    var creates = _collect(spannable, metadata, compiler,
-        enqueuer.annotationCreatesClass, lookupType);
-    var returns = _collect(spannable, metadata, compiler,
-        enqueuer.annotationReturnsClass, lookupType);
+    List creates =
+        _collect(metadata, helpers.annotationCreatesClass, lookupType);
+    List returns =
+        _collect(metadata, helpers.annotationReturnsClass, lookupType);
 
     if (creates != null) {
-      typesInstantiated
+      _behavior.typesInstantiated
         ..clear()
         ..addAll(creates);
     }
     if (returns != null) {
-      typesReturned
+      _behavior.typesReturned
         ..clear()
         ..addAll(returns);
     }
@@ -898,12 +865,11 @@ class NativeBehavior {
    * [annotationClass].
    * Returns `null` if no constraints.
    */
-  static _collect(Spannable spannable, Iterable<ConstantExpression> metadata,
-      Compiler compiler, Element annotationClass, TypeLookup lookupType) {
-    DiagnosticReporter reporter = compiler.reporter;
+  List _collect(Iterable<ConstantExpression> metadata, Element annotationClass,
+      TypeLookup lookupType) {
     var types = null;
     for (ConstantExpression constant in metadata) {
-      ConstantValue value = compiler.constants.getConstantValue(constant);
+      ConstantValue value = constants.getConstantValue(constant);
       if (!value.isConstructedObject) continue;
       ConstructedConstantValue constructedObject = value;
       if (constructedObject.type.element != annotationClass) continue;
@@ -911,13 +877,14 @@ class NativeBehavior {
       Iterable<ConstantValue> fields = constructedObject.fields.values;
       // TODO(sra): Better validation of the constant.
       if (fields.length != 1 || !fields.single.isString) {
-        reporter.internalError(spannable,
+        reporter.internalError(CURRENT_ELEMENT_SPANNABLE,
             'Annotations needs one string: ${constant.toStructuredText()}');
       }
       StringConstantValue specStringConstant = fields.single;
       String specString = specStringConstant.toDartString().slowToString();
       for (final typeString in specString.split('|')) {
-        var type = _parseType(typeString, spannable, reporter, lookupType);
+        var type = NativeBehavior._parseType(
+            typeString, CURRENT_ELEMENT_SPANNABLE, reporter, lookupType);
         if (types == null) types = [];
         types.add(type);
       }
@@ -927,16 +894,18 @@ class NativeBehavior {
 
   /// Models the behavior of having intances of [type] escape from Dart code
   /// into native code.
-  void _escape(ResolutionDartType type, Resolution resolution) {
-    type.computeUnaliased(resolution);
+  void _escape(DartType type) {
+    if (type is ResolutionDartType) {
+      type.computeUnaliased(resolution);
+    }
     type = type.unaliased;
-    if (type is ResolutionFunctionType) {
-      ResolutionFunctionType functionType = type;
+    if (type is FunctionType) {
+      FunctionType functionType = type;
       // A function might be called from native code, passing us novel
       // parameters.
-      _escape(functionType.returnType, resolution);
-      for (ResolutionDartType parameter in functionType.parameterTypes) {
-        _capture(parameter, resolution);
+      _escape(functionType.returnType);
+      for (DartType parameter in functionType.parameterTypes) {
+        _capture(parameter);
       }
     }
   }
@@ -947,40 +916,40 @@ class NativeBehavior {
   ///
   /// We assume that JS-interop APIs cannot instantiate Dart types or
   /// non-JSInterop native types.
-  void _capture(ResolutionDartType type, Resolution resolution,
-      {bool isInterop: false, Compiler compiler}) {
-    type.computeUnaliased(resolution);
+  void _capture(DartType type, {bool isInterop: false}) {
+    if (type is ResolutionDartType) {
+      type.computeUnaliased(resolution);
+    }
     type = type.unaliased;
-    if (type is ResolutionFunctionType) {
-      ResolutionFunctionType functionType = type;
-      _capture(functionType.returnType, resolution,
-          isInterop: isInterop, compiler: compiler);
-      for (ResolutionDartType parameter in functionType.parameterTypes) {
-        _escape(parameter, resolution);
+    if (type is FunctionType) {
+      FunctionType functionType = type;
+      _capture(functionType.returnType, isInterop: isInterop);
+      for (DartType parameter in functionType.parameterTypes) {
+        _escape(parameter);
       }
     } else {
-      JavaScriptBackend backend = compiler?.backend;
       if (!isInterop) {
-        typesInstantiated.add(type);
+        _behavior.typesInstantiated.add(type);
       } else {
-        if (type.element != null && backend.isNative(type.element)) {
+        if (type is InterfaceType &&
+            backendClasses.isNativeClass(type.element)) {
           // Any declared native or interop type (isNative implies isJsInterop)
           // is assumed to be allocated.
-          typesInstantiated.add(type);
+          _behavior.typesInstantiated.add(type);
         }
 
-        if (!compiler.options.trustJSInteropTypeAnnotations ||
+        if (!trustJSInteropTypeAnnotations ||
             type.isDynamic ||
-            type.isObject) {
+            type == commonElements.objectType) {
           // By saying that only JS-interop types can be created, we prevent
           // pulling in every other native type (e.g. all of dart:html) when a
           // JS interop API returns dynamic or when we don't trust the type
           // annotations. This means that to some degree we still use the return
           // type to decide whether to include native types, even if we don't
           // trust the type annotation.
-          ClassElement cls = backend.helpers.jsJavaScriptObjectClass;
+          ClassElement cls = helpers.jsJavaScriptObjectClass;
           cls.ensureResolved(resolution);
-          typesInstantiated.add(cls.thisType);
+          _behavior.typesInstantiated.add(cls.thisType);
         } else {
           // Otherwise, when the declared type is a Dart type, we do not
           // register an allocation because we assume it cannot be instantiated
@@ -991,28 +960,91 @@ class NativeBehavior {
     }
   }
 
-  static dynamic /*DartType|SpecialType*/ _parseType(String typeString,
-      Spannable spannable, DiagnosticReporter reporter, TypeLookup lookupType) {
-    if (typeString == '=Object') return SpecialType.JsObject;
-    if (typeString == 'dynamic') {
-      return const ResolutionDynamicType();
+  NativeBehavior ofFieldLoad(DartType type,

[Siggi Cherem (dart-lang), 2017/02/08 17:00:28]

Let's rename these now that they are no longer on NativeBehavior (the
"ofFieldLoad" made sense when read in context like
"NativeBehavior.ofFieldLoad").

Suggestions:
 - buildFieldLoadBehavior
 - fieldLoadBehaviorOf

I prefer the former (builderFieldLoadBehavior) since it's not expilcit in the
arguments what is the field (what the "Of" is about). 

[Johnni Winther, 2017/02/10 09:11:45]

Done.

+      Iterable<ConstantExpression> metadata, TypeLookup lookupType,
+      {bool isJsInterop}) {
+    _behavior = new NativeBehavior();
+    // TODO(sigmund,sra): consider doing something better for numeric types.
+    _behavior.typesReturned.add(!isJsInterop || trustJSInteropTypeAnnotations
+        ? type
+        : commonElements.dynamicType);
+    // Declared types are nullable.
+    _behavior.typesReturned.add(commonElements.nullType);
+    _capture(type, isInterop: isJsInterop);
+    _overrideWithAnnotations(metadata, lookupType);
+    return _behavior;
+  }
+
+  NativeBehavior ofFieldStore(DartType type) {
+    _behavior = new NativeBehavior();
+    _escape(type);
+    // We don't override the default behaviour - the annotations apply to
+    // loading the field.
+    return _behavior;
+  }
+
+  NativeBehavior ofMethod(FunctionType type, List<ConstantExpression> metadata,
+      TypeLookup lookupType,
+      {bool isJsInterop}) {
+    _behavior = new NativeBehavior();
+    DartType returnType = type.returnType;
+    // Note: For dart:html and other internal libraries we maintain, we can
+    // trust the return type and use it to limit what we enqueue. We have to
+    // be more conservative about JS interop types and assume they can return
+    // anything (unless the user provides the experimental flag to trust the
+    // type of js-interop APIs). We do restrict the allocation effects and say
+    // that interop calls create only interop types (which may be unsound if
+    // an interop call returns a DOM type and declares a dynamic return type,
+    // but otherwise we would include a lot of code by default).
+    // TODO(sigmund,sra): consider doing something better for numeric types.
+    _behavior.typesReturned.add(!isJsInterop || trustJSInteropTypeAnnotations
+        ? returnType
+        : commonElements.dynamicType);
+    if (!type.returnType.isVoid) {
+      // Declared types are nullable.
+      _behavior.typesReturned.add(commonElements.nullType);
     }
-    var type = lookupType(typeString);
-    if (type != null) return type;
+    _capture(type, isInterop: isJsInterop);
 
-    int index = typeString.indexOf('<');
-    if (index < 1) {
-      reporter.reportErrorMessage(spannable, MessageKind.GENERIC,
-          {'text': "Type '$typeString' not found."});
-      return const ResolutionDynamicType();
+    for (DartType type in type.optionalParameterTypes) {
+      _escape(type);
     }
-    type = lookupType(typeString.substring(0, index));
-    if (type != null) {
-      // TODO(sra): Parse type parameters.
-      return type;
+    for (DartType type in type.namedParameterTypes) {
+      _escape(type);
     }
-    reporter.reportErrorMessage(spannable, MessageKind.GENERIC,
-        {'text': "Type '$typeString' not found."});
-    return const ResolutionDynamicType();
+
+    _overrideWithAnnotations(metadata, lookupType);
+    return _behavior;
   }
 }
+
+class BehaviorComputerImpl extends BehaviorComputer {

[Siggi Cherem (dart-lang), 2017/02/08 17:00:28]

nit: rename to ResolverBehaviorBuilder? (so the new one can be called
KernelBehaviorBuilder?)

[Johnni Winther, 2017/02/10 09:11:45]

Done.

+  final Compiler compiler;
+
+  BehaviorComputerImpl(this.compiler);
+
+  @override
+  CommonElements get commonElements => compiler.commonElements;
+
+  @override
+  bool get trustJSInteropTypeAnnotations =>
+      compiler.options.trustJSInteropTypeAnnotations;
+
+  @override
+  ConstantEnvironment get constants => compiler.constants;
+
+  @override
+  DiagnosticReporter get reporter => compiler.reporter;
+
+  @override
+  BackendHelpers get helpers {
+    JavaScriptBackend backend = compiler.backend;
+    return backend.helpers;
+  }
+
+  @override
+  BackendClasses get backendClasses => compiler.backend.backendClasses;
+
+  @override
+  Resolution get resolution => compiler.resolution;
+}