From 38319b0f334dd07925a2cf9e445657e202ea34db Mon Sep 17 00:00:00 2001 From: "regis@google.com" Date: Wed, 21 May 2014 00:32:32 +0000 Subject: [PATCH] Fix issue 18435 (2nd attempt). Add regression test. R=hausner@google.com Review URL: https://codereview.chromium.org//293013005 git-svn-id: https://dart.googlecode.com/svn/branches/bleeding_edge/dart@36402 260f80e4-7a28-3924-810f-c04153c831b5 --- runtime/vm/ast_printer.cc | 46 +++++++++++++++----------- runtime/vm/ast_printer.h | 6 ++-- runtime/vm/code_generator_test.cc | 26 +++++++-------- runtime/vm/parser.cc | 8 ++--- runtime/vm/scopes.cc | 17 ++++++++++ runtime/vm/scopes.h | 5 +++ tests/language/regress_18435_test.dart | 24 ++++++++++++++ 7 files changed, 94 insertions(+), 38 deletions(-) create mode 100644 tests/language/regress_18435_test.dart diff --git a/runtime/vm/ast_printer.cc b/runtime/vm/ast_printer.cc index 9ac20a589cf..c1c957d0665 100644 --- a/runtime/vm/ast_printer.cc +++ b/runtime/vm/ast_printer.cc @@ -391,29 +391,37 @@ void AstPrinter::PrintNode(AstNode* node) { } +void AstPrinter::PrintLocalScopeVariable(const LocalScope* scope, + LocalVariable* var) { + ASSERT(scope != NULL); + ASSERT(var != NULL); + OS::Print("(%s%s '%s'", + var->is_final() ? "final " : "", + String::Handle(var->type().Name()).ToCString(), + var->name().ToCString()); + if (var->owner() != scope) { + OS::Print(" alias"); + } + if (var->HasIndex()) { + OS::Print(" @%d", var->index()); + if (var->is_captured()) { + OS::Print(" ctx %d", var->owner()->context_level()); + } + } else if (var->owner()->function_level() != 0) { + OS::Print(" lev %d", var->owner()->function_level()); + } + OS::Print(" valid %" Pd "-%" Pd ")", + var->token_pos(), + scope->end_token_pos()); +} + + void AstPrinter::PrintLocalScope(const LocalScope* scope, int start_index) { ASSERT(scope != NULL); for (int i = start_index; i < scope->num_variables(); i++) { LocalVariable* var = scope->VariableAt(i); - OS::Print("(%s%s '%s'", - var->is_final() ? "final " : "", - String::Handle(var->type().Name()).ToCString(), - var->name().ToCString()); - if (var->owner() != scope) { - OS::Print(" alias"); - } - if (var->HasIndex()) { - OS::Print(" @%d", var->index()); - if (var->is_captured()) { - OS::Print(" ctx %d", var->owner()->context_level()); - } - } else if (var->owner()->function_level() != 0) { - OS::Print(" lev %d", var->owner()->function_level()); - } - OS::Print(" valid %" Pd "-%" Pd ")", - var->token_pos(), - scope->end_token_pos()); + PrintLocalScopeVariable(scope, var); } const LocalScope* child = scope->child(); while (child != NULL) { @@ -455,7 +463,7 @@ void AstPrinter::PrintFunctionScope(const ParsedFunction& parsed_function) { int pos = 0; // Current position of variable in scope. while (pos < num_params) { LocalVariable* param = scope->VariableAt(pos); - ASSERT(param->owner() == scope); + ASSERT(param->owner() == scope); // No aliases should precede parameters. OS::Print("(param %s%s '%s'", param->is_final() ? "final " : "", String::Handle(param->type().Name()).ToCString(), diff --git a/runtime/vm/ast_printer.h b/runtime/vm/ast_printer.h index f64d9795802..8d5d31f1f90 100644 --- a/runtime/vm/ast_printer.h +++ b/runtime/vm/ast_printer.h @@ -18,8 +18,6 @@ class AstPrinter : public AstNodeVisitor { static void PrintNode(AstNode* node); static void PrintFunctionScope(const ParsedFunction& parsed_function); static void PrintFunctionNodes(const ParsedFunction& parsed_function); - static void PrintLocalScope(const LocalScope* scope, int variable_index); - #define DECLARE_VISITOR_FUNCTION(BaseName) \ virtual void Visit##BaseName##Node(BaseName##Node* node); @@ -31,6 +29,10 @@ class AstPrinter : public AstNodeVisitor { AstPrinter(); ~AstPrinter(); + static void PrintLocalScopeVariable(const LocalScope* scope, + LocalVariable* var); + static void PrintLocalScope(const LocalScope* scope, int variable_index); + void VisitGenericAstNode(AstNode* node); void VisitGenericLocalNode(AstNode* node, const LocalVariable& local); void VisitGenericFieldNode(AstNode* node, const Field& field); diff --git a/runtime/vm/code_generator_test.cc b/runtime/vm/code_generator_test.cc index b838224ba7e..89fcc20f8f7 100644 --- a/runtime/vm/code_generator_test.cc +++ b/runtime/vm/code_generator_test.cc @@ -58,7 +58,7 @@ CODEGEN_TEST_GENERATE(ReturnParameterCodegen, test) { const int num_params = 1; LocalVariable* parameter = NewTestLocalVariable("parameter"); LocalScope* local_scope = node_seq->scope(); - local_scope->AddVariable(parameter); + local_scope->InsertParameterAt(0, parameter); ASSERT(local_scope->num_variables() == num_params); const Function& function = test->function(); function.set_num_fixed_parameters(num_params); @@ -88,8 +88,8 @@ CODEGEN_TEST_GENERATE(SmiParamSumCodegen, test) { const int num_locals = 1; LocalVariable* sum = NewTestLocalVariable("sum"); LocalScope* local_scope = node_seq->scope(); - local_scope->AddVariable(param1); - local_scope->AddVariable(param2); + local_scope->InsertParameterAt(0, param1); + local_scope->InsertParameterAt(1, param2); local_scope->AddVariable(sum); ASSERT(local_scope->num_variables() == num_params + num_locals); const Function& function = test->function(); @@ -206,8 +206,8 @@ CODEGEN_TEST_GENERATE(NativeDecCodegen, test) { const int num_opt_params = 1; const int num_params = num_fixed_params + num_opt_params; LocalScope* local_scope = node_seq->scope(); - local_scope->AddVariable(NewTestLocalVariable("a")); - local_scope->AddVariable(NewTestLocalVariable("b")); + local_scope->InsertParameterAt(0, NewTestLocalVariable("a")); + local_scope->InsertParameterAt(1, NewTestLocalVariable("b")); ASSERT(local_scope->num_variables() == num_params); const Array& default_values = Array::ZoneHandle(Array::New(num_opt_params)); default_values.SetAt(0, Smi::ZoneHandle(Smi::New(1))); // b = 1. @@ -377,11 +377,11 @@ CODEGEN_TEST_GENERATE(NativeSumCodegen, test) { const int num_opt_params = 3; const int num_params = num_fixed_params + num_opt_params; LocalScope* local_scope = node_seq->scope(); - local_scope->AddVariable(NewTestLocalVariable("a")); - local_scope->AddVariable(NewTestLocalVariable("b")); - local_scope->AddVariable(NewTestLocalVariable("c")); - local_scope->AddVariable(NewTestLocalVariable("d")); - local_scope->AddVariable(NewTestLocalVariable("e")); + local_scope->InsertParameterAt(0, NewTestLocalVariable("a")); + local_scope->InsertParameterAt(1, NewTestLocalVariable("b")); + local_scope->InsertParameterAt(2, NewTestLocalVariable("c")); + local_scope->InsertParameterAt(3, NewTestLocalVariable("d")); + local_scope->InsertParameterAt(4, NewTestLocalVariable("e")); ASSERT(local_scope->num_variables() == num_params); const Array& default_values = Array::ZoneHandle(Array::New(num_opt_params)); default_values.SetAt(0, Smi::ZoneHandle(Smi::New(10))); @@ -474,9 +474,9 @@ CODEGEN_TEST_GENERATE(NativeNonNullSumCodegen, test) { SequenceNode* node_seq = test->node_sequence(); const int num_params = 3; LocalScope* local_scope = node_seq->scope(); - local_scope->AddVariable(NewTestLocalVariable("a")); - local_scope->AddVariable(NewTestLocalVariable("b")); - local_scope->AddVariable(NewTestLocalVariable("c")); + local_scope->InsertParameterAt(0, NewTestLocalVariable("a")); + local_scope->InsertParameterAt(1, NewTestLocalVariable("b")); + local_scope->InsertParameterAt(2, NewTestLocalVariable("c")); ASSERT(local_scope->num_variables() == num_params); const Function& function = test->function(); function.set_is_native(true); diff --git a/runtime/vm/parser.cc b/runtime/vm/parser.cc index 66018e5894c..4e6182335fe 100644 --- a/runtime/vm/parser.cc +++ b/runtime/vm/parser.cc @@ -2476,13 +2476,13 @@ SequenceNode* Parser::MakeImplicitConstructor(const Function& func) { LocalVariable* receiver = new LocalVariable( Scanner::kNoSourcePos, Symbols::This(), *ReceiverType(current_class())); - current_block_->scope->AddVariable(receiver); + current_block_->scope->InsertParameterAt(0, receiver); LocalVariable* phase_parameter = new LocalVariable(Scanner::kNoSourcePos, Symbols::PhaseParameter(), Type::ZoneHandle(Type::SmiType())); - current_block_->scope->AddVariable(phase_parameter); + current_block_->scope->InsertParameterAt(1, phase_parameter); // Parse expressions of instance fields that have an explicit // initializer expression. @@ -2523,7 +2523,7 @@ SequenceNode* Parser::MakeImplicitConstructor(const Function& func) { Scanner::kNoSourcePos, String::ZoneHandle(func.ParameterNameAt(i)), Type::ZoneHandle(Type::DynamicType())); - current_block_->scope->AddVariable(param); + current_block_->scope->InsertParameterAt(i, param); forwarding_args->Add(new LoadLocalNode(Scanner::kNoSourcePos, param)); } } @@ -5393,7 +5393,7 @@ void Parser::AddFormalParamsToScope(const ParamList* params, const String* name = param_desc.name; LocalVariable* parameter = new LocalVariable( param_desc.name_pos, *name, *param_desc.type); - if (!scope->AddVariable(parameter)) { + if (!scope->InsertParameterAt(i, parameter)) { ErrorMsg(param_desc.name_pos, "name '%s' already exists in scope", param_desc.name->ToCString()); diff --git a/runtime/vm/scopes.cc b/runtime/vm/scopes.cc index b6a6064610e..d9f2df16e61 100644 --- a/runtime/vm/scopes.cc +++ b/runtime/vm/scopes.cc @@ -66,6 +66,19 @@ bool LocalScope::AddVariable(LocalVariable* variable) { } +bool LocalScope::InsertParameterAt(intptr_t pos, LocalVariable* parameter) { + ASSERT(parameter != NULL); + if (LocalLookupVariable(parameter->name()) != NULL) { + return false; + } + variables_.InsertAt(pos, parameter); + // InsertParameterAt is not used to add aliases of parameters. + ASSERT(parameter->owner() == NULL); + parameter->set_owner(this); + return true; +} + + bool LocalScope::AddLabel(SourceLabel* label) { if (LocalLookupLabel(label->name()) != NULL) { return false; @@ -171,6 +184,10 @@ int LocalScope::AllocateVariables(int first_parameter_index, while (pos < num_parameters) { LocalVariable* parameter = VariableAt(pos); pos++; + // Parsing formal parameter default values may add local variable aliases + // to the local scope before the formal parameters are added. However, + // the parameters get inserted in front of the aliases, therefore, no + // aliases can be encountered among the first num_parameters variables. ASSERT(parameter->owner() == this); if (parameter->is_captured()) { // A captured parameter has a slot allocated in the frame and one in the diff --git a/runtime/vm/scopes.h b/runtime/vm/scopes.h index 6bd93ac1d56..6cb2030a4e2 100644 --- a/runtime/vm/scopes.h +++ b/runtime/vm/scopes.h @@ -234,6 +234,11 @@ class LocalScope : public ZoneAllocated { // same name is already present. bool AddVariable(LocalVariable* variable); + // Insert a formal parameter variable to the scope at the given position, + // possibly in front of aliases already added with AddVariable. + // Returns false if a variable with the same name is already present. + bool InsertParameterAt(intptr_t pos, LocalVariable* parameter); + // Add a label to the scope. Returns false if a label with the same name // is already present. bool AddLabel(SourceLabel* label); diff --git a/tests/language/regress_18435_test.dart b/tests/language/regress_18435_test.dart new file mode 100644 index 00000000000..267562f0d4e --- /dev/null +++ b/tests/language/regress_18435_test.dart @@ -0,0 +1,24 @@ +// Copyright (c) 2014, 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 issue 18435. + +import "package:expect/expect.dart"; + +main() { + const MISSING_VALUE = "MISSING_VALUE"; + + void foo([var p1 = MISSING_VALUE, var p2 = MISSING_VALUE]) { + Expect.equals("P1", p1); + Expect.equals("P2", p2); + } + + void bar([var p1 = "MISSING_VALUE", var p2 = "MISSING_VALUE"]) { + Expect.equals("P1", p1); + Expect.equals("P2", p2); + } + + foo("P1", "P2"); + bar("P1", "P2"); +}