[vm] Support multiple local variables with the same name in the same scope

When lowering patterns, front-end can add multiple distinct local
variables with the same name into the same local scope.
Previously, VM identified local variables in a local scope by name.
However, this no longer works with patterns.

With this change, local variables are now identified by pair (name,
kernel offset). Name is still taken into account as compiler can add
extra variables which do not correspond to kernel variables,
such as 'this'.

TEST=runtime/tests/vm/dart/regress_51091_test.dart
Fixes #51091
Issue #49755

Change-Id: I0263769cb31f3f8d9652f5d6534800510ac882fb
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/279650
Reviewed-by: Slava Egorov <[email protected]>
Commit-Queue: Alexander Markov <[email protected]>
Reviewed-by: Ryan Macnak <[email protected]>

Author: alexmarkov

runtime/tests/vm/dart/regress_51091_test.dart

@@ -0,0 +1,36 @@
+// Copyright (c) 2023, 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.
+
+// Regression test for https://github.com/dart-lang/sdk/issues/51091.
+// Verifies that compiler doesn't crash if there are two local
+// variables with the same name in the same local scope.
+
+// SharedOptions=--enable-experiment=patterns
+
+import 'package:expect/expect.dart';
+
+void test1() {
+  if (0 case var x) {
+    Expect.equals(0, x);
+  }
+  var x = 1;
+  Expect.equals(1, x);
+}
+
+void test2(int arg) {
+  switch(arg) {
+    case == 0 && var x:
+      Expect.equals(0, x);
+    case == 1 && var x:
+      Expect.equals(1, x);
+  }
+  var x = 1;
+  Expect.equals(1, x);
+}
+
+void main() {
+  test1();
+  test2(0);
+  test2(1);
+}

runtime/vm/compiler/backend/il_test_helper.h

@@ -291,10 +291,10 @@ class FlowGraphBuilderHelper {
   void AddVariable(const char* name,
                    const AbstractType& static_type,
                    CompileType* param_type = nullptr) {
-    LocalVariable* v =
-        new LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
-                          String::Handle(Symbols::New(Thread::Current(), name)),
-                          static_type, param_type);
+    LocalVariable* v = new LocalVariable(
+        TokenPosition::kNoSource, TokenPosition::kNoSource,
+        String::Handle(Symbols::New(Thread::Current(), name)), static_type,
+        LocalVariable::kNoKernelOffset, param_type);
     v->set_type_check_mode(LocalVariable::kTypeCheckedByCaller);
     flow_graph()->parsed_function().scope()->AddVariable(v);
   }

runtime/vm/compiler/compiler_state.cc

@@ -48,7 +48,8 @@ LocalVariable* CompilerState::GetDummyCapturedVariable(intptr_t context_id,
             Z, Symbols::NewFormatted(thread(), ":context_var%" Pd, index));
         LocalVariable* var = new (Z)
             LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
-                          name, dynamic_type, /*param_type=*/nullptr);
+                          name, dynamic_type, LocalVariable::kNoKernelOffset,
+                          /*param_type=*/nullptr);
         var->set_is_captured();
         var->set_index(VariableIndex(index));
         return var;

runtime/vm/compiler/frontend/kernel_to_il.cc

@@ -771,6 +771,7 @@ Fragment FlowGraphBuilder::ThrowNoSuchMethodError(const Function& target,
 LocalVariable* FlowGraphBuilder::LookupVariable(intptr_t kernel_offset) {
   LocalVariable* local = scopes_->locals.Lookup(kernel_offset);
   ASSERT(local != NULL);
+  ASSERT(local->kernel_offset() == kernel_offset);
   return local;
 }
 
@@ -1740,9 +1741,9 @@ static const LocalScope* MakeImplicitClosureScope(Zone* Z, const Class& klass) {
   // and not the signature type.
   Type& klass_type = Type::ZoneHandle(Z, klass.DeclarationType());
 
-  LocalVariable* receiver_variable = new (Z)
-      LocalVariable(TokenPosition::kNoSource, TokenPosition::kNoSource,
-                    Symbols::This(), klass_type, /*param_type=*/nullptr);
+  LocalVariable* receiver_variable = new (Z) LocalVariable(
+      TokenPosition::kNoSource, TokenPosition::kNoSource, Symbols::This(),
+      klass_type, LocalVariable::kNoKernelOffset, /*param_type=*/nullptr);
 
   receiver_variable->set_is_captured();
   //  receiver_variable->set_is_final();

runtime/vm/compiler/frontend/scope_builder.cc

@@ -278,7 +278,7 @@ ScopeBuildingResult* ScopeBuilder::BuildScopes() {
               TokenPosition::kNoSource, TokenPosition::kNoSource,
               Symbols::Value(),
               AbstractType::ZoneHandle(Z, function.ParameterTypeAt(pos)),
-              &parameter_type);
+              LocalVariable::kNoKernelOffset, &parameter_type);
         } else {
           result_->setter_value = MakeVariable(
               TokenPosition::kNoSource, TokenPosition::kNoSource,
@@ -577,12 +577,12 @@ void ScopeBuilder::VisitFunctionNode() {
   // Mark known chained futures such as _Future::timeout()'s _future.
   if (function.recognized_kind() == MethodRecognizer::kFutureTimeout &&
       depth_.function_ == 1) {
-    LocalVariable* future = scope_->LookupVariable(Symbols::_future(), true);
+    LocalVariable* future = scope_->LookupVariableByName(Symbols::_future());
     ASSERT(future != nullptr);
     future->set_is_chained_future();
   } else if (function.recognized_kind() == MethodRecognizer::kFutureWait &&
              depth_.function_ == 1) {
-    LocalVariable* future = scope_->LookupVariable(Symbols::_future(), true);
+    LocalVariable* future = scope_->LookupVariableByName(Symbols::_future());
     ASSERT(future != nullptr);
     future->set_is_chained_future();
   }
@@ -1311,7 +1311,8 @@ void ScopeBuilder::VisitArguments() {
 void ScopeBuilder::VisitVariableDeclaration() {
   PositionScope scope(&helper_.reader_);
 
-  intptr_t kernel_offset_no_tag = helper_.ReaderOffset();
+  const intptr_t kernel_offset =
+      helper_.data_program_offset_ + helper_.ReaderOffset();
   VariableDeclarationHelper helper(&helper_);
   helper.ReadUntilExcluding(VariableDeclarationHelper::kType);
   AbstractType& type = BuildAndVisitVariableType();
@@ -1336,7 +1337,7 @@ void ScopeBuilder::VisitVariableDeclaration() {
     end_position = end_position.Next();
   }
   LocalVariable* variable =
-      MakeVariable(helper.position_, end_position, name, type);
+      MakeVariable(helper.position_, end_position, name, type, kernel_offset);
   if (helper.IsFinal()) {
     variable->set_is_final();
   }
@@ -1346,8 +1347,7 @@ void ScopeBuilder::VisitVariableDeclaration() {
   }
 
   scope_->AddVariable(variable);
-  result_->locals.Insert(helper_.data_program_offset_ + kernel_offset_no_tag,
-                         variable);
+  result_->locals.Insert(kernel_offset, variable);
 }
 
 AbstractType& ScopeBuilder::BuildAndVisitVariableType() {
@@ -1486,7 +1486,8 @@ void ScopeBuilder::VisitTypeParameterType() {
       // The type argument vector is passed as the very first argument to the
       // factory constructor function.
       HandleSpecialLoad(&result_->type_arguments_variable,
-                        Symbols::TypeArgumentsParameter());
+                        Symbols::TypeArgumentsParameter(),
+                        LocalVariable::kNoKernelOffset);
     } else {
       // If the type parameter is a parameter to this or an enclosing function,
       // we can read it directly from the function type arguments vector later.
@@ -1608,9 +1609,12 @@ void ScopeBuilder::AddVariableDeclarationParameter(
     intptr_t pos,
     ParameterTypeCheckMode type_check_mode,
     const ProcedureAttributesMetadata& attrs) {
-  intptr_t kernel_offset = helper_.ReaderOffset();  // no tag.
+  // Convert kernel offset of variable declaration to absolute.
+  const intptr_t kernel_offset =
+      helper_.data_program_offset_ + helper_.ReaderOffset();
+  // MetadataHelper expects relative offsets and adjusts them internally
   const InferredTypeMetadata parameter_type =
-      inferred_type_metadata_helper_.GetInferredType(kernel_offset);
+      inferred_type_metadata_helper_.GetInferredType(helper_.ReaderOffset());
   VariableDeclarationHelper helper(&helper_);
   helper.ReadUntilExcluding(VariableDeclarationHelper::kType);
   String& name = H.DartSymbolObfuscate(helper.name_index_);
@@ -1619,8 +1623,9 @@ void ScopeBuilder::AddVariableDeclarationParameter(
   helper.SetJustRead(VariableDeclarationHelper::kType);
   helper.ReadUntilExcluding(VariableDeclarationHelper::kInitializer);
 
-  LocalVariable* variable = MakeVariable(helper.position_, helper.position_,
-                                         name, type, &parameter_type);
+  LocalVariable* variable =
+      MakeVariable(helper.position_, helper.position_, name, type,
+                   kernel_offset, &parameter_type);
   if (helper.IsFinal()) {
     variable->set_is_final();
   }
@@ -1681,8 +1686,7 @@ void ScopeBuilder::AddVariableDeclarationParameter(
   }
 
   scope_->InsertParameterAt(pos, variable);
-  result_->locals.Insert(helper_.data_program_offset_ + kernel_offset,
-                         variable);
+  result_->locals.Insert(kernel_offset, variable);
 
   // The default value may contain 'let' bindings for which the constant
   // evaluator needs scope bindings.
@@ -1697,6 +1701,7 @@ LocalVariable* ScopeBuilder::MakeVariable(
     TokenPosition token_pos,
     const String& name,
     const AbstractType& type,
+    intptr_t kernel_offset,
     const InferredTypeMetadata* param_type_md /* = NULL */) {
   CompileType* param_type = nullptr;
   const Object* param_value = nullptr;
@@ -1707,7 +1712,7 @@ LocalVariable* ScopeBuilder::MakeVariable(
     }
   }
   return new (Z) LocalVariable(declaration_pos, token_pos, name, type,
-                               param_type, param_value);
+                               kernel_offset, param_type, param_value);
 }
 
 void ScopeBuilder::AddExceptionVariable(
@@ -1823,7 +1828,7 @@ void ScopeBuilder::VisitVariableGet(intptr_t declaration_binary_offset) {
 LocalVariable* ScopeBuilder::LookupVariable(
     intptr_t declaration_binary_offset) {
   LocalVariable* variable = result_->locals.Lookup(declaration_binary_offset);
-  if (variable == NULL) {
+  if (variable == nullptr) {
     // We have not seen a declaration of the variable, so it must be the
     // case that we are compiling a nested function and the variable is
     // declared in an outer scope.  In that case, look it up in the scope by
@@ -1834,11 +1839,13 @@ LocalVariable* ScopeBuilder::LookupVariable(
         parsed_function_->function());
 
     const String& name = H.DartSymbolObfuscate(var_name);
-    variable = current_function_scope_->parent()->LookupVariable(name, true);
-    ASSERT(variable != NULL);
+    variable = current_function_scope_->parent()->LookupVariable(
+        name, declaration_binary_offset, true);
+    ASSERT(variable != nullptr);
     result_->locals.Insert(declaration_binary_offset, variable);
   }
 
+  ASSERT(variable->owner() != nullptr);
   if (variable->owner()->function_level() < scope_->function_level()) {
     // We call `LocalScope->CaptureVariable(variable)` in two scenarios for two
     // different reasons:
@@ -1885,8 +1892,8 @@ void ScopeBuilder::HandleLoadReceiver() {
       current_function_scope_->parent() != nullptr) {
     // Lazily populate receiver variable using the parent function scope.
     parsed_function_->set_receiver_var(
-        current_function_scope_->parent()->LookupVariable(Symbols::This(),
-                                                          true));
+        current_function_scope_->parent()->LookupVariable(
+            Symbols::This(), LocalVariable::kNoKernelOffset, true));
   }
 
   if ((current_function_scope_->parent() != nullptr) ||
@@ -1899,13 +1906,14 @@ void ScopeBuilder::HandleLoadReceiver() {
 }
 
 void ScopeBuilder::HandleSpecialLoad(LocalVariable** variable,
-                                     const String& symbol) {
+                                     const String& symbol,
+                                     intptr_t kernel_offset) {
   if (current_function_scope_->parent() != NULL) {
     // We are building the scope tree of a closure function and saw [node]. We
     // lazily populate the variable using the parent function scope.
     if (*variable == NULL) {
-      *variable =
-          current_function_scope_->parent()->LookupVariable(symbol, true);
+      *variable = current_function_scope_->parent()->LookupVariable(
+          symbol, kernel_offset, true);
       ASSERT(*variable != NULL);
     }
   }
@@ -1919,14 +1927,5 @@ void ScopeBuilder::HandleSpecialLoad(LocalVariable** variable,
   }
 }
 
-void ScopeBuilder::LookupCapturedVariableByName(LocalVariable** variable,
-                                                const String& name) {
-  if (*variable == NULL) {
-    *variable = scope_->LookupVariable(name, true);
-    ASSERT(*variable != NULL);
-    scope_->CaptureVariable(*variable);
-  }
-}
-
 }  // namespace kernel
 }  // namespace dart

runtime/vm/compiler/frontend/scope_builder.h

@@ -98,11 +98,13 @@ class ScopeBuilder {
       ParameterTypeCheckMode type_check_mode,
       const ProcedureAttributesMetadata& attrs);
 
-  LocalVariable* MakeVariable(TokenPosition declaration_pos,
-                              TokenPosition token_pos,
-                              const String& name,
-                              const AbstractType& type,
-                              const InferredTypeMetadata* param_type_md = NULL);
+  LocalVariable* MakeVariable(
+      TokenPosition declaration_pos,
+      TokenPosition token_pos,
+      const String& name,
+      const AbstractType& type,
+      intptr_t kernel_offset = LocalVariable::kNoKernelOffset,
+      const InferredTypeMetadata* param_type_md = NULL);
 
   void AddExceptionVariable(GrowableArray<LocalVariable*>* variables,
                             const char* prefix,
@@ -130,9 +132,9 @@ class ScopeBuilder {
   const String& GenerateName(const char* prefix, intptr_t suffix);
 
   void HandleLoadReceiver();
-  void HandleSpecialLoad(LocalVariable** variable, const String& symbol);
-  void LookupCapturedVariableByName(LocalVariable** variable,
-                                    const String& name);
+  void HandleSpecialLoad(LocalVariable** variable,
+                         const String& symbol,
+                         intptr_t kernel_offset);
 
   struct DepthState {
     explicit DepthState(intptr_t function)