[kernel] Change how TypeParameterType is calculated.

runtime/vm/kernel_binary_flowgraph.cc

 // 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.
 
 #include "vm/kernel_binary_flowgraph.h"
 
 #include "vm/compiler.h"
 #include "vm/longjump.h"
 #include "vm/object_store.h"
 
@@ skipping 49 matching lines @@
   ASSERT(scope_ == NULL && depth_.loop_ == 0 && depth_.function_ == 0);
   result_ = new (Z) ScopeBuildingResult();
 
   ParsedFunction* parsed_function = parsed_function_;
   const Function& function = parsed_function->function();
 
   // Setup a [ActiveClassScope] and a [ActiveMemberScope] which will be used
   // 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);
+  builder_->DiscoverEnclosingElements(Z, function, &outermost_function);
 
-  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);
+  ActiveClassScope active_class_scope(&active_class_, &klass);
+  ActiveMemberScope active_member(&active_class_, &outermost_function);
 
   LocalScope* enclosing_scope = NULL;
   if (function.IsLocalFunction()) {
     enclosing_scope = LocalScope::RestoreOuterScope(
         ContextScope::Handle(Z, function.context_scope()));
   }
   current_function_scope_ = scope_ = new (Z) LocalScope(enclosing_scope, 0, 0);
   scope_->set_begin_token_pos(function.token_pos());
   scope_->set_end_token_pos(function.end_token_pos());
 
   // Add function type arguments variable before current context variable.
   if (FLAG_reify_generic_functions && function.IsGeneric()) {
     LocalVariable* type_args_var = MakeVariable(
         TokenPosition::kNoSource, TokenPosition::kNoSource,
         Symbols::FunctionTypeArgumentsVar(), AbstractType::dynamic_type());
     scope_->AddVariable(type_args_var);
     parsed_function_->set_function_type_arguments(type_args_var);
   }
 
   LocalVariable* context_var = parsed_function->current_context_var();
   context_var->set_is_forced_stack();
   scope_->AddVariable(context_var);
 
   parsed_function->SetNodeSequence(
       new SequenceNode(TokenPosition::kNoSource, scope_));
 
-  intptr_t parent_offset = -1;
   builder_->SetOffset(kernel_offset_);
 
   FunctionNodeHelper function_node_helper(builder_);
 
   switch (function.kind()) {
     case RawFunction::kClosureFunction:
     case RawFunction::kImplicitClosureFunction:
     case RawFunction::kConvertedClosureFunction:
     case RawFunction::kRegularFunction:
     case RawFunction::kGetterFunction:
     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_;
       // NOTE: FunctionNode is read further below the if.
 
       intptr_t pos = 0;
       if (function.IsClosureFunction()) {
         LocalVariable* variable = MakeVariable(
             TokenPosition::kNoSource, TokenPosition::kNoSource,
             Symbols::ClosureParameter(), AbstractType::dynamic_type());
@@ skipping 479 matching lines @@
       intptr_t list_length = builder_->ReadListLength();  // read list length.
       for (intptr_t i = 0; i < list_length; ++i) {
         VisitExpression();  // read ith key.
         VisitExpression();  // read ith value.
       }
       return;
     }
     case kFunctionExpression: {
       intptr_t offset =
           builder_->ReaderOffset() - 1;  // -1 to include tag byte.
-      HandleLocalFunction(offset);
+      HandleLocalFunction(offset);       // read function node.
       return;
     }
     case kLet: {
       PositionScope scope(builder_->reader_);
       intptr_t offset =
           builder_->ReaderOffset() - 1;  // -1 to include tag byte.
 
       EnterScope(offset);
 
       VisitVariableDeclaration();  // read variable declaration.
@@ skipping 458 matching lines @@
     // factory constructor function.
     HandleSpecialLoad(&result_->type_arguments_variable,
                       Symbols::TypeArgumentsParameter());
   } else {
     // The type argument vector is stored on the instance object. We therefore
     // need to capture `this`.
     HandleSpecialLoad(&result_->this_variable, Symbols::This());
   }
 
   builder_->ReadUInt();              // read index for parameter.
-  builder_->ReadUInt();              // read list binary offset.
-  builder_->ReadUInt();              // read index in list.
   builder_->SkipOptionalDartType();  // read bound bound.
 }
 
 void StreamingScopeBuilder::HandleLocalFunction(intptr_t parent_kernel_offset) {
   // "Peek" ahead into the function node
   intptr_t offset = builder_->ReaderOffset();
 
   FunctionNodeHelper function_node_helper(builder_);
   function_node_helper.ReadUntilExcluding(
       FunctionNodeHelper::kPositionalParameters);
@@ skipping 455 matching lines @@
         ClassFinalizer::FinalizeType(*active_class_->klass, signature_type);
     // Do not refer to signature_function anymore, since it may have been
     // replaced during canonicalization.
     signature_function = Function::null();
   }
 
   result_ = signature_type.raw();
 }
 
 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();
+
+  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();
+  }
+
+  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;
+  }
 
   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 (scope->parameters_offset() == binary_offset) {
+    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;
     }
   }
 
-  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;
-      }
-    }
-  }
-
-  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;
-  }
-
   UNREACHABLE();
 }
 
 const TypeArguments& StreamingDartTypeTranslator::BuildTypeArguments(
     intptr_t length) {
   bool only_dynamic = true;
   intptr_t offset = builder_->ReaderOffset();
   for (intptr_t i = 0; i < length; ++i) {
     if (builder_->ReadTag() != kDynamicType) {  // Read the ith types tag.
       only_dynamic = false;
@@ skipping 819 matching lines @@
     script_.set_compile_time_constants(array);
   }
   KernelConstantsMap constants(script_.compile_time_constants());
   constants.InsertNewOrGetValue(kernel_offset, value);
   script_.set_compile_time_constants(constants.Release());
 }
 
 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() {
   const Tag tag = PeekTag();
   if (tag == kProcedure) {
     ProcedureHelper procedure_helper(this);
     procedure_helper.ReadUntilExcluding(ProcedureHelper::kFunction);
     if (ReadTag() == kNothing) {  // read function node tag.
       // Running a procedure without a function node doesn't make sense.
       UNREACHABLE();
@@ skipping 793 matching lines @@
 
 FlowGraph* StreamingFlowGraphBuilder::BuildGraph(intptr_t kernel_offset) {
   const Function& function = parsed_function()->function();
 
   // Setup a [ActiveClassScope] and a [ActiveMemberScope] which will be used
   // 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;
-  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);
-  }
+  DiscoverEnclosingElements(Z, function, &outermost_function);
 
-  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);
+  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
   // local variable stack slot allocated for the current context and (I
   // think) that the runtime will expect it to be at a fixed offset which
   // requires allocating an unused expression temporary variable.
   set_scopes(parsed_function()->EnsureKernelScopes());
 
   SetOffset(kernel_offset);
 
@@ skipping 325 matching lines @@
       SkipInterfaceType(true);
       return;
     case kFunctionType:
       SkipFunctionType(false);
       return;
     case kSimpleFunctionType:
       SkipFunctionType(true);
       return;
     case kTypeParameterType:
       ReadUInt();              // read index for parameter.
-      ReadUInt();              // read list binary offset.
-      ReadUInt();              // read index in list.
       SkipOptionalDartType();  // read bound bound.
       return;
     default:
       UNREACHABLE();
   }
 }
 
 void StreamingFlowGraphBuilder::SkipOptionalDartType() {
   Tag tag = ReadTag();  // read tag.
   if (tag == kNothing) {
@@ skipping 3512 matching lines @@
     }
   }
 
   return Array::Handle(Array::null());
 }
 
 }  // namespace kernel
 }  // namespace dart
 
 #endif  // !defined(DART_PRECOMPILED_RUNTIME)