[kernel] Change how TypeParameterType is calculated.

runtime/vm/kernel_binary_flowgraph.cc

Index: runtime/vm/kernel_binary_flowgraph.cc
diff --git a/runtime/vm/kernel_binary_flowgraph.cc b/runtime/vm/kernel_binary_flowgraph.cc
index 9d363ad04cf936b5cc9c4a0501305bf9387c1b04..a07c181e16663716c9d1e3ceea4aa2bf9686c692 100644
--- a/runtime/vm/kernel_binary_flowgraph.cc
+++ b/runtime/vm/kernel_binary_flowgraph.cc
@@ -67,37 +67,12 @@ ScopeBuildingResult* StreamingScopeBuilder::BuildScopes() {
   // e.g. for type translation.
   const dart::Class& klass =
       dart::Class::Handle(zone_, parsed_function_->function().Owner());
+
   Function& outermost_function = Function::Handle(Z);
-  intptr_t outermost_kernel_offset = -1;
-  intptr_t parent_class_offset = -1;
-  builder_->DiscoverEnclosingElements(Z, function, &outermost_function,
-                                      &outermost_kernel_offset,
-                                      &parent_class_offset);
-  // Use [klass]/[kernel_class] as active class.  Type parameters will get
-  // resolved via [kernel_class] unless we are nested inside a static factory
-  // in which case we will use [member].
-  intptr_t class_type_parameters = 0;
-  intptr_t class_type_parameters_offset_start = -1;
-  if (parent_class_offset > 0) {
-    builder_->GetTypeParameterInfoForClass(parent_class_offset,
-                                           &class_type_parameters,
-                                           &class_type_parameters_offset_start);
-  }
-  ActiveClassScope active_class_scope(&active_class_, class_type_parameters,
-                                      class_type_parameters_offset_start,
-                                      &klass);
-
-  bool member_is_procedure = false;
-  bool is_factory_procedure = false;
-  intptr_t member_type_parameters = 0;
-  intptr_t member_type_parameters_offset_start = -1;
-  builder_->GetTypeParameterInfoForPossibleProcedure(
-      outermost_kernel_offset, &member_is_procedure, &is_factory_procedure,
-      &member_type_parameters, &member_type_parameters_offset_start);
-
-  ActiveMemberScope active_member(&active_class_, member_is_procedure,
-                                  is_factory_procedure, member_type_parameters,
-                                  member_type_parameters_offset_start);
+  builder_->DiscoverEnclosingElements(Z, function, &outermost_function);
+
+  ActiveClassScope active_class_scope(&active_class_, &klass);
+  ActiveMemberScope active_member(&active_class_, &outermost_function);
 
   LocalScope* enclosing_scope = NULL;
   if (function.IsLocalFunction()) {
@@ -124,7 +99,6 @@ ScopeBuildingResult* StreamingScopeBuilder::BuildScopes() {
   parsed_function->SetNodeSequence(
       new SequenceNode(TokenPosition::kNoSource, scope_));
 
-  intptr_t parent_offset = -1;
   builder_->SetOffset(kernel_offset_);
 
   FunctionNodeHelper function_node_helper(builder_);
@@ -138,7 +112,7 @@ ScopeBuildingResult* StreamingScopeBuilder::BuildScopes() {
     case RawFunction::kSetterFunction:
     case RawFunction::kConstructor: {
       const Tag tag = builder_->PeekTag();
-      parent_offset = builder_->ReadUntilFunctionNode();
+      intptr_t parent_offset = builder_->ReadUntilFunctionNode();
       function_node_helper.ReadUntilExcluding(
           FunctionNodeHelper::kPositionalParameters);
       current_function_async_marker_ = function_node_helper.async_marker_;
@@ -638,7 +612,7 @@ void StreamingScopeBuilder::VisitExpression() {
     case kFunctionExpression: {
       intptr_t offset =
           builder_->ReaderOffset() - 1;  // -1 to include tag byte.
-      HandleLocalFunction(offset);
+      HandleLocalFunction(offset);       // read function node.
       return;
     }
     case kLet: {
@@ -1117,8 +1091,6 @@ void StreamingScopeBuilder::VisitTypeParameterType() {
   }
 
   builder_->ReadUInt();              // read index for parameter.
-  builder_->ReadUInt();              // read list binary offset.
-  builder_->ReadUInt();              // read index in list.
   builder_->SkipOptionalDartType();  // read bound bound.
 }
 
@@ -1594,63 +1566,73 @@ void StreamingDartTypeTranslator::BuildFunctionType(bool simple) {
 }
 
 void StreamingDartTypeTranslator::BuildTypeParameterType() {
-  builder_->ReadUInt();                           // read parameter index.
-  intptr_t binary_offset = builder_->ReadUInt();  // read lists binary offset.
-  intptr_t list_index = builder_->ReadUInt();     // read index in list.
-  builder_->SkipOptionalDartType();               // read bound.
+  intptr_t parameter_index = builder_->ReadUInt();  // read parameter index.
+  builder_->SkipOptionalDartType();                 // read bound.
 
-  ASSERT(binary_offset > 0);
+  TypeArguments& class_types =
+      dart::TypeArguments::Handle(Z, active_class_->klass->type_parameters());
+  if (class_types.Length() > parameter_index) {

[Kevin Millikin (Google), 2017/08/08 14:08:51]

It doesn't really matter but I find this and the comparisons below easier to
think about if they are flipped around (parameter_index < class_types.Length()),
which is the way I think of the comparison we need to make indexing into
class_types safe.

I suggest flipping them.

[jensj, 2017/08/09 08:39:46]

Done.

+    // The index of the type parameter in [parameters] is
+    // the same index into the `klass->type_parameters()` array.
+    result_ ^= class_types.TypeAt(parameter_index);
+    return;
+  }
+  parameter_index -= class_types.Length();
 
-  for (TypeParameterScope* scope = type_parameter_scope_; scope != NULL;
-       scope = scope->outer()) {
-    if (scope->parameters_offset() == binary_offset) {
-      result_ ^= dart::Type::DynamicType();
+  if (active_class_->MemberIsFactoryProcedure()) {

[Kevin Millikin (Google), 2017/08/08 14:08:51]

It's kind of hard to reason about it, but I think that MemberIsFactoryProcedure
and MemberIsProcedure cannot both be true.  If that is the case then the code is
clearer if it's written:

if (...MemberIsFactoryProcedure()) {
  ...
} else if (...MemberIsProcedure()) {
  ...
}

[jensj, 2017/08/09 08:39:47]

Actually, a FactoryProcedure is always a procedure:

  bool MemberIsProcedure() {
    if (member == NULL) return false;
    RawFunction::Kind function_kind = member->kind();
    return [...] ||
           member->IsFactory();
  }

  bool MemberIsFactoryProcedure() {
    if (member == NULL) return false;
    return member->IsFactory();
  }

+    //
+    // WARNING: This is a little hackish:
+    //
+    // We have a static factory constructor. The kernel IR gives the factory
+    // constructor function it's own type parameters (which are equal in name
+    // and number to the ones of the enclosing class).
+    // I.e.,
+    //
+    //   class A<T> {
+    //     factory A.x() { return new B<T>(); }
+    //   }
+    //
+    //  is basically translated to this:
+    //
+    //   class A<T> {
+    //     static A.x<T'>() { return new B<T'>(); }
+    //   }
+    //
+    if (class_types.Length() > parameter_index) {
+      result_ ^= class_types.TypeAt(parameter_index);
       return;
     }
+    parameter_index -= class_types.Length();
   }
 
-  if (active_class_->member_is_procedure) {
-    if (active_class_->member_type_parameters > 0) {
-      //
-      // WARNING: This is a little hackish:
-      //
-      // We have a static factory constructor. The kernel IR gives the factory
-      // constructor function it's own type parameters (which are equal in name
-      // and number to the ones of the enclosing class).
-      // I.e.,
-      //
-      //   class A<T> {
-      //     factory A.x() { return new B<T>(); }
-      //   }
-      //
-      //  is basically translated to this:
-      //
-      //   class A<T> {
-      //     static A.x<T'>() { return new B<T'>(); }
-      //   }
-      //
-      if (active_class_->member_type_parameters_offset_start == binary_offset) {
-        if (active_class_->member_is_factory_procedure) {
-          // The index of the type parameter in [parameters] is
-          // the same index into the `klass->type_parameters()` array.
-          result_ ^= dart::TypeArguments::Handle(
-                         Z, active_class_->klass->type_parameters())
-                         .TypeAt(list_index);
-        } else {
-          result_ ^= dart::Type::DynamicType();
-        }
-        return;
-      }
+  bool member_is_procedure = active_class_->MemberIsProcedure();

[Kevin Millikin (Google), 2017/08/08 14:08:51]

It seems weird to name it when there is only one use.

[jensj, 2017/08/09 08:39:47]

Fixed.

+  intptr_t procedure_type_parameter_count =
+      member_is_procedure ? active_class_->MemberTypeParameterCount(Z) : 0;
+  if (procedure_type_parameter_count > 0) {
+    if (procedure_type_parameter_count > parameter_index) {
+      // Here we technically could load the correct one via something like
+      //     result_ ^= dart::TypeArguments::Handle(
+      //                    Z, active_class_->member->type_parameters())
+      //                    .TypeAt(parameter_index);
+      // but that isn't currently supported elsewhere
+      // (FlowGraphBuilder::LoadFunctionTypeArguments()).
+      result_ ^= dart::Type::DynamicType();
+      return;
     }
+    parameter_index -= procedure_type_parameter_count;
   }
 
-  if (active_class_->class_type_parameters_offset_start == binary_offset) {
-    // The index of the type parameter in [parameters] is
-    // the same index into the `klass->type_parameters()` array.
-    result_ ^=
-        dart::TypeArguments::Handle(Z, active_class_->klass->type_parameters())
-            .TypeAt(list_index);
-    return;
+  for (TypeParameterScope* scope = type_parameter_scope_; scope != NULL;

[Kevin Millikin (Google), 2017/08/08 14:08:51]

This code just return DynamicType unless the binary is not well-formed.  It's
confusing to use the loop, just check that the index is in range of
scope->outer_parameters_count() + scope->parameters_count().  If it is return
DynamicType and if it's not longjmp out of here.

[jensj, 2017/08/09 08:39:47]

While I'm not quite sure what you mean by longjmp out of here, I have changed it
to remove the loop.

+       scope = scope->outer()) {
+    if (parameter_index - scope->summed_outer_parameters_count() >= 0) {
+      if (parameter_index - scope->summed_outer_parameters_count() >=
+          scope->parameters_count()) {
+        UNREACHABLE();
+      }
+      parameter_index -= scope->summed_outer_parameters_count();
+      result_ ^= dart::Type::DynamicType();
+      return;
+    }
   }
 
   UNREACHABLE();
@@ -2492,92 +2474,13 @@ void StreamingConstantEvaluator::CacheConstantValue(intptr_t kernel_offset,
 void StreamingFlowGraphBuilder::DiscoverEnclosingElements(
     Zone* zone,
     const Function& function,
-    Function* outermost_function,
-    intptr_t* outermost_kernel_offset,
-    intptr_t* parent_class_offset) {
+    Function* outermost_function) {
   // Find out if there is an enclosing kernel class (which will be used to
   // resolve type parameters).
   *outermost_function = function.raw();
   while (outermost_function->parent_function() != Object::null()) {
     *outermost_function = outermost_function->parent_function();
   }
-
-  if (outermost_function->kernel_offset() > 0) {
-    *outermost_kernel_offset = outermost_function->kernel_offset();
-    *parent_class_offset = GetParentOffset(*outermost_kernel_offset);
-  }
-}
-
-intptr_t StreamingFlowGraphBuilder::GetParentOffset(intptr_t offset) {
-  AlternativeReadingScope alt(reader_, offset);
-
-  Tag tag = PeekTag();
-  switch (tag) {
-    case kConstructor: {
-      ConstructorHelper constructor_helper(this);
-      constructor_helper.ReadUntilIncluding(
-          ConstructorHelper::kParentClassBinaryOffset);
-      return constructor_helper.parent_class_binary_offset_;
-    }
-    case kProcedure: {
-      ProcedureHelper procedure_helper(this);
-      procedure_helper.ReadUntilIncluding(
-          ProcedureHelper::kParentClassBinaryOffset);
-      return procedure_helper.parent_class_binary_offset_;
-    }
-    case kField: {
-      FieldHelper field_helper(this);
-      field_helper.ReadUntilIncluding(FieldHelper::kParentClassBinaryOffset);
-      return field_helper.parent_class_binary_offset_;
-    }
-    default:
-      UNIMPLEMENTED();
-      return -1;
-  }
-}
-
-void StreamingFlowGraphBuilder::GetTypeParameterInfoForClass(
-    intptr_t class_offset,
-    intptr_t* type_paremeter_counts,
-    intptr_t* type_paremeter_offset) {
-  AlternativeReadingScope alt(reader_, class_offset);
-
-  ClassHelper class_helper(this);
-  class_helper.ReadUntilExcluding(ClassHelper::kTypeParameters);
-  *type_paremeter_counts =
-      ReadListLength();  // read type_parameters list length.
-  *type_paremeter_offset = ReaderOffset();
-}
-
-void StreamingFlowGraphBuilder::GetTypeParameterInfoForPossibleProcedure(
-    intptr_t outermost_kernel_offset,
-    bool* member_is_procedure,
-    bool* is_factory_procedure,
-    intptr_t* member_type_parameters,
-    intptr_t* member_type_parameters_offset_start) {
-  if (outermost_kernel_offset >= 0) {
-    AlternativeReadingScope alt(reader_, outermost_kernel_offset);
-    Tag tag = PeekTag();
-    if (tag == kProcedure) {
-      *member_is_procedure = true;
-
-      ProcedureHelper procedure_helper(this);
-      procedure_helper.ReadUntilExcluding(ProcedureHelper::kFunction);
-      *is_factory_procedure = procedure_helper.kind_ == Procedure::kFactory;
-      if (ReadTag() == kSomething) {
-        FunctionNodeHelper function_node_helper(this);
-        function_node_helper.ReadUntilExcluding(
-            FunctionNodeHelper::kTypeParameters);
-
-        // read type_parameters list length.
-        intptr_t list_length = ReadListLength();
-        if (list_length > 0) {
-          *member_type_parameters = list_length;
-          *member_type_parameters_offset_start = ReaderOffset();
-        }
-      }
-    }
-  }
 }
 
 intptr_t StreamingFlowGraphBuilder::ReadUntilFunctionNode() {
@@ -3391,35 +3294,10 @@ FlowGraph* StreamingFlowGraphBuilder::BuildGraph(intptr_t kernel_offset) {
       dart::Class::Handle(zone_, parsed_function()->function().Owner());
 
   Function& outermost_function = Function::Handle(Z);
-  intptr_t outermost_kernel_offset = -1;
-  intptr_t parent_class_offset = -1;
-  DiscoverEnclosingElements(Z, function, &outermost_function,
-                            &outermost_kernel_offset, &parent_class_offset);
-  // Use [klass]/[kernel_class] as active class.  Type parameters will get
-  // resolved via [kernel_class] unless we are nested inside a static factory
-  // in which case we will use [member].
-  intptr_t class_type_parameters = 0;
-  intptr_t class_type_parameters_offset_start = -1;
-  if (parent_class_offset > 0) {
-    GetTypeParameterInfoForClass(parent_class_offset, &class_type_parameters,
-                                 &class_type_parameters_offset_start);
-  }
-
-  ActiveClassScope active_class_scope(active_class(), class_type_parameters,
-                                      class_type_parameters_offset_start,
-                                      &klass);
-
-  bool member_is_procedure = false;
-  bool is_factory_procedure = false;
-  intptr_t member_type_parameters = 0;
-  intptr_t member_type_parameters_offset_start = -1;
-  GetTypeParameterInfoForPossibleProcedure(
-      outermost_kernel_offset, &member_is_procedure, &is_factory_procedure,
-      &member_type_parameters, &member_type_parameters_offset_start);
-
-  ActiveMemberScope active_member(active_class(), member_is_procedure,
-                                  is_factory_procedure, member_type_parameters,
-                                  member_type_parameters_offset_start);
+  DiscoverEnclosingElements(Z, function, &outermost_function);
+
+  ActiveClassScope active_class_scope(active_class(), &klass);
+  ActiveMemberScope active_member(active_class(), &outermost_function);
 
   // The IR builder will create its own local variables and scopes, and it
   // will not need an AST.  The code generator will assume that there is a
@@ -3765,8 +3643,6 @@ void StreamingFlowGraphBuilder::SkipDartType() {
       return;
     case kTypeParameterType:
       ReadUInt();              // read index for parameter.
-      ReadUInt();              // read list binary offset.
-      ReadUInt();              // read index in list.
       SkipOptionalDartType();  // read bound bound.
       return;
     default: