Generate better code for '=='.

pkg/dev_compiler/lib/src/compiler/code_generator.dart

Index: pkg/dev_compiler/lib/src/compiler/code_generator.dart
diff --git a/pkg/dev_compiler/lib/src/compiler/code_generator.dart b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
index 9b1c72fe7718123745657f34ee42af5a0433cf74..1bc01925d19b05ed4a4c9dbab07f810a3f9d9e9a 100644
--- a/pkg/dev_compiler/lib/src/compiler/code_generator.dart
+++ b/pkg/dev_compiler/lib/src/compiler/code_generator.dart
@@ -46,6 +46,7 @@ import 'js_interop.dart';
 import 'js_metalet.dart' as JS;
 import 'js_names.dart' as JS;
 import 'js_typeref_codegen.dart' show JsTypeRefCodegen;
+import 'js_typerep.dart' show JSTypeRep;
 import 'module_builder.dart' show pathToJSIdentifier;
 import 'nullable_type_inference.dart' show NullableTypeInference;
 import 'property_model.dart';
@@ -57,7 +58,7 @@ import 'type_utilities.dart';
 ///
 /// Takes as input resolved Dart ASTs for every compilation unit in every
 /// library in the module. Produces a single JavaScript AST for the module as
-/// output, along with its source map.
+// output, along with its source map.
 ///
 /// This class attempts to preserve identifier names and structure of the input
 /// Dart code, whenever this is possible to do in the generated code.
@@ -75,6 +76,7 @@ class CodeGenerator extends Object
 
   final CompilerOptions options;
   final StrongTypeSystemImpl rules;
+  JSTypeRep typeRep;
 
   /// The set of libraries we are currently compiling, and the temporaries used
   /// to refer to them.
@@ -210,7 +212,9 @@ class CodeGenerator extends Object
         functionClass = _getLibrary(c, 'dart:core').getType('Function'),
         privateSymbolClass =
             _getLibrary(c, 'dart:_internal').getType('PrivateSymbol'),
-        dartJSLibrary = _getLibrary(c, 'dart:js');
+        dartJSLibrary = _getLibrary(c, 'dart:js') {
+    typeRep = new JSTypeRep(rules, types);
+  }
 
   Element get currentElement => _currentElements.last;
 
@@ -698,7 +702,7 @@ class CodeGenerator extends Object
     if (rules.isSubtypeOf(from, to)) return jsFrom;
 
     // All Dart number types map to a JS double.
-    if (_isNumberInJS(from) && _isNumberInJS(to)) {
+    if (typeRep.isNumber(from) && typeRep.isNumber(to)) {
       // Make sure to check when converting to int.
       if (from != types.intType && to == types.intType) {
         // TODO(jmesserly): fuse this with notNull check.
@@ -746,7 +750,7 @@ class CodeGenerator extends Object
   }
 
   String _jsTypeofName(DartType t) {
-    if (_isNumberInJS(t)) return 'number';
+    if (typeRep.isNumber(t)) return 'number';
     if (t == types.stringType) return 'string';
     if (t == types.boolType) return 'boolean';
     return null;
@@ -3519,6 +3523,50 @@ class CodeGenerator extends Object
     }
   }
 
+  bool _doubleEqIsIdentity(Expression left, Expression right, DartType leftType,
+      DartType rightType) {

[Jennifer Messerly, 2017/06/26 22:42:58]

personally I'd refactor this: remove leftType and rightType, use left.staticType
and right.staticType.

Less parameters simplifies things :)

I'm 99% sure getStaticType is an old obsolete workaround for an analyzer bug.
I'll see if I can get a CL to remove it. worst case, feel free to call
`getStaticType(left)` and `getStaticType(right)` below

Also: do we need to canonicalize the numeric type?

[Leaf, 2017/06/30 21:55:32]

Done.
No, handled by equalityMayConvert. 

+    // If we statically know LHS or RHS is null we can use ==.
+    if (_isNull(left) || _isNull(right)) return true;
+    // If the representation of the  two types will not induce conversion in
+    // JS then we can use == .
+    if (!typeRep.equalityMayConvert(leftType, rightType)) return true;

[Jennifer Messerly, 2017/06/26 22:42:57]

this can be simplified too:

return !typeRep.equalityMayConvert(leftType, rightType);

[Leaf, 2017/06/30 21:55:31]

Doh.  How embarrassing, I hate code like this.  

+    return false;
+  }
+
+  bool _tripleEqIsIdentity(Expression left, Expression right, DartType leftType,
+      DartType rightType) {

[Jennifer Messerly, 2017/06/26 22:42:57]

leftType and rightType are unused

[Leaf, 2017/06/30 21:55:32]

Done.

+    // If either is non-nullable, then we don't need to worry about
+    // equating null and undefined, and so we can use triple equals.
+    if (!isNullable(left) || !isNullable(right)) return true;

[Jennifer Messerly, 2017/06/26 22:42:58]

return !isNullable(left) || !isNullable(right);

[Leaf, 2017/06/30 21:55:32]

Aargh!  Strong No Hire! 

+    return false;
+  }
+
+  bool _isCoreIdentical(Expression function) {
+    if (function is Identifier) {

[Jennifer Messerly, 2017/06/26 22:42:57]

based on suggestion above, this can be just:

    return function is Identifier && function.staticElement == _coreIdentical;

(I'd be tempted to name the parameter "node" to save a few chars)

[Leaf, 2017/06/30 21:55:31]

Done.

+      var element = function.staticElement;
+      var name = element?.name;
+      var inCore = element?.library?.isDartCore;
+      return name == "identical" && inCore == true;
+    }
+    return false;
+  }
+
+  JS.Expression _emitCoreIdenticalCall(
+      InvocationExpression node, JS.Expression fn, List<JS.Expression> args) {

[Jennifer Messerly, 2017/06/26 22:42:58]

based on suggestions below, the "fn" parameter can be removed

[Leaf, 2017/06/30 21:55:32]

Done.

+    if (args.length != 2) return new JS.Call(fn, args);

[Jennifer Messerly, 2017/06/26 22:42:58]

this can only come up with unsafe flags. I think we could just emit a throw like
we do now in other locations:

  return _callHelper(
    'throw(Error("compile error: calls to `identical` require 2 args")');

[Leaf, 2017/06/30 21:55:31]

Done.

+    var left = node.argumentList.arguments[0];
+    var right = node.argumentList.arguments[1];
+    var leftType = getStaticType(left);
+    var rightType = getStaticType(right);

[Jennifer Messerly, 2017/06/26 22:42:58]

these two lines will go away based on suggestions above

[Leaf, 2017/06/30 21:55:32]

Done.

+    if (_tripleEqIsIdentity(left, right, leftType, rightType)) {
+      return js.call('# === #', args);

[Jennifer Messerly, 2017/06/26 22:42:58]

should we also optimize `!identical(left, right)` ?

(discussed further below in the equality case)

[Leaf, 2017/06/30 21:55:31]

It's a bit awkward to code up (at first glance, I'd have to put a peephole
optimization in _visitTest to see if the thing being negated is core.identical),
and I'd be very surprised if it made any noticeable performance difference.  V8
should be able to do this optimization itself if it cares.  

I guess maybe generating a core.notIdentical call could be a win, but overall
I'm inclined to leave it for now.  Left a TODO.

+    }
+    if (_doubleEqIsIdentity(left, right, leftType, rightType)) {
+      return js.call('# == #', args);
+    }
+    return new JS.Call(fn, args);

[Jennifer Messerly, 2017/06/26 22:42:58]

once we have _coreIdentical, it's possible to just:

return new JS.Call(_emitTopLevelName(_coreIdentical), args);

[Leaf, 2017/06/30 21:55:31]

Done.

+  }
+
   /// Emits a function call, to a top-level function, local function, or
   /// an expression.
   JS.Expression _emitFunctionCall(InvocationExpression node,
@@ -3530,6 +3578,8 @@ class CodeGenerator extends Object
     var args = _emitArgumentList(node.argumentList);
     if (isDynamicInvoke(function)) {
       return _emitDynamicInvoke(node, fn, args);
+    } else if (_isCoreIdentical(function)) {
+      return _emitCoreIdenticalCall(node, fn, args);
     } else {
       return new JS.Call(_applyInvokeTypeArguments(fn, node), args);
     }
@@ -4079,20 +4129,7 @@ class CodeGenerator extends Object
         element, type, name, node.argumentList, node.isConst);
   }
 
-  /// True if this type is built-in to JS, and we use the values unwrapped.
-  /// For these types we generate a calling convention via static
-  /// "extension methods". This allows types to be extended without adding
-  /// extensions directly on the prototype.
-  bool isPrimitiveType(DartType t) =>
-      typeIsPrimitiveInJS(t) || t == types.stringType;
-
-  bool typeIsPrimitiveInJS(DartType t) =>
-      _isNumberInJS(t) || t == types.boolType;
-
-  bool binaryOperationIsPrimitive(DartType leftT, DartType rightT) =>
-      typeIsPrimitiveInJS(leftT) && typeIsPrimitiveInJS(rightT);
-
-  bool unaryOperationIsPrimitive(DartType t) => typeIsPrimitiveInJS(t);
+  bool isPrimitiveType(DartType t) => typeRep.isPrimitive(t);
 
   JS.Expression notNull(Expression expr) {
     if (expr == null) return null;
@@ -4101,6 +4138,75 @@ class CodeGenerator extends Object
     return _callHelper('notNull(#)', jsExpr);
   }
 
+  JS.Expression _emitEqualityOperator(BinaryExpression node, Token op) {
+    var left = node.leftOperand;
+    var right = node.rightOperand;
+
+    JS.Expression doOperator(String eq, String neq) {
+      var code = op.type == TokenType.EQ_EQ ? '# $eq #' : '# $neq #';
+      return js.call(code, [_visit(left), _visit(right)]);
+    }
+
+    JS.Expression doubleEq() => doOperator('==', '!=');
+    JS.Expression tripleEq() => doOperator('===', '!==');
+
+    // All number types are the same to us.
+    var leftType = typeRep.canonicalizeNumTypes(getStaticType(left));
+    var rightType = typeRep.canonicalizeNumTypes(getStaticType(right));
+
+    // If either is null, we can use simple equality.
+    // We need to equate null and undefined, so if both are nullable
+    //  (but not known to be null), we cannot directly use JS ==
+    //  unless we know that conversion will not happen.
+    // Functions may or may not have an [dartx[`==`]] method attached.
+    //   - If they are tearoffs they will, otherwise they won't and equality is
+    // identity.
+    //
+
+    // If we statically know LHS or RHS is null we can use ==.
+    if (_isNull(left) || _isNull(right)) return doubleEq();
+
+    if (left is SuperExpression) {
+      return _emitSend(left, op.lexeme, [right]);
+    }
+
+    // Equality on enums and primitives is identity.
+    // TODO(leafp): Walk the class hierarchy and check to see if == was
+    // overridden
+    var isEnum = leftType is InterfaceType && leftType.element.isEnum;
+    var usesIdentity = typeRep.isPrimitive(leftType) || isEnum;
+
+    // If we know that the left type uses identity for equality, we can
+    // sometimes emit better code.
+    if (usesIdentity) {
+      if (_tripleEqIsIdentity(left, right, leftType, rightType)) {

[Jennifer Messerly, 2017/06/26 22:42:57]

it would be really great if this could reuse _emitCoreIdentical.

I think it's possible to factor it so that works. It'll need negation support,
and removing the JS.Expression for "core.identical" (suggested above). But it'd
be nice to share the logic otherwise. You wouldn't necessarily need the separate
functions for _tripleEqIsIdentity/_doubleEqIsIdentity at that point (tho I think
they'd be fine to keep)

[Leaf, 2017/06/30 21:55:31]

Done.  It changes the generated code a little bit: we generate core.identical
for equality when we know that equality is identity, but we don't know that we
can use triple or double eq.  But that's probably reasonable.

+        return tripleEq();
+      }
+      if (_doubleEqIsIdentity(left, right, leftType, rightType)) {
+        return doubleEq();
+      }
+    }
+
+    var bang = op.type == TokenType.BANG_EQ ? '!' : '';
+
+    if (typeRep.objectMethodsOnPrototype(leftType)) {

[Jennifer Messerly, 2017/06/26 22:42:58]

It looks like you're trying to make sure there's an explicit "==" to call, and
that it's fast to call. That's an impl detail of CodeGenerator and of DDC's core
library/runtime.

Currently == is guaranteed for Dart class types (that aren't JS interop types).
We could put it on JS primitives but that would be slow, so we didn't. We could
add it to functions if we wanted.

// We can call == directly on non-null Dart classes that are
// not JS primitives or interop types.
if (leftType is InterfaceType &&
    !_isJSNative(leftType.element) &&
    typeRep.jsObject(leftType) && 
    !isNullable(left) {
  // ...
}


... the reason I'm putting this suggestion here, is that these details are
specific to `==`, rather than being something about JS Objects or even about all
Dart Object members. (so I think we can remove objectMethodsOnPrototype)

[Leaf, 2017/06/30 21:55:31]

Done.

+      // If left is not nullable, then we don't need to worry about
+      // null/undefined.
+      // TODO(leafp): consider using (left || dart.EQ)['=='](right))
+      // when left is nullable but not falsey
+      if (!isNullable(left)) {
+        var name = _emitMemberName('==', type: leftType);
+        var code = '${bang}#[#](#)';
+        return js.call(code, [_visit(left), name, _visit(right)]);
+      }
+    }
+    // Fall back to equality for now.
+    {

[Jennifer Messerly, 2017/06/26 22:42:58]

is this block needed?

[Leaf, 2017/06/30 21:55:32]

Probably not.  I prefer limiting the scope of local variables with blocks, but
that's probably idiosyncratic in Dart.  Removed.

+      var code = '${bang}#.equals(#, #)';
+      return js.call(code, [_runtimeModule, _visit(left), _visit(right)]);
+    }
+  }
+
   @override
   JS.Expression visitBinaryExpression(BinaryExpression node) {
     var op = node.operator;
@@ -4112,28 +4218,11 @@ class CodeGenerator extends Object
       return _visitTest(node);
     }
 
+    if (op.type.isEqualityOperator) return _emitEqualityOperator(node, op);
+
     var left = node.leftOperand;
     var right = node.rightOperand;
 
-    var leftType = getStaticType(left);
-    var rightType = getStaticType(right);
-
-    var code;
-    if (op.type.isEqualityOperator) {
-      // If we statically know LHS or RHS is null we can generate a clean check.
-      // We can also do this if both sides are the same primitive type.
-      if (_canUsePrimitiveEquality(left, right)) {
-        code = op.type == TokenType.EQ_EQ ? '# == #' : '# != #';
-      } else if (left is SuperExpression) {
-        return _emitSend(left, op.lexeme, [right]);
-      } else {
-        var bang = op.type == TokenType.BANG_EQ ? '!' : '';
-        code = '${bang}#.equals(#, #)';
-        return js.call(code, [_runtimeModule, _visit(left), _visit(right)]);
-      }
-      return js.call(code, [_visit(left), _visit(right)]);
-    }
-
     if (op.type.lexeme == '??') {
       // TODO(jmesserly): leave RHS for debugging?
       // This should be a hint or warning for dead code.
@@ -4147,7 +4236,10 @@ class CodeGenerator extends Object
       ]);
     }
 
-    if (binaryOperationIsPrimitive(leftType, rightType) ||
+    var leftType = getStaticType(left);
+    var rightType = getStaticType(right);
+
+    if (typeRep.binaryOperationIsPrimitive(leftType, rightType) ||
         leftType == types.stringType && op.type == TokenType.PLUS) {
       // special cases where we inline the operation
       // these values are assumed to be non-null (determined by the checker)
@@ -4382,24 +4474,8 @@ class CodeGenerator extends Object
     return bitWidth(expr, 0) < 32;
   }
 
-  /// If the type [t] is [int] or [double], or a type parameter
-  /// bounded by [int], [double] or [num] returns [num].
-  /// Otherwise returns [t].
-  DartType _canonicalizeNumTypes(DartType t) {
-    var numType = types.numType;
-    if (rules.isSubtypeOf(t, numType)) return numType;
-    return t;
-  }
-
-  bool _canUsePrimitiveEquality(Expression left, Expression right) {
-    if (_isNull(left) || _isNull(right)) return true;
-
-    var leftType = _canonicalizeNumTypes(getStaticType(left));
-    var rightType = _canonicalizeNumTypes(getStaticType(right));
-    return isPrimitiveType(leftType) && leftType == rightType;
-  }
-
-  bool _isNull(Expression expr) => expr is NullLiteral;
+  bool _isNull(Expression expr) =>
+      expr is NullLiteral || getStaticType(expr).isDartCoreNull;

[Jennifer Messerly, 2017/06/26 22:42:57]

FYI. We may need to also change our nullable analysis to know this.

[Leaf, 2017/06/30 21:55:31]

Acknowledged.

 
   SimpleIdentifier _createTemporary(String name, DartType type,
       {bool nullable: true, JS.Expression variable, bool dynamicInvoke}) {
@@ -4544,7 +4620,7 @@ class CodeGenerator extends Object
     var expr = node.operand;
 
     var dispatchType = getStaticType(expr);
-    if (unaryOperationIsPrimitive(dispatchType)) {
+    if (typeRep.unaryOperationIsPrimitive(dispatchType)) {
       if (!isNullable(expr)) {
         return js.call('#$op', _visit(expr));
       }
@@ -4581,9 +4657,9 @@ class CodeGenerator extends Object
     var expr = node.operand;
 
     var dispatchType = getStaticType(expr);
-    if (unaryOperationIsPrimitive(dispatchType)) {
+    if (typeRep.unaryOperationIsPrimitive(dispatchType)) {
       if (op.lexeme == '~') {
-        if (_isNumberInJS(dispatchType)) {
+        if (typeRep.isNumber(dispatchType)) {
           JS.Expression jsExpr = js.call('~#', notNull(expr));
           return _coerceBitOperationResultToUnsigned(node, jsExpr);
         }
@@ -5327,7 +5403,7 @@ class CodeGenerator extends Object
     }
     if (node is AsExpression && CoercionReifier.isImplicitCast(node)) {
       assert(node.staticType == types.boolType);
-      return _callHelper('test(#)', _visit(node.expression));
+      return _callHelper('dtest(#)', _visit(node.expression));
     }
     JS.Expression result = _visit(node);
     if (isNullable(node)) result = _callHelper('test(#)', result);
@@ -5530,16 +5606,6 @@ class CodeGenerator extends Object
     return node..sourceInformation = original;
   }
 
-  /// Returns true if this is any kind of object represented by `Number` in JS.
-  ///
-  /// In practice, this is 4 types: num, int, double, and JSNumber.
-  ///
-  /// JSNumber is the type that actually "implements" all numbers, hence it's
-  /// a subtype of int and double (and num). It's in our "dart:_interceptors".
-  bool _isNumberInJS(DartType t) =>
-      rules.isSubtypeOf(t, types.numType) &&
-      !rules.isSubtypeOf(t, types.nullType);
-
   /// Return true if this is one of the methods/properties on all Dart Objects
   /// (toString, hashCode, noSuchMethod, runtimeType).
   ///