[torque] avoid ambiguity if a catch catches from other handlers

Torque desugars try-catch/label constructs with several handlers
into nested try structures, with the first handler ending-up
innermost. So currently, if you write

try {
...
} label Foo {
  Throw(...);
} catch (e) {

}

The catch will catch the preceding Throw in another handler.
This is different from how multiple try-catch handlers are done in
languages like Java, where throwing from a preceding catch handler
is not caught by a later one. To avoid this possible ambiguity, this
CL prohibits this pattern, enforcing that a catch handler comes first,
before any other label-handler attached to the same try.
This way, a catch handler never catches from any other handler on the
same try, since they have to come later.

Bug: v8:7793
Change-Id: I943f14b2393d307c4254a3fc3a78f236dbcf86df
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2169098
Commit-Queue: Tobias Tebbi <tebbi@chromium.org>
Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Reviewed-by: Marja Hölttä <marja@chromium.org>
Reviewed-by: Jakob Gruber <jgruber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#67516}

src/builtins/promise-all.tq

@@ -333,7 +333,6 @@ namespace promise {
     // not trigger redundant ExceptionEvents
     const capability = NewPromiseCapability(receiver, False);
 
-    try {
     try {
       // Let iterator be GetIterator(iterable).
       // IfAbruptRejectPromise(iterator, promiseCapability).
@@ -350,7 +349,6 @@ namespace promise {
     } catch (e) deferred {
       goto Reject(e);
     }
-    }
     label Reject(e: Object) deferred {
       // Exception must be bound to a JS value.
       const e = UnsafeCast<JSAny>(e);

src/builtins/promise-any.tq

@@ -270,12 +270,11 @@ namespace promise {
                 context, rejectElement, kPromiseForwardingHandlerSymbol, True);
           }
       }
-    }
-    label Done {}
-    catch (e) deferred {
+    } catch (e) deferred {
       iterator::IteratorCloseOnException(iteratorRecord, e)
           otherwise Reject;
     }
+    label Done {}
 
     // (8.d)
     //   i. Set iteratorRecord.[[Done]] to true.

src/torque/ast.h

@@ -93,7 +93,7 @@ namespace torque {
   AST_STATEMENT_NODE_KIND_LIST(V)       \
   AST_DECLARATION_NODE_KIND_LIST(V)     \
   V(Identifier)                         \
-  V(LabelBlock)                         \
+  V(TryHandler)                         \
   V(ClassBody)
 
 struct AstNode {
@@ -780,14 +780,17 @@ struct ForLoopStatement : Statement {
   Statement* body;
 };
 
-struct LabelBlock : AstNode {
-  DEFINE_AST_NODE_LEAF_BOILERPLATE(LabelBlock)
-  LabelBlock(SourcePosition pos, Identifier* label,
+struct TryHandler : AstNode {
+  DEFINE_AST_NODE_LEAF_BOILERPLATE(TryHandler)
+  enum class HandlerKind { kCatch, kLabel };
+  TryHandler(SourcePosition pos, HandlerKind handler_kind, Identifier* label,
              const ParameterList& parameters, Statement* body)
       : AstNode(kKind, pos),
+        handler_kind(handler_kind),
         label(label),
         parameters(parameters),
         body(std::move(body)) {}
+  HandlerKind handler_kind;
   Identifier* label;
   ParameterList parameters;
   Statement* body;
@@ -802,15 +805,13 @@ struct StatementExpression : Expression {
 
 struct TryLabelExpression : Expression {
   DEFINE_AST_NODE_LEAF_BOILERPLATE(TryLabelExpression)
-  TryLabelExpression(SourcePosition pos, bool catch_exceptions,
-                     Expression* try_expression, LabelBlock* label_block)
+  TryLabelExpression(SourcePosition pos, Expression* try_expression,
+                     TryHandler* label_block)
       : Expression(kKind, pos),
-        catch_exceptions(catch_exceptions),
         try_expression(try_expression),
         label_block(label_block) {}
-  bool catch_exceptions;
   Expression* try_expression;
-  LabelBlock* label_block;
+  TryHandler* label_block;
 };
 
 struct BlockStatement : Statement {

src/torque/earley-parser.h

@@ -52,8 +52,7 @@ enum class ParseResultHolderBase::TypeId {
   kDeclarationPtr,
   kTypeExpressionPtr,
   kOptionalTypeExpressionPtr,
-  kLabelBlockPtr,
-  kOptionalLabelBlockPtr,
+  kTryHandlerPtr,
   kNameAndTypeExpression,
   kImplicitParameters,
   kOptionalImplicitParameters,
@@ -82,7 +81,7 @@ enum class ParseResultHolderBase::TypeId {
   kOptionalTypeList,
   kLabelAndTypes,
   kStdVectorOfLabelAndTypes,
-  kStdVectorOfLabelBlockPtr,
+  kStdVectorOfTryHandlerPtr,
   kOptionalStatementPtr,
   kOptionalExpressionPtr,
   kTypeswitchCase,

src/torque/torque-parser.cc

@@ -86,12 +86,8 @@ V8_EXPORT_PRIVATE const ParseResultTypeId
     ParseResultHolder<base::Optional<TypeExpression*>>::id =
         ParseResultTypeId::kOptionalTypeExpressionPtr;
 template <>
-V8_EXPORT_PRIVATE const ParseResultTypeId ParseResultHolder<LabelBlock*>::id =
-    ParseResultTypeId::kLabelBlockPtr;
-template <>
-V8_EXPORT_PRIVATE const ParseResultTypeId
-    ParseResultHolder<base::Optional<LabelBlock*>>::id =
-        ParseResultTypeId::kOptionalLabelBlockPtr;
+V8_EXPORT_PRIVATE const ParseResultTypeId ParseResultHolder<TryHandler*>::id =
+    ParseResultTypeId::kTryHandlerPtr;
 template <>
 V8_EXPORT_PRIVATE const ParseResultTypeId ParseResultHolder<Expression*>::id =
     ParseResultTypeId::kExpressionPtr;
@@ -215,8 +211,8 @@ V8_EXPORT_PRIVATE const ParseResultTypeId
         ParseResultTypeId::kStdVectorOfLabelAndTypes;
 template <>
 V8_EXPORT_PRIVATE const ParseResultTypeId
-    ParseResultHolder<std::vector<LabelBlock*>>::id =
-        ParseResultTypeId::kStdVectorOfLabelBlockPtr;
+    ParseResultHolder<std::vector<TryHandler*>>::id =
+        ParseResultTypeId::kStdVectorOfTryHandlerPtr;
 template <>
 V8_EXPORT_PRIVATE const ParseResultTypeId
     ParseResultHolder<base::Optional<Statement*>>::id =
@@ -321,7 +317,7 @@ Expression* MakeCall(IdentifierExpression* callee,
   // used as labels identifiers. All other statements in a call's otherwise
   // must create intermediate Labels for the otherwise's statement code.
   size_t label_id = 0;
-  std::vector<LabelBlock*> temp_labels;
+  std::vector<TryHandler*> temp_labels;
   for (auto* statement : otherwise) {
     if (auto* e = ExpressionStatement::DynamicCast(statement)) {
       if (auto* id = IdentifierExpression::DynamicCast(e->expression)) {
@@ -336,9 +332,10 @@ Expression* MakeCall(IdentifierExpression* callee,
     auto label_id = MakeNode<Identifier>(label_name);
     label_id->pos = SourcePosition::Invalid();
     labels.push_back(label_id);
-    auto* label_block =
-        MakeNode<LabelBlock>(label_id, ParameterList::Empty(), statement);
-    temp_labels.push_back(label_block);
+    auto* handler =
+        MakeNode<TryHandler>(TryHandler::HandlerKind::kLabel, label_id,
+                             ParameterList::Empty(), statement);
+    temp_labels.push_back(handler);
   }
 
   // Create nested try-label expression for all of the temporary Labels that
@@ -351,7 +348,7 @@ Expression* MakeCall(IdentifierExpression* callee,
   }
 
   for (auto* label : temp_labels) {
-    result = MakeNode<TryLabelExpression>(false, result, label);
+    result = MakeNode<TryLabelExpression>(result, label);
   }
   return result;
 }
@@ -1410,8 +1407,9 @@ base::Optional<ParseResult> MakeTypeswitchStatement(
       BlockStatement* next_block = MakeNode<BlockStatement>();
       current_block->statements.push_back(
           MakeNode<ExpressionStatement>(MakeNode<TryLabelExpression>(
-              false, MakeNode<StatementExpression>(case_block),
-              MakeNode<LabelBlock>(MakeNode<Identifier>(kNextCaseLabelName),
+              MakeNode<StatementExpression>(case_block),
+              MakeNode<TryHandler>(TryHandler::HandlerKind::kLabel,
+                                   MakeNode<Identifier>(kNextCaseLabelName),
                                    ParameterList::Empty(), next_block))));
       current_block = next_block;
     }
@@ -1512,18 +1510,21 @@ base::Optional<ParseResult> MakeTryLabelExpression(
   auto try_block = child_results->NextAs<Statement*>();
   CheckNotDeferredStatement(try_block);
   Statement* result = try_block;
-  auto label_blocks = child_results->NextAs<std::vector<LabelBlock*>>();
-  auto catch_block = child_results->NextAs<base::Optional<LabelBlock*>>();
-  if (label_blocks.empty() && !catch_block.has_value()) {
+  auto handlers = child_results->NextAs<std::vector<TryHandler*>>();
+  if (handlers.empty()) {
     Error("Try blocks without catch or label don't make sense.");
   }
-  for (auto block : label_blocks) {
-    result = MakeNode<ExpressionStatement>(MakeNode<TryLabelExpression>(
-        false, MakeNode<StatementExpression>(result), block));
+  for (size_t i = 0; i < handlers.size(); ++i) {
+    if (i != 0 &&
+        handlers[i]->handler_kind == TryHandler::HandlerKind::kCatch) {
+      Error(
+          "A catch handler always has to be first, before any label handler, "
+          "to avoid ambiguity about whether it catches exceptions from "
+          "preceding handlers or not.")
+          .Position(handlers[i]->pos);
     }
-  if (catch_block) {
     result = MakeNode<ExpressionStatement>(MakeNode<TryLabelExpression>(
-        true, MakeNode<StatementExpression>(result), *catch_block));
+        MakeNode<StatementExpression>(result), handlers[i]));
   }
   return ParseResult{result};
 }
@@ -1549,7 +1550,8 @@ base::Optional<ParseResult> MakeLabelBlock(ParseResultIterator* child_results) {
   }
   auto parameters = child_results->NextAs<ParameterList>();
   auto body = child_results->NextAs<Statement*>();
-  LabelBlock* result = MakeNode<LabelBlock>(label, std::move(parameters), body);
+  TryHandler* result = MakeNode<TryHandler>(TryHandler::HandlerKind::kLabel,
+                                            label, std::move(parameters), body);
   return ParseResult{result};
 }
 
@@ -1564,8 +1566,9 @@ base::Optional<ParseResult> MakeCatchBlock(ParseResultIterator* child_results) {
   parameters.types.push_back(MakeNode<BasicTypeExpression>(
       std::vector<std::string>{}, "JSAny", std::vector<TypeExpression*>{}));
   parameters.has_varargs = false;
-  LabelBlock* result = MakeNode<LabelBlock>(
-      MakeNode<Identifier>(kCatchLabelName), std::move(parameters), body);
+  TryHandler* result = MakeNode<TryHandler>(
+      TryHandler::HandlerKind::kCatch, MakeNode<Identifier>(kCatchLabelName),
+      std::move(parameters), body);
   return ParseResult{result};
 }
 
@@ -2254,13 +2257,11 @@ struct TorqueGrammar : Grammar {
                         List<Statement*>(&statement), Token("}")},
                        MakeBlockStatement)};
 
-  // Result: LabelBlock*
-  Symbol labelBlock = {
+  // Result: TryHandler*
+  Symbol tryHandler = {
       Rule({Token("label"), &name,
             TryOrDefault<ParameterList>(&parameterListNoVararg), &block},
-           MakeLabelBlock)};
-
-  Symbol catchBlock = {
+           MakeLabelBlock),
       Rule({Token("catch"), Token("("), &identifier, Token(")"), &block},
            MakeCatchBlock)};
 
@@ -2306,13 +2307,16 @@ struct TorqueGrammar : Grammar {
            MakeIfStatement),
       Rule(
           {
-              Token("typeswitch"), Token("("), expression, Token(")"),
-              Token("{"), NonemptyList<TypeswitchCase>(&typeswitchCase),
+              Token("typeswitch"),
+              Token("("),
+              expression,
+              Token(")"),
+              Token("{"),
+              NonemptyList<TypeswitchCase>(&typeswitchCase),
               Token("}"),
           },
           MakeTypeswitchStatement),
-      Rule({Token("try"), &block, List<LabelBlock*>(&labelBlock),
-            Optional<LabelBlock*>(&catchBlock)},
+      Rule({Token("try"), &block, List<TryHandler*>(&tryHandler)},
            MakeTryLabelExpression),
       Rule({OneOf({"assert", "check"}), Token("("), &expressionWithSource,
             Token(")"), Token(";")},

test/torque/test-torque.tq

@@ -681,14 +681,13 @@ namespace test {
     let r: Smi = 0;
     try {
       TestCatch3WrapperWithLabel() otherwise Abort;
+    } catch (_e) {
+      r = 2;
+      return r;
     }
     label Abort {
       return -1;
     }
-    catch (_e) {
-      r = 2;
-      return r;
-    }
   }
 
   // This test doesn't actually test the functionality of iterators,

test/unittests/torque/torque-unittest.cc

@@ -757,6 +757,20 @@ TEST(Torque, References) {
                            HasSubstr("cannot assign to const value"));
 }
 
+TEST(Torque, CatchFirstHandler) {
+  ExpectFailingCompilation(
+      R"(
+    @export
+    macro Test() {
+      try {
+      } label Foo {
+      } catch (e) {}
+    }
+  )",
+      HasSubstr(
+          "catch handler always has to be first, before any label handler"));
+}
+
 }  // namespace torque
 }  // namespace internal
 }  // namespace v8