Save exception and stack trace variables in async catch clauses and use them

runtime/vm/parser.cc

Index: runtime/vm/parser.cc
===================================================================
--- runtime/vm/parser.cc	(revision 43993)
+++ runtime/vm/parser.cc	(working copy)
@@ -5973,6 +5973,7 @@
   LocalVariable* context_var = current_block_->scope->LookupVariable(
       Symbols::SavedTryContextVar(), false);
   ASSERT(context_var != NULL);
+
   LocalVariable* exception_var = current_block_->scope->LookupVariable(
       Symbols::ExceptionVar(), false);
   if (exception_param.var != NULL) {
@@ -5984,6 +5985,7 @@
         exception_param.var,
         new(Z) LoadLocalNode(Scanner::kNoSourcePos, exception_var)));
   }
+
   LocalVariable* stack_trace_var =
       current_block_->scope->LookupVariable(Symbols::StackTraceVar(), false);
   if (stack_trace_param.var != NULL) {
@@ -5997,6 +5999,9 @@
         new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
   }
 
+  AddSavedExceptionAndStacktraceToScope(
+      exception_var, stack_trace_var, current_block_->scope);
+
   ASSERT(try_blocks_list_ != NULL);
   ASSERT(innermost_function().IsAsyncClosure() ||
          innermost_function().IsAsyncFunction());
@@ -8192,7 +8197,7 @@
         *exception_param->type);
     var->set_is_final();
     bool added_to_scope = scope->AddVariable(var);
-    ASSERT(added_to_scope);
+    ASSERT(added_to_scope);  // TODO(regis): Why do we check below but not here?

[hausner, 2015/02/24 20:57:31]

FWIW, I don't know either.

[regis, 2015/02/24 22:05:13]

I get it now.
The error can only be reported below when the name of the exception param is
reused for the stack trace param.
I have added a comment.

     exception_param->var = var;
   }
   if (stack_trace_param->name != NULL) {
@@ -8206,12 +8211,56 @@
       ReportError(stack_trace_param->token_pos,
                   "name '%s' already exists in scope",
                   stack_trace_param->name->ToCString());
-       }
+    }
     stack_trace_param->var = var;
   }
 }
 
 
+// Populate local scope of the catch block with the saved exception and saved
+// stack trace.
+void Parser::AddSavedExceptionAndStacktraceToScope(

[Ivan Posva, 2015/02/24 20:52:39]

These variables are being added unconditionally here. You should assert that we
are in a async/sync*/async* body closure.

[regis, 2015/02/24 22:05:13]

Done.

+    LocalVariable* exception_var,
+    LocalVariable* stack_trace_var,
+    LocalScope* scope) {
+  // Add :saved_exception_var and :saved_stack_trace_var to scope.
+  LocalVariable* saved_exception_var = new (Z) LocalVariable(
+      Scanner::kNoSourcePos,
+      Symbols::SavedExceptionVar(),
+      Type::ZoneHandle(Z, Type::DynamicType()));
+  scope->AddVariable(saved_exception_var);
+  saved_exception_var->set_is_captured();

[Ivan Posva, 2015/02/24 20:52:39]

These variables should be automatically captured as is the rest of the
variables. If not we need to know why they are not being captured by the rest of
the system.
These variables also should be marked as final.

[hausner, 2015/02/24 20:57:31]

Capturing explicitly should not be necessary. After the function is parsed,
there is a recursive pass over all scopes to capture all variables. I think that
would also capture these two variables.

[regis, 2015/02/24 22:05:13]

Removed the explicit capturing and marked as final.

+  LocalVariable* saved_stack_trace_var = new (Z) LocalVariable(
+      Scanner::kNoSourcePos,
+      Symbols::SavedStackTraceVar(),
+      Type::ZoneHandle(Z, Type::DynamicType()));
+  scope->AddVariable(saved_stack_trace_var);
+  saved_stack_trace_var->set_is_captured();
+
+  // Generate code to load the exception object (:exception_var) into
+  // the saved exception variable (:saved_exception_var) used to rethrow.
+  saved_exception_var = current_block_->scope->LookupVariable(
+      Symbols::SavedExceptionVar(), false);
+  ASSERT(saved_exception_var != NULL);
+  ASSERT(exception_var != NULL);
+  current_block_->statements->Add(new(Z) StoreLocalNode(
+      Scanner::kNoSourcePos,
+      saved_exception_var,
+      new(Z) LoadLocalNode(Scanner::kNoSourcePos, exception_var)));
+
+  // Generate code to load the stack trace object (:stack_trace_var) into
+  // the saved stacktrace variable (:saved_stack_trace_var) used to rethrow.
+  saved_stack_trace_var = current_block_->scope->LookupVariable(
+      Symbols::SavedStackTraceVar(), false);
+  ASSERT(saved_stack_trace_var != NULL);
+  ASSERT(stack_trace_var != NULL);
+  current_block_->statements->Add(new(Z) StoreLocalNode(
+      Scanner::kNoSourcePos,
+      saved_stack_trace_var,
+      new(Z) LoadLocalNode(Scanner::kNoSourcePos, stack_trace_var)));
+}
+
+
 SequenceNode* Parser::ParseFinallyBlock() {
   TRACE_PARSER("ParseFinallyBlock");
   OpenBlock();
@@ -8345,7 +8394,9 @@
     // following code:
     // 1) Store exception object and stack trace object into user-defined
     //    variables (as needed).
-    // 2) Nested block with source code from catch clause block.
+    // 2) In async code, save exception object and stack trace object into
+    //    captured :saved_exception_var and :saved_stack_trace_var.
+    // 3) Nested block with source code from catch clause block.
     OpenBlock();
     AddCatchParamsToScope(&exception_param, &stack_trace_param,
                           current_block_->scope);
@@ -8393,6 +8444,8 @@
       } else {
         parsed_function()->reset_saved_try_ctx_vars();
       }
+      AddSavedExceptionAndStacktraceToScope(
+          exception_var, stack_trace_var, current_block_->scope);
     }
 
     current_block_->statements->Add(ParseNestedStatement(false, NULL));
@@ -8537,7 +8590,7 @@
 
 
 // Restore the currently relevant :saved_try_context_var on the stack
-// from the captured :async_saved_try_cts_var.
+// from the captured :async_saved_try_ctx_var_.
 // * Try blocks: Set the context variable for this try block.
 // * Catch/finally blocks: Set the context variable for any outer try block (if
 //   existent).
@@ -8931,16 +8984,31 @@
     if ((try_blocks_list_ == NULL) || !try_blocks_list_->inside_catch()) {
       ReportError(statement_pos, "rethrow of an exception is not valid here");
     }
-    // The exception and stack trace variables are bound in the block
-    // containing the try.
-    LocalScope* scope = try_blocks_list_->try_block()->scope->parent();
-    ASSERT(scope != NULL);
-    LocalVariable* excp_var =
-        scope->LocalLookupVariable(Symbols::ExceptionVar());
+
+    // If in async code, use :saved_exception_var and :saved_stack_trace_var
+    // instead of :exception_var and :stack_trace_var.
+    LocalVariable* excp_var;
+    LocalVariable* trace_var;
+    if (innermost_function().IsAsyncClosure() ||
+        innermost_function().IsAsyncFunction() ||
+        innermost_function().IsSyncGenClosure() ||
+        innermost_function().IsSyncGenerator()) {
+      // The saved exception and stack trace variables are bound in the block
+      // containing the catch.
+      LocalScope* scope = current_block_->scope;
+      excp_var = scope->LookupVariable(Symbols::SavedExceptionVar(), false);

[Ivan Posva, 2015/02/24 20:52:39]

Why is this not LocalLookupVariable?

[hausner, 2015/02/24 20:57:31]

I think Ivan is right, this should only look up in the innermost scope.
Otherwise you might find the saved exception var of an outer try-catch. Compare
to the else branch below.

[regis, 2015/02/24 22:05:13]

As explained by the comments (obviously not clearly enough), the saved exception
var and saved stack trace var are looked for in the catch scope, not in the try
scope as for the vars of the else part. The try scope is available, but not the
catch scope. It's why I have to start from the current scope. There is no danger
to find these vars in an outer scope, since they are declared unconditionally.
I have updated the comments.

+      trace_var = scope->LookupVariable(Symbols::SavedStackTraceVar(), false);
+    } else {
+      // The exception and stack trace variables are bound in the block
+      // containing the try.
+      LocalScope* scope = try_blocks_list_->try_block()->scope->parent();
+      ASSERT(scope != NULL);
+      excp_var = scope->LocalLookupVariable(Symbols::ExceptionVar());
+      trace_var = scope->LocalLookupVariable(Symbols::StackTraceVar());
+    }
     ASSERT(excp_var != NULL);
-    LocalVariable* trace_var =
-        scope->LocalLookupVariable(Symbols::StackTraceVar());
     ASSERT(trace_var != NULL);
+
     statement = new(Z) ThrowNode(
         statement_pos,
         new(Z) LoadLocalNode(statement_pos, excp_var),