Restructure handling of paren expressions (#417)

* Move nontrivial logic out of `parser.ypp` into `FieldList`, rename it to `ParenContents`, make it a class, and add tests
* Use a `FieldInitializer` struct instead of `std::pair<std::string, Expression*>` to represent the fields of a tuple

executable_semantics/ast/BUILD

@@ -27,13 +27,6 @@ cc_library(
     hdrs = ["expression.h"],
 )
 
-cc_library(
-    name = "field_list",
-    srcs = ["field_list.cpp"],
-    hdrs = ["field_list.h"],
-    deps = [":expression"],
-)
-
 cc_library(
     name = "function_definition",
     srcs = ["function_definition.cpp"],

executable_semantics/ast/field_list.cpp

@@ -1,29 +0,0 @@
-// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#include "executable_semantics/ast/field_list.h"
-
-namespace Carbon {
-
-auto MakeFieldList(std::list<std::pair<std::string, Expression*>>* fields)
-    -> FieldList* {
-  auto e = new FieldList();
-  e->fields = fields;
-  return e;
-}
-
-auto MakeConsField(FieldList* e1, FieldList* e2) -> FieldList* {
-  auto fields = new std::list<std::pair<std::string, Expression*>>();
-  for (auto& field : *e1->fields) {
-    fields->push_back(field);
-  }
-  for (auto& field : *e2->fields) {
-    fields->push_back(field);
-  }
-  auto result = MakeFieldList(fields);
-  result->has_explicit_comma = true;
-  return result;
-}
-
-}  // namespace Carbon

executable_semantics/ast/field_list.h

@@ -1,26 +0,0 @@
-// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
-// Exceptions. See /LICENSE for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-
-#ifndef EXECUTABLE_SEMANTICS_AST_FIELD_LIST_H_
-#define EXECUTABLE_SEMANTICS_AST_FIELD_LIST_H_
-
-#include <list>
-
-#include "executable_semantics/ast/expression.h"
-
-namespace Carbon {
-
-// This is used in the parsing of tuples and parenthesized expressions.
-struct FieldList {
-  std::list<std::pair<std::string, Expression*>>* fields;
-  bool has_explicit_comma = false;
-};
-
-auto MakeFieldList(std::list<std::pair<std::string, Expression*>>* fields)
-    -> FieldList*;
-auto MakeConsField(FieldList* e1, FieldList* e2) -> FieldList*;
-
-}  // namespace Carbon
-
-#endif  // EXECUTABLE_SEMANTICS_AST_FIELD_LIST_H_

executable_semantics/syntax/BUILD

@@ -28,14 +28,31 @@ cc_library(
         "-Wno-writable-strings",
     ],
     deps = [
+        ":paren_contents",
         "//executable_semantics:tracing_flag",
         "//executable_semantics/ast:declaration",
         "//executable_semantics/ast:expression",
-        "//executable_semantics/ast:field_list",
         "//executable_semantics/interpreter",
     ],
 )
 
+cc_library(
+    name = "paren_contents",
+    srcs = ["paren_contents.cpp"],
+    hdrs = ["paren_contents.h"],
+    deps = ["//executable_semantics/ast:expression"],
+)
+
+cc_test(
+    name = "paren_contents_test",
+    srcs = ["paren_contents_test.cpp"],
+    deps = [
+        ":paren_contents",
+        "@llvm-project//llvm:gtest",
+        "@llvm-project//llvm:gtest_main",
+    ],
+)
+
 genrule(
     name = "syntax_bison_srcs",
     srcs = ["parser.ypp"],

executable_semantics/syntax/paren_contents.cpp

@@ -0,0 +1,26 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "executable_semantics/syntax/paren_contents.h"
+
+namespace Carbon {
+
+Expression* ParenContents::AsExpression(int line_number) const {
+  if (fields_.size() == 1 && fields_.front().name == "" &&
+      has_trailing_comma_ == HasTrailingComma::No) {
+    return fields_.front().expression;
+  } else {
+    return AsTuple(line_number);
+  }
+}
+
+Expression* ParenContents::AsTuple(int line_number) const {
+  auto vec = new std::vector<std::pair<std::string, Carbon::Expression*>>();
+  for (const FieldInitializer& initializer : fields_) {
+    vec->push_back({initializer.name, initializer.expression});
+  }
+  return MakeTuple(line_number, vec);
+}
+
+}  // namespace Carbon

executable_semantics/syntax/paren_contents.h

@@ -0,0 +1,61 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#ifndef EXECUTABLE_SEMANTICS_SYNTAX_PAREN_CONTENTS_H_
+#define EXECUTABLE_SEMANTICS_SYNTAX_PAREN_CONTENTS_H_
+
+#include <list>
+
+#include "executable_semantics/ast/expression.h"
+
+namespace Carbon {
+
+// A FieldInitializer represents the initialization of a single tuple field.
+struct FieldInitializer {
+  // The field name. An empty string indicates that this represents a
+  // positional field.
+  std::string name;
+
+  // The expression that initializes the field.
+  Expression* expression;
+};
+
+// Represents the syntactic contents of an expression delimited by
+// parentheses. Such expressions can be interpreted as either tuples or
+// arbitrary expressions, depending on their context and the syntax of their
+// contents; this class helps calling code resolve that ambiguity. Since that
+// ambiguity is purely syntactic, this class should only be needed during
+// parsing.
+class ParenContents {
+ public:
+  // Indicates whether the paren expression's contents end with a comma.
+  enum class HasTrailingComma { Yes, No };
+
+  // Constructs a ParenContents representing the contents of "()".
+  ParenContents() : fields_({}), has_trailing_comma_(HasTrailingComma::No) {}
+
+  // Constructs a ParenContents representing the given list of fields,
+  // with or without a trailing comma.
+  ParenContents(std::vector<FieldInitializer> fields,
+                HasTrailingComma has_trailing_comma)
+      : fields_(fields), has_trailing_comma_(has_trailing_comma) {}
+
+  ParenContents(const ParenContents&) = default;
+  ParenContents& operator=(const ParenContents&) = default;
+
+  // Returns the paren expression, interpreted as a tuple.
+  Expression* AsTuple(int line_number) const;
+
+  // Returns the paren expression, with no external constraints on what kind
+  // of expression it represents.
+  Expression* AsExpression(int line_number) const;
+
+ private:
+  std::vector<FieldInitializer> fields_;
+  HasTrailingComma has_trailing_comma_;
+};
+
+}  // namespace Carbon
+
+#endif  // EXECUTABLE_SEMANTICS_SYNTAX_PAREN_CONTENTS_H_

executable_semantics/syntax/paren_contents_test.cpp

@@ -0,0 +1,113 @@
+// Part of the Carbon Language project, under the Apache License v2.0 with LLVM
+// Exceptions. See /LICENSE for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+
+#include "executable_semantics/syntax/paren_contents.h"
+
+#include "gtest/gtest.h"
+
+namespace Carbon {
+namespace {
+
+TEST(ParenContentsTest, EmptyAsExpression) {
+  ParenContents contents;
+  Expression* expression = contents.AsExpression(/*line_num=*/1);
+  EXPECT_EQ(expression->line_num, 1);
+  ASSERT_EQ(expression->tag, ExpressionKind::Tuple);
+  EXPECT_EQ(expression->u.tuple.fields->size(), 0);
+}
+
+TEST(ParenContentsTest, EmptyAsTuple) {
+  ParenContents contents;
+  Expression* tuple = contents.AsTuple(/*line_num=*/1);
+  EXPECT_EQ(tuple->line_num, 1);
+  ASSERT_EQ(tuple->tag, ExpressionKind::Tuple);
+  EXPECT_EQ(tuple->u.tuple.fields->size(), 0);
+}
+
+TEST(ParenContentsTest, UnaryNoCommaAsExpression) {
+  // Equivalent to a code fragment like
+  // ```
+  // (
+  //   42
+  // )
+  // ```
+  ParenContents contents({{.expression = MakeInt(/*line_num=*/2, 42)}},
+                         ParenContents::HasTrailingComma::No);
+
+  Expression* expression = contents.AsExpression(/*line_num=*/1);
+  EXPECT_EQ(expression->line_num, 2);
+  ASSERT_EQ(expression->tag, ExpressionKind::Integer);
+}
+
+TEST(ParenContentsTest, UnaryNoCommaAsTuple) {
+  ParenContents contents({{.expression = MakeInt(/*line_num=*/2, 42)}},
+                         ParenContents::HasTrailingComma::No);
+
+  Expression* tuple = contents.AsTuple(/*line_num=*/1);
+  EXPECT_EQ(tuple->line_num, 1);
+  ASSERT_EQ(tuple->tag, ExpressionKind::Tuple);
+  std::vector<std::pair<std::string, Expression*>> fields =
+      *tuple->u.tuple.fields;
+  ASSERT_EQ(fields.size(), 1);
+  EXPECT_EQ(fields[0].second->tag, ExpressionKind::Integer);
+}
+
+TEST(ParenContentsTest, UnaryWithCommaAsExpression) {
+  ParenContents contents({{.expression = MakeInt(/*line_num=*/2, 42)}},
+                         ParenContents::HasTrailingComma::Yes);
+
+  Expression* expression = contents.AsExpression(/*line_num=*/1);
+  EXPECT_EQ(expression->line_num, 1);
+  ASSERT_EQ(expression->tag, ExpressionKind::Tuple);
+  std::vector<std::pair<std::string, Expression*>> fields =
+      *expression->u.tuple.fields;
+  ASSERT_EQ(fields.size(), 1);
+  EXPECT_EQ(fields[0].second->tag, ExpressionKind::Integer);
+}
+
+TEST(ParenContentsTest, UnaryWithCommaAsTuple) {
+  ParenContents contents({{.expression = MakeInt(/*line_num=*/2, 42)}},
+                         ParenContents::HasTrailingComma::Yes);
+
+  Expression* tuple = contents.AsTuple(/*line_num=*/1);
+  EXPECT_EQ(tuple->line_num, 1);
+  ASSERT_EQ(tuple->tag, ExpressionKind::Tuple);
+  std::vector<std::pair<std::string, Expression*>> fields =
+      *tuple->u.tuple.fields;
+  ASSERT_EQ(fields.size(), 1);
+  EXPECT_EQ(fields[0].second->tag, ExpressionKind::Integer);
+}
+
+TEST(ParenContentsTest, BinaryAsExpression) {
+  ParenContents contents({{.expression = MakeInt(/*line_num=*/2, 42)},
+                          {.expression = MakeInt(/*line_num=*/3, 42)}},
+                         ParenContents::HasTrailingComma::Yes);
+
+  Expression* expression = contents.AsExpression(/*line_num=*/1);
+  EXPECT_EQ(expression->line_num, 1);
+  ASSERT_EQ(expression->tag, ExpressionKind::Tuple);
+  std::vector<std::pair<std::string, Expression*>> fields =
+      *expression->u.tuple.fields;
+  ASSERT_EQ(fields.size(), 2);
+  EXPECT_EQ(fields[0].second->tag, ExpressionKind::Integer);
+  EXPECT_EQ(fields[1].second->tag, ExpressionKind::Integer);
+}
+
+TEST(ParenContentsTest, BinaryAsTuple) {
+  ParenContents contents({{.expression = MakeInt(/*line_num=*/2, 42)},
+                          {.expression = MakeInt(/*line_num=*/3, 42)}},
+                         ParenContents::HasTrailingComma::Yes);
+
+  Expression* tuple = contents.AsTuple(/*line_num=*/1);
+  EXPECT_EQ(tuple->line_num, 1);
+  ASSERT_EQ(tuple->tag, ExpressionKind::Tuple);
+  std::vector<std::pair<std::string, Expression*>> fields =
+      *tuple->u.tuple.fields;
+  ASSERT_EQ(fields.size(), 2);
+  EXPECT_EQ(fields[0].second->tag, ExpressionKind::Integer);
+  EXPECT_EQ(fields[1].second->tag, ExpressionKind::Integer);
+}
+
+}  // namespace
+}  // namespace Carbon

executable_semantics/syntax/parser.ypp

@@ -50,8 +50,8 @@
 
 #include "executable_semantics/ast/abstract_syntax_tree.h"
 #include "executable_semantics/ast/declaration.h"
-#include "executable_semantics/ast/field_list.h"
 #include "executable_semantics/ast/function_definition.h"
+#include "executable_semantics/syntax/paren_contents.h"
 
 namespace Carbon {
 class ParseAndLexContext;
@@ -89,8 +89,9 @@ void yy::parser::error(
 %type <Carbon::Member*> variable_declaration
 %type <Carbon::Member*> member
 %type <std::list<Carbon::Member*>*> member_list
-%type <Carbon::FieldList*> field
-%type <Carbon::FieldList*> field_list
+%type <Carbon::FieldInitializer> field_initializer
+%type <Carbon::ParenContents> paren_contents
+%type <std::vector<Carbon::FieldInitializer>> paren_contents_without_trailing_comma
 %type <std::pair<std::string, Carbon::Expression*>*> alternative
 %type <std::list<std::pair<std::string, Carbon::Expression*>>*> alternative_list
 %type <std::pair<Carbon::Expression*, Carbon::Statement*>*> clause
@@ -208,51 +209,40 @@ expression:
 ;
 designator: "." identifier { $$ = $2; }
 ;
-paren_expression: "(" field_list ")"
-    {
-     if ($2->fields->size() == 1 &&
-         $2->fields->front().first == "" &&
-	 !$2->has_explicit_comma) {
-	$$ = $2->fields->front().second;
-     } else {
-        auto vec = new std::vector<std::pair<std::string,Carbon::Expression*>>(
-            $2->fields->begin(), $2->fields->end());
-        $$ = Carbon::MakeTuple(yylineno, vec);
-      }
-    }
+paren_expression: "(" paren_contents ")"
+    { $$ = $2.AsExpression(yylineno); }
 ;
-tuple: "(" field_list ")"
-    {
-     auto vec = new std::vector<std::pair<std::string,Carbon::Expression*>>(
-         $2->fields->begin(), $2->fields->end());
-     $$ = Carbon::MakeTuple(yylineno, vec);
-    }
-field:
+tuple: "(" paren_contents ")"
+    { $$ = $2.AsTuple(yylineno); }
+;
+field_initializer:
   pattern
+    { $$ = Carbon::FieldInitializer({"", $1}); }
+| designator "=" pattern
+    { $$ = Carbon::FieldInitializer({$1, $3}); }
+;
+paren_contents:
+  // Empty
+    { $$ = Carbon::ParenContents(); }
+| paren_contents_without_trailing_comma
     {
-      auto fields =
-          new std::list<std::pair<std::string, Carbon::Expression*>>();
-      fields->push_back(std::make_pair("", $1));
-      $$ = Carbon::MakeFieldList(fields);
+      $$ = Carbon::ParenContents($1,
+                                 Carbon::ParenContents::HasTrailingComma::No);
     }
-| designator "=" pattern
+| paren_contents_without_trailing_comma ","
     {
-      auto fields =
-          new std::list<std::pair<std::string, Carbon::Expression*>>();
-      fields->push_back(std::make_pair($1, $3));
-      $$ = Carbon::MakeFieldList(fields);
+      $$ = Carbon::ParenContents($1,
+                                 Carbon::ParenContents::HasTrailingComma::Yes);
     }
 ;
-field_list:
-  // Empty
+paren_contents_without_trailing_comma:
+  field_initializer
+    { $$ = {$1}; }
+| paren_contents_without_trailing_comma "," field_initializer
     {
-      $$ = Carbon::MakeFieldList(
-          new std::list<std::pair<std::string, Carbon::Expression*>>());
+      $$ = $1;
+      $$.push_back($3);
     }
-| field
-    { $$ = $1; }
-| field "," field_list
-    { $$ = Carbon::MakeConsField($1, $3); }
 ;
 clause:
   CASE pattern DBLARROW statement

executable_semantics/testdata/tuple2.6c

@@ -3,6 +3,7 @@
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
 fn main() -> Int {
-  var (Int,): t = (5,);
-  return t[0] - 5;
+  var (Int,): t1 = (5,);
+  var (Int, Int): t2 = (2, 3,);
+  return t1[0] - t2[0] - t2[1];
 }