Kernel-ify buildIsNode logic and add generics type variable checking in buildIsNode.

pkg/compiler/lib/src/ssa/builder_kernel.dart

 // Copyright (c) 2016, 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.
 
 import 'package:kernel/ast.dart' as ir;
 
 import '../closure.dart';
 import '../common.dart';
 import '../common/codegen.dart' show CodegenRegistry, CodegenWorkItem;
 import '../common/names.dart';
@@ skipping 1924 matching lines @@
   }
 
   @override
   void visitIsExpression(ir.IsExpression isExpression) {
     isExpression.operand.accept(this);
     HInstruction expression = pop();
     push(buildIsNode(isExpression, isExpression.type, expression));
   }
 
   HInstruction buildIsNode(
-      ir.Node node, ir.DartType dart_type, HInstruction expression) {
-    // TODO(sra): Convert the type testing logic here to use ir.DartType.
-    DartType type = astAdapter.getDartType(dart_type);
-
-    type = localsHandler.substInContext(type).unaliased;

[Siggi Cherem (dart-lang), 2016/12/20 22:37:29]

Are the kernel type-variables substituted by the time you get the type here?

If the answer is yes, that's neat, we might be able to skip tracking of type
variables in the local handler in the future. In that case, it might be worth
adding a TODO in localHandler to come back to it.

If the answer is no, we might need to convert this logic in the new builder
using kernel substitution functions. (since some checks you do further down rely
on this substitution to be there).

For example, if you have:

  class B<Q> {}

  class A<T> {
      m<Y>() {
         (x is B<T>) // *1
         (x is B<Y>) // *2
      }
  }


we treat *2 as a raw type, but *1 not (since Y is treated as dynamic, so *2 is
like doing `x is B`)

-
-    if (type is MethodTypeVariableType) {
-      return graph.addConstantBool(true, closedWorld);
-    }
-
-    if (type is MalformedType) {
-      ErroneousElement element = type.element;
-      generateTypeError(node, element.message);
-      return new HIs.compound(type, expression, pop(), commonMasks.boolType);
-    }
-
-    if (type.isFunctionType) {
-      List arguments = <HInstruction>[buildFunctionType(type), expression];
-      _pushDynamicInvocation(node, commonMasks.boolType, arguments,
+      ir.Node node, ir.DartType type, HInstruction expression) {
+    if (type is ir.InvalidType) {
+      generateTypeError(node,
+          (astAdapter.getDartType(type).element as ErroneousElement).message);

[Siggi Cherem (dart-lang), 2016/12/20 22:37:30]

we call astAdapter.getDartType on all branches here and below, my inclination
would be to pull it out into a temporary variable that we define upfront at the
beginning of this function.

  DartType typeValue = astAdapter.getDartType(type);

I know it makes it look closer to what Stephen had before, though. But as long
as we don't use `typeValue` in the logic below, I think it will be clear that
his TODO no longer applies. Maybe we need a new TODO about getting rid of it
entirely at some point (that might require changing the ssa graph to refer to
kernel types rather than using our own DartType representation.)

[Emily Fortuna, 2016/12/22 19:22:12]

Done.

+      return new HIs.compound(astAdapter.getDartType(type), expression, pop(),
+          commonMasks.boolType);
+    } else if (type is ir.FunctionType) {

[Siggi Cherem (dart-lang), 2016/12/20 22:37:30]

style nit (here and below): I know this comes from the old ssa builder, but lets
take the opportunity to remove the `else-if` when we return from the previous
branch.

it is a bit more readable that way (a bit like it looked on the left before the
change):

if (type is ir.InvalidType) {
  ...
  return;
}


if (type is ir.FunctionType) {
  ...
  return;
}

[Emily Fortuna, 2016/12/22 19:22:12]

ah, I didn't know this was the preferred style. Fixed!

+      List arguments = [
+        buildFunctionType(astAdapter.getDartType(type)),
+        expression
+      ];
+      _pushDynamicInvocation(node, null, arguments,
           selector: new Selector.call(
-              new PrivateName('_isTest', astAdapter.jsHelperLibrary),
+              new PrivateName('_isTest', backend.helpers.jsHelperLibrary),
               CallStructure.ONE_ARG));
-      return new HIs.compound(type, expression, pop(), commonMasks.boolType);
-    }
-
-    if (type.isTypeVariable) {
-      HInstruction runtimeType =
-          typeBuilder.addTypeVariableReference(type, sourceElement);
+      return new HIs.compound(astAdapter.getDartType(type), expression, pop(),
+          commonMasks.boolType);
+    } else if (type is ir.TypeParameterType) {
+      HInstruction runtimeType = typeBuilder.addTypeVariableReference(
+          astAdapter.getDartType(type), sourceElement);
       _pushStaticInvocation(astAdapter.checkSubtypeOfRuntimeType,
           <HInstruction>[expression, runtimeType], commonMasks.boolType);
-      return new HIs.variable(type, expression, pop(), commonMasks.boolType);
+      return new HIs.variable(astAdapter.getDartType(type), expression, pop(),
+          commonMasks.boolType);
+    } else if (_isInterfaceWithNoRawTypes(type)) {
+      HInstruction representations =
+          typeBuilder.buildTypeArgumentRepresentations(
+              astAdapter.getDartType(type), sourceElement);
+      add(representations);
+      ClassElement element = astAdapter.getDartType(type).element;
+      js.Name operator = backend.namer.operatorIs(element);
+      HInstruction isFieldName =
+          graph.addConstantStringFromName(operator, closedWorld);
+      HInstruction asFieldName =
+          closedWorld.hasAnyStrictSubtype(element)
+              ? graph.addConstantStringFromName(
+                  backend.namer.substitutionName(element), closedWorld)
+              : graph.addConstantNull(closedWorld);
+      List<HInstruction> inputs = <HInstruction>[
+        expression,
+        isFieldName,
+        representations,
+        asFieldName
+      ];
+      _pushStaticInvocation(
+          astAdapter.checkSubtype, inputs, commonMasks.boolType);
+      return new HIs.compound(astAdapter.getDartType(type), expression, pop(),
+          commonMasks.boolType);
+    } else {
+      if (backend.hasDirectCheckFor(astAdapter.getDartType(type))) {
+        return new HIs.direct(
+            astAdapter.getDartType(type), expression, commonMasks.boolType);
+      }
+      // The interceptor is not always needed.  It is removed by optimization
+      // when the receiver type or tested type permit.
+      return new HIs.raw(astAdapter.getDartType(type), expression,
+          _interceptorFor(expression), commonMasks.boolType);
     }
+  }
 
-    // TODO(sra): Type with type parameters.
-
-    if (backend.hasDirectCheckFor(type)) {
-      return new HIs.direct(type, expression, commonMasks.boolType);
-    }
-
-    // The interceptor is not always needed.  It is removed by optimization
-    // when the receiver type or tested type permit.
-    HInterceptor interceptor = _interceptorFor(expression);
-    return new HIs.raw(type, expression, interceptor, commonMasks.boolType);
+  bool _isInterfaceWithNoRawTypes(ir.DartType type) {

[Siggi Cherem (dart-lang), 2016/12/20 22:37:30]

nit: we might need a different name here. One idea:
_isInterfaceWithNonDynamicTypeArgument?... it's quite the mouthful though :-(

[Emily Fortuna, 2016/12/22 19:22:12]

Done.

+    return type is ir.InterfaceType &&
+        (type as ir.InterfaceType)
+            .typeArguments
+            .any((ir.DartType typeArgType) => typeArgType is! ir.DynamicType);

[Siggi Cherem (dart-lang), 2016/12/20 22:37:30]

I believe we should also exclude a couple other cases that today we treat as
dynamic, namely:
  - malformed types (ir.InvalidType)
  - method type arguments (like the example *2 in my other comment)

[Emily Fortuna, 2016/12/22 19:22:12]

Done.

   }
 
   @override
   void visitThrow(ir.Throw throwNode) {
     _visitThrowExpression(throwNode.expression);
     if (isReachable) {
       push(new HThrowExpression(pop(), null));
       isReachable = false;
     }
   }
@@ skipping 281 matching lines @@
     kernelBuilder.open(exitBlock);
     enterBlock.setBlockFlow(
         new HTryBlockInformation(
             kernelBuilder.wrapStatementGraph(bodyGraph),
             exception,
             kernelBuilder.wrapStatementGraph(catchGraph),
             kernelBuilder.wrapStatementGraph(finallyGraph)),
         exitBlock);
   }
 }