Address feedback from #856 (#864)

* Make tuple/struct fields mutable.
* Avoid ExpectType when we know it will fail.

executable_semantics/ast/expression.cpp

@@ -68,7 +68,7 @@ static void PrintFields(llvm::raw_ostream& out,
                         std::string_view separator) {
   llvm::ListSeparator sep;
   for (const auto& field : fields) {
-    out << sep << "." << field.name << separator << *field.expression;
+    out << sep << "." << field.name() << separator << *field.expression();
   }
 }
 
@@ -86,7 +86,7 @@ void Expression::Print(llvm::raw_ostream& out) const {
     }
     case Expression::Kind::TupleLiteral:
       out << "(";
-      PrintFields(out, cast<TupleLiteral>(*this).Fields(), " = ");
+      PrintFields(out, cast<TupleLiteral>(*this).fields(), " = ");
       out << ")";
       break;
     case Expression::Kind::StructLiteral:

executable_semantics/ast/expression.h

@@ -76,16 +76,24 @@ auto TupleExpressionFromParenContents(
     Nonnull<Arena*> arena, SourceLocation source_loc,
     const ParenContents<Expression>& paren_contents) -> Nonnull<Expression*>;
 
-// A FieldInitializer represents the initialization of a single tuple field.
-struct FieldInitializer {
+// A FieldInitializer represents the initialization of a single tuple or
+// struct field.
+class FieldInitializer {
+ public:
   FieldInitializer(std::string name, Nonnull<Expression*> expression)
-      : name(std::move(name)), expression(expression) {}
+      : name_(std::move(name)), expression_(expression) {}
+
+  auto name() const -> const std::string& { return name_; }
 
+  auto expression() const -> Nonnull<const Expression*> { return expression_; }
+  auto expression() -> Nonnull<Expression*> { return expression_; }
+
+ private:
   // The field name. Cannot be empty.
-  std::string name;
+  std::string name_;
 
   // The expression that initializes the field.
-  Nonnull<Expression*> expression;
+  Nonnull<Expression*> expression_;
 };
 
 enum class Operator {
@@ -224,16 +232,18 @@ class TupleLiteral : public Expression {
 
   explicit TupleLiteral(SourceLocation source_loc,
                         std::vector<FieldInitializer> fields)
-      : Expression(Kind::TupleLiteral, source_loc), fields(std::move(fields)) {}
+      : Expression(Kind::TupleLiteral, source_loc),
+        fields_(std::move(fields)) {}
 
   static auto classof(const Expression* exp) -> bool {
     return exp->kind() == Kind::TupleLiteral;
   }
 
-  auto Fields() const -> const std::vector<FieldInitializer>& { return fields; }
+  auto fields() const -> llvm::ArrayRef<FieldInitializer> { return fields_; }
+  auto fields() -> llvm::MutableArrayRef<FieldInitializer> { return fields_; }
 
  private:
-  std::vector<FieldInitializer> fields;
+  std::vector<FieldInitializer> fields_;
 };
 
 // A non-empty literal value of a struct type.
@@ -255,9 +265,8 @@ class StructLiteral : public Expression {
     return exp->kind() == Kind::StructLiteral;
   }
 
-  auto fields() const -> const std::vector<FieldInitializer>& {
-    return fields_;
-  }
+  auto fields() const -> llvm::ArrayRef<FieldInitializer> { return fields_; }
+  auto fields() -> llvm::MutableArrayRef<FieldInitializer> { return fields_; }
 
  private:
   std::vector<FieldInitializer> fields_;
@@ -279,9 +288,8 @@ class StructTypeLiteral : public Expression {
     return exp->kind() == Kind::StructTypeLiteral;
   }
 
-  auto fields() const -> const std::vector<FieldInitializer>& {
-    return fields_;
-  }
+  auto fields() const -> llvm::ArrayRef<FieldInitializer> { return fields_; }
+  auto fields() -> llvm::MutableArrayRef<FieldInitializer> { return fields_; }
 
  private:
   std::vector<FieldInitializer> fields_;

executable_semantics/ast/expression_test.cpp

@@ -22,8 +22,8 @@ using testing::IsEmpty;
 // Matches a FieldInitializer named `name` whose `expression` is an
 // `IntLiteral`
 MATCHER_P(IntFieldNamed, name, "") {
-  return arg.name == std::string(name) &&
-         arg.expression->kind() == Expression::Kind::IntLiteral;
+  return arg.name() == std::string(name) &&
+         arg.expression()->kind() == Expression::Kind::IntLiteral;
 }
 
 static auto FakeSourceLoc(int line_num) -> SourceLocation {
@@ -42,7 +42,7 @@ TEST_F(ExpressionTest, EmptyAsExpression) {
       ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(expression->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(expression->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*expression).Fields(), IsEmpty());
+  EXPECT_THAT(cast<TupleLiteral>(*expression).fields(), IsEmpty());
 }
 
 TEST_F(ExpressionTest, EmptyAsTuple) {
@@ -52,7 +52,7 @@ TEST_F(ExpressionTest, EmptyAsTuple) {
       TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(tuple->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*tuple).Fields(), IsEmpty());
+  EXPECT_THAT(cast<TupleLiteral>(*tuple).fields(), IsEmpty());
 }
 
 TEST_F(ExpressionTest, UnaryNoCommaAsExpression) {
@@ -83,7 +83,7 @@ TEST_F(ExpressionTest, UnaryNoCommaAsTuple) {
       TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(tuple->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*tuple).Fields(),
+  EXPECT_THAT(cast<TupleLiteral>(*tuple).fields(),
               ElementsAre(IntFieldNamed("0")));
 }
 
@@ -97,7 +97,7 @@ TEST_F(ExpressionTest, UnaryWithCommaAsExpression) {
       ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(expression->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(expression->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*expression).Fields(),
+  EXPECT_THAT(cast<TupleLiteral>(*expression).fields(),
               ElementsAre(IntFieldNamed("0")));
 }
 
@@ -111,7 +111,7 @@ TEST_F(ExpressionTest, UnaryWithCommaAsTuple) {
       TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(tuple->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*tuple).Fields(),
+  EXPECT_THAT(cast<TupleLiteral>(*tuple).fields(),
               ElementsAre(IntFieldNamed("0")));
 }
 
@@ -127,7 +127,7 @@ TEST_F(ExpressionTest, BinaryAsExpression) {
       ExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(expression->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(expression->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*expression).Fields(),
+  EXPECT_THAT(cast<TupleLiteral>(*expression).fields(),
               ElementsAre(IntFieldNamed("0"), IntFieldNamed("1")));
 }
 
@@ -143,7 +143,7 @@ TEST_F(ExpressionTest, BinaryAsTuple) {
       TupleExpressionFromParenContents(&arena, FakeSourceLoc(1), contents);
   EXPECT_EQ(tuple->source_loc(), FakeSourceLoc(1));
   ASSERT_EQ(tuple->kind(), Expression::Kind::TupleLiteral);
-  EXPECT_THAT(cast<TupleLiteral>(*tuple).Fields(),
+  EXPECT_THAT(cast<TupleLiteral>(*tuple).fields(),
               ElementsAre(IntFieldNamed("0"), IntFieldNamed("1")));
 }
 

executable_semantics/interpreter/interpreter.cpp

@@ -201,11 +201,11 @@ auto Interpreter::CreateTuple(Nonnull<Action*> act,
   //    { { (v1,...,vn) :: C, E, F} :: S, H}
   // -> { { `(v1,...,vn) :: C, E, F} :: S, H}
   const auto& tup_lit = cast<TupleLiteral>(*exp);
-  CHECK(act->results().size() == tup_lit.Fields().size());
+  CHECK(act->results().size() == tup_lit.fields().size());
   std::vector<TupleElement> elements;
   for (size_t i = 0; i < act->results().size(); ++i) {
     elements.push_back(
-        {.name = tup_lit.Fields()[i].name, .value = act->results()[i]});
+        {.name = tup_lit.fields()[i].name(), .value = act->results()[i]});
   }
 
   return arena->New<TupleValue>(std::move(elements));
@@ -217,7 +217,7 @@ auto Interpreter::CreateStruct(const std::vector<FieldInitializer>& fields,
   CHECK(fields.size() == values.size());
   std::vector<TupleElement> elements;
   for (size_t i = 0; i < fields.size(); ++i) {
-    elements.push_back({.name = fields[i].name, .value = values[i]});
+    elements.push_back({.name = fields[i].name(), .value = values[i]});
   }
 
   return arena->New<StructValue>(std::move(elements));
@@ -448,13 +448,13 @@ auto Interpreter::StepLvalue() -> Transition {
     }
     case Expression::Kind::TupleLiteral: {
       if (act->pos() <
-          static_cast<int>(cast<TupleLiteral>(*exp).Fields().size())) {
+          static_cast<int>(cast<TupleLiteral>(*exp).fields().size())) {
         //    { { vk :: (f1=v1,..., fk=[],fk+1=ek+1,...) :: C, E, F} :: S,
         //    H}
         // -> { { ek+1 :: (f1=v1,..., fk=vk, fk+1=[],...) :: C, E, F} :: S,
         // H}
         Nonnull<const Expression*> elt =
-            cast<TupleLiteral>(*exp).Fields()[act->pos()].expression;
+            cast<TupleLiteral>(*exp).fields()[act->pos()].expression();
         return Spawn{arena->New<LValAction>(elt)};
       } else {
         return Done{CreateTuple(act, exp)};
@@ -516,13 +516,13 @@ auto Interpreter::StepExp() -> Transition {
     }
     case Expression::Kind::TupleLiteral: {
       if (act->pos() <
-          static_cast<int>(cast<TupleLiteral>(*exp).Fields().size())) {
+          static_cast<int>(cast<TupleLiteral>(*exp).fields().size())) {
         //    { { vk :: (f1=v1,..., fk=[],fk+1=ek+1,...) :: C, E, F} :: S,
         //    H}
         // -> { { ek+1 :: (f1=v1,..., fk=vk, fk+1=[],...) :: C, E, F} :: S,
         // H}
         Nonnull<const Expression*> elt =
-            cast<TupleLiteral>(*exp).Fields()[act->pos()].expression;
+            cast<TupleLiteral>(*exp).fields()[act->pos()].expression();
         return Spawn{arena->New<ExpressionAction>(elt)};
       } else {
         return Done{CreateTuple(act, exp)};
@@ -532,7 +532,7 @@ auto Interpreter::StepExp() -> Transition {
       const auto& literal = cast<StructLiteral>(*exp);
       if (act->pos() < static_cast<int>(literal.fields().size())) {
         Nonnull<const Expression*> elt =
-            literal.fields()[act->pos()].expression;
+            literal.fields()[act->pos()].expression();
         return Spawn{arena->New<ExpressionAction>(elt)};
       } else {
         return Done{CreateStruct(literal.fields(), act->results())};
@@ -542,11 +542,11 @@ auto Interpreter::StepExp() -> Transition {
       const auto& struct_type = cast<StructTypeLiteral>(*exp);
       if (act->pos() < static_cast<int>(struct_type.fields().size())) {
         return Spawn{arena->New<ExpressionAction>(
-            struct_type.fields()[act->pos()].expression)};
+            struct_type.fields()[act->pos()].expression())};
       } else {
         VarValues fields;
         for (size_t i = 0; i < struct_type.fields().size(); ++i) {
-          fields.push_back({struct_type.fields()[i].name, act->results()[i]});
+          fields.push_back({struct_type.fields()[i].name(), act->results()[i]});
         }
         return Done{arena->New<StructType>(std::move(fields))};
       }

executable_semantics/interpreter/type_checker.cpp

@@ -147,12 +147,18 @@ static auto ArgumentDeduction(SourceLocation source_loc, TypeEnv deduced,
     }
     case Value::Kind::TupleValue: {
       if (arg->kind() != Value::Kind::TupleValue) {
-        ExpectType(source_loc, "argument deduction", param, arg);
+        FATAL_COMPILATION_ERROR(source_loc)
+            << "type error in argument deduction\n"
+            << "expected: " << *param << "\n"
+            << "actual: " << *arg;
       }
       const auto& param_tup = cast<TupleValue>(*param);
       const auto& arg_tup = cast<TupleValue>(*arg);
       if (param_tup.Elements().size() != arg_tup.Elements().size()) {
-        ExpectType(source_loc, "argument deduction", param, arg);
+        FATAL_COMPILATION_ERROR(source_loc)
+            << "mismatch in tuple sizes, expected "
+            << param_tup.Elements().size() << " but got "
+            << arg_tup.Elements().size();
       }
       for (size_t i = 0; i < param_tup.Elements().size(); ++i) {
         if (param_tup.Elements()[i].name != arg_tup.Elements()[i].name) {
@@ -168,12 +174,18 @@ static auto ArgumentDeduction(SourceLocation source_loc, TypeEnv deduced,
     }
     case Value::Kind::StructType: {
       if (arg->kind() != Value::Kind::StructType) {
-        ExpectType(source_loc, "argument deduction", param, arg);
+        FATAL_COMPILATION_ERROR(source_loc)
+            << "type error in argument deduction\n"
+            << "expected: " << *param << "\n"
+            << "actual: " << *arg;
       }
       const auto& param_struct = cast<StructType>(*param);
       const auto& arg_struct = cast<StructType>(*arg);
       if (param_struct.fields().size() != arg_struct.fields().size()) {
-        ExpectType(source_loc, "argument deduction", param, arg);
+        FATAL_COMPILATION_ERROR(source_loc)
+            << "mismatch in struct field counts, expected "
+            << param_struct.fields().size() << " but got "
+            << arg_struct.fields().size();
       }
       for (size_t i = 0; i < param_struct.fields().size(); ++i) {
         if (param_struct.fields()[i].first != arg_struct.fields()[i].first) {
@@ -189,7 +201,10 @@ static auto ArgumentDeduction(SourceLocation source_loc, TypeEnv deduced,
     }
     case Value::Kind::FunctionType: {
       if (arg->kind() != Value::Kind::FunctionType) {
-        ExpectType(source_loc, "argument deduction", param, arg);
+        FATAL_COMPILATION_ERROR(source_loc)
+            << "type error in argument deduction\n"
+            << "expected: " << *param << "\n"
+            << "actual: " << *arg;
       }
       const auto& param_fn = cast<FunctionType>(*param);
       const auto& arg_fn = cast<FunctionType>(*arg);
@@ -202,7 +217,10 @@ static auto ArgumentDeduction(SourceLocation source_loc, TypeEnv deduced,
     }
     case Value::Kind::PointerType: {
       if (arg->kind() != Value::Kind::PointerType) {
-        ExpectType(source_loc, "argument deduction", param, arg);
+        FATAL_COMPILATION_ERROR(source_loc)
+            << "type error in argument deduction\n"
+            << "expected: " << *param << "\n"
+            << "actual: " << *arg;
       }
       return ArgumentDeduction(source_loc, deduced,
                                cast<PointerType>(*param).Type(),
@@ -341,11 +359,11 @@ auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e, TypeEnv types,
       std::vector<FieldInitializer> new_args;
       std::vector<TupleElement> arg_types;
       auto new_types = types;
-      for (const auto& arg : cast<TupleLiteral>(*e).Fields()) {
-        auto arg_res = TypeCheckExp(arg.expression, new_types, values);
+      for (auto& arg : cast<TupleLiteral>(*e).fields()) {
+        auto arg_res = TypeCheckExp(arg.expression(), new_types, values);
         new_types = arg_res.types;
-        new_args.push_back(FieldInitializer(arg.name, arg_res.exp));
-        arg_types.push_back({.name = arg.name, .value = arg_res.type});
+        new_args.push_back(FieldInitializer(arg.name(), arg_res.exp));
+        arg_types.push_back({.name = arg.name(), .value = arg_res.type});
       }
       auto tuple_e = arena->New<TupleLiteral>(e->source_loc(), new_args);
       auto tuple_t = arena->New<TupleValue>(std::move(arg_types));
@@ -355,26 +373,26 @@ auto TypeChecker::TypeCheckExp(Nonnull<Expression*> e, TypeEnv types,
       std::vector<FieldInitializer> new_args;
       VarValues arg_types;
       auto new_types = types;
-      for (const auto& arg : cast<StructLiteral>(*e).fields()) {
-        auto arg_res = TypeCheckExp(arg.expression, new_types, values);
+      for (auto& arg : cast<StructLiteral>(*e).fields()) {
+        auto arg_res = TypeCheckExp(arg.expression(), new_types, values);
         new_types = arg_res.types;
-        new_args.push_back(FieldInitializer(arg.name, arg_res.exp));
-        arg_types.push_back({arg.name, arg_res.type});
+        new_args.push_back(FieldInitializer(arg.name(), arg_res.exp));
+        arg_types.push_back({arg.name(), arg_res.type});
       }
       auto new_e = arena->New<StructLiteral>(e->source_loc(), new_args);
       auto type = arena->New<StructType>(std::move(arg_types));
       return TCExpression(new_e, type, new_types);
     }
     case Expression::Kind::StructTypeLiteral: {
-      const auto& struct_type = cast<StructTypeLiteral>(*e);
+      auto& struct_type = cast<StructTypeLiteral>(*e);
       std::vector<FieldInitializer> new_args;
       auto new_types = types;
-      for (const auto& arg : struct_type.fields()) {
-        auto arg_res = TypeCheckExp(arg.expression, new_types, values);
+      for (auto& arg : struct_type.fields()) {
+        auto arg_res = TypeCheckExp(arg.expression(), new_types, values);
         new_types = arg_res.types;
         Nonnull<const Value*> type = interpreter.InterpExp(values, arg_res.exp);
         new_args.push_back(
-            FieldInitializer(arg.name, ReifyType(type, e->source_loc())));
+            FieldInitializer(arg.name(), ReifyType(type, e->source_loc())));
       }
       auto new_e = arena->New<StructTypeLiteral>(e->source_loc(), new_args);
       Nonnull<const Value*> type;